On Mon, 28 Sep 2020, Dave Hansen wrote:
> On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index c7a47603537f..5632f02146ca 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long
> > map_start,
> > step_size = PMD_SIZE;
> > max_pfn_mapped = 0; /* will get exact value next */
> > min_pfn_mapped = real_end >> PAGE_SHIFT;
> > - last_start = start = real_end;
> > + last_start = real_end;
>
> Thanks for finding this.
>
> This becomes even more obviously correct if we just move the 'start'
> declaration into the while() loop. If we do that, it puts the three
> assignment locations right next to the definition, and its trivial to
> spot that the initialization was not missed:
>
> while (last_start > map_start) {
> unsigned long start;
>
> if (last_start > step_size) {
> start = round_down(last_start - 1, step_size);
> if (start < map_start)
> start = map_start;
> } else
> start = map_start;
> ...
>
Agree, this point is simply a question of style:
Shall local variables be defined as "local" as possible or simply
consistently at the beginning of each function?
If there are no strong opinions of style, I would just keep this patch
as-is.
> Either way, your patch looks correct to me:
>
> Acked-by: Dave Hansen <dave.han...@linux.intel.com>
>
Thanks for the Ack.
Lukas
