[U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory

[Simon Glass]

Hi Sjoerd,

On 28 February 2015 at 07:12, Sjoerd Simons
<sjoerd.simons at collabora.co.uk> wrote:
> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>> Hi Sjoerd,
>>
>> On 19 February 2015 at 15:41, Sjoerd Simons
>> <sjoerd.simons at collabora.co.uk> wrote:
>> > Properly map memory through map_sysmem so that pxe can be used from the
>> > sandbox.
>> >
>> > Signed-off-by: Sjoerd Simons <sjoerd.simons at collabora.co.uk>
>>
>> Please run your patches through patman as you seem to have style
>> violations. Also can you add some notes about how you have tested this
>> on real hardware?
>
> Will do for the next round together with addressing your comments. One
> confused me tough, see below.
>
>> > ---
>> >  common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
>> >  1 file changed, 46 insertions(+), 34 deletions(-)
>
>> > @@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
>> >   *
>> >   * Returns 1 on success, < 0 on error.
>> >   */
>> > -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level)
>> > +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
>> > +                               struct pxe_menu *cfg, int nest_level)
>>
>> s/b/base/ or something more meaningful (fix above/below also)
>> >  {
>> >         struct token t;
>> > -       char *s, *b, *label_name;
>> > +       char *s, *label_name, *base;
>> >         int err;
>> >
>> > -       b = p;
>> > +       base = p;
>>
>> This worries me - assigning a pointer to a ulong.
>
>
> base is a pointer here though. So this comment confuses me.

Actually it confused me. I don't think it's good to use base as a
pointer or a ulong in the same file. Maybe use base for the ulong and
base_ptr for the pointer. Or base_addr and base. But code is harder to
read if different functions in the file have different conventions.

Regards,
Simon