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

[Aleksey Makarov]

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
>>>