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

[Aleksey Makarov]

On 02/26/2016 09:39 AM, Zheng, Lv wrote:
> Hi, Aleksey
> 
> Though I promised you to have this done in 1 week:
> https://github.com/zetalog/acpica/commit/a234569
> I implemented the idea and tried the cleanup in Linux.
> But unfortunately the cleanup triggered several locking issues inside of ACPICA table API users.
> So it doesn't seem to be so easy and quick to make it upstreamed.
> And I have to continue to test and improve the cleanup.
> 
> I'm sorry for that, hope you can find substitute solutions before the cleanup is upstreamed.

That's fine, I use the approach I described in the last mail, it works.

Thank you
Aleksey Makarov

> 
> Thanks and best regards
> -Lv
> 
>> From: Aleksey Makarov [mailto:aleksey.makarov@xxxxxxxxxx]
>> Sent: Monday, February 22, 2016 10:58 PM
>> Subject: Re: [PATCH v3 1/5] ACPI: change __init to __ref for
>> early_acpi_os_unmap_memory()
>>
>> Hi,
>>
>> On 02/22/2016 05:24 AM, 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,
>>>>
>>>> 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?
>>> [Lv Zheng]
>>> Do not change __init for now.
>>>
>>> Currently you should:
>>> 1. before acpi_gbl_permanent_mmap is set:
>>> acpi_get_table_with_size()
>>> parse the table
>>> early_acpi_os_unmap_memory()
>>> Do your driver early stuff here
>>>
>>> 2. after acpi_gbl_permanent_mmap is set:
>>> acpi_get_table()
>>> Parse the table
>>> Do your driver late stuff here
>>> <- note there is no API now being an inverse of acpi_get_table().
>>
>> That's fine.  These are two different methods to access the table.
>> I need one that works in both cases.  Of course, they could be combined,
>> but I am not sure if I can access the acpi_gbl_permanent_mmap variable--it
>> seems to be an implementation detail of ACPI code.
>>
>> Instead I am going to use the 1st method once and cache the result like this:
>>
>>
>> static int __init parse(void)
>> {
>> 	static bool parsed;
>>
>> 	if (!parsed) {
>> 		acpi_get_table_with_size()
>> 		/* parse the table and cache the result */
>> 		early_acpi_os_unmap_memory()
>> 		parse = true;
>> 	}
>> }
>>
>> arch_initcal(parse());
>>
>> int __ref the_handler_where_I_need_the_parse_results(void)
>> {
>> 	parse();
>>
>> 	/* use the data */
>> }
>>
>> I hope you are OK with it.
>>
>>> Besides, I'm about to insert error messages between 1 and 2.
>>> If an early map is not release, error message will be prompted to the
>> developers.
>>
>> AFAICS, there is such an error message and I saw it.
>> Refer to check_early_ioremap_leak() at mm/early_ioremap.c
>>
>>> And if you don't follow the above rules, it mean you are trying to lay a mine,
>> waiting for me to step on it.
>>> That's why this change is entirely not acceptable.
>>
>> Ok, I see.
>>
>>> I'm about to send out the cleanup series in 1 week, and will Cc you.
>>> You can rebase your code on top of the cleanup series.
>>
>> Thank you
>> Aleksey Makarov
>>
>>>
>>> Thanks and best regards
>>> -Lv
>>>
>>>>
>>>>> 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
>>>>>