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

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

From: Christopher Covington <hidden>
Date: 2016-02-19 17:20:51
Also in: linux-acpi, linux-arm-kernel, lkml


On February 19, 2016 10:25:50 AM EST, Peter Hurley [off-list ref] wrote:

On 02/19/2016 02:42 AM, Aleksey Makarov wrote:

quoted

Hi Peter,

Thank you for review.

On 02/19/2016 01:03 AM, Peter Hurley wrote:

quoted

On 02/17/2016 07:36 PM, Zheng, Lv wrote:

quoted

Hi,

quoted

From: Aleksey Makarov [mailto:aleksey.makarov@linaro.org]
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:

[..]

quoted

early_acpi_os_unmap_memory() is marked as __init because it

calls

quoted

quoted
quoted
quoted
quoted
quoted
quoted
__acpi_unmap_table(), but only when acpi_gbl_permanent_mmap is

not

quoted

set.

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

quoted

quoted
quoted
quoted
quoted
quoted
quoted
We need this function to be non-__init because we need access

to

quoted

some tables at unpredictable time--it may be before or after
acpi_gbl_permanent_mmap is set.  For example, SPCR (Serial Port

Console

quoted

quoted
quoted
quoted
quoted
quoted
quoted
Redirection) table is needed each time a new console is

registered.

quoted

quoted
quoted
quoted
quoted
quoted
quoted
It can be quite early (console_initcall) or when a module is

inserted.

quoted

When this table accessed before acpi_gbl_permanent_mmap is set,
the pointer should be unmapped.  This is exactly what this

function

quoted

quoted
quoted
quoted
quoted
quoted
quoted
does.
[Lv Zheng]
Why don't you use another API instead of

early_acpi_os_unmap_memory()

quoted

in

quoted

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:

quoted

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

quoted

should be unmapped.

2  After acpi_gbl_permanent_mmap is set this pointer should not be

unmapped

quoted

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.

quoted

In fact, we have a series to introduce acpi_put_table() to achieve

this.

quoted

So your argument is wrong from very first point.

quoted

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

quoted

been set,
it would have tried to free that pointer with an oops (actually, I

checked that

quoted

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.

quoted

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.

quoted

Why do you need the map to be persistent in the kernel address

space?

quoted

You can always map a small table, but what if the table size is

very big?

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.

quoted

Probably it (and/or other functions in that api) should be

renamed.

quoted

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

quoted

[Lv Zheng]
One more thing is:
If you can't switch your driver to use acpi_os_unmap_memory()

instead of

quoted

early_acpi_os_unmap_memory(),

quoted

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.

quoted

Exactly.

The central problem here is the way Aleksey is trying to hookup a

console.

quoted

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

quoted

4. unmap SPCR

This does not work.  

SPCR specifies address of the console, but add_preferred_console()

accepts

quoted

name of console and its index.  There are no general method to

translate address

quoted

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.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help