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. There is another reason why I prefer to parse SPCR each time a console is registered. Some consoles can be registered very early in the initialization process and we have to be sure add_preferred_console() is called before that. Of course, I could just parse it once and cache the results, but this still requires accessing SPCR before acpi_gbl_permanent_mmap is set *or* after that. > 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(). So are you suggesting a separate method for each possible console? This is not acceptable. Do you suggest making up separate new name (like "uart" for 8250) for each type of conosole SPCR can specify? > 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(). Please do. Or just send a link to to that submission. Do you mean the Leif Lindholm's patches: https://lkml.kernel.org/g/1441716217-23786-1-git-send-email-leif.lindholm@xxxxxxxxxx He did same thing as I did in my v3 exept 1) he parses ACPI tree to match device (I just match SPCR against data that struct uart_port already has) 2) As you are suggesting, he parses SPCR once at a predefined point in initialization. And that's why his submission is RFC: he had troubles with the order of initialization. Thank you Aleksey Makarov > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
