Hi Lv,

Thank you for review.

On 02/17/2016 05:51 AM, Zheng, Lv wrote:

[..]

quoted

quoted

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.

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.

quoted

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

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.

Another option, as you describe, is to get this pointer early, don't free it 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.

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.

Thank you
Aleksey Makarov

Thanks and best regards
-Lv

quoted

Thanks and best regards
-Lv

quoted

Signed-off-by: Aleksey Makarov <redacted>
---
 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)

quoted

+/*
+ * 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)

quoted

 {
 	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 at vger.kernel.org
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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html