dw8250_handle_irq() takes port's lock multiple times with no good
reason to release it in between and calls serial8250_handle_irq()
that also takes port's lock.
As serial8250_handle_irq() takes port's lock itself, create
serial8250_handle_irq_locked() that allows caller to hold port's lock
across the call. Take port's lock only once in dw8250_handle_irq() and
call serial8250_handle_irq_locked() directly.
As IIR_NO_INT check in serial8250_handle_irq() was outside of port's
lock, it has to be done already in dw8250_handle_irq().
DW UART can, in addition to IIR_NO_INT, report BUSY_DETECT (0x7) which
collided with the IIR_NO_INT (0x1) check in serial8250_handle_irq()
(because & is used instead of ==) meaning that no other work is done by
serial8250_handle_irq() during an BUSY_DETECT interrupt.
This allows reorganizing code in dw8250_handle_irq() to do both
IIR_NO_INT and BUSY_DETECT handling right at the start simplifying
the logic.
Tested-by: "Bandal, Shankar" <shankar.bandal@intel.com>
Tested-by: "Murthy, Shanth" <shanth.murthy@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/tty/serial/8250/8250_dw.c | 35 ++++++++++++++++-------------
drivers/tty/serial/8250/8250_port.c | 24 +++++++++++++-------
include/linux/serial_8250.h | 1 +
3 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 57a760b43da9..7cee89f14179 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -9,6 +9,8 @@
* LCR is written whilst busy. If it is, then a busy detect interrupt is
* raised, the LCR needs to be rewritten and the uart status register read.
*/
+#include <linux/bitfield.h>
+#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
@@ -40,6 +42,8 @@
#define RZN1_UART_RDMACR 0x110 /* DMA Control Register Receive Mode */
/* DesignWare specific register fields */
+#define DW_UART_IIR_IID GENMASK(3, 0)
+
#define DW_UART_MCR_SIRE BIT(6)
/* Renesas specific register fields */
@@ -313,7 +317,19 @@ static int dw8250_handle_irq(struct uart_port *p)
bool rx_timeout = (iir & 0x3f) == UART_IIR_RX_TIMEOUT;
unsigned int quirks = d->pdata->quirks;
unsigned int status;
- unsigned long flags;
+
+ switch (FIELD_GET(DW_UART_IIR_IID, iir)) {
+ case UART_IIR_NO_INT:
+ return 0;
+
+ case UART_IIR_BUSY:
+ /* Clear the USR */
+ serial_port_in(p, d->pdata->usr_reg);
+
+ return 1;
+ }
+
+ guard(uart_port_lock_irqsave)(p);
/*
* There are ways to get Designware-based UARTs into a state where
@@ -326,20 +342,15 @@ static int dw8250_handle_irq(struct uart_port *p)
* so we limit the workaround only to non-DMA mode.
*/
if (!up->dma && rx_timeout) {
- uart_port_lock_irqsave(p, &flags);
status = serial_lsr_in(up);
if (!(status & (UART_LSR_DR | UART_LSR_BI)))
serial_port_in(p, UART_RX);
-
- uart_port_unlock_irqrestore(p, flags);
}
/* Manually stop the Rx DMA transfer when acting as flow controller */
if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
- uart_port_lock_irqsave(p, &flags);
status = serial_lsr_in(up);
- uart_port_unlock_irqrestore(p, flags);
if (status & (UART_LSR_DR | UART_LSR_BI)) {
dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
@@ -347,17 +358,9 @@ static int dw8250_handle_irq(struct uart_port *p)
}
}
- if (serial8250_handle_irq(p, iir))
- return 1;
-
- if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
- /* Clear the USR */
- serial_port_in(p, d->pdata->usr_reg);
+ serial8250_handle_irq_locked(p, iir);
- return 1;
- }
-
- return 0;
+ return 1;
}
static void dw8250_clk_work_cb(struct work_struct *work)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f7a3c5555204..02576ed30abb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -16,6 +16,7 @@
#include <linux/ioport.h>
#include <linux/init.h>
#include <linux/irq.h>
+#include <linux/lockdep.h>
#include <linux/console.h>
#include <linux/gpio/consumer.h>
#include <linux/sysrq.h>
@@ -1782,20 +1783,16 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
}
/*
- * This handles the interrupt from one port.
+ * Context: port's lock must be held by the caller.
*/
-int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
+void serial8250_handle_irq_locked(struct uart_port *port, unsigned int iir)
{
struct uart_8250_port *up = up_to_u8250p(port);
struct tty_port *tport = &port->state->port;
bool skip_rx = false;
- unsigned long flags;
u16 status;
- if (iir & UART_IIR_NO_INT)
- return 0;
-
- uart_port_lock_irqsave(port, &flags);
+ lockdep_assert_held_once(&port->lock);
status = serial_lsr_in(up);
@@ -1828,8 +1825,19 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
else if (!up->dma->tx_running)
__stop_tx(up);
}
+}
+EXPORT_SYMBOL_GPL(serial8250_handle_irq_locked);
- uart_unlock_and_check_sysrq_irqrestore(port, flags);
+/*
+ * This handles the interrupt from one port.
+ */
+int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
+{
+ if (iir & UART_IIR_NO_INT)
+ return 0;
+
+ guard(uart_port_lock_irqsave)(port);
+ serial8250_handle_irq_locked(port, iir);
return 1;
}
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 01efdce0fda0..a95b2d143d24 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -195,6 +195,7 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl);
void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
unsigned int quot);
int fsl8250_handle_irq(struct uart_port *port);
+void serial8250_handle_irq_locked(struct uart_port *port, unsigned int iir);
int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr);
void serial8250_read_char(struct uart_8250_port *up, u16 lsr);
--
2.39.5
On Fri, Jan 23, 2026 at 07:27:36PM +0200, Ilpo Järvinen wrote: > dw8250_handle_irq() takes port's lock multiple times with no good > reason to release it in between and calls serial8250_handle_irq() > that also takes port's lock. > > As serial8250_handle_irq() takes port's lock itself, create > serial8250_handle_irq_locked() that allows caller to hold port's lock > across the call. Take port's lock only once in dw8250_handle_irq() and > call serial8250_handle_irq_locked() directly. Sounds to me that the latter can be split to a prerequisite patch. > As IIR_NO_INT check in serial8250_handle_irq() was outside of port's > lock, it has to be done already in dw8250_handle_irq(). > > DW UART can, in addition to IIR_NO_INT, report BUSY_DETECT (0x7) which > collided with the IIR_NO_INT (0x1) check in serial8250_handle_irq() > (because & is used instead of ==) meaning that no other work is done by > serial8250_handle_irq() during an BUSY_DETECT interrupt. > > This allows reorganizing code in dw8250_handle_irq() to do both > IIR_NO_INT and BUSY_DETECT handling right at the start simplifying > the logic. ... > +#include <linux/bitfield.h> > +#include <linux/bits.h> + cleanup.h > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/device.h> ... > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > #include <linux/ioport.h> > #include <linux/init.h> > #include <linux/irq.h> > +#include <linux/lockdep.h> I would still keep more order. > #include <linux/console.h> > #include <linux/gpio/consumer.h> Giving the context we have, the better place for a new inclusion is somewhere here. > #include <linux/sysrq.h> (Also perhaps sorting headers in a separate patch helps with finding better places for the future inclusions?) ... > +EXPORT_SYMBOL_GPL(serial8250_handle_irq_locked); Wondering if we can start exporting with a namespace... -- With Best Regards, Andy Shevchenko
On Sat, 24 Jan 2026, Andy Shevchenko wrote: > On Fri, Jan 23, 2026 at 07:27:36PM +0200, Ilpo Järvinen wrote: > > dw8250_handle_irq() takes port's lock multiple times with no good > > reason to release it in between and calls serial8250_handle_irq() > > that also takes port's lock. > > > > As serial8250_handle_irq() takes port's lock itself, create > > serial8250_handle_irq_locked() that allows caller to hold port's lock > > across the call. Take port's lock only once in dw8250_handle_irq() and > > call serial8250_handle_irq_locked() directly. > > Sounds to me that the latter can be split to a prerequisite patch. It's not easy to split this DW-side IIR rework and locking changes. What I can do is to make 8250_port change separately. I guess I'll do just that and only the 8250_dw change in this patch. > > As IIR_NO_INT check in serial8250_handle_irq() was outside of port's > > lock, it has to be done already in dw8250_handle_irq(). > > > > DW UART can, in addition to IIR_NO_INT, report BUSY_DETECT (0x7) which > > collided with the IIR_NO_INT (0x1) check in serial8250_handle_irq() > > (because & is used instead of ==) meaning that no other work is done by > > serial8250_handle_irq() during an BUSY_DETECT interrupt. > > > > This allows reorganizing code in dw8250_handle_irq() to do both > > IIR_NO_INT and BUSY_DETECT handling right at the start simplifying > > the logic. > > ... > > > +#include <linux/bitfield.h> > > +#include <linux/bits.h> > > + cleanup.h > > > #include <linux/clk.h> > > #include <linux/delay.h> > > #include <linux/device.h> > > ... > > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > > #include <linux/ioport.h> > > #include <linux/init.h> > > #include <linux/irq.h> > > > +#include <linux/lockdep.h> > > I would still keep more order. > > > #include <linux/console.h> > > #include <linux/gpio/consumer.h> > > Giving the context we have, the better place for a new inclusion is somewhere > here. Feels to me something that is in the eye of the beholder, but whatever, I can move it from one's "correct" place to somebody elses "correct" place. :-) > > #include <linux/sysrq.h> > > (Also perhaps sorting headers in a separate patch helps with finding better > places for the future inclusions?) Yes, later (not in this series). > ... > > > +EXPORT_SYMBOL_GPL(serial8250_handle_irq_locked); > > Wondering if we can start exporting with a namespace... I'll do that. I picked "SERIAL_8250", is that fine or should I use e.g. "8250" instead? -- i.
On Tue, Jan 27, 2026 at 02:48:30PM +0200, Ilpo Järvinen wrote: > On Sat, 24 Jan 2026, Andy Shevchenko wrote: > > On Fri, Jan 23, 2026 at 07:27:36PM +0200, Ilpo Järvinen wrote: ... > > > dw8250_handle_irq() takes port's lock multiple times with no good > > > reason to release it in between and calls serial8250_handle_irq() > > > that also takes port's lock. > > > > > > As serial8250_handle_irq() takes port's lock itself, create > > > serial8250_handle_irq_locked() that allows caller to hold port's lock > > > across the call. Take port's lock only once in dw8250_handle_irq() and > > > call serial8250_handle_irq_locked() directly. > > > > Sounds to me that the latter can be split to a prerequisite patch. > > It's not easy to split this DW-side IIR rework and locking changes. What I > can do is to make 8250_port change separately. I guess I'll do just that > and only the 8250_dw change in this patch. Yes, that's what I had in mind. ... > > > +++ b/drivers/tty/serial/8250/8250_port.c > > > > > #include <linux/ioport.h> > > > #include <linux/init.h> > > > #include <linux/irq.h> > > > > > +#include <linux/lockdep.h> > > > > I would still keep more order. > > > > > #include <linux/console.h> > > > #include <linux/gpio/consumer.h> > > > > Giving the context we have, the better place for a new inclusion is somewhere > > here. > > Feels to me something that is in the eye of the beholder, but whatever, I > can move it from one's "correct" place to somebody elses "correct" > place. :-) The idea is to have the longest ordered chain even if it's broken by some unordered pieces. In long-term it helps to cleanup without an additional churn. > > > #include <linux/sysrq.h> > > > > (Also perhaps sorting headers in a separate patch helps with finding better > > places for the future inclusions?) > > Yes, later (not in this series). Sure! ... > > > +EXPORT_SYMBOL_GPL(serial8250_handle_irq_locked); > > > > Wondering if we can start exporting with a namespace... > > I'll do that. I picked "SERIAL_8250", is that fine or should I use e.g. > "8250" instead? Since it's a string now, I have no preferences, but SERIAL_8250 sounds like slightly better choice as it has not only digits (its own namespace in the naming) and less chances to collide in the future. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.