diff mbox series

[2/2] printk: nbcon: check uart port is nbcon or not in nbcon_release

Message ID 20240123054033.183114-3-junxiao.chang@intel.com
State New
Headers
Series nbcon locking issue with v6.6.10-rt18 kernel |

Commit Message

Chang, Junxiao Jan. 23, 2024, 5:40 a.m. UTC 
  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(+)

Comments

John Ogness Jan. 24, 2024, 9:57 a.m. UTC | #1 
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
Chang, Junxiao Jan. 26, 2024, 2:33 a.m. UTC | #2 
> 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
diff mbox series

Patch

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;