From: peter@hurleysoftware•com (Peter Hurley)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 1/5] ACPI: change __init to __ref for early_acpi_os_unmap_memory()
Date: Sun, 21 Feb 2016 21:37:39 -0800 [thread overview]
Message-ID: <56CA9EA3.9020000@hurleysoftware.com> (raw)
In-Reply-To: <79D370BB-B7A3-4540-B7FB-81D5806853D6@codeaurora.org>
On 02/19/2016 09:20 AM, Christopher Covington wrote:
>
>
> On February 19, 2016 10:25:50 AM EST, Peter Hurley <peter@hurleysoftware•com> 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 at 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:
>>>>>>
>>>>>> [..]
>>>>>>
>>>>>>>>> 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?
I don't see why not. Since earlycon is a bootconsole, it's disabled when
the first regular console starts. I use earlycon + KGDB now on the same
device (ttyS0 == uart0 @ mmio 0x44e09000):
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.5.0-rc2+ (peter at thor) (gcc version 5.1.1 20150608 (Linaro GCC 5.1-2015.08) ) #60 PREE
MPT Sun Feb 21 20:17:16 PST 2016
[ 0.000000] CPU: ARMv7 Processor [413fc082] revision 2 (ARMv7), cr=10c5387d
[ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[ 0.000000] Machine model: TI AM335x BeagleBone Black
[ 0.000000] earlycon: omap8250 at MMIO 0x44e09000 (options '')
[ 0.000000] bootconsole [omap8250] enabled
....
[ 3.086800] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled
[ 3.099818] console [ttyS0] disabled
[ 3.112402] 44e09000.serial: ttyS0 at MMIO 0x44e09000 (irq = 158, base_baud = 3000000) is a 8250
[ 3.121575] console [ttyS0] enabled
[ 3.121575] console [ttyS0] enabled
[ 3.128678] bootconsole [omap8250] disabled
[ 3.128678] bootconsole [omap8250] disabled
[ 3.152397] 481a8000.serial: ttyS4 at MMIO 0x481a8000 (irq = 159, base_baud = 3000000) is a 8250
[ 3.176393] 481aa000.serial: ttyS5 at MMIO 0x481aa000 (irq = 160, base_baud = 3000000) is a 8250
[ 3.185998] KGDB: Registered I/O driver kgdboc
[ 3.200374] KGDB: Waiting for connection from remote gdb...
> If it can only be used for one, let's make sure the choice of
> earlycon vs KGDB is intentional rather than accidental.
If anything, a future feature would be to run KGDB over earlycon.
Regards,
Peter Hurley
next prev parent reply other threads:[~2016-02-22 5:37 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-15 18:05 [PATCH v3 0/5] ACPI: parse the SPCR table Aleksey Makarov
2016-02-15 18:05 ` [PATCH v3 1/5] ACPI: change __init to __ref for early_acpi_os_unmap_memory() Aleksey Makarov
2016-02-17 2:44 ` Zheng, Lv
2016-02-17 2:51 ` Zheng, Lv
2016-02-17 13:08 ` Aleksey Makarov
2016-02-18 3:36 ` Zheng, Lv
2016-02-18 22:03 ` Peter Hurley
2016-02-19 2:58 ` Zheng, Lv
2016-02-19 11:02 ` Aleksey Makarov
2016-02-22 2:24 ` Zheng, Lv
2016-02-22 14:58 ` Aleksey Makarov
2016-02-23 0:19 ` Zheng, Lv
2016-02-26 6:39 ` Zheng, Lv
2016-02-26 10:33 ` Aleksey Makarov
2016-02-19 10:42 ` Aleksey Makarov
2016-02-19 15:25 ` Peter Hurley
2016-02-19 17:20 ` Christopher Covington
2016-02-22 5:37 ` Peter Hurley [this message]
2016-02-22 14:43 ` Aleksey Makarov
2016-02-22 14:35 ` Aleksey Makarov
2016-03-01 15:24 ` Peter Hurley
2016-02-15 18:05 ` [PATCH v3 2/5] ACPI: parse SPCR and enable matching console Aleksey Makarov
2016-02-18 22:19 ` Peter Hurley
2016-02-19 10:47 ` Aleksey Makarov
2016-02-19 16:13 ` Peter Hurley
2016-02-21 9:42 ` Yury Norov
2016-02-22 15:03 ` Aleksey Makarov
2016-02-15 18:05 ` [PATCH v3 3/5] ACPI: enable ACPI_SPCR_TABLE on ARM64 Aleksey Makarov
2016-02-15 18:05 ` [PATCH v3 4/5] ACPI: add definition of DBG2 subtypes Aleksey Makarov
2016-02-15 18:05 ` [PATCH v3 5/5] serial: pl011: use SPCR to setup 32-bit access Aleksey Makarov
2016-02-16 19:11 ` [PATCH v3 0/5] ACPI: parse the SPCR table Mark Salter
2016-02-17 2:36 ` Christopher Covington
2016-02-18 21:15 ` Jeremy Linton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56CA9EA3.9020000@hurleysoftware.com \
--to=peter@hurleysoftware$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox