Re: [PATCH v2 03/20] x86, ACPI, mm: Kill max_low_pfn_mapped

[Yinghai Lu]

On Thu, Apr 4, 2013 at 10:36 AM, Tejun Heo <tj@xxxxxxxxxx> wrote:
> Hello,
>
> On Sat, Mar 09, 2013 at 10:44:30PM -0800, Yinghai Lu wrote:
>> Now we have arch_pfn_mapped array, and max_low_pfn_mapped should not
>> be used anymore.
>>
>> User should use arch_pfn_mapped or just 1UL<<(32-PAGE_SHIFT) instead.
>>
>> Only user is ACPI_INITRD_TABLE_OVERRIDE, and it should not use that,
>> as later accessing is using early_ioremap(). Change to try to 4G below
>> and then 4G above.
> ...
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 586e7e9..c08cdb6 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -624,9 +624,13 @@ void __init acpi_initrd_override(void *data, size_t size)
>>       if (table_nr == 0)
>>               return;
>>
>> -     acpi_tables_addr =
>> -             memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT,
>> -                                    all_tables_size, PAGE_SIZE);
>> +     /* under 4G at first, then above 4G */
>> +     acpi_tables_addr = memblock_find_in_range(0, (1ULL<<32) - 1,
>> +                                     all_tables_size, PAGE_SIZE);
>> +     if (!acpi_tables_addr)
>> +             acpi_tables_addr = memblock_find_in_range(0,
>> +                                     ~(phys_addr_t)0,
>> +                                     all_tables_size, PAGE_SIZE);
>
> So, it's changing the allocation from <=4G to <=4G first and then >4G.
> The only explanation given is "as later accessing is using
> early_ioremap()", but I can't see why that can be a reason for that.
> early_ioremap() doesn't care whether the given physaddr is under 4G or
> not, it unconditionally maps it into fixmap, so whether the allocated
> address is below or above 4G doesn't make any difference.
>
> Changing the allowed range of the allocation should be a separate
> patch.  It has some chance of its own breakage and the change itself
> isn't really related to this one.

Ok, will separate that  "try above 4G" to another patch.

>
> Please try to elaborate the reasoning behind "why", so that readers of
> the description don't have to deduce (oh well, guess) your intentions
> behind the changes.  As much as it would help the readers, it'd also
> help you even more as you would have had to explicitly write something
> like "the table is accessed with early_ioremap() so the address
> doesn't need to be restricted under 4G; however, to avoid unnecessary
> remappings, first try <= 4G and then > 4G."  Then, you would be
> compelled to check whether the statement you explicitly wrote is true,
> which isn't in this case and you would also realize that the change
> isn't trivial and doesn't really belong with this patch.  By not doing
> the due diligence, you're offloading what you should have done to
> others, which isn't very nice.
>
> I think the descriptions are better in this posting than the last time
> but it's still lacking, so, please putfff more effort into describing
> the changes and reasoning behind them.

ok.

Thanks a lot.

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/