Subject: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Use late_initcall_sync insted of module init to be sure that
serial driver is really probed and get take handover from
early driver. The 8250 register the platform driver after
the 8250 core is initialized. As shown by kdbg

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

This section of the code is too early because in this case
the omap serial is not probed

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt
0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
at ./include/linux/serial_core.h:677
4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2860
5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2912
6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
at drivers/tty/serial/serial_core.c:2285
7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
at drivers/tty/serial/serial_core.c:2656
8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
at drivers/tty/tty_io.c:410
9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
at drivers/base/dd.c:800
15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
at drivers/base/dd.c:958
17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
26 do_initcalls () at init/main.c:1314
27 do_basic_setup () at init/main.c:1333
28 kernel_init_freeable () at init/main.c:1551
29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857

Signed-off-by: Michael Trimarchi <[email protected]>
---
drivers/tty/serial/kgdboc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 7ce7bb164005..7f8364507f55 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -622,7 +622,7 @@ console_initcall(kgdboc_earlycon_late_init);

#endif /* IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE) */

-module_init(init_kgdboc);
+late_initcall_sync(init_kgdboc);
module_exit(exit_kgdboc);
module_param_call(kgdboc, param_set_kgdboc_var, param_get_string, &kps, 0644);
MODULE_PARM_DESC(kgdboc, "<serial_device>[,baud]");
--
2.40.1


2023-12-11 21:39:23

by Doug Anderson

[permalink] [raw]
Subject: Re: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi,

On Fri, Dec 8, 2023 at 1:28 PM Michael Trimarchi
<[email protected]> wrote:
>
> Use late_initcall_sync insted of module init to be sure that
> serial driver is really probed and get take handover from
> early driver.

Awesome that you used the earlycon driver to debug problems with
registering the normal driver! :-P


> The 8250 register the platform driver after
> the 8250 core is initialized. As shown by kdbg
>
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1]
> _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 584 __raw_writeb(value, PCI_IOBASE + addr);
> (gdb) bt
>
> This section of the code is too early because in this case
> the omap serial is not probed
>
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1]
> _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 584 __raw_writeb(value, PCI_IOBASE + addr);
> (gdb) bt
>
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1]
> _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 584 __raw_writeb(value, PCI_IOBASE + addr);
> (gdb) bt
> 0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
> 2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
> 3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
> at ./include/linux/serial_core.h:677
> 4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
> at drivers/tty/serial/8250/8250_port.c:2860
> 5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
> at drivers/tty/serial/8250/8250_port.c:2912
> 6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
> at drivers/tty/serial/serial_core.c:2285
> 7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
> at drivers/tty/serial/serial_core.c:2656
> 8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
> at drivers/tty/tty_io.c:410
> 9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
> 10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
> 11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
> 12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
> 13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
> 14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
> at drivers/base/dd.c:800
> 15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
> 16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
> at drivers/base/dd.c:958
> 17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
> fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
> 18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
> 19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
> 20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
> 21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
> 22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
> 23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
> 24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
> 25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
> 26 do_initcalls () at init/main.c:1314
> 27 do_basic_setup () at init/main.c:1333
> 28 kernel_init_freeable () at init/main.c:1551
> 29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
> 30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857
>
> Signed-off-by: Michael Trimarchi <[email protected]>
> ---
> drivers/tty/serial/kgdboc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 7ce7bb164005..7f8364507f55 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -622,7 +622,7 @@ console_initcall(kgdboc_earlycon_late_init);
>
> #endif /* IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE) */
>
> -module_init(init_kgdboc);
> +late_initcall_sync(init_kgdboc);

While I'm not denying that you hit a bug, I don't think this is the
correct fix. The way it's supposed to work is:

1. init_kgdboc() runs and registers the singleton kgdb "platform driver".

2. The platform driver's probe function, kgdboc_probe(), runs and
checks to see if the console is ready by looking at the return value
of configure_kgdboc(). If it's ready then we're good to go. If it's
not ready then we defer.

So I think the bug here is that somehow the console looks "ready"
(because tty_find_polling_driver() can find it) but it isn't actually
ready yet (because it crashes). That's what you need to fix.

I'll note that, in the past, I've definitely used kgdb on 8250-based
UARTs. Is your hardware somehow special or is this a regression?

