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

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

[Michael Trimarchi]

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 <michael@amarulasolutions.com>
---
 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

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

[Doug Anderson]

Hi,

On Fri, Dec 8, 2023 at 1:28 PM Michael Trimarchi
<michael@amarulasolutions.com> 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 <michael@amarulasolutions.com>
> ---
>  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

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

[Michael Nazzareno Trimarchi]

Hi Doug

On Mon, Dec 11, 2023 at 10:39 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Dec 8, 2023 at 1:28 PM Michael Trimarchi
> <michael@amarulasolutions.com> 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 <michael@amarulasolutions.com>
> > ---
> >  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
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

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

[Doug Anderson]

Hi,

On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> 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

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

[Michael Nazzareno Trimarchi]

Hi Doug

On Mon, Dec 11, 2023 at 11:00 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> 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
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

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

[Michael Nazzareno Trimarchi]

Hi Doug

On Tue, Dec 12, 2023 at 9:54 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Doug
>
> On Mon, Dec 11, 2023 at 11:00 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> 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
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

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

[Michael Nazzareno Trimarchi]

Hi

On Sat, Dec 16, 2023 at 2:45 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Doug
>
> On Tue, Dec 12, 2023 at 9:54 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi Doug
> >
> > On Mon, Dec 11, 2023 at 11:00 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Dec 11, 2023 at 1:42 PM Michael Nazzareno Trimarchi
> > > <michael@amarulasolutions.com> 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
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

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

[Michael Trimarchi]

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 <michael@amarulasolutions.com>
---
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

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

[kernel test robot]

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/202312171302.vjOAqLOI-lkp@intel.com/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/202312171302.vjOAqLOI-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312171302.vjOAqLOI-lkp@intel.com/

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

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

[kernel test robot]

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/202312171453.mT4pH4uH-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/202312171453.mT4pH4uH-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312171453.mT4pH4uH-lkp@intel.com/

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

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

[Michael Trimarchi]

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 <michael@amarulasolutions.com>
---
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

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

[Doug Anderson]

Hi,

On Sun, Dec 17, 2023 at 11:34 PM Michael Trimarchi
<michael@amarulasolutions.com> 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 <michael@amarulasolutions.com>
> ---
> 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 <dianders@chromium.org>


> 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

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

[Michael Trimarchi]

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 <dianders@chromium.org>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
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