All of lore.kernel.org
 help / color / mirror / Atom feed
From: Emil Kronborg <emil.kronborg@protonmail.com>
To: jirislaby@kernel.org, gregkh@linuxfoundation.org
Cc: linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev,
	Emil Kronborg <emil.kronborg@protonmail.com>,
	stable@vger.kernel.org
Subject: [PATCH v2] serial: mxs-auart: add spinlock around changing cts state
Date: Tue, 19 Mar 2024 18:02:47 +0000	[thread overview]
Message-ID: <20240319180239.69765-1-emil.kronborg@protonmail.com> (raw)

The uart_handle_cts_change() function in serial_core expects the caller
to hold uport->lock. For example, I have seen the below kernel splat,
when the Bluetooth driver is loaded on an i.MX28 board.

    [   85.119255] ------------[ cut here ]------------
    [   85.124413] WARNING: CPU: 0 PID: 27 at /drivers/tty/serial/serial_core.c:3453 uart_handle_cts_change+0xb4/0xec
    [   85.134694] Modules linked in: hci_uart bluetooth ecdh_generic ecc wlcore_sdio configfs
    [   85.143314] CPU: 0 PID: 27 Comm: kworker/u3:0 Not tainted 6.6.3-00021-gd62a2f068f92 #1
    [   85.151396] Hardware name: Freescale MXS (Device Tree)
    [   85.156679] Workqueue: hci0 hci_power_on [bluetooth]
    (...)
    [   85.191765]  uart_handle_cts_change from mxs_auart_irq_handle+0x380/0x3f4
    [   85.198787]  mxs_auart_irq_handle from __handle_irq_event_percpu+0x88/0x210
    (...)

Cc: stable@vger.kernel.org # v6.1+
Fixes: 4d90bb147ef6 ("serial: core: Document and assert lock requirements for irq helpers")
Signed-off-by: Emil Kronborg <emil.kronborg@protonmail.com>
---
Changes in v2:
- Add more of the stack trace to show why the lock is necessary. Note
  that i removed some unrelated noise for unwinding, showing and dumping
  the stack trace.
- Add Fixes tag. Note that this commit do not fix 4d90bb147ef6 ("serial:
  core: Document and assert lock requirements for irq helpers") as such,
  but it was the closest commit I could find that makes sense.
- Cc stable. Commit b0af4bcb4946 ("serial: core: Provide port lock
  wrappers") is a prerequisite. Therefore, v6.1+.

 drivers/tty/serial/mxs-auart.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 4749331fe618..1e8853eae504 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1086,11 +1086,13 @@ static void mxs_auart_set_ldisc(struct uart_port *port,
 
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 {
-	u32 istat;
+	u32 istat, stat;
 	struct mxs_auart_port *s = context;
 	u32 mctrl_temp = s->mctrl_prev;
-	u32 stat = mxs_read(s, REG_STAT);
 
+	uart_port_lock(&s->port);
+
+	stat = mxs_read(s, REG_STAT);
 	istat = mxs_read(s, REG_INTR);
 
 	/* ack irq */
@@ -1126,6 +1128,8 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 		istat &= ~AUART_INTR_TXIS;
 	}
 
+	uart_port_unlock(&s->port);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.44.0



WARNING: multiple messages have this Message-ID (diff)
From: Emil Kronborg <emil.kronborg@protonmail.com>
To: jirislaby@kernel.org, gregkh@linuxfoundation.org
Cc: linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev,
	Emil Kronborg <emil.kronborg@protonmail.com>,
	stable@vger.kernel.org
Subject: [PATCH v2] serial: mxs-auart: add spinlock around changing cts state
Date: Tue, 19 Mar 2024 18:02:47 +0000	[thread overview]
Message-ID: <20240319180239.69765-1-emil.kronborg@protonmail.com> (raw)

The uart_handle_cts_change() function in serial_core expects the caller
to hold uport->lock. For example, I have seen the below kernel splat,
when the Bluetooth driver is loaded on an i.MX28 board.

    [   85.119255] ------------[ cut here ]------------
    [   85.124413] WARNING: CPU: 0 PID: 27 at /drivers/tty/serial/serial_core.c:3453 uart_handle_cts_change+0xb4/0xec
    [   85.134694] Modules linked in: hci_uart bluetooth ecdh_generic ecc wlcore_sdio configfs
    [   85.143314] CPU: 0 PID: 27 Comm: kworker/u3:0 Not tainted 6.6.3-00021-gd62a2f068f92 #1
    [   85.151396] Hardware name: Freescale MXS (Device Tree)
    [   85.156679] Workqueue: hci0 hci_power_on [bluetooth]
    (...)
    [   85.191765]  uart_handle_cts_change from mxs_auart_irq_handle+0x380/0x3f4
    [   85.198787]  mxs_auart_irq_handle from __handle_irq_event_percpu+0x88/0x210
    (...)

Cc: stable@vger.kernel.org # v6.1+
Fixes: 4d90bb147ef6 ("serial: core: Document and assert lock requirements for irq helpers")
Signed-off-by: Emil Kronborg <emil.kronborg@protonmail.com>
---
Changes in v2:
- Add more of the stack trace to show why the lock is necessary. Note
  that i removed some unrelated noise for unwinding, showing and dumping
  the stack trace.
- Add Fixes tag. Note that this commit do not fix 4d90bb147ef6 ("serial:
  core: Document and assert lock requirements for irq helpers") as such,
  but it was the closest commit I could find that makes sense.
- Cc stable. Commit b0af4bcb4946 ("serial: core: Provide port lock
  wrappers") is a prerequisite. Therefore, v6.1+.

 drivers/tty/serial/mxs-auart.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 4749331fe618..1e8853eae504 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1086,11 +1086,13 @@ static void mxs_auart_set_ldisc(struct uart_port *port,
 
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 {
-	u32 istat;
+	u32 istat, stat;
 	struct mxs_auart_port *s = context;
 	u32 mctrl_temp = s->mctrl_prev;
-	u32 stat = mxs_read(s, REG_STAT);
 
+	uart_port_lock(&s->port);
+
+	stat = mxs_read(s, REG_STAT);
 	istat = mxs_read(s, REG_INTR);
 
 	/* ack irq */
@@ -1126,6 +1128,8 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 		istat &= ~AUART_INTR_TXIS;
 	}
 
+	uart_port_unlock(&s->port);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.44.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

             reply	other threads:[~2024-03-19 18:03 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-19 18:02 Emil Kronborg [this message]
2024-03-19 18:02 ` [PATCH v2] serial: mxs-auart: add spinlock around changing cts state Emil Kronborg
2024-03-19 18:22 ` Frank Li
2024-03-19 18:22   ` Frank Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240319180239.69765-1-emil.kronborg@protonmail.com \
    --to=emil.kronborg@protonmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=imx@lists.linux.dev \
    --cc=jirislaby@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.