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

[Christopher Covington]

On February 19, 2016 10:25:50 AM EST, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
>On 02/19/2016 02:42 AM, Aleksey Makarov wrote:
>> Hi Peter,
>> 
>> Thank you for review.
>> 
>> On 02/19/2016 01:03 AM, Peter Hurley wrote:
>>> 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 does not work.  
>> 
>> SPCR specifies address of the console, but add_preferred_console()
>accepts
>> name of console and its index.  There are no general method to
>translate address
>> to name at such an early stage.
>
>
>	add_preferred_console(uart, 0, "io,0x3f8,115200");
>
>This will start a console with the 8250 driver. I've already pointed
>this out to you in an earlier review. This is what existing firmware
>already does.
>
>This is also the method you will use to start an earlycon from the
>DBG2 table, by additionally calling setup_earlycon().

Can the device specified in DBG2 be used for both earlycon and KGDB? If it can only be used for one, let's make sure the choice of earlycon vs KGDB is intentional rather than accidental.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Sent from my Snapdragon powered Android device with K-9 Mail. Please excuse my brevity.