On Thu, Apr 4, 2013 at 10:36 AM, Tejun Heo tj@kernel.org 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