Hi Lv,
Thank you for review.
On 02/17/2016 05:51 AM, Zheng, Lv wrote:
[..]
>>> early_acpi_os_unmap_memory() is marked as __init because it calls
>>> __acpi_unmap_table(), but only when acpi_gbl_permanent_mmap is not set.
>>>
>>> acpi_gbl_permanent_mmap is set in __init acpi_early_init()
>>> so it is safe to call early_acpi_os_unmap_memory() from anywhere
>>>
>>> We need this function to be non-__init because we need access to
>>> some tables at unpredictable time--it may be before or after
>>> acpi_gbl_permanent_mmap is set. For example, SPCR (Serial Port Console
>>> Redirection) table is needed each time a new console is registered.
>>> It can be quite early (console_initcall) or when a module is inserted.
>>> When this table accessed before acpi_gbl_permanent_mmap is set,
>>> the pointer should be unmapped. This is exactly what this function
>>> does.
>> [Lv Zheng]
>> Why don't you use another API instead of early_acpi_os_unmap_memory() in
>> case you want to unmap things in any cases.
>> acpi_os_unmap_memory() should be the one to match this purpose.
>> It checks acpi_gbl_ppermanent_mmap in acpi_os_unmap_iomem().
As far as I understand, there exist two steps in ACPI initialization:
1. Before acpi_gbl_permanent_mmap is set, tables received with acpi_get_table_with_size()
are mapped by early_memremap(). If a subsystem gets such a pointer it should be unmapped.
2 After acpi_gbl_permanent_mmap is set this pointer should not be unmapped at all.
That exactly what early_acpi_os_unmap_memory() does--it checks acpi_gbl_permanent_mmap.
If I had used acpi_os_unmap_memory() after acpi_gbl_permanent_mmap had been set,
it would have tried to free that pointer with an oops (actually, I checked that and got that oops).
So using acpi_os_unmap_memory() is not an option here, but early_acpi_os_unmap_memory()
match perfectly.
>> And in fact early_acpi_os_unmap_memory() should be removed.
I don't think so -- I have explained why. It does different thing.
Probably it (and/or other functions in that api) should be renamed.
> [Lv Zheng]
> One more thing is:
> If you can't switch your driver to use acpi_os_unmap_memory() instead of early_acpi_os_unmap_memory(),
> then it implies that your driver does have a defect.
I still don't understand what defect, sorry.
> There is an early bootup requirement in Linux.
> Maps acquired during the early stage should be freed by the driver during the early stage.
> And the driver should re-acquire the memory map after booting.
Exactly. That's why I use early_acpi_os_unmap_memory(). The point is that that code can be
called *before* acpi_gbl_permanent_mmap is set (at early initialization of for example 8250 console)
or *after* acpi_gbl_permanent_mmap is set (at insertion of a module that registers a console),
We just can not tell if the received table pointer should or sould not be freed with early_memunmap()
(actually __acpi_unmap_table() or whatever) without checking acpi_gbl_permanent_mmap,
but that's all too low level.
Another option, as you describe, is to get this pointer early, don't free it untill
acpi_gbl_permanent_mmap is set, then free it (with early_acpi_os_unmap_memory(), that's
ok because acpi_gbl_permanent_mmap is set in an init code), then get the persistent
pointer to the table. The problem with it is that we can not just watch acpi_gbl_permanent_mmap
to catch this exact moment. And also accessing acpi_gbl_permanent_mmap is not good as it probably is
an implementation detail (i. e. too lowlevel) of the ACPI code.
Or I probably miss what you are suggesting.
> This is because, during early bootup stage, there are only limited slots available for drivers to map memory.
> So by changing __init to __ref here, you probably will hide many such defects.
What defects? This funcions checks acpi_gbl_permanent_mmap.
BTW, exactly the same thing is done in the beginning of acpi_os_unmap_memory() and than's ok,
that function is __ref.
> And solving issues in this way doesn't seem to be an improvement.
Could you please tell me where I am wrong? I still don't understand your point.
Thank you
Aleksey Makarov
>
> Thanks and best regards
> -Lv
>
>>
>> Thanks and best regards
>> -Lv
>>
>>>
>>> Signed-off-by: Aleksey Makarov <aleksey.makarov@xxxxxxxxxx>
>>> ---
>>> drivers/acpi/osl.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>>> index 67da6fb..8a552cd 100644
>>> --- a/drivers/acpi/osl.c
>>> +++ b/drivers/acpi/osl.c
>>> @@ -497,7 +497,11 @@ void __ref acpi_os_unmap_memory(void *virt,
>>> acpi_size size)
>>> }
>>> EXPORT_SYMBOL_GPL(acpi_os_unmap_memory);
>>>
>>> -void __init early_acpi_os_unmap_memory(void __iomem *virt, acpi_size
>> size)
>>> +/*
>>> + * acpi_gbl_permanent_mmap is set in __init acpi_early_init()
>>> + * so it is safe to call early_acpi_os_unmap_memory() from anywhere
>>> + */
>>> +void __ref early_acpi_os_unmap_memory(void __iomem *virt, acpi_size
>> size)
>>> {
>>> if (!acpi_gbl_permanent_mmap)
>>> __acpi_unmap_table(virt, size);
>>> --
>>> 2.7.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
