[1/2] printk: nbcon: move locked_port flag to struct uart_port

  Console pointer in uart_port might be shared among multiple uart
ports. Flag port locked by nbcon should be saved in uart_port
structure instead of in console structure.

Fixes: 6424f396c49e ("printk: nbcon: Implement processing in port->lock wrapper")
Suggested-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
---
 include/linux/console.h     | 2 --
 include/linux/serial_core.h | 1 +
 kernel/printk/nbcon.c       | 8 ++++----
 3 files changed, 5 insertions(+), 6 deletions(-)
  

On 2024-01-23, Junxiao Chang <junxiao.chang@intel.com> wrote:
> Console pointer in uart_port might be shared among multiple uart
> ports.

I still want to investigate why the pointer is shared. This sounds
sloppy or dangerous.

> Flag port locked by nbcon should be saved in uart_port
> structure instead of in console structure.

If it turns out that the pointer sharing is necessary, this patch will
fix the reported problem.

Reviewed-by: John Ogness <john.ogness@linutronix.de>
  

On 2024-01-24 10:53:10 [+0106], John Ogness wrote:
> On 2024-01-23, Junxiao Chang <junxiao.chang@intel.com> wrote:
> > Console pointer in uart_port might be shared among multiple uart
> > ports.
> 
> I still want to investigate why the pointer is shared. This sounds
> sloppy or dangerous.

I have x86 a server box and PNP enumerates two UARTs (8250). Only one is
wired up but both can be specified as console=.
What do I need to do to reproduce this here? Using console= twice does
not do the trick.

Sebastian
  

> > > Console pointer in uart_port might be shared among multiple uart 
> > > ports.
> > 
> > I still want to investigate why the pointer is shared. This sounds 
> > sloppy or dangerous.

> I have x86 a server box and PNP enumerates two UARTs (8250). Only one is wired up but both can be specified as console=.
> What do I need to do to reproduce this here? Using console= twice does not do the trick.

Issue could be reproduced with our hardware every time. My cmdline is: BOOT_IMAGE=(hd0,gpt2)/boot/bzImage-linux-intel-iot-lts-rt-6.6-kernel root=PARTLABEL=primary rootwait console=ttyS0,115200 console=tty0 init=/sbin/preinit-env console=ttyS4,115200n8 console=ttyS5,115200n8

If you would like to try any debug patch with my ADL hardware, please feel free to let me know.

For console pointer sharing issue, from code logic point of view, the call chain looks like:
serial8250_register_8250_port -> uart_add_one_port -> serial_ctrl_register_port -> serial_core_register_port -> serial_core_add_one_port

In API serial_core_add_one_port, uart_port's console pointer is assigned with driver's console pointer:
	uport->cons = drv->cons;
Driver's console pointer points to static structure "univ8250_console" which is defined in 8250_core.c

That is, all 8250 serial devices' console pointer are same, they point to univ8250_console.

Below is debug log output:
sh-5.1# dmesg | grep @@@@
[    1.687121] @@@@ univ8250_console_init univ8250_console address:ffffffff935df1e0
[    3.419880] @@@@ serial8250_register_8250_port: name:ttyS4, cons pointer:ffffffff935df1e0
[    3.534954] @@@@ serial8250_register_8250_port: name:ttyS5, cons pointer:ffffffff935df1e0
[   11.971345] @@@@ serial8250_do_shutdown, curr thread:(agetty), name:ttyS0, line 0, cons pointer:ffffffff935df1e0
[   11.971506] @@@@ serial8250_do_shutdown, curr thread:(agetty), name:ttyS4, line 4, cons pointer:ffffffff935df1e0
[   11.971849] @@@@ serial8250_do_shutdown, curr thread:(agetty), name:ttyS4, line 4, cons pointer:ffffffff935df1e0
[   11.983072] @@@@ serial8250_do_shutdown, curr thread:agetty, name:ttyS4, line 4, cons pointer:ffffffff935df1e0
[   11.983595] @@@@ serial8250_do_shutdown, curr thread:(agetty), name:ttyS5, line 5, cons pointer:ffffffff935df1e0
[   11.983895] @@@@ serial8250_do_shutdown, curr thread:(agetty), name:ttyS5, line 5, cons pointer:ffffffff935df1e0
[   12.009632] @@@@ serial8250_do_shutdown, curr thread:agetty, name:ttyS5, line 5, cons pointer:ffffffff935df1e0
sh-5.1#