-Doug

Subject: Re: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi Doug

On Mon, Dec 11, 2023 at 10:39 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Fri, Dec 8, 2023 at 1:28 PM Michael Trimarchi
> <[email protected]> wrote:
> >
> > Use late_initcall_sync insted of module init to be sure that
> > serial driver is really probed and get take handover from
> > early driver.
>
> Awesome that you used the earlycon driver to debug problems with
> registering the normal driver! :-P
>
>
> > The 8250 register the platform driver after
> > the 8250 core is initialized. As shown by kdbg
> >
> > Thread 2 received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 1]
> > _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> > 584 __raw_writeb(value, PCI_IOBASE + addr);
> > (gdb) bt
> >
> > This section of the code is too early because in this case
> > the omap serial is not probed
> >
> > Thread 2 received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 1]
> > _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> > 584 __raw_writeb(value, PCI_IOBASE + addr);
> > (gdb) bt
> >
> > Thread 2 received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 1]
> > _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> > 584 __raw_writeb(value, PCI_IOBASE + addr);
> > (gdb) bt
> > 0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> > 1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
> > 2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
> > 3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
> > at ./include/linux/serial_core.h:677
> > 4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
> > at drivers/tty/serial/8250/8250_port.c:2860
> > 5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
> > at drivers/tty/serial/8250/8250_port.c:2912
> > 6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
> > at drivers/tty/serial/serial_core.c:2285
> > 7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
> > at drivers/tty/serial/serial_core.c:2656
> > 8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
> > at drivers/tty/tty_io.c:410
> > 9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
> > 10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
> > 11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
> > 12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
> > 13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
> > 14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
> > at drivers/base/dd.c:800
> > 15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
> > 16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
> > at drivers/base/dd.c:958
> > 17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
> > fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
> > 18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
> > 19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
> > 20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
> > 21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
> > 22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
> > 23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
> > 24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
> > 25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
> > 26 do_initcalls () at init/main.c:1314
> > 27 do_basic_setup () at init/main.c:1333
> > 28 kernel_init_freeable () at init/main.c:1551
> > 29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
> > 30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857
> >
> > Signed-off-by: Michael Trimarchi <[email protected]>
> > ---
> > drivers/tty/serial/kgdboc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index 7ce7bb164005..7f8364507f55 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -622,7 +622,7 @@ console_initcall(kgdboc_earlycon_late_init);
> >
> > #endif /* IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE) */
> >
> > -module_init(init_kgdboc);
> > +late_initcall_sync(init_kgdboc);
>
> While I'm not denying that you hit a bug, I don't think this is the
> correct fix. The way it's supposed to work is:
>

Yes it's a bug

> 1. init_kgdboc() runs and registers the singleton kgdb "platform driver".
>
> 2. The platform driver's probe function, kgdboc_probe(), runs and
> checks to see if the console is ready by looking at the return value
> of configure_kgdboc(). If it's ready then we're good to go. If it's
> not ready then we defer.
>
> So I think the bug here is that somehow the console looks "ready"
> (because tty_find_polling_driver() can find it) but it isn't actually
> ready yet (because it crashes). That's what you need to fix.
>

The polling driver look for uart and uart8250_core is registered and 4 fake uart
are there but there are not still replaced by platform driver that can
come later.
The try_polling find it but it's the isa-8250 driver. It means that
add_uart 8250 is
not still happen

> I'll note that, in the past, I've definitely used kgdb on 8250-based
> UARTs. Is your hardware somehow special or is this a regression?
>

Michael

> -Doug



--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com

2023-12-11 22:01:04

by Doug Anderson

[permalink] [raw]
Subject: Re: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi,

On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
<[email protected]> wrote:
>
> > 1. init_kgdboc() runs and registers the singleton kgdb "platform driver".
> >
> > 2. The platform driver's probe function, kgdboc_probe(), runs and
> > checks to see if the console is ready by looking at the return value
> > of configure_kgdboc(). If it's ready then we're good to go. If it's
> > not ready then we defer.
> >
> > So I think the bug here is that somehow the console looks "ready"
> > (because tty_find_polling_driver() can find it) but it isn't actually
> > ready yet (because it crashes). That's what you need to fix.
> >
>
> The polling driver look for uart and uart8250_core is registered and 4 fake uart
> are there but there are not still replaced by platform driver that can
> come later.
> The try_polling find it but it's the isa-8250 driver. It means that
> add_uart 8250 is
> not still happen

