[PATCH] tty: hvc: dcc: Detect JTAG connection only on CPU core 0

[PATCH] tty: hvc: dcc: Detect JTAG connection only on CPU core 0

[shatyuka]

From: Shatyuka <shatyuka@qq.com>

On SMP, migration can occur inside hvc_dcc_check() during
device_initcall, so the CPU writing the probe character and the CPU
reading the TXfull status may not be the same, which can hang the
system.

Commit 4419da5d5d4b1788568b ("tty: hvc: dcc: Bind driver to CPU core0
for reads and writes") cached the DCC availability detected on core 0
during console_initcall. However, this is not sufficient: if DCC is
not detected as available and another CPU later calls
hvc_dcc_check(), the check is not actually performed on core 0.

Multiple checks are unnecessary: nobody is going to plug/unplug JTAG
during kernel boot and expect it to work. Cache the return value of
hvc_dcc_check() from device_initcall and reuse the cached value during
console_initcall. This avoids complex SMP-related code and ensures the
result is always from core 0.

Signed-off-by: Shatyuka <shatyuka@qq.com>
---
 drivers/tty/hvc/hvc_dcc.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index dfc5c9c38f07..fc8dcbce9b5b 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -29,6 +29,9 @@ static DEFINE_SPINLOCK(dcc_lock);
 static DEFINE_KFIFO(inbuf, u8, DCC_INBUF_SIZE);
 static DEFINE_KFIFO(outbuf, u8, DCC_OUTBUF_SIZE);
 
+/* Cached result of hvc_dcc_check() */
+static bool dcc_core0_available;
+
 static void dcc_uart_console_putchar(struct uart_port *port, u8 ch)
 {
 	while (__dcc_getstatus() & DCC_STATUS_TX)
@@ -90,36 +93,18 @@ static ssize_t hvc_dcc_get_chars(uint32_t vt, u8 *buf, size_t count)
 }
 
 /*
- * Check if the DCC is enabled. If CONFIG_HVC_DCC_SERIALIZE_SMP is enabled,
- * then we assume then this function will be called first on core0. That way,
- * dcc_core0_available will be true only if it's available on core0.
+ * Check if the DCC is enabled. This function will be called once on core 0.
  */
 static bool hvc_dcc_check(void)
 {
 	unsigned long time = jiffies + (HZ / 10);
-	static bool dcc_core0_available;
-
-	/*
-	 * If we're not on core 0, but we previously confirmed that DCC is
-	 * active, then just return true.
-	 */
-	int cpu = get_cpu();
-
-	if (IS_ENABLED(CONFIG_HVC_DCC_SERIALIZE_SMP) && cpu && dcc_core0_available) {
-		put_cpu();
-		return true;
-	}
-
-	put_cpu();
 
 	/* Write a test character to check if it is handled */
 	__dcc_putchar('\n');
 
 	while (time_is_after_jiffies(time)) {
-		if (!(__dcc_getstatus() & DCC_STATUS_TX)) {
-			dcc_core0_available = true;
+		if (!(__dcc_getstatus() & DCC_STATUS_TX))
 			return true;
-		}
 	}
 
 	return false;
@@ -265,7 +250,8 @@ static int __init hvc_dcc_console_init(void)
 {
 	int ret;
 
-	if (!hvc_dcc_check())
+	dcc_core0_available = hvc_dcc_check();
+	if (!dcc_core0_available)
 		return -ENODEV;
 
 	/* Returns -1 if error */
@@ -279,7 +265,7 @@ static int __init hvc_dcc_init(void)
 {
 	struct hvc_struct *p;
 
-	if (!hvc_dcc_check())
+	if (!dcc_core0_available)
 		return -ENODEV;
 
 	if (IS_ENABLED(CONFIG_HVC_DCC_SERIALIZE_SMP)) {
-- 
2.36.1

Re: [PATCH] tty: hvc: dcc: Detect JTAG connection only on CPU core 0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tty: hvc: dcc: Meaningful code and commit message mismatch
--

commit 0189cf9a56296b2c974669020e309b13432dff40
Author: Shatyuka <shatyuka@qq.com>

tty: hvc: dcc: Detect JTAG connection only on CPU core 0

This commit aims to prevent system hangs caused by SMP migration during JTAG
DCC detection by caching the availability status. It simplifies the logic by
performing the check once and reusing the cached value.

>    Multiple checks are unnecessary: nobody is going to plug/unplug JTAG
>    during kernel boot and expect it to work. Cache the return value of
>    hvc_dcc_check() from device_initcall and reuse the cached value during
>    console_initcall. This avoids complex SMP-related code and ensures the
>    result is always from core 0.

[Severity: Medium]
Is the direction of the caching described correctly here?

The code implementation seems to do the opposite: it caches the value during
console_initcall and reuses it during device_initcall.

> diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
> index dfc5c9c38f074..fc8dcbce9b5b1 100644
> --- a/drivers/tty/hvc/hvc_dcc.c
> +++ b/drivers/tty/hvc/hvc_dcc.c

[ ... ]

> @@ -265,7 +250,8 @@ static int __init hvc_dcc_console_init(void)
>  {
>  	int ret;
>  
> -	if (!hvc_dcc_check())
> +	dcc_core0_available = hvc_dcc_check();
> +	if (!dcc_core0_available)
>  		return -ENODEV;

[Severity: Medium]
Is the return value of hvc_dcc_check() actually cached here in
hvc_dcc_console_init() during console_initcall, contradicting the commit
message?

>  
>  	/* Returns -1 if error */
> @@ -279,7 +265,7 @@ static int __init hvc_dcc_init(void)
>  {
>  	struct hvc_struct *p;
>  
> -	if (!hvc_dcc_check())
> +	if (!dcc_core0_available)
>  		return -ENODEV;

[Severity: Medium]
Could this be the exact reverse of what is stated in the commit message,
since the cached value is reused here in hvc_dcc_init() during
device_initcall?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_323D31BEC6A1CD2A843E2E48242C774ECE09@qq.com?part=1