* [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release @ 2024-01-17 6:52 Junxiao Chang 2024-01-17 8:23 ` John Ogness 0 siblings, 1 reply; 21+ messages in thread From: Junxiao Chang @ 2024-01-17 6:52 UTC (permalink / raw) To: bigeasy, tglx, rostedt, linux-kernel Cc: john.ogness, hao3.li, lili.li, jianfeng.gao, linux-rt-users Different uart ports might have same console pointer, not all of uart ports are nbcon. When uart port is shutdown, only release nbcon if it is nbcon. There is same nbcon checking in API nbcon_acquire. Fixes: 6424f396c49e ("printk: nbcon: Implement processing in port->lock wrapper") Signed-off-by: Junxiao Chang <junxiao.chang@intel.com> --- kernel/printk/nbcon.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index 1b1b585b1675b..e53b8bebbb57e 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1623,6 +1623,9 @@ void nbcon_release(struct uart_port *up) .prio = NBCON_PRIO_NORMAL, }; + if (!uart_is_nbcon(up)) + return; + if (!con->locked_port) return; -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 6:52 [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release Junxiao Chang @ 2024-01-17 8:23 ` John Ogness 2024-01-17 8:45 ` Chang, Junxiao 0 siblings, 1 reply; 21+ messages in thread From: John Ogness @ 2024-01-17 8:23 UTC (permalink / raw) To: Junxiao Chang, bigeasy, tglx, rostedt, linux-kernel Cc: hao3.li, lili.li, jianfeng.gao, linux-rt-users On 2024-01-17, Junxiao Chang <junxiao.chang@intel.com> wrote: > Different uart ports might have same console pointer, Please explain how this is possible. It would be a major bug. John ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 8:23 ` John Ogness @ 2024-01-17 8:45 ` Chang, Junxiao 2024-01-17 10:03 ` John Ogness 0 siblings, 1 reply; 21+ messages in thread From: Chang, Junxiao @ 2024-01-17 8:45 UTC (permalink / raw) To: John Ogness, bigeasy, tglx, rostedt, linux-kernel Cc: Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users There are several serial ports in one Intel ADL hardware, they are enumerated as ttyS0, ttyS1, ttyS4, and so on. Multiple console options might be appended to kernel command line. For example, "console=ttyS0,115200n8 console=ttyS4,115200n8 console=ttyS5,115200n8". In this case, several uarts "cons" pointers are same. During booting up process, login thread "agetty" might shutdown all uart. All uart ports could release nbcon lock even if they are "non nbcon" console. However, in "nbcon_acquire" API, if uart is not nbcon, it returns directly didn't lock anything. This patch fixes a boot hang issue which could be reproduced every time with Intel ADL hardware/v6.6.7-rt18 kernel. BTW, this issue couldn't be reproduced with v6.6.7-rt17 kernel. Thanks, Junxiao -----Original Message----- From: John Ogness <john.ogness@linutronix.de> Sent: Wednesday, January 17, 2024 4:24 PM To: Chang, Junxiao <junxiao.chang@intel.com>; bigeasy@linutronix.de; tglx@linutronix.de; rostedt@goodmis.org; linux-kernel@vger.kernel.org Cc: Li, Hao3 <hao3.li@intel.com>; Li, Lili <lili.li@intel.com>; Gao, Jianfeng <jianfeng.gao@intel.com>; linux-rt-users@vger.kernel.org Subject: Re: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release On 2024-01-17, Junxiao Chang <junxiao.chang@intel.com> wrote: > Different uart ports might have same console pointer, Please explain how this is possible. It would be a major bug. John ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 8:45 ` Chang, Junxiao @ 2024-01-17 10:03 ` John Ogness 2024-01-17 10:24 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 21+ messages in thread From: John Ogness @ 2024-01-17 10:03 UTC (permalink / raw) To: Chang, Junxiao, bigeasy, tglx, rostedt, linux-kernel Cc: Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users On 2024-01-17, "Chang, Junxiao" <junxiao.chang@intel.com> wrote: > There are several serial ports in one Intel ADL hardware, they are > enumerated as ttyS0, ttyS1, ttyS4, and so on. Multiple console options > might be appended to kernel command line. For example, > "console=ttyS0,115200n8 console=ttyS4,115200n8 > console=ttyS5,115200n8". > > In this case, several uarts "cons" pointers are same. Typically a UART driver will register the console structure in the driver's initcall(), which is only called once per driver. This is why it is usually not possible to have multiple UART consoles. If a driver _does_ allow registering consoles for multiple devices, then it must allocate separate console structs for each registration. Note that register_console() will generate a warning and abort if a driver attempts to register the same console struct twice: WARN(con == newcon, "console '%s%d' already registered\n", con->name, con->index) So I ask again. Please explain how this is possible. John ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 10:03 ` John Ogness @ 2024-01-17 10:24 ` Sebastian Andrzej Siewior 2024-01-17 13:08 ` Chang, Junxiao 0 siblings, 1 reply; 21+ messages in thread From: Sebastian Andrzej Siewior @ 2024-01-17 10:24 UTC (permalink / raw) To: John Ogness Cc: Chang, Junxiao, tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users On 2024-01-17 11:09:24 [+0106], John Ogness wrote: > On 2024-01-17, "Chang, Junxiao" <junxiao.chang@intel.com> wrote: > > There are several serial ports in one Intel ADL hardware, they are > > enumerated as ttyS0, ttyS1, ttyS4, and so on. Multiple console options > > might be appended to kernel command line. For example, > > "console=ttyS0,115200n8 console=ttyS4,115200n8 > > console=ttyS5,115200n8". > > > > In this case, several uarts "cons" pointers are same. > > So I ask again. Please explain how this is possible. I have here | 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A | 00:04: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A and I have | root 2315 0.0 0.0 5480 1792 ttyS0 Ss+ 11:19 0:00 /sbin/agetty -o -p -- \u --keep-baud 115200,57600,38400,9600 - vt220 | root 2502 0.1 0.0 5480 1792 ttyS1 Ss+ 11:20 0:00 /sbin/agetty -o -p -- \u --keep-baud 115200,57600,38400,9600 - vt220 and I can stop both of them without any trouble. Can this be reproduced on an ordinary x86 hardware given they have more than one UART (up to four). Is any of this ADL hardware upstream? > John Sebastian ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 10:24 ` Sebastian Andrzej Siewior @ 2024-01-17 13:08 ` Chang, Junxiao 2024-01-17 13:42 ` John Ogness 0 siblings, 1 reply; 21+ messages in thread From: Chang, Junxiao @ 2024-01-17 13:08 UTC (permalink / raw) To: Sebastian Andrzej Siewior, John Ogness Cc: tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users Hi John, As you mentioned, same console driver is only registered once. 8250 console driver is registered once, its "struct console *newcon" parameter is address of "univ8250_console" which is defined in drivers\tty\serial\8250\8250_core.c. However, in each serial port device is registered, their cons pointer( "struct console *cons;" in "struct uart_port") will be assigned with same cons in API serial_core_add_one_port: uport->cons = drv->cons; That is, multiple similar 8250 uart_port devices have same console pointer which points to above univ8250_console. Hi Sebastain, The ADL hardware I used has two UART devices, one is lpss 8250, another is 8250_dw. Usually there is no serial port in consumer product. Maybe there is serial port in Intel ADL industrial product. With my hardware, hang issue could be reproduced every time with 6.6.7-rt18 if serial console is enabled. If you or John need me to run some test build and get debug log with my hardware, please feel free to let me know. The main problem is that there is "nbcon" checking in nbcon_acquire, but no this checking in nbcon_release. It makes nbcon lock not balance. My patch is just to add same "uart_is_nbcon" checking in nb_release. void nbcon_acquire(struct uart_port *up) { ... if (!uart_is_nbcon(up)) return; ... Thanks, Junxiao -----Original Message----- From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Sent: Wednesday, January 17, 2024 6:25 PM To: John Ogness <john.ogness@linutronix.de> Cc: Chang, Junxiao <junxiao.chang@intel.com>; tglx@linutronix.de; rostedt@goodmis.org; linux-kernel@vger.kernel.org; Li, Hao3 <hao3.li@intel.com>; Li, Lili <lili.li@intel.com>; Gao, Jianfeng <jianfeng.gao@intel.com>; linux-rt-users@vger.kernel.org Subject: Re: RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release On 2024-01-17 11:09:24 [+0106], John Ogness wrote: > On 2024-01-17, "Chang, Junxiao" <junxiao.chang@intel.com> wrote: > > There are several serial ports in one Intel ADL hardware, they are > > enumerated as ttyS0, ttyS1, ttyS4, and so on. Multiple console > > options might be appended to kernel command line. For example, > > "console=ttyS0,115200n8 console=ttyS4,115200n8 > > console=ttyS5,115200n8". > > > > In this case, several uarts "cons" pointers are same. > > So I ask again. Please explain how this is possible. I have here | 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A | 00:04: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A and I have | root 2315 0.0 0.0 5480 1792 ttyS0 Ss+ 11:19 0:00 /sbin/agetty -o -p -- \u --keep-baud 115200,57600,38400,9600 - vt220 | root 2502 0.1 0.0 5480 1792 ttyS1 Ss+ 11:20 0:00 /sbin/agetty -o -p -- \u --keep-baud 115200,57600,38400,9600 - vt220 and I can stop both of them without any trouble. Can this be reproduced on an ordinary x86 hardware given they have more than one UART (up to four). Is any of this ADL hardware upstream? > John Sebastian ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 13:08 ` Chang, Junxiao @ 2024-01-17 13:42 ` John Ogness 2024-01-23 3:05 ` Chang, Junxiao 0 siblings, 1 reply; 21+ messages in thread From: John Ogness @ 2024-01-17 13:42 UTC (permalink / raw) To: Chang, Junxiao, Sebastian Andrzej Siewior Cc: tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users On 2024-01-17, "Chang, Junxiao" <junxiao.chang@intel.com> wrote: > As you mentioned, same console driver is only registered once. 8250 > console driver is registered once, its "struct console *newcon" > parameter is address of "univ8250_console" which is defined in > drivers\tty\serial\8250\8250_core.c. > > However, in each serial port device is registered, their cons pointer( > "struct console *cons;" in "struct uart_port") will be assigned with > same cons in API serial_core_add_one_port: > > uport->cons = drv->cons; > > That is, multiple similar 8250 uart_port devices have same console > pointer which points to above univ8250_console. Ah. This explains the "(port)->cons->index == (port)->line" check in uart_console(). Thank you for clarifying. > BTW, this issue couldn't be reproduced with v6.6.7-rt17 kernel. The reason for the change is because console registration/unregistration is not related to the port lock. There is a potential race condition if nbcon unlocking is based on the UART port still being registered as a console. We could move the @locked_port flag to the struct uart_port. (Probably renaming it to @nbcon_locked_port.) I think that would be the appropriate fix. John ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-17 13:42 ` John Ogness @ 2024-01-23 3:05 ` Chang, Junxiao 2024-01-23 5:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel Junxiao Chang 0 siblings, 1 reply; 21+ messages in thread From: Chang, Junxiao @ 2024-01-23 3:05 UTC (permalink / raw) To: John Ogness, Sebastian Andrzej Siewior Cc: tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users Your idea makes sense. We tried to implement a new patch according to your idea, it could fix our booting hang problem. We will push patch for review soon. BTW, it is better to have " uart_is_nbcon " checking in nbcon_release as well as it in nbcon_acquire. 😊 Two patches will be pushed together for review. > We could move the @locked_port flag to the struct uart_port. (Probably renaming it to @nbcon_locked_port.) I think that would be the appropriate fix. > John ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel 2024-01-23 3:05 ` Chang, Junxiao @ 2024-01-23 5:40 ` Junxiao Chang 2024-01-23 5:40 ` [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port Junxiao Chang ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Junxiao Chang @ 2024-01-23 5:40 UTC (permalink / raw) To: bigeasy, tglx, rostedt, linux-kernel Cc: john.ogness, hao3.li, lili.li, jianfeng.gao, linux-rt-users There are two serial port devices in one Intel ADL hardware, one is 8250 lpss, another is 8250 dw. Multiple uart devices are enumerated as ttyS0, ttyS4, ttyS5,... With 6.6.10 rt18 kernel, booting hangs in nbcon_release if console is enabled by appending "console=ttySx,115200n8" to kernel command line. According to nbcon author John's suggestion, lock flag is moved from console structure to uart_port. Another patch is to add uart_is_nbcon checking in nbcon_release. Junxiao Chang (2): printk: nbcon: move locked_port flag to struct uart_port printk: nbcon: check uart port is nbcon or not in nbcon_release include/linux/console.h | 2 -- include/linux/serial_core.h | 1 + kernel/printk/nbcon.c | 11 +++++++---- 3 files changed, 8 insertions(+), 6 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-23 5:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel Junxiao Chang @ 2024-01-23 5:40 ` Junxiao Chang 2024-01-24 9:47 ` John Ogness 2024-01-23 5:40 ` [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release Junxiao Chang 2024-01-24 9:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel John Ogness 2 siblings, 1 reply; 21+ messages in thread From: Junxiao Chang @ 2024-01-23 5:40 UTC (permalink / raw) To: bigeasy, tglx, rostedt, linux-kernel Cc: john.ogness, hao3.li, lili.li, jianfeng.gao, linux-rt-users 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(-) diff --git a/include/linux/console.h b/include/linux/console.h index f8a0628678886..1eb9580e9b18a 100644 --- a/include/linux/console.h +++ b/include/linux/console.h @@ -304,7 +304,6 @@ struct nbcon_write_context { * @nbcon_state: State for nbcon consoles * @nbcon_seq: Sequence number of the next record for nbcon to print * @pbufs: Pointer to nbcon private buffer - * @locked_port: True, if the port lock is locked by nbcon * @kthread: Printer kthread for this console * @rcuwait: RCU-safe wait object for @kthread waking * @irq_work: Defer @kthread waking to IRQ work context @@ -338,7 +337,6 @@ struct console { atomic_t __private nbcon_state; atomic_long_t __private nbcon_seq; struct printk_buffers *pbufs; - bool locked_port; struct task_struct *kthread; struct rcuwait rcuwait; struct irq_work irq_work; diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index 245c11753effd..b2221a50fcb29 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -488,6 +488,7 @@ struct uart_port { struct uart_icount icount; /* statistics */ struct console *cons; /* struct console, if any */ + bool nbcon_locked_port; /* True, if the port is locked by nbcon */ /* flags must be updated while holding port mutex */ upf_t flags; diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index 1b1b585b1675b..b53d93585ee71 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1586,7 +1586,7 @@ void nbcon_acquire(struct uart_port *up) if (!uart_is_nbcon(up)) return; - WARN_ON_ONCE(con->locked_port); + WARN_ON_ONCE(up->nbcon_locked_port); do { do { @@ -1597,7 +1597,7 @@ void nbcon_acquire(struct uart_port *up) } while (!nbcon_context_enter_unsafe(&ctxt)); - con->locked_port = true; + up->nbcon_locked_port = true; } EXPORT_SYMBOL_GPL(nbcon_acquire); @@ -1623,13 +1623,13 @@ void nbcon_release(struct uart_port *up) .prio = NBCON_PRIO_NORMAL, }; - if (!con->locked_port) + if (!up->nbcon_locked_port) return; if (nbcon_context_exit_unsafe(&ctxt)) nbcon_context_release(&ctxt); - con->locked_port = false; + up->nbcon_locked_port = false; } EXPORT_SYMBOL_GPL(nbcon_release); -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-23 5:40 ` [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port Junxiao Chang @ 2024-01-24 9:47 ` John Ogness 2024-01-24 10:05 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 21+ messages in thread From: John Ogness @ 2024-01-24 9:47 UTC (permalink / raw) To: Junxiao Chang, bigeasy, tglx, rostedt, linux-kernel Cc: hao3.li, lili.li, jianfeng.gao, linux-rt-users 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> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-24 9:47 ` John Ogness @ 2024-01-24 10:05 ` Sebastian Andrzej Siewior 2024-01-25 1:08 ` Chang, Junxiao 0 siblings, 1 reply; 21+ messages in thread From: Sebastian Andrzej Siewior @ 2024-01-24 10:05 UTC (permalink / raw) To: John Ogness Cc: Junxiao Chang, tglx, rostedt, linux-kernel, hao3.li, lili.li, jianfeng.gao, linux-rt-users 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-24 10:05 ` Sebastian Andrzej Siewior @ 2024-01-25 1:08 ` Chang, Junxiao 2024-01-25 13:35 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 21+ messages in thread From: Chang, Junxiao @ 2024-01-25 1:08 UTC (permalink / raw) To: Sebastian Andrzej Siewior, John Ogness Cc: tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users > > > 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: RE: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-25 1:08 ` Chang, Junxiao @ 2024-01-25 13:35 ` Sebastian Andrzej Siewior 2024-01-25 23:20 ` Chang, Junxiao 2024-01-26 7:58 ` John Ogness 0 siblings, 2 replies; 21+ messages in thread From: Sebastian Andrzej Siewior @ 2024-01-25 13:35 UTC (permalink / raw) To: Chang, Junxiao Cc: John Ogness, tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: RE: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-25 13:35 ` Sebastian Andrzej Siewior @ 2024-01-25 23:20 ` Chang, Junxiao 2024-01-26 7:58 ` John Ogness 1 sibling, 0 replies; 21+ messages in thread From: Chang, Junxiao @ 2024-01-25 23:20 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: John Ogness, tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-25 13:35 ` Sebastian Andrzej Siewior 2024-01-25 23:20 ` Chang, Junxiao @ 2024-01-26 7:58 ` John Ogness 2024-01-26 16:39 ` Sebastian Andrzej Siewior 1 sibling, 1 reply; 21+ messages in thread From: John Ogness @ 2024-01-26 7:58 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Chang, Junxiao Cc: tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port 2024-01-26 7:58 ` John Ogness @ 2024-01-26 16:39 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 21+ messages in thread From: Sebastian Andrzej Siewior @ 2024-01-26 16:39 UTC (permalink / raw) To: John Ogness Cc: Chang, Junxiao, tglx, rostedt, linux-kernel, Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-23 5:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel Junxiao Chang 2024-01-23 5:40 ` [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port Junxiao Chang @ 2024-01-23 5:40 ` Junxiao Chang 2024-01-24 9:57 ` John Ogness 2024-01-24 9:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel John Ogness 2 siblings, 1 reply; 21+ messages in thread From: Junxiao Chang @ 2024-01-23 5:40 UTC (permalink / raw) To: bigeasy, tglx, rostedt, linux-kernel Cc: john.ogness, hao3.li, lili.li, jianfeng.gao, linux-rt-users Try to release nbcon only if current uart port is nbcon, as it does in nbcon_acquire. Signed-off-by: Junxiao Chang <junxiao.chang@intel.com> --- kernel/printk/nbcon.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index b53d93585ee71..d8c6f30adde8b 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1623,6 +1623,9 @@ void nbcon_release(struct uart_port *up) .prio = NBCON_PRIO_NORMAL, }; + if (!uart_is_nbcon(up)) + return; + if (!up->nbcon_locked_port) return; -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-23 5:40 ` [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release Junxiao Chang @ 2024-01-24 9:57 ` John Ogness 2024-01-26 2:33 ` Chang, Junxiao 0 siblings, 1 reply; 21+ messages in thread From: John Ogness @ 2024-01-24 9:57 UTC (permalink / raw) To: Junxiao Chang, bigeasy, tglx, rostedt, linux-kernel Cc: hao3.li, lili.li, jianfeng.gao, linux-rt-users On 2024-01-23, Junxiao Chang <junxiao.chang@intel.com> wrote: > Try to release nbcon only if current uart port is nbcon, as it does > in nbcon_acquire. The release needs to undo what acquire did. Why should it have its own checks that would cause it to _not_ undo what acquire did? Keep in mind that an nbcon console could be unregistered while another CPU is holding the nbcon lock. The port lock (and nbcon lock) are protecting access to the hardware. They are not related to console registration/unregistration. John ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release 2024-01-24 9:57 ` John Ogness @ 2024-01-26 2:33 ` Chang, Junxiao 0 siblings, 0 replies; 21+ messages in thread From: Chang, Junxiao @ 2024-01-26 2:33 UTC (permalink / raw) To: John Ogness, bigeasy, tglx, rostedt, linux-kernel Cc: Li, Hao3, Li, Lili, Gao, Jianfeng, linux-rt-users > The release needs to undo what acquire did. Why should it have its own checks that would cause it to _not_ undo what acquire did? > I agree with you that release needs to be undo what acquire did. The problem is sometimes it might do release while it didn't acquire. In nbcon_acquire, it checks current uart_port is nbcon or not. If it is not nbcon, it returns without locking anything. So in nbcon_release, it should have this checking as well. If current uart_port is not nbcon, it should not do any lock release, this is reason I added this checking in nbcon_release. For example, there are two uart port ttyS0 and ttyS4, ttyS0 is console and nbcon. ttyS4 is not console or nbcon. When ttyS4 uart port is shutdown, it firstly calls nbcon_acquire, then calls nbcon_release in "serial8250_do_shutdown". During nbcon_acquire, it locks nothing because current uart is not nbcon(There is uart_is_nbcon checking in nbcon_acquire). When it calls nbcon_release, it should not release anything either because it is not nbcon - it doesn't lock anything in nbcon_acquire. But with current code logic, it will release nbcon lock which should not be released. Regards, Junxiao ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel 2024-01-23 5:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel Junxiao Chang 2024-01-23 5:40 ` [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port Junxiao Chang 2024-01-23 5:40 ` [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release Junxiao Chang @ 2024-01-24 9:40 ` John Ogness 2 siblings, 0 replies; 21+ messages in thread From: John Ogness @ 2024-01-24 9:40 UTC (permalink / raw) To: Junxiao Chang, bigeasy, tglx, rostedt, linux-kernel Cc: hao3.li, lili.li, jianfeng.gao, linux-rt-users On 2024-01-23, Junxiao Chang <junxiao.chang@intel.com> wrote: > There are two serial port devices in one Intel ADL hardware, one is > 8250 lpss, another is 8250 dw. Multiple uart devices are enumerated as > ttyS0, ttyS4, ttyS5,... With 6.6.10 rt18 kernel, booting hangs in > nbcon_release if console is enabled by appending > "console=ttySx,115200n8" to kernel command line. According to nbcon > author John's suggestion, lock flag is moved from console structure to > uart_port. Another patch is to add uart_is_nbcon checking in > nbcon_release. Isn't the real issue that the console pointer is copied to device that are not consoles? I am wondering why that is. Is it possible to dynamically switch the console index during runtime? If not, I think a proper fix would be to only assign @cons if it actually registered as a console. This would also simplify the uart_console() macro. It is critical that a struct console is not shared by multiple devices. I do not like the idea (or see the point) of having non-console devices store a struct console pointer that is registered with another device. I will take a closer look at that. John ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-01-26 16:39 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-17 6:52 [PATCH] printk: nbcon: check uart port is nbcon or not in nbcon_release Junxiao Chang 2024-01-17 8:23 ` John Ogness 2024-01-17 8:45 ` Chang, Junxiao 2024-01-17 10:03 ` John Ogness 2024-01-17 10:24 ` Sebastian Andrzej Siewior 2024-01-17 13:08 ` Chang, Junxiao 2024-01-17 13:42 ` John Ogness 2024-01-23 3:05 ` Chang, Junxiao 2024-01-23 5:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel Junxiao Chang 2024-01-23 5:40 ` [PATCH 1/2] printk: nbcon: move locked_port flag to struct uart_port Junxiao Chang 2024-01-24 9:47 ` John Ogness 2024-01-24 10:05 ` Sebastian Andrzej Siewior 2024-01-25 1:08 ` Chang, Junxiao 2024-01-25 13:35 ` Sebastian Andrzej Siewior 2024-01-25 23:20 ` Chang, Junxiao 2024-01-26 7:58 ` John Ogness 2024-01-26 16:39 ` Sebastian Andrzej Siewior 2024-01-23 5:40 ` [PATCH 2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release Junxiao Chang 2024-01-24 9:57 ` John Ogness 2024-01-26 2:33 ` Chang, Junxiao 2024-01-24 9:40 ` [PATCH 0/2] nbcon locking issue with v6.6.10-rt18 kernel John Ogness
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).