The 8250 driver is always a maze, so you might need to do a bunch of
digging. ...but it sure sounds like the console shouldn't be
registered until the correct ops are in place. That either means
getting the ops put in place earlier or deferring when the console is
registered...

-Doug

Subject: Re: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi Doug

On Mon, Dec 11, 2023 at 11:00 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
> <[email protected]> wrote:
> >
> > > 1. init_kgdboc() runs and registers the singleton kgdb "platform driver".
> > >
> > > 2. The platform driver's probe function, kgdboc_probe(), runs and
> > > checks to see if the console is ready by looking at the return value
> > > of configure_kgdboc(). If it's ready then we're good to go. If it's
> > > not ready then we defer.
> > >
> > > So I think the bug here is that somehow the console looks "ready"
> > > (because tty_find_polling_driver() can find it) but it isn't actually
> > > ready yet (because it crashes). That's what you need to fix.
> > >
> >
> > The polling driver look for uart and uart8250_core is registered and 4 fake uart
> > are there but there are not still replaced by platform driver that can
> > come later.
> > The try_polling find it but it's the isa-8250 driver. It means that
> > add_uart 8250 is
> > not still happen
>
> The 8250 driver is always a maze, so you might need to do a bunch of
> digging. ...but it sure sounds like the console shouldn't be
> registered until the correct ops are in place. That either means
> getting the ops put in place earlier or deferring when the console is
> registered...
>

Your point is pretty clear and my initial idea was to find a real fix.
This come to avoid
breaking existing setup but anyway I will dig in it more

Michael

> -Doug



--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com

Subject: Re: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi Doug

On Tue, Dec 12, 2023 at 9:54 AM Michael Nazzareno Trimarchi
<[email protected]> wrote:
>
> Hi Doug
>
> On Mon, Dec 11, 2023 at 11:00 PM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
> > <[email protected]> wrote:
> > >
> > > > 1. init_kgdboc() runs and registers the singleton kgdb "platform driver".
> > > >
> > > > 2. The platform driver's probe function, kgdboc_probe(), runs and
> > > > checks to see if the console is ready by looking at the return value
> > > > of configure_kgdboc(). If it's ready then we're good to go. If it's
> > > > not ready then we defer.
> > > >
> > > > So I think the bug here is that somehow the console looks "ready"
> > > > (because tty_find_polling_driver() can find it) but it isn't actually
> > > > ready yet (because it crashes). That's what you need to fix.
> > > >
> > >
> > > The polling driver look for uart and uart8250_core is registered and 4 fake uart
> > > are there but there are not still replaced by platform driver that can
> > > come later.
> > > The try_polling find it but it's the isa-8250 driver. It means that
> > > add_uart 8250 is
> > > not still happen
> >
> > The 8250 driver is always a maze, so you might need to do a bunch of
> > digging. ...but it sure sounds like the console shouldn't be
> > registered until the correct ops are in place. That either means
> > getting the ops put in place earlier or deferring when the console is
> > registered...
> >
>
> Your point is pretty clear and my initial idea was to find a real fix.
> This come to avoid
> breaking existing setup but anyway I will dig in it more
>
> Michael

What about this?

--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -385,6 +385,7 @@ struct tty_driver *tty_find_polling_driver(char
*name, int *line)
int tty_line = 0;
int len;
char *str, *stp;
+ int index;

for (str = name; *str; str++)
if ((*str >= '0' && *str <= '9') || *str == ',')
@@ -406,7 +407,7 @@ struct tty_driver *tty_find_polling_driver(char
*name, int *line)
if (*stp == '\0')
stp = NULL;

- if (tty_line >= 0 && tty_line < p->num && p->ops &&
+ if (tty_line >= 0 && tty_line < p->num && p->ops &&
console_device(&index) == p &&
p->ops->poll_init && !p->ops->poll_init(p, tty_line, stp)) {
res = tty_driver_kref_get(p);
*line = tty_line;

I will send proper patch

[ 18.885348] printk: legacy console [ttyS2] disabled
[ 18.890821] 2800000.serial: ttyS2 at MMIO 0x2800000 (irq = 283,
base_baud = 3000000) is a 8250
[ 18.899727] printk: legacy console [ttyS2] enabled
[ 18.909440] printk: legacy bootconsole [ns16550a0] disabled
[ 18.923263] omap8250_probe: register uart 2800000.serial

Michael
>
> > -Doug
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> [email protected]
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> [email protected]
> http://www.amarulasolutions.com



--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com

Subject: Re: [RFC PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi

On Sat, Dec 16, 2023 at 2:45 PM Michael Nazzareno Trimarchi
<[email protected]> wrote:
>
> Hi Doug
>
> On Tue, Dec 12, 2023 at 9:54 AM Michael Nazzareno Trimarchi
> <[email protected]> wrote:
> >
> > Hi Doug
> >
> > On Mon, Dec 11, 2023 at 11:00 PM Doug Anderson <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
> > > <[email protected]> wrote:
> > > >
> > > > > 1. init_kgdboc() runs and registers the singleton kgdb "platform driver".
> > > > >
> > > > > 2. The platform driver's probe function, kgdboc_probe(), runs and
> > > > > checks to see if the console is ready by looking at the return value
> > > > > of configure_kgdboc(). If it's ready then we're good to go. If it's
> > > > > not ready then we defer.
> > > > >
> > > > > So I think the bug here is that somehow the console looks "ready"
> > > > > (because tty_find_polling_driver() can find it) but it isn't actually
> > > > > ready yet (because it crashes). That's what you need to fix.
> > > > >
> > > >
> > > > The polling driver look for uart and uart8250_core is registered and 4 fake uart
> > > > are there but there are not still replaced by platform driver that can
> > > > come later.
> > > > The try_polling find it but it's the isa-8250 driver. It means that
> > > > add_uart 8250 is
> > > > not still happen
> > >
> > > The 8250 driver is always a maze, so you might need to do a bunch of
> > > digging. ...but it sure sounds like the console shouldn't be
> > > registered until the correct ops are in place. That either means
> > > getting the ops put in place earlier or deferring when the console is
> > > registered...
> > >
> >
> > Your point is pretty clear and my initial idea was to find a real fix.
> > This come to avoid
> > breaking existing setup but anyway I will dig in it more
> >
> > Michael
>
> What about this?
>
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -385,6 +385,7 @@ struct tty_driver *tty_find_polling_driver(char
> *name, int *line)
> int tty_line = 0;
> int len;
> char *str, *stp;
> + int index;
>
> for (str = name; *str; str++)
> if ((*str >= '0' && *str <= '9') || *str == ',')
> @@ -406,7 +407,7 @@ struct tty_driver *tty_find_polling_driver(char
> *name, int *line)
> if (*stp == '\0')
> stp = NULL;
>
> - if (tty_line >= 0 && tty_line < p->num && p->ops &&
> + if (tty_line >= 0 && tty_line < p->num && p->ops &&
> console_device(&index) == p &&
> p->ops->poll_init && !p->ops->poll_init(p, tty_line, stp)) {
> res = tty_driver_kref_get(p);
> *line = tty_line;
>
> I will send proper patch
>
> [ 18.885348] printk: legacy console [ttyS2] disabled
> [ 18.890821] 2800000.serial: ttyS2 at MMIO 0x2800000 (irq = 283,
> base_baud = 3000000) is a 8250
> [ 18.899727] printk: legacy console [ttyS2] enabled
> [ 18.909440] printk: legacy bootconsole [ns16550a0] disabled
> [ 18.923263] omap8250_probe: register uart 2800000.serial

I read better the documentation is this can not work, because the
requirement can be any uart and not the console one

Micahel
>
> Michael
> >
> > > -Doug
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > [email protected]
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > [email protected]
> > http://www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> [email protected]
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> [email protected]
> http://www.amarulasolutions.com



--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com

Subject: [PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Check if port type is not PORT_UNKNOWN in the serial driver.
The kgdboc calls the tty_find_polling_driver that check
if the serial is able to use poll_init. The poll_init calls
the uart uart_poll_init that try to configure the uart with the
selected parameters. The uart must be ready and we can check it
using type as in other tty_io functions.

The crash happen for instance in am62x architecture where the 8250
register the platform driver after the 8250 core is initialized.

As shown by kdbg the iobase and membase is not configured

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

This section of the code is too early because in this case
the omap serial is not probed

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt
0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
at ./include/linux/serial_core.h:677
4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2860
5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2912
6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
at drivers/tty/serial/serial_core.c:2285
7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
at drivers/tty/serial/serial_core.c:2656
8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
at drivers/tty/tty_io.c:410
9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
at drivers/base/dd.c:800
15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
at drivers/base/dd.c:958
17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
26 do_initcalls () at init/main.c:1314
27 do_basic_setup () at init/main.c:1333
28 kernel_init_freeable () at init/main.c:1551
29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857

Signed-off-by: Michael Trimarchi <[email protected]>
---
RFC -> v1:
- refuse uart that has type PORT_UNKNOWN
---
drivers/tty/serial/serial_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f1348a509552..aa07eb894a6e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2633,7 +2633,7 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
mutex_lock(&tport->mutex);

port = uart_port_check(state);
- if (!port || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
+ if (!port || port->type = PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
ret = -1;
goto out;
}
--
2.40.1


2023-12-17 05:18:33

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi Michael,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus linus/master v6.7-rc5 next-20231215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Michael-Trimarchi/tty-serial-kgdboc-Fix-8250_-kgd-over-serial/20231217-013726
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20231216173409.1264655-1-michael%40amarulasolutions.com
patch subject: [PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial
config: i386-buildonly-randconfig-003-20231217 (https://download.01.org/0day-ci/archive/20231217/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

>> drivers/tty/serial/serial_core.c:2636:26: error: expression is not assignable
if (!port || port->type = PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
~~~~~~~~~~~~~~~~~~~ ^
1 error generated.


vim +2636 drivers/tty/serial/serial_core.c

2618
2619 static int uart_poll_init(struct tty_driver *driver, int line, char *options)
2620 {
2621 struct uart_driver *drv = driver->driver_state;
2622 struct uart_state *state = drv->state + line;
2623 enum uart_pm_state pm_state;
2624 struct tty_port *tport;
2625 struct uart_port *port;
2626 int baud = 9600;
2627 int bits = 8;
2628 int parity = 'n';
2629 int flow = 'n';
2630 int ret = 0;
2631
2632 tport = &state->port;
2633 mutex_lock(&tport->mutex);
2634
2635 port = uart_port_check(state);
> 2636 if (!port || port->type = PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
2637 ret = -1;
2638 goto out;
2639 }
2640
2641 pm_state = state->pm_state;
2642 uart_change_pm(state, UART_PM_STATE_ON);
2643
2644 if (port->ops->poll_init) {
2645 /*
2646 * We don't set initialized as we only initialized the hw,
2647 * e.g. state->xmit is still uninitialized.
2648 */
2649 if (!tty_port_initialized(tport))
2650 ret = port->ops->poll_init(port);
2651 }
2652
2653 if (!ret && options) {
2654 uart_parse_options(options, &baud, &parity, &bits, &flow);
2655 console_list_lock();
2656 ret = uart_set_options(port, NULL, baud, parity, bits, flow);
2657 console_list_unlock();
2658 }
2659 out:
2660 if (ret)
2661 uart_change_pm(state, pm_state);
2662 mutex_unlock(&tport->mutex);
2663 return ret;
2664 }
2665

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-12-17 06:43:46

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi Michael,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus linus/master v6.7-rc5 next-20231215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Michael-Trimarchi/tty-serial-kgdboc-Fix-8250_-kgd-over-serial/20231217-013726
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20231216173409.1264655-1-michael%40amarulasolutions.com
patch subject: [PATCH] tty: serial: kgdboc: Fix 8250_* kgd over serial
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20231217/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

drivers/tty/serial/serial_core.c: In function 'uart_poll_init':
>> drivers/tty/serial/serial_core.c:2636:33: error: lvalue required as left operand of assignment
2636 | if (!port || port->type = PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
| ^


vim +2636 drivers/tty/serial/serial_core.c

2618
2619 static int uart_poll_init(struct tty_driver *driver, int line, char *options)
2620 {
2621 struct uart_driver *drv = driver->driver_state;
2622 struct uart_state *state = drv->state + line;
2623 enum uart_pm_state pm_state;
2624 struct tty_port *tport;
2625 struct uart_port *port;
2626 int baud = 9600;
2627 int bits = 8;
2628 int parity = 'n';
2629 int flow = 'n';
2630 int ret = 0;
2631
2632 tport = &state->port;
2633 mutex_lock(&tport->mutex);
2634
2635 port = uart_port_check(state);
> 2636 if (!port || port->type = PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
2637 ret = -1;
2638 goto out;
2639 }
2640
2641 pm_state = state->pm_state;
2642 uart_change_pm(state, UART_PM_STATE_ON);
2643
2644 if (port->ops->poll_init) {
2645 /*
2646 * We don't set initialized as we only initialized the hw,
2647 * e.g. state->xmit is still uninitialized.
2648 */
2649 if (!tty_port_initialized(tport))
2650 ret = port->ops->poll_init(port);
2651 }
2652
2653 if (!ret && options) {
2654 uart_parse_options(options, &baud, &parity, &bits, &flow);
2655 console_list_lock();
2656 ret = uart_set_options(port, NULL, baud, parity, bits, flow);
2657 console_list_unlock();
2658 }
2659 out:
2660 if (ret)
2661 uart_change_pm(state, pm_state);
2662 mutex_unlock(&tport->mutex);
2663 return ret;
2664 }
2665

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Subject: [PATCH V2] tty: serial: kgdboc: Fix 8250_* kgd over serial

Check if port type is not PORT_UNKNOWN during poll_init.
The kgdboc calls the tty_find_polling_driver that check
if the serial is able to use poll_init. The poll_init calls
the uart uart_poll_init that try to configure the uart with the
selected boot parameters. The uart must be ready before setting
parameters. Seems that PORT_UNKNOWN is already used by other
functions in serial_core to detect uart status, so use the same
to avoid to use it in invalid state.

The crash happen for instance in am62x architecture where the 8250
register the platform driver after the 8250 core is initialized.

Follow the report crash coming from KGDB

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

This section of the code is too early because in this case
the omap serial is not probed

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt
0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
at ./include/linux/serial_core.h:677
4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2860
5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2912
6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
at drivers/tty/serial/serial_core.c:2285
7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
at drivers/tty/serial/serial_core.c:2656
8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
at drivers/tty/tty_io.c:410
9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
at drivers/base/dd.c:800
15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
at drivers/base/dd.c:958
17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
26 do_initcalls () at init/main.c:1314
27 do_basic_setup () at init/main.c:1333
28 kernel_init_freeable () at init/main.c:1551
29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857

Signed-off-by: Michael Trimarchi <[email protected]>
---
v1 -> v2:
- fix if condition during submission
- improve a bit the commit message
RFC -> v1:
- refuse uart that has type PORT_UNKNOWN

---
drivers/tty/serial/serial_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f1348a509552..9b7ed4aac77a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2633,7 +2633,7 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
mutex_lock(&tport->mutex);

port = uart_port_check(state);
- if (!port || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
+ if (!port || port->type == PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
ret = -1;
goto out;
}
--
2.40.1


2023-12-18 22:34:55

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH V2] tty: serial: kgdboc: Fix 8250_* kgd over serial

Hi,

On Sun, Dec 17, 2023 at 11:34 PM Michael Trimarchi
<[email protected]> wrote:
>
> Check if port type is not PORT_UNKNOWN during poll_init.
> The kgdboc calls the tty_find_polling_driver that check
> if the serial is able to use poll_init. The poll_init calls
> the uart uart_poll_init that try to configure the uart with the
> selected boot parameters. The uart must be ready before setting
> parameters. Seems that PORT_UNKNOWN is already used by other
> functions in serial_core to detect uart status, so use the same
> to avoid to use it in invalid state.
>
> The crash happen for instance in am62x architecture where the 8250
> register the platform driver after the 8250 core is initialized.
>
> Follow the report crash coming from KGDB
>
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1]
> _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 584 __raw_writeb(value, PCI_IOBASE + addr);
> (gdb) bt
>
> This section of the code is too early because in this case
> the omap serial is not probed
>
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1]
> _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 584 __raw_writeb(value, PCI_IOBASE + addr);
> (gdb) bt
>
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1]
> _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 584 __raw_writeb(value, PCI_IOBASE + addr);
> (gdb) bt
> 0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
> 1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
> 2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
> 3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
> at ./include/linux/serial_core.h:677
> 4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
> at drivers/tty/serial/8250/8250_port.c:2860
> 5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
> at drivers/tty/serial/8250/8250_port.c:2912
> 6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
> at drivers/tty/serial/serial_core.c:2285
> 7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
> at drivers/tty/serial/serial_core.c:2656
> 8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
> at drivers/tty/tty_io.c:410
> 9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
> 10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
> 11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
> 12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
> 13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
> 14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
> at drivers/base/dd.c:800
> 15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
> 16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
> at drivers/base/dd.c:958
> 17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
> fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
> 18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
> 19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
> 20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
> 21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
> 22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
> 23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
> 24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
> 25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
> 26 do_initcalls () at init/main.c:1314
> 27 do_basic_setup () at init/main.c:1333
> 28 kernel_init_freeable () at init/main.c:1551
> 29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
> 30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857
>
> Signed-off-by: Michael Trimarchi <[email protected]>
> ---
> v1 -> v2:
> - fix if condition during submission
> - improve a bit the commit message
> RFC -> v1:
> - refuse uart that has type PORT_UNKNOWN
>
> ---
> drivers/tty/serial/serial_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

I'm not a total expert on this code, but this seems reasonable to me.
One nit is ${SUBJECT} should probably be "kgdb over serial" instead of
"kgd over serial"

Reviewed-by: Douglas Anderson <[email protected]>


> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f1348a509552..9b7ed4aac77a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2633,7 +2633,7 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
> mutex_lock(&tport->mutex);
>
> port = uart_port_check(state);
> - if (!port || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
> + if (!port || port->type == PORT_UNKNOWN || !(port->ops->poll_get_char && port->ops->poll_put_char)) {

Another slight nit is that the above line feels a little long,
clocking in at 110 columns. I know the 80 column limit isn't so firm
these days, but if it were me I'd split it across 2 lines.

-Doug

Subject: [PATCH V3] tty: serial: kgdboc: Fix 8250_* kgdb over serial

Check if port type is not PORT_UNKNOWN during poll_init.
The kgdboc calls the tty_find_polling_driver that check
if the serial is able to use poll_init. The poll_init calls
the uart uart_poll_init that try to configure the uart with the
selected boot parameters. The uart must be ready before setting
parameters. Seems that PORT_UNKNOWN is already used by other
functions in serial_core to detect uart status, so use the same
to avoid to use it in invalid state.

The crash happen for instance in am62x architecture where the 8250
register the platform driver after the 8250 core is initialized.

Follow the report crash coming from KGDB

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

This section of the code is too early because in this case
the omap serial is not probed

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1]
_outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
584 __raw_writeb(value, PCI_IOBASE + addr);
(gdb) bt
0 _outb (addr=<optimized out>, value=<optimized out>) at ./include/asm-generic/io.h:584
1 logic_outb (value=0 '\000', addr=18446739675637874689) at lib/logic_pio.c:299
2 0xffff80008082dfcc in io_serial_out (p=0x0, offset=16760830, value=0) at drivers/tty/serial/8250/8250_port.c:416
3 0xffff80008082fe34 in serial_port_out (value=<optimized out>, offset=<optimized out>, up=<optimized out>)
at ./include/linux/serial_core.h:677
4 serial8250_do_set_termios (port=0xffff8000828ee940 <serial8250_ports+1568>, termios=0xffff80008292b93c, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2860
5 0xffff800080830064 in serial8250_set_termios (port=0xfffffbfffe800000, termios=0xffbffe, old=0x0)
at drivers/tty/serial/8250/8250_port.c:2912
6 0xffff80008082571c in uart_set_options (port=0xffff8000828ee940 <serial8250_ports+1568>, co=0x0, baud=115200, parity=110, bits=8, flow=110)
at drivers/tty/serial/serial_core.c:2285
7 0xffff800080828434 in uart_poll_init (driver=0xfffffbfffe800000, line=16760830, options=0xffff8000828f7506 <config+6> "115200n8")
at drivers/tty/serial/serial_core.c:2656
8 0xffff800080801690 in tty_find_polling_driver (name=0xffff8000828f7500 <config> "ttyS2,115200n8", line=0xffff80008292ba90)
at drivers/tty/tty_io.c:410
9 0xffff80008086c0b0 in configure_kgdboc () at drivers/tty/serial/kgdboc.c:194
10 0xffff80008086c1ec in kgdboc_probe (pdev=0xfffffbfffe800000) at drivers/tty/serial/kgdboc.c:249
11 0xffff8000808b399c in platform_probe (_dev=0xffff000000ebb810) at drivers/base/platform.c:1404
12 0xffff8000808b0b44 in call_driver_probe (drv=<optimized out>, dev=<optimized out>) at drivers/base/dd.c:579
13 really_probe (dev=0xffff000000ebb810, drv=0xffff80008277f138 <kgdboc_platform_driver+48>) at drivers/base/dd.c:658
14 0xffff8000808b0d2c in __driver_probe_device (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, dev=0xffff000000ebb810)
at drivers/base/dd.c:800
15 0xffff8000808b0eb8 in driver_probe_device (drv=0xfffffbfffe800000, dev=0xffff000000ebb810) at drivers/base/dd.c:830
16 0xffff8000808b0ff4 in __device_attach_driver (drv=0xffff80008277f138 <kgdboc_platform_driver+48>, _data=0xffff80008292bc48)
at drivers/base/dd.c:958
17 0xffff8000808ae970 in bus_for_each_drv (bus=0xfffffbfffe800000, start=0x0, data=0xffff80008292bc48,
fn=0xffff8000808b0f3c <__device_attach_driver>) at drivers/base/bus.c:457
18 0xffff8000808b1408 in __device_attach (dev=0xffff000000ebb810, allow_async=true) at drivers/base/dd.c:1030
19 0xffff8000808b16d8 in device_initial_probe (dev=0xfffffbfffe800000) at drivers/base/dd.c:1079
20 0xffff8000808af9f4 in bus_probe_device (dev=0xffff000000ebb810) at drivers/base/bus.c:532
21 0xffff8000808ac77c in device_add (dev=0xfffffbfffe800000) at drivers/base/core.c:3625
22 0xffff8000808b3428 in platform_device_add (pdev=0xffff000000ebb800) at drivers/base/platform.c:716
23 0xffff800081b5dc0c in init_kgdboc () at drivers/tty/serial/kgdboc.c:292
24 0xffff800080014db0 in do_one_initcall (fn=0xffff800081b5dba4 <init_kgdboc>) at init/main.c:1236
25 0xffff800081b0114c in do_initcall_level (command_line=<optimized out>, level=<optimized out>) at init/main.c:1298
26 do_initcalls () at init/main.c:1314
27 do_basic_setup () at init/main.c:1333
28 kernel_init_freeable () at init/main.c:1551
29 0xffff8000810271ec in kernel_init (unused=0xfffffbfffe800000) at init/main.c:1441
30 0xffff800080015e80 in ret_from_fork () at arch/arm64/kernel/entry.S:857

Reviewed-by: Douglas Anderson <[email protected]>
Signed-off-by: Michael Trimarchi <[email protected]>
---
v2 -> v3:
- add reviewed-by
- fix {SUBJECT} "kgdb over serial" instead of "kgd over serial"
- split long lines in 2 lines
v1 -> v2:
- fix if condition during submission
- improve a bit the commit message
RFC -> v1:
- refuse uart that has type PORT_UNKNOWN
---
drivers/tty/serial/serial_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f1348a509552..497be381c647 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2633,7 +2633,8 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
mutex_lock(&tport->mutex);

port = uart_port_check(state);
- if (!port || !(port->ops->poll_get_char && port->ops->poll_put_char)) {
+ if (!port || port->type == PORT_UNKNOWN ||
+ !(port->ops->poll_get_char && port->ops->poll_put_char)) {
ret = -1;
goto out;
}
--
2.40.1