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

[Peter Hurley]

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.

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