Re: [PATCH v3 1/5] ACPI: change __init to __ref for early_acpi_os_unmap_memory()

[Aleksey Makarov]

Hi Lv,

On 02/19/2016 05:58 AM, Zheng, Lv wrote:
> Hi,
> 
>> From: Peter Hurley [mailto:peter@xxxxxxxxxxxxxxxxxx]
>> Subject: Re: [PATCH v3 1/5] ACPI: change __init to __ref for
>> early_acpi_os_unmap_memory()
>>
>> On 02/17/2016 07:36 PM, Zheng, Lv wrote:
>>> Hi,
>>>
>>>> From: Aleksey Makarov [mailto:aleksey.makarov@xxxxxxxxxx]
>>>> Subject: Re: [PATCH v3 1/5] ACPI: change __init to __ref for
>>>> early_acpi_os_unmap_memory()
>>>>
>>>> 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.
>>>>
>>> [Lv Zheng]
>>> This statement is wrong, this should be:
>>> As long as there is a __reference__ to the mapped table, the pointer should
>> not be unmapped.
>>> In fact, we have a series to introduce acpi_put_table() to achieve this.
>>> So your argument is wrong from very first point.
>>>
>>>> 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.
>>> [Lv Zheng]
>>> I don't think so.
>>> For definition block tables, we know for sure there will always be references,
>> until "Unload" opcode is invoked by the AML interpreter.
>>> But for the data tables, OSPMs should use them in this way:
>>> 1. map the table
>>> 2. parse the table and convert it to OS specific structures
>>> 3. unmap the table
>>> This helps to shrink virtual memory address space usages.
>>>
>>> So from this point of view, all data tables should be unmapped right after
>> being parsed.
>>> Why do you need the map to be persistent in the kernel address space?
>>> You can always map a small table, but what if the table size is very big?
>>>
>>>>
>>>>>> 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]
>>> Just let me ask one more question.
>>> eary_acpi_os_unmap_memory() is not used inside of ACPICA.
>>> How ACPICA can work with just acpi_os_unmap_memory()?
>>> You can check drivers/acpi/tbxxx.c.
>>> Especially: acpi_tb_release_temp_table() and the code invoking it.
>>>
>>>>> [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.
>>> [Lv Zheng]
>>> If you can't ensure this sequence for using the data tables:
>>> 1. map the table
>>> 2. parse the table and convert it to OS specific structure
>>> 3. unmap the table
>>> It implies there is a bug in the driver or a bug in the ACPI subsystem core.
>>
>> Exactly.
> [Lv Zheng] 
> So it looks to me:
> Changing __init to __ref here is entirely not acceptable.
> This API should stay being invoked during early stage.
> Otherwise, it may leave us untrackable code that maps tables during early stage and leaks maps to the late stage.
> If Linux contains such kind of code, I'm afraid, it will become impossible to introduce acpi_put_table() to clean up the mappings.
> Because when acpi_put_table() is called during the late stage to free a map acquired during the early stage, it then obviously will end up with panic.

Can you please sugggest a common method to access ACPI tables that
works both before *and* after acpi_gbl_permanent_mmap is set and __init code
is removed?

> Thanks and best regards
> -Lv
> 
>> The central problem here is the way Aleksey is trying to hookup a console.
>>
>> What should be happening in this series is:
>> 1. early map SPCR
>> 2. parse the SPCR table
>> 3. call add_preferred_console() to add the SPCR console to the console table
>> 4. unmap SPCR
>>
>> This trivial and unobtrusive method would already have a 8250 console
>> running via SPCR. I've already pointed this out in previous reviews.
>>
>> Further, the above method *will be required anyway* for the DBG2 table to
>> start an earlycon, which I've already pointed out in previous reviews.
>>
>> Then to enable amba-pl011 console via ACPI, add a console match() method
>> similar to the 8250 console match() method, univ8250_console_match().
>>
>> FWIW, PCI earlycon + console support was already submitted once before but
>> never picked up by GregKH. I think I'll just grab that and re-submit so
>> you would know what to emit for console options in the
>> add_preferred_console().
>>
>>
>> Regards,
>> Peter Hurley
>>
>>
>>>>> 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.
>>> [Lv Zheng]
>>> The driver should make sure that:
>>> Map/unmap is paired during early stage.
>>> For late stage, it should be another pair of map/unmap.
>>>
>>>>
>>>> Another option, as you describe, is to get this pointer early, don't free it
>>> [Lv Zheng]
>>> I mean you should free it early.
>>>
>>>> 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.
>>>>
>>> [Lv Zheng]
>>> I mean, you should:
>>> During early stage:
>>> acpi_os_map_memory()
>>> Parse the table.
>>> acpi_os_unmap_memory().
>>>
>>>>> 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.
>>> [Lv Zheng]
>>> But anyway, the defect should be in ACPI subsystem core.
>>> The cause should be the API itself - acpi_get_table().
>>>
>>> So I agree you can use early_acpi_os_unmap_memory() during the period the
>> root causes are not cleaned up.
>>> But the bottom line is: the driver need to ensure that
>> early_acpi_os_unmap_memory() is always invoked.
>>> As long as you can ensure this, I don't have objections for deploying
>> early_acpi_os_unmap_memory()  for now.
>>>
>>> Thanks and best regards
>>> -Lv
>>>
>>>>
>>>> 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
>