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.
Take port's lock only once in dw8250_handle_irq() and use
serial8250_handle_irq_locked() to avoid releasing port's lock in
between.
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>
Cc: stable@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/tty/serial/8250/8250_dw.c | 37 ++++++++++++++++++-------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 7500b1ff1ac1..964750d17186 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -9,6 +9,9 @@
* 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/cleanup.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
@@ -40,6 +43,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 */
@@ -312,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
@@ -325,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);
@@ -346,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)
@@ -858,6 +862,7 @@ static struct platform_driver dw8250_platform_driver = {
module_platform_driver(dw8250_platform_driver);
+MODULE_IMPORT_NS("SERIAL_8250");
MODULE_AUTHOR("Jamie Iles");
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Synopsys DesignWare 8250 serial port driver");
--
2.39.5
On Wed, Jan 28, 2026 at 12:52:58PM +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.
>
> Take port's lock only once in dw8250_handle_irq() and use
> serial8250_handle_irq_locked() to avoid releasing port's lock in
> between.
>
> 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.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
...
> if (!up->dma && rx_timeout) {
> status = serial_lsr_in(up);
>
> if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> serial_port_in(p, UART_RX);
> }
>
> /* 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) {
> status = serial_lsr_in(up);
>
> if (status & (UART_LSR_DR | UART_LSR_BI)) {
> dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
...
> }
> }
Looks like now (perhaps in a separate change) this may be refactored even more.
if (rx_timeout) {
status = serial_lsr_in(up);
// Although not sure about moving the above read out from the specific conditions.
if (up->dma && (status & (UART_LSR_DR | UART_LSR_BI))) {
/* Manually stop the Rx DMA transfer when acting as flow controller */
if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma->rx_running) {
dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
dw8250_writel_ext(p, DW_UART_DMASA, 1);
} else if (!up->dma && !(status & (UART_LSR_DR | UART_LSR_BI)))
/* ... */
serial_port_in(p, UART_RX);
}
}
--
With Best Regards,
Andy Shevchenko
On Wed, 28 Jan 2026, Andy Shevchenko wrote:
> On Wed, Jan 28, 2026 at 12:52:58PM +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.
> >
> > Take port's lock only once in dw8250_handle_irq() and use
> > serial8250_handle_irq_locked() to avoid releasing port's lock in
> > between.
> >
> > 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.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> ...
>
> > if (!up->dma && rx_timeout) {
> > status = serial_lsr_in(up);
> >
> > if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> > serial_port_in(p, UART_RX);
> > }
> >
> > /* 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) {
> > status = serial_lsr_in(up);
> >
> > if (status & (UART_LSR_DR | UART_LSR_BI)) {
> > dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
> ...
> > }
> > }
>
> Looks like now (perhaps in a separate change) this may be refactored even more.
Definitely, as was noted in the coverletter. ;-)
--
i.
>
> if (rx_timeout) {
> status = serial_lsr_in(up);
>
> // Although not sure about moving the above read out from the specific conditions.
>
> if (up->dma && (status & (UART_LSR_DR | UART_LSR_BI))) {
> /* Manually stop the Rx DMA transfer when acting as flow controller */
> if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma->rx_running) {
> dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
> dw8250_writel_ext(p, DW_UART_DMASA, 1);
> } else if (!up->dma && !(status & (UART_LSR_DR | UART_LSR_BI)))
> /* ... */
> serial_port_in(p, UART_RX);
> }
> }
>
>
© 2016 - 2026 Red Hat, Inc.