printk: nbcon: check uart port is nbcon or not in nbcon_release

  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(+)
  

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
  

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
  

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
  

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
  

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
  

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
  

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
  

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