All cons pointers address are same "ffffffff935df1e0".

Debug patch:
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 30434718fad80..740fd7e133a28 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -755,6 +755,7 @@ static int __init univ8250_console_init(void)

        serial8250_isa_init_ports();
        register_console(&univ8250_console);
+       printk("@@@@ %s univ8250_console address:%lx\n", __func__, (unsigned long)(&univ8250_console));
        return 0;
 }
 console_initcall(univ8250_console_init);
@@ -1181,6 +1182,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
                        if (ret)
                                goto err;

+                       printk("@@@@ %s: name:%s, cons pointer:%lx\n", __func__, uart->port.name, (unsigned long)uart->port.cons);
                        ret = uart->port.line;
                } else {
                        dev_info(uart->port.dev,
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 437a7d3d34cde..93f1e548d1301 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2510,6 +2510,7 @@ void serial8250_do_shutdown(struct uart_port *port)
        struct uart_8250_port *up = up_to_u8250p(port);
        unsigned long flags;

+       printk("@@@@ %s, curr thread:%s, name:%s, line %d, cons pointer:%lx\n", __func__, current->comm, port->name, port->line, (unsigned long)port->cons);
        serial8250_rpm_get(up);
        /*
         * Disable interrupts from this port

Thanks,
Junxiao
  

On 2024-01-25 01:08:24 [+0000], Chang, Junxiao wrote:
> 
> Issue could be reproduced with our hardware every time. My cmdline is:
> BOOT_IMAGE=(hd0,gpt2)/boot/bzImage-linux-intel-iot-lts-rt-6.6-kernel
> root=PARTLABEL=primary rootwait console=ttyS0,115200 console=tty0
> init=/sbin/preinit-env console=ttyS4,115200n8 console=ttyS5,115200n8
> 
> If you would like to try any debug patch with my ADL hardware, please feel free to let me know.
> 
> For console pointer sharing issue, from code logic point of view, the call chain looks like:
> serial8250_register_8250_port -> uart_add_one_port -> serial_ctrl_register_port -> serial_core_register_port -> serial_core_add_one_port
> 
> In API serial_core_add_one_port, uart_port's console pointer is assigned with driver's console pointer:
> 	uport->cons = drv->cons;
> Driver's console pointer points to static structure "univ8250_console" which is defined in 8250_core.c
> 
> That is, all 8250 serial devices' console pointer are same, they point to univ8250_console.

Okay, So that I see this and the unbalanced acquire/ release part with
the attached patch. I leave it to John…
Btw. You don't see kernel log output on ttyS4 + ttyS5, right? Just a
login prompt.
…
> Thanks,
> Junxiao

Sebastian
  

> >  That is, all 8250 serial devices' console pointer are same, they point to univ8250_console.

> Okay, So that I see this and the unbalanced acquire/ release part with the attached patch. I leave it to John… Btw. You don't see kernel log output on ttyS4 + ttyS5, right? Just a login prompt.
>
> Sebastian

Right. At that time, only ttyS0 is console and nbcon.

Thanks,
Junxiao
  

Hi Sebastian,

On 2024-01-25, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> Okay, So that I see this and the unbalanced acquire/ release part with
> the attached patch. I leave it to John...

Please add this one patch to the 6.6-rt and later queues. The 2nd patch
in this series is not needed.

For 6.8 the fix may end up looking different, i.e. by eliminating the
struct console pointer sharing instead. But for now this patch is fully
sufficient.

Thanks.

John
  

On 2024-01-26 09:04:34 [+0106], John Ogness wrote:
> Hi Sebastian,
Hi,

> Please add this one patch to the 6.6-rt and later queues. The 2nd patch
> in this series is not needed.
Okay. I just dropped a v6.8-RT with this included and I am going to poke
Clark regarding v6.6 next week.

> Thanks.
> 
> John

Sebastian