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, > > 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. [Lv Zheng] You should not access acpi_gbl_permanent_mmap. The variable will be deleted after combining the 2 methods. What we need to ensure is that during the period where the 2 different methods are not combined, early_acpi_os_unmap_memory() should always be invoked. And __init can help us to prevent developers submitting wrong 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. [Lv Zheng] I'm OK with the code flow in parse(). As long as there is no compiler complaining detected against the __init declarator of early_acpi_os_unmap_memory(), I'm OK with the rest. > > > 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 [Lv Zheng] We also need ACPICA upstream to be aware of this, otherwise wrong changes could be merged from ACPICA upstream. Thanks -Lv > > > 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 > >>> ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
