[PATCH 0/6] 8250 DW UART fixes when under constant Rx pressure

[PATCH 0/6] 8250 DW UART fixes when under constant Rx pressure

[Ilpo Järvinen]

Hi all,

Here are fixes to 8250 DW UART conditions that mostly occur in scenarios
under constant Rx pressure which are made complicated by BUSY handling
of DW UARTs (used when !uart_16550_compatible).

A few of the changes touch also 8250_port but it's mostly moving existing
code around (except for the extra synchronize_irq() in shutdown).

I'll do UART_IIR_RX_TIMEOUT move to switch/case separately from this
fix series.

Ilpo Järvinen (6):
  serial: 8250: Protect LCR write in shutdown
  serial: 8250_dw: Avoid unnecessary LCR writes
  serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling
  serial: 8250_dw: Rework IIR_NO_INT handling to stop interrupt storm
  serial: 8250: Add late synchronize_irq() to shutdown to handle DW UART
    BUSY
  serial: 8250_dw: Ensure BUSY is deasserted

 drivers/tty/serial/8250/8250.h      |  25 +++
 drivers/tty/serial/8250/8250_dw.c   | 297 ++++++++++++++++++++++------
 drivers/tty/serial/8250/8250_port.c |  69 ++++---
 include/linux/serial_8250.h         |   1 +
 4 files changed, 301 insertions(+), 91 deletions(-)


base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
-- 
2.39.5

[PATCH 1/6] serial: 8250: Protect LCR write in shutdown

[Ilpo Järvinen]

The 8250_dw driver needs to potentially perform very complex operations
during LCR writes because its BUSY handling prevents updates to LCR
while UART is BUSY (which is not fully under our control without those
complex operations). Thus, LCR writes should occur under port's lock.

Move LCR write under port's lock in serial8250_do_shutdown(). Also
split the LCR RMW so that the logic is on a separate line for clarity.

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_port.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 719faf92aa8a..f7a3c5555204 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2350,6 +2350,7 @@ static int serial8250_startup(struct uart_port *port)
 void serial8250_do_shutdown(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
+	u32 lcr;
 
 	serial8250_rpm_get(up);
 	/*
@@ -2376,13 +2377,13 @@ void serial8250_do_shutdown(struct uart_port *port)
 			port->mctrl &= ~TIOCM_OUT2;
 
 		serial8250_set_mctrl(port, port->mctrl);
+
+		/* Disable break condition */
+		lcr = serial_port_in(port, UART_LCR);
+		lcr &= ~UART_LCR_SBC;
+		serial_port_out(port, UART_LCR, lcr);
 	}
 
-	/*
-	 * Disable break condition and FIFOs
-	 */
-	serial_port_out(port, UART_LCR,
-			serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
 	serial8250_clear_fifos(up);
 
 	rsa_disable(up);
-- 
2.39.5

[PATCH 2/6] serial: 8250_dw: Avoid unnecessary LCR writes

[Ilpo Järvinen]

When DW UART is configured with BUSY flag, LCR writes may not always
succeed which can make any LCR write complex and very expensive.
Performing write directly can trigger IRQ and the driver has to perform
complex and distruptive sequence while retrying the write.

Therefore, it's better to avoid doing LCR write that would not change
the value of the LCR register. Add LCR write avoidance code into the
8250_dw driver's serial_out functions.

Reported-by: "Bandal, Shankar" <shankar.bandal@intel.com>
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 | 32 +++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 27af83f0ff46..57a760b43da9 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -181,6 +181,23 @@ static void dw8250_check_lcr(struct uart_port *p, unsigned int offset, u32 value
 	 */
 }
 
+/*
+ * With BUSY, LCR writes can be very expensive (IRQ + complex retry logic).
+ * If the write would not change the value of the LCR register, skip write
+ * entirely.
+ */
+static bool dw8250_can_skip_reg_write(struct uart_port *p, unsigned int offset, u32 value)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	u32 lcr;
+
+	if (offset != UART_LCR || d->uart_16550_compatible)
+		return false;
+
+	lcr = serial_port_in(p, offset);
+	return lcr == value;
+}
+
 /* Returns once the transmitter is empty or we run out of retries */
 static void dw8250_tx_wait_empty(struct uart_port *p)
 {
@@ -207,12 +224,18 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
 
 static void dw8250_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
+	if (dw8250_can_skip_reg_write(p, offset, value))
+		return;
+
 	writeb(value, p->membase + (offset << p->regshift));
 	dw8250_check_lcr(p, offset, value);
 }
 
 static void dw8250_serial_out38x(struct uart_port *p, unsigned int offset, u32 value)
 {
+	if (dw8250_can_skip_reg_write(p, offset, value))
+		return;
+
 	/* Allow the TX to drain before we reconfigure */
 	if (offset == UART_LCR)
 		dw8250_tx_wait_empty(p);
@@ -237,6 +260,9 @@ static u32 dw8250_serial_inq(struct uart_port *p, unsigned int offset)
 
 static void dw8250_serial_outq(struct uart_port *p, unsigned int offset, u32 value)
 {
+	if (dw8250_can_skip_reg_write(p, offset, value))
+		return;
+
 	value &= 0xff;
 	__raw_writeq(value, p->membase + (offset << p->regshift));
 	/* Read back to ensure register write ordering. */
@@ -248,6 +274,9 @@ static void dw8250_serial_outq(struct uart_port *p, unsigned int offset, u32 val
 
 static void dw8250_serial_out32(struct uart_port *p, unsigned int offset, u32 value)
 {
+	if (dw8250_can_skip_reg_write(p, offset, value))
+		return;
+
 	writel(value, p->membase + (offset << p->regshift));
 	dw8250_check_lcr(p, offset, value);
 }
@@ -261,6 +290,9 @@ static u32 dw8250_serial_in32(struct uart_port *p, unsigned int offset)
 
 static void dw8250_serial_out32be(struct uart_port *p, unsigned int offset, u32 value)
 {
+	if (dw8250_can_skip_reg_write(p, offset, value))
+		return;
+
 	iowrite32be(value, p->membase + (offset << p->regshift));
 	dw8250_check_lcr(p, offset, value);
 }
-- 
2.39.5

[PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling

[Ilpo Järvinen]

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

[PATCH 4/6] serial: 8250_dw: Rework IIR_NO_INT handling to stop interrupt storm

[Ilpo Järvinen]

INTC10EE UART can end up into an interrupt storm where it reports
IIR_NO_INT (0x1). If the storm happens during active UART operation, it
is promptly stopped by IIR value change due to Rx or Tx events.
However, when there is no activity, either due to idle serial line or
due to specific circumstances such as during shutdown that writes
IER=0, there is nothing to stop the storm.

During shutdown the storm is particularly problematic because
serial8250_do_shutdown() calls synchronize_irq() that will hang in
waiting for the storm to finish which never happens.

This problem can also result in triggering a warning:

  irq 45: nobody cared (try booting with the "irqpoll" option)
  [...snip...]
  handlers:
    serial8250_interrupt
  Disabling IRQ #45

Normal means to reset interrupt status by reading LSR, MSR, USR, or RX
register do not result in the UART deasserting the IRQ.

Add a quirk to INTC10EE UARTs to enable Tx interrupts if UART's Tx is
currently empty and inactive. Rework IIR_NO_INT to keep track of the
number of consecutive IIR_NO_INT, and on third one perform the quirk.
Enabling Tx interrupts should change IIR value from IIR_NO_INT to
IIR_THRI which has been observed to stop the storm.

Reported-by: "Bandal, Shankar" <shankar.bandal@intel.com>
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 | 60 ++++++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 7cee89f14179..a40c0851f39c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -60,6 +60,7 @@
 #define DW_UART_QUIRK_IS_DMA_FC		BIT(3)
 #define DW_UART_QUIRK_APMC0D08		BIT(4)
 #define DW_UART_QUIRK_CPR_VALUE		BIT(5)
+#define DW_UART_QUIRK_IER_KICK		BIT(6)
 
 struct dw8250_platform_data {
 	u8 usr_reg;
@@ -81,6 +82,8 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+
+	u64			no_int_count;
 };
 
 static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
@@ -308,6 +311,29 @@ static u32 dw8250_serial_in32be(struct uart_port *p, unsigned int offset)
        return dw8250_modify_msr(p, offset, value);
 }
 
+/*
+ * INTC10EE UART can IRQ storm while reporting IIR_NO_INT. Inducing IIR value
+ * change has been observed to break the storm.
+ *
+ * If Tx is empty (THRE asserted), we use here IER_THRI to cause IIR_NO_INT ->
+ * IIR_THRI transition.
+ */
+static void dw8250_quirk_ier_kick(struct uart_port *p)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+	u32 lsr;
+
+	if (up->ier & UART_IER_THRI)
+		return;
+
+	lsr = serial_lsr_in(up);
+	if (!(lsr & UART_LSR_THRE))
+		return;
+
+	serial_out(up, UART_IER, up->ier | UART_IER_THRI);
+	serial_in(up, UART_LCR);	/* safe, no side-effects */
+	serial_out(up, UART_IER, up->ier);
+}
 
 static int dw8250_handle_irq(struct uart_port *p)
 {
@@ -318,18 +344,29 @@ static int dw8250_handle_irq(struct uart_port *p)
 	unsigned int quirks = d->pdata->quirks;
 	unsigned int status;
 
+	guard(uart_port_lock_irqsave)(p);
+
 	switch (FIELD_GET(DW_UART_IIR_IID, iir)) {
 	case UART_IIR_NO_INT:
+		if (d->uart_16550_compatible || up->dma)
+			return 0;
+
+		d->no_int_count++;
+		if (d->no_int_count > 2 && quirks & DW_UART_QUIRK_IER_KICK)
+			dw8250_quirk_ier_kick(p);
+
 		return 0;
 
 	case UART_IIR_BUSY:
 		/* Clear the USR */
 		serial_port_in(p, d->pdata->usr_reg);
 
+		d->no_int_count = 0;
+
 		return 1;
 	}
 
-	guard(uart_port_lock_irqsave)(p);
+	d->no_int_count = 0;
 
 	/*
 	 * There are ways to get Designware-based UARTs into a state where
@@ -562,6 +599,14 @@ static void dw8250_reset_control_assert(void *data)
 	reset_control_assert(data);
 }
 
+static void dw8250_shutdown(struct uart_port *port)
+{
+	struct dw8250_data *d = to_dw8250_data(port->private_data);
+
+	serial8250_do_shutdown(port);
+	d->no_int_count = 0;
+}
+
 static int dw8250_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port uart = {}, *up = &uart;
@@ -685,10 +730,12 @@ static int dw8250_probe(struct platform_device *pdev)
 		dw8250_quirks(p, data);
 
 	/* If the Busy Functionality is not implemented, don't handle it */
-	if (data->uart_16550_compatible)
+	if (data->uart_16550_compatible) {
 		p->handle_irq = NULL;
-	else if (data->pdata)
+	} else if (data->pdata) {
 		p->handle_irq = dw8250_handle_irq;
+		p->shutdown = dw8250_shutdown;
+	}
 
 	dw8250_setup_dma_filter(p, data);
 
@@ -815,6 +862,11 @@ static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
 	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
 };
 
+static const struct dw8250_platform_data dw8250_intc10ee = {
+	.usr_reg = DW_UART_USR,
+	.quirks = DW_UART_QUIRK_IER_KICK,
+};
+
 static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
 	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
@@ -844,7 +896,7 @@ static const struct acpi_device_id dw8250_acpi_match[] = {
 	{ "INT33C5", (kernel_ulong_t)&dw8250_dw_apb },
 	{ "INT3434", (kernel_ulong_t)&dw8250_dw_apb },
 	{ "INT3435", (kernel_ulong_t)&dw8250_dw_apb },
-	{ "INTC10EE", (kernel_ulong_t)&dw8250_dw_apb },
+	{ "INTC10EE", (kernel_ulong_t)&dw8250_intc10ee },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, dw8250_acpi_match);
-- 
2.39.5

[PATCH 5/6] serial: 8250: Add late synchronize_irq() to shutdown to handle DW UART BUSY

[Ilpo Järvinen]

When DW UART is !uart_16550_compatible, it can indicate BUSY at any
point (when under constant Rx pressure) unless a complex sequence of
steps is performed. Any LCR write can run a foul with the condition
that prevents writing LCR while the UART is BUSY, which triggers
BUSY_DETECT interrupt that seems unmaskable using IER bits.

Normal flow is that dw8250_handle_irq() handles BUSY_DETECT condition
by reading USR register. This BUSY feature, however, breaks the
assumptions made in serial8250_do_shutdown(), which runs
synchronize_irq() after clearing IER and assumes no interrupts can
occur after that point but then proceeds to update LCR, which on DW
UART can trigger an interrupt.

If serial8250_do_shutdown() releases the interrupt handler before the
handler has run and processed the BUSY_DETECT condition by read the USR
register, the IRQ is not deasserted resulting in interrupt storm that
triggers "irq x: nobody cared" warning leading to disabling the IRQ.

Add late synchronize_irq() into serial8250_do_shutdown() to ensure
BUSY_DETECT from DW UART is handled before port's interrupt handler is
released. Alternative would be to add DW UART specific shutdown
function but it would mostly duplicate the generic code and the extra
synchronize_irq() seems pretty harmless in serial8250_do_shutdown().

Reported-by: "Bandal, Shankar" <shankar.bandal@intel.com>
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_port.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 02576ed30abb..fa982e5cbe90 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2401,6 +2401,12 @@ void serial8250_do_shutdown(struct uart_port *port)
 	 * the IRQ chain.
 	 */
 	serial_port_in(port, UART_RX);
+	/*
+	 * LCR writes on DW UART can trigger late (unmaskable) IRQs.
+	 * Handle them before releasing the handler.
+	 */
+	synchronize_irq(port->irq);
+
 	serial8250_rpm_put(up);
 
 	up->ops->release_irq(up);
-- 
2.39.5

[PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Ilpo Järvinen]

DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
Existance of BUSY depends on uart_16550_compatible, if UART HW is
configured with 16550 compatible those registers can always be written.

There currently is dw8250_force_idle() which attempts to archive
non-BUSY state by disabling FIFO, however, the solution is unreliable
when Rx keeps getting more and more characters.

Create a sequence of operations to enforce that ensures UART cannot
keep BUSY asserted indefinitely. The new sequence relies on enabling
loopback mode temporarily to prevent incoming Rx characters keeping
UART BUSY.

Ensure no Tx in ongoing while the UART is switches into the loopback
mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding
DMA Tx pause/resume functions).

According to tests performed by Adriana Nicolae <adriana@arista.com>,
simply disabling FIFO or clearing FIFOs only once does not always
ensure BUSY is deasserted but up to two tries may be needed. This could
be related to ongoing Rx of a character (a guess, not known for sure).
Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary
number but using, e.g., p->fifosize seems overly large). Tests
performed by others did not exhibit similar challenge but it does not
seem harmful to leave the FIFO clearing loop in place for all DW UARTs
with BUSY functionality.

Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
writes. In case of plain LCR writes, opportunistically try to update
LCR first and only invoke dw8250_idle_enter() if the write did not
succeed (it has been observed that in practice most LCR writes do
succeed without complications).

This issue was first reported by qianfan Zhao who put lots of debugging
effort into understanding the solution space.

Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround")
Fixes: 7d4008ebb1c9 ("tty: add a DesignWare 8250 driver")
Cc: <stable@vger.kernel.org>
Reported-by: qianfan Zhao <qianfanguijin@163.com>
Link: https://lore.kernel.org/linux-serial/289bb78a-7509-1c5c-2923-a04ed3b6487d@163.com/
Reported-by: Adriana Nicolae <adriana@arista.com>
Link: https://lore.kernel.org/linux-serial/20250819182322.3451959-1-adriana@arista.com/
Reported-by: "Bandal, Shankar" <shankar.bandal@intel.com>
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.h      |  25 ++++
 drivers/tty/serial/8250/8250_dw.c   | 172 ++++++++++++++++++++--------
 drivers/tty/serial/8250/8250_port.c |  28 ++---
 3 files changed, 166 insertions(+), 59 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 8caecfc85d93..77fe0588fd6b 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -175,7 +175,9 @@ static unsigned int __maybe_unused serial_icr_read(struct uart_8250_port *up,
 	return value;
 }
 
+void serial8250_clear_fifos(struct uart_8250_port *p);
 void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p);
+void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count);
 
 void serial8250_rpm_get(struct uart_8250_port *p);
 void serial8250_rpm_put(struct uart_8250_port *p);
@@ -400,6 +402,26 @@ static inline bool serial8250_tx_dma_running(struct uart_8250_port *p)
 
 	return dma && dma->tx_running;
 }
+
+static inline void serial8250_tx_dma_pause(struct uart_8250_port *p)
+{
+	struct uart_8250_dma *dma = p->dma;
+
+	if (!dma->tx_running)
+		return;
+
+	dmaengine_pause(dma->txchan);
+}
+
+static inline void serial8250_tx_dma_resume(struct uart_8250_port *p)
+{
+	struct uart_8250_dma *dma = p->dma;
+
+	if (!dma->tx_running)
+		return;
+
+	dmaengine_resume(dma->txchan);
+}
 #else
 static inline int serial8250_tx_dma(struct uart_8250_port *p)
 {
@@ -421,6 +443,9 @@ static inline bool serial8250_tx_dma_running(struct uart_8250_port *p)
 {
 	return false;
 }
+
+static inline void serial8250_tx_dma_pause(struct uart_8250_port *p) { }
+static inline void serial8250_tx_dma_resume(struct uart_8250_port *p) { }
 #endif
 
 static inline int ns16550a_goto_highspeed(struct uart_8250_port *up)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a40c0851f39c..8166ed15fd08 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/lockdep.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
@@ -46,6 +47,8 @@
 
 #define DW_UART_MCR_SIRE		BIT(6)
 
+#define DW_UART_USR_BUSY		BIT(0)
+
 /* Renesas specific register fields */
 #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
 #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
@@ -82,6 +85,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		in_idle:1;
 
 	u64			no_int_count;
 };
@@ -115,77 +119,152 @@ static inline u32 dw8250_modify_msr(struct uart_port *p, unsigned int offset, u3
 }
 
 /*
- * This function is being called as part of the uart_port::serial_out()
- * routine. Hence, it must not call serial_port_out() or serial_out()
- * against the modified registers here, i.e. LCR.
+ * Ensure BUSY is not asserted. If DW UART is configured with
+ * !uart_16550_compatible, the writes to LCR, DLL, and DLH fail while
+ * BUSY is asserted.
+ *
+ * Context: port's lock must be held
  */
-static void dw8250_force_idle(struct uart_port *p)
+static int dw8250_idle_enter(struct uart_port *p)
 {
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	struct uart_8250_port *up = up_to_u8250p(p);
-	unsigned int lsr;
+	unsigned int usr_reg = DW_UART_USR;
+	int retries;
+	u32 lsr;
 
-	/*
-	 * The following call currently performs serial_out()
-	 * against the FCR register. Because it differs to LCR
-	 * there will be no infinite loop, but if it ever gets
-	 * modified, we might need a new custom version of it
-	 * that avoids infinite recursion.
-	 */
-	serial8250_clear_and_reinit_fifos(up);
+	lockdep_assert_held_once(&p->lock);
+
+	if (d->uart_16550_compatible)
+		return 0;
+
+	if (d->pdata)
+		usr_reg = d->pdata->usr_reg;
+
+	d->in_idle = 1;
+
+	/* Prevent triggering interrupt from RBR filling */
+	p->serial_out(p, UART_IER, 0);
+
+	if (up->dma) {
+		serial8250_rx_dma_flush(up);
+		if (serial8250_tx_dma_running(up))
+			serial8250_tx_dma_pause(up);
+	}
 
 	/*
-	 * With PSLVERR_RESP_EN parameter set to 1, the device generates an
-	 * error response when an attempt to read an empty RBR with FIFO
-	 * enabled.
+	 * Wait until Tx becomes empty + one extra frame time to ensure all bits
+	 * have been sent on the wire.
 	 */
-	if (up->fcr & UART_FCR_ENABLE_FIFO) {
-		lsr = serial_port_in(p, UART_LSR);
-		if (!(lsr & UART_LSR_DR))
-			return;
+	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
+	ndelay(p->frame_time);
+
+	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
+
+	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
+	do {
+		serial8250_clear_fifos(up);
+		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
+			break;
+		ndelay(p->frame_time);
+	} while (--retries);
+
+	lsr = serial_lsr_in(up);
+	if (lsr & UART_LSR_DR) {
+		p->serial_in(p, UART_RX);
+		up->lsr_saved_flags = 0;
 	}
 
-	serial_port_in(p, UART_RX);
+	/* Now guaranteed to have BUSY deasserted? Just sanity check */
+	if (p->serial_in(p, usr_reg) & DW_UART_USR_BUSY)
+		return -EBUSY;
+
+	return 0;
+}
+
+static void dw8250_idle_exit(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	if (d->uart_16550_compatible)
+		return;
+
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, up->fcr);
+	p->serial_out(p, UART_MCR, up->mcr);
+	p->serial_out(p, UART_IER, up->ier);
+
+	/* DMA Rx is restarted by IRQ handler as needed. */
+	if (up->dma)
+		serial8250_tx_dma_resume(up);
+
+	d->in_idle = 0;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+			       unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+	int ret;
+
+	ret = dw8250_idle_enter(p);
+	if (ret < 0)
+		goto idle_failed;
+
+	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+	if (!(p->serial_in(p, UART_LCR) & UART_LCR_DLAB))
+		goto idle_failed;
+
+	serial_dl_write(up, quot);
+	p->serial_out(p, UART_LCR, up->lcr);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /*
  * This function is being called as part of the uart_port::serial_out()
- * routine. Hence, it must not call serial_port_out() or serial_out()
- * against the modified registers here, i.e. LCR.
+ * routine. Hence, special care must be taken when serial_port_out() or
+ * serial_out() against the modified registers here, i.e. LCR (d->in_idle is
+ * used to break recursion loop).
  */
 static void dw8250_check_lcr(struct uart_port *p, unsigned int offset, u32 value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
-	void __iomem *addr = p->membase + (offset << p->regshift);
-	int tries = 1000;
+	u32 lcr;
+	int ret;
 
 	if (offset != UART_LCR || d->uart_16550_compatible)
 		return;
 
-	/* Make sure LCR write wasn't ignored */
-	while (tries--) {
-		u32 lcr = serial_port_in(p, offset);
+	lcr = p->serial_in(p, UART_LCR);
 
-		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-			return;
+	/* Make sure LCR write wasn't ignored */
+	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+		return;
 
-		dw8250_force_idle(p);
+	if (d->in_idle) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		return;
+	}
 
-#ifdef CONFIG_64BIT
-		if (p->type == PORT_OCTEON)
-			__raw_writeq(value & 0xff, addr);
-		else
-#endif
-		if (p->iotype == UPIO_MEM32)
-			writel(value, addr);
-		else if (p->iotype == UPIO_MEM32BE)
-			iowrite32be(value, addr);
-		else
-			writeb(value, addr);
+	ret = dw8250_idle_enter(p);
+	if (ret < 0) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		goto idle_failed;
 	}
-	/*
-	 * FIXME: this deadlocks if port->lock is already held
-	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
-	 */
+
+	p->serial_out(p, UART_LCR, value);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /*
@@ -627,6 +706,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->dev		= dev;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->set_divisor	= dw8250_set_divisor;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index fa982e5cbe90..d5ea0f08a854 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -489,7 +489,7 @@ serial_port_out_sync(struct uart_port *p, int offset, int value)
 /*
  * FIFO support.
  */
-static void serial8250_clear_fifos(struct uart_8250_port *p)
+void serial8250_clear_fifos(struct uart_8250_port *p)
 {
 	if (p->capabilities & UART_CAP_FIFO) {
 		serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
@@ -498,6 +498,7 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
 		serial_out(p, UART_FCR, 0);
 	}
 }
+EXPORT_SYMBOL_GPL(serial8250_clear_fifos);
 
 static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t);
 static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t);
@@ -3200,6 +3201,17 @@ void serial8250_set_defaults(struct uart_8250_port *up)
 }
 EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 
+void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
+{
+	unsigned int i;
+
+	for (i = 0; i < count; i++) {
+		if (wait_for_lsr(up, UART_LSR_THRE))
+			return;
+	}
+}
+EXPORT_SYMBOL_GPL(serial8250_fifo_wait_for_lsr_thre);
+
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 
 static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
@@ -3241,16 +3253,6 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
-static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
-{
-	unsigned int i;
-
-	for (i = 0; i < count; i++) {
-		if (wait_for_lsr(up, UART_LSR_THRE))
-			return;
-	}
-}
-
 /*
  * Print a string to the serial port using the device FIFO
  *
@@ -3269,7 +3271,7 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 
 	while (s != end) {
 		/* Allow timeout for each byte of a possibly full FIFO */
-		fifo_wait_for_lsr(up, fifosize);
+		serial8250_fifo_wait_for_lsr_thre(up, fifosize);
 
 		for (i = 0; i < fifosize && s != end; ++i) {
 			if (*s == '\n' && !cr_sent) {
@@ -3287,7 +3289,7 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 	 * Allow timeout for each byte written since the caller will only wait
 	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
 	 */
-	fifo_wait_for_lsr(up, tx_count);
+	serial8250_fifo_wait_for_lsr_thre(up, tx_count);
 }
 
 /*
-- 
2.39.5

Re: [PATCH 2/6] serial: 8250_dw: Avoid unnecessary LCR writes

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 07:27:35PM +0200, Ilpo Järvinen wrote:
> When DW UART is configured with BUSY flag, LCR writes may not always
> succeed which can make any LCR write complex and very expensive.
> Performing write directly can trigger IRQ and the driver has to perform
> complex and distruptive sequence while retrying the write.
> 
> Therefore, it's better to avoid doing LCR write that would not change
> the value of the LCR register. Add LCR write avoidance code into the
> 8250_dw driver's serial_out functions.

.serial_out() ?

...

> +/*
> + * With BUSY, LCR writes can be very expensive (IRQ + complex retry logic).
> + * If the write would not change the value of the LCR register, skip write

would --> does

skip write --> skip it

> + * entirely.
> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling

[Andy Shevchenko]

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

Re: [PATCH 4/6] serial: 8250_dw: Rework IIR_NO_INT handling to stop interrupt storm

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 07:27:37PM +0200, Ilpo Järvinen wrote:
> INTC10EE UART can end up into an interrupt storm where it reports
> IIR_NO_INT (0x1). If the storm happens during active UART operation, it
> is promptly stopped by IIR value change due to Rx or Tx events.
> However, when there is no activity, either due to idle serial line or
> due to specific circumstances such as during shutdown that writes
> IER=0, there is nothing to stop the storm.
> 
> During shutdown the storm is particularly problematic because
> serial8250_do_shutdown() calls synchronize_irq() that will hang in
> waiting for the storm to finish which never happens.
> 
> This problem can also result in triggering a warning:
> 
>   irq 45: nobody cared (try booting with the "irqpoll" option)
>   [...snip...]
>   handlers:
>     serial8250_interrupt
>   Disabling IRQ #45
> 
> Normal means to reset interrupt status by reading LSR, MSR, USR, or RX
> register do not result in the UART deasserting the IRQ.
> 
> Add a quirk to INTC10EE UARTs to enable Tx interrupts if UART's Tx is
> currently empty and inactive. Rework IIR_NO_INT to keep track of the
> number of consecutive IIR_NO_INT, and on third one perform the quirk.
> Enabling Tx interrupts should change IIR value from IIR_NO_INT to
> IIR_THRI which has been observed to stop the storm.

...

> +	u64			no_int_count;

Why so big?

...

> +		d->no_int_count++;
> +		if (d->no_int_count > 2 && quirks & DW_UART_QUIRK_IER_KICK)
> +			dw8250_quirk_ier_kick(p);

Usual way is to use modulo. And perhaps use 4 for the sake of avoiding
division:

		if (d->no_int_count == 3 && quirks & DW_UART_QUIRK_IER_KICK)
			dw8250_quirk_ier_kick(p);

		d->no_int_count = (d->no_int_count + 1) % 4;

where 4 may be defined with meaningful name. With that u8 is more than enough.

>  		return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> Existance of BUSY depends on uart_16550_compatible, if UART HW is
> configured with 16550 compatible those registers can always be written.

with 16550 compatible --> with it

> There currently is dw8250_force_idle() which attempts to archive
> non-BUSY state by disabling FIFO, however, the solution is unreliable
> when Rx keeps getting more and more characters.
> 
> Create a sequence of operations to enforce that ensures UART cannot
> keep BUSY asserted indefinitely. The new sequence relies on enabling
> loopback mode temporarily to prevent incoming Rx characters keeping
> UART BUSY.

What if UART was already in a loopback mode? I assume that Tx pause
described below should not affect the case.

The real case scenario that I am thinking of is a stress test of UART
using loopback mode.

> Ensure no Tx in ongoing while the UART is switches into the loopback
> mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding
> DMA Tx pause/resume functions).
> 
> According to tests performed by Adriana Nicolae <adriana@arista.com>,
> simply disabling FIFO or clearing FIFOs only once does not always
> ensure BUSY is deasserted but up to two tries may be needed. This could
> be related to ongoing Rx of a character (a guess, not known for sure).

Sounds like a plausible theory because UART has shift registers that are
working independently on the current situation with FIFO. They are actual
frontends for Tx and Rx data on the wire.

> Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary
> number but using, e.g., p->fifosize seems overly large). Tests
> performed by others did not exhibit similar challenge but it does not
> seem harmful to leave the FIFO clearing loop in place for all DW UARTs
> with BUSY functionality.
> 
> Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> writes. In case of plain LCR writes, opportunistically try to update
> LCR first and only invoke dw8250_idle_enter() if the write did not
> succeed (it has been observed that in practice most LCR writes do
> succeed without complications).
> 
> This issue was first reported by qianfan Zhao who put lots of debugging
> effort into understanding the solution space.

...

> +	/* Prevent triggering interrupt from RBR filling */
> +	p->serial_out(p, UART_IER, 0);

Do we specifically use callbacks directly and not wrappers all over the change?

...

> +	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
> +	ndelay(p->frame_time);

Wouldn't be a problem on lowest baud rates (exempli gratia 110)?

...

> +	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
> +	do {
> +		serial8250_clear_fifos(up);
> +		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
> +			break;
> +		ndelay(p->frame_time);
> +	} while (--retries);

read_poll_timeout_atomic() ? I assume it can't be used due to small frame time?

...

> +	if (d->in_idle) {

> +		/*
> +		 * FIXME: this deadlocks if port->lock is already held
> +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> +		 */

Hmm... That FIXME should gone since we have non-blocking consoles, no?

> +		return;
> +	}

...

> +	ret = dw8250_idle_enter(p);
> +	if (ret < 0) {
> +		/*
> +		 * FIXME: this deadlocks if port->lock is already held
> +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> +		 */
> +		goto idle_failed;
>  	}
> -	/*
> -	 * FIXME: this deadlocks if port->lock is already held
> -	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> -	 */

Ditto.

>  }

...

>  	p->dev		= dev;

Maybe put an added line here?

>  	p->set_ldisc	= dw8250_set_ldisc;
>  	p->set_termios	= dw8250_set_termios;
> +	p->set_divisor	= dw8250_set_divisor;

...

> +EXPORT_SYMBOL_GPL(serial8250_clear_fifos);

Same Q, perhaps start exporting with a namespace?

>  }
>  EXPORT_SYMBOL_GPL(serial8250_set_defaults);

...

> +void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < count; i++) {

	while (count--) ?

Ah, it's existing code... OK then.

> +		if (wait_for_lsr(up, UART_LSR_THRE))
> +			return;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Greg Kroah-Hartman]

On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> Existance of BUSY depends on uart_16550_compatible, if UART HW is
> configured with 16550 compatible those registers can always be written.
> 
> There currently is dw8250_force_idle() which attempts to archive
> non-BUSY state by disabling FIFO, however, the solution is unreliable
> when Rx keeps getting more and more characters.
> 
> Create a sequence of operations to enforce that ensures UART cannot
> keep BUSY asserted indefinitely. The new sequence relies on enabling
> loopback mode temporarily to prevent incoming Rx characters keeping
> UART BUSY.
> 
> Ensure no Tx in ongoing while the UART is switches into the loopback
> mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding
> DMA Tx pause/resume functions).
> 
> According to tests performed by Adriana Nicolae <adriana@arista.com>,
> simply disabling FIFO or clearing FIFOs only once does not always
> ensure BUSY is deasserted but up to two tries may be needed. This could
> be related to ongoing Rx of a character (a guess, not known for sure).
> Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary
> number but using, e.g., p->fifosize seems overly large). Tests
> performed by others did not exhibit similar challenge but it does not
> seem harmful to leave the FIFO clearing loop in place for all DW UARTs
> with BUSY functionality.
> 
> Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> writes. In case of plain LCR writes, opportunistically try to update
> LCR first and only invoke dw8250_idle_enter() if the write did not
> succeed (it has been observed that in practice most LCR writes do
> succeed without complications).
> 
> This issue was first reported by qianfan Zhao who put lots of debugging
> effort into understanding the solution space.
> 
> Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround")
> Fixes: 7d4008ebb1c9 ("tty: add a DesignWare 8250 driver")
> Cc: <stable@vger.kernel.org>

Why is patch 6/6 only marked for stable?  If this is needed "now",
shouldn't this be a separate patch?  Do you need all of the first 5 for
this to work properly?

I can't take this series as-is because I don't know how to route it :(

thanks,

greg k-h

Re: [PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]

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.

Re: [PATCH 4/6] serial: 8250_dw: Rework IIR_NO_INT handling to stop interrupt storm

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Sat, 24 Jan 2026, Andy Shevchenko wrote:

> On Fri, Jan 23, 2026 at 07:27:37PM +0200, Ilpo Järvinen wrote:
> > INTC10EE UART can end up into an interrupt storm where it reports
> > IIR_NO_INT (0x1). If the storm happens during active UART operation, it
> > is promptly stopped by IIR value change due to Rx or Tx events.
> > However, when there is no activity, either due to idle serial line or
> > due to specific circumstances such as during shutdown that writes
> > IER=0, there is nothing to stop the storm.
> > 
> > During shutdown the storm is particularly problematic because
> > serial8250_do_shutdown() calls synchronize_irq() that will hang in
> > waiting for the storm to finish which never happens.
> > 
> > This problem can also result in triggering a warning:
> > 
> >   irq 45: nobody cared (try booting with the "irqpoll" option)
> >   [...snip...]
> >   handlers:
> >     serial8250_interrupt
> >   Disabling IRQ #45
> > 
> > Normal means to reset interrupt status by reading LSR, MSR, USR, or RX
> > register do not result in the UART deasserting the IRQ.
> > 
> > Add a quirk to INTC10EE UARTs to enable Tx interrupts if UART's Tx is
> > currently empty and inactive. Rework IIR_NO_INT to keep track of the
> > number of consecutive IIR_NO_INT, and on third one perform the quirk.
> > Enabling Tx interrupts should change IIR value from IIR_NO_INT to
> > IIR_THRI which has been observed to stop the storm.
> 
> ...
> 
> > +	u64			no_int_count;
> 
> Why so big?
> 
> ...

No particular reason, it's a leftover from debugging this issue.

> > +		d->no_int_count++;
> > +		if (d->no_int_count > 2 && quirks & DW_UART_QUIRK_IER_KICK)
> > +			dw8250_quirk_ier_kick(p);
> 
> Usual way is to use modulo. And perhaps use 4 for the sake of avoiding
> division:
> 
> 		if (d->no_int_count == 3 && quirks & DW_UART_QUIRK_IER_KICK)
> 			dw8250_quirk_ier_kick(p);
> 
> 		d->no_int_count = (d->no_int_count + 1) % 4;

This doesn't look equivalent code as it only fires on 4th NO_INT, but I 
guess the difference doesn't matter that much so changing to your 
suggestion so that the kick will only occurs on fourth NO_INT interrupt.

-- 
 i.

> where 4 may be defined with meaningful name. With that u8 is more than enough.
>
> >  		return 0;
> 
> 

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 6692 bytes --]

On Sat, 24 Jan 2026, Andy Shevchenko wrote:

> On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> > DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> > Existance of BUSY depends on uart_16550_compatible, if UART HW is
> > configured with 16550 compatible those registers can always be written.
> 
> with 16550 compatible --> with it
> 
> > There currently is dw8250_force_idle() which attempts to archive
> > non-BUSY state by disabling FIFO, however, the solution is unreliable
> > when Rx keeps getting more and more characters.
> > 
> > Create a sequence of operations to enforce that ensures UART cannot
> > keep BUSY asserted indefinitely. The new sequence relies on enabling
> > loopback mode temporarily to prevent incoming Rx characters keeping
> > UART BUSY.
> 
> What if UART was already in a loopback mode? I assume that Tx pause
> described below should not affect the case.
>
> The real case scenario that I am thinking of is a stress test of UART
> using loopback mode.

If you're running a stress test that transfers characters while writing to 
LCR, IMO you get to keep all the pieces yourself.

What will happen though is that LCR write would succeed still because of 
the locking that will prevent Tx'ing more to loopback, but the stress test 
might lose some pieces instead of getting to keep them. :-)

In general, I don't see sane reasons to mess with LCR while a real 
transfer is going on. How is it sane to change line settings such as # of 
bits while xferring something!?!


This is to fix scenarios where what's happening on the serial line is not 
under our control (the other end keeps sending characters). There's 
nothing we can do to stop that unlike running a loopback the stress test 
while writing to LCR which is plain stupidity.

> > Ensure no Tx in ongoing while the UART is switches into the loopback
> > mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding
> > DMA Tx pause/resume functions).
> > 
> > According to tests performed by Adriana Nicolae <adriana@arista.com>,
> > simply disabling FIFO or clearing FIFOs only once does not always
> > ensure BUSY is deasserted but up to two tries may be needed. This could
> > be related to ongoing Rx of a character (a guess, not known for sure).
> 
> Sounds like a plausible theory because UART has shift registers that are
> working independently on the current situation with FIFO. They are actual
> frontends for Tx and Rx data on the wire.

Yes. I just mentioned it's a guess as it's hard to verify, so if somebody 
looks at this commit from the history, they know I've not been able to 
confirm but just made an educated guess. And if they've been able to
acquire better information, they're more likely to rely on that info 
instead of my guesswork.

> > Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary
> > number but using, e.g., p->fifosize seems overly large). Tests
> > performed by others did not exhibit similar challenge but it does not
> > seem harmful to leave the FIFO clearing loop in place for all DW UARTs
> > with BUSY functionality.
> > 
> > Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> > writes. In case of plain LCR writes, opportunistically try to update
> > LCR first and only invoke dw8250_idle_enter() if the write did not
> > succeed (it has been observed that in practice most LCR writes do
> > succeed without complications).
> > 
> > This issue was first reported by qianfan Zhao who put lots of debugging
> > effort into understanding the solution space.
> 
> ...
> 
> > +	/* Prevent triggering interrupt from RBR filling */
> > +	p->serial_out(p, UART_IER, 0);
> 
> Do we specifically use callbacks directly and not wrappers all over the change?

I guess it's just a habit, I suppose you meant using serial_port_in/out 
instead. I can try to change those.

> ...
> 
> > +	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
> > +	ndelay(p->frame_time);
> 
> Wouldn't be a problem on lowest baud rates (exempli gratia 110)?

Perhaps, but until somebody comes with an issue report related to 110, I'm 
wondering if this really is worth trying to address. Any suggestion how is 
welcome as well?

> > +	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
> > +	do {
> > +		serial8250_clear_fifos(up);
> > +		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
> > +			break;
> > +		ndelay(p->frame_time);
> > +	} while (--retries);
> 
> read_poll_timeout_atomic() ? I assume it can't be used due to small frame time?

Frame time is in nanoseconds yes. I did consider 
read_poll_timeout_atomic() but it would have required nsec -> usec 
conversion so I left this as it is.

> > +	if (d->in_idle) {
> 
> > +		/*
> > +		 * FIXME: this deadlocks if port->lock is already held
> > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > +		 */
> 
> Hmm... That FIXME should gone since we have non-blocking consoles, no?

No, lockdep still gets angry if printing is used while holding port's 
lock.

What would be possible though, is to mark the port's lock critical section 
for print deferral (but it's outside the scope of this series). In case of 
serial, it would be justified to use deferred printing (which is only 
meant for special cases) because serial console and printing are related.

> > +		return;
> > +	}
> 
> ...
> 
> > +	ret = dw8250_idle_enter(p);
> > +	if (ret < 0) {
> > +		/*
> > +		 * FIXME: this deadlocks if port->lock is already held
> > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > +		 */
> > +		goto idle_failed;
> >  	}
> > -	/*
> > -	 * FIXME: this deadlocks if port->lock is already held
> > -	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > -	 */
> 
> Ditto.
> 
> >  }
> 
> ...
> 
> >  	p->dev		= dev;
> 
> Maybe put an added line here?
> 
> >  	p->set_ldisc	= dw8250_set_ldisc;
> >  	p->set_termios	= dw8250_set_termios;
> > +	p->set_divisor	= dw8250_set_divisor;
> 
> ...
> 
> > +EXPORT_SYMBOL_GPL(serial8250_clear_fifos);
> 
> Same Q, perhaps start exporting with a namespace?

Yes, I'll put this and the wait func into NS.

> >  }
> >  EXPORT_SYMBOL_GPL(serial8250_set_defaults);
> 
> ...
> 
> > +void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < count; i++) {
> 
> 	while (count--) ?
> 
> Ah, it's existing code... OK then.
>
> > +		if (wait_for_lsr(up, UART_LSR_THRE))
> > +			return;
> > +	}
> > +}
> 
> 

-- 
 i.

Re: [PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling

[Andy Shevchenko]

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

Re: [PATCH 4/6] serial: 8250_dw: Rework IIR_NO_INT handling to stop interrupt storm

[Andy Shevchenko]

On Tue, Jan 27, 2026 at 03:01:46PM +0200, Ilpo Järvinen wrote:
> On Sat, 24 Jan 2026, Andy Shevchenko wrote:
> > On Fri, Jan 23, 2026 at 07:27:37PM +0200, Ilpo Järvinen wrote:

...

> > > +		d->no_int_count++;
> > > +		if (d->no_int_count > 2 && quirks & DW_UART_QUIRK_IER_KICK)
> > > +			dw8250_quirk_ier_kick(p);
> > 
> > Usual way is to use modulo. And perhaps use 4 for the sake of avoiding
> > division:
> > 
> > 		if (d->no_int_count == 3 && quirks & DW_UART_QUIRK_IER_KICK)
> > 			dw8250_quirk_ier_kick(p);
> > 
> > 		d->no_int_count = (d->no_int_count + 1) % 4;
> 
> This doesn't look equivalent code as it only fires on 4th NO_INT,

Correct, I forgot to clarify this in my original reply. Yes, bumping to
power-of-two for simplicity, but as you noticed it bumps also the loop to
"every 4th". (I was under impression that I wrote it somewhere else in
the reply, but now I see it's not the case.)

> but I guess the difference doesn't matter that much so changing to your
> suggestion so that the kick will only occurs on fourth NO_INT interrupt.

> > where 4 may be defined with meaningful name. With that u8 is more than enough.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Andy Shevchenko]

On Tue, Jan 27, 2026 at 03:35:27PM +0200, Ilpo Järvinen wrote:
> On Sat, 24 Jan 2026, Andy Shevchenko wrote:
> > On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:

+Cc: printk people to check on printing from a serial driver routines.

...

> > > +	/* Prevent triggering interrupt from RBR filling */
> > > +	p->serial_out(p, UART_IER, 0);
> > 
> > Do we specifically use callbacks directly and not wrappers all over the change?
> 
> I guess it's just a habit, I suppose you meant using serial_port_in/out 
> instead. I can try to change those.

Not (only) me. Jiri updated this driver (and many others) to use callbacks.
That's why I added comments here and there about possible recursions.

...

> > > +	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
> > > +	ndelay(p->frame_time);
> > 
> > Wouldn't be a problem on lowest baud rates (exempli gratia 110)?
> 
> Perhaps, but until somebody comes with an issue report related to 110, I'm 
> wondering if this really is worth trying to address. Any suggestion how is 
> welcome as well?

Polling work? Timer?

> > > +	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
> > > +	do {
> > > +		serial8250_clear_fifos(up);
> > > +		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
> > > +			break;
> > > +		ndelay(p->frame_time);
> > > +	} while (--retries);
> > 
> > read_poll_timeout_atomic() ? I assume it can't be used due to small frame time?
> 
> Frame time is in nanoseconds yes. I did consider 
> read_poll_timeout_atomic() but it would have required nsec -> usec 
> conversion so I left this as it is.

Yeah with the same issue on low baud rates. So far I think we need to consider
9600 as commonly used by the old HW (which may be connected to a modern PC with
this new kernel running), so the frame time sounds like close to a millisecond.
And this can be met in real life.

Maybe put TODO/FIXME around these ndelay() calls?

> > > +	if (d->in_idle) {
> > 
> > > +		/*
> > > +		 * FIXME: this deadlocks if port->lock is already held
> > > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > > +		 */
> > 
> > Hmm... That FIXME should gone since we have non-blocking consoles, no?
> 
> No, lockdep still gets angry if printing is used while holding port's 
> lock.

Hmm... Let's ask PRINTK people about this. John, do we still have a gap
with nbcon? Or did I misunderstand the scope of its use?

> What would be possible though, is to mark the port's lock critical section 
> for print deferral (but it's outside the scope of this series). In case of 
> serial, it would be justified to use deferred printing (which is only 
> meant for special cases) because serial console and printing are related.
> 
> > > +		return;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

On Tue, 27 Jan 2026, Andy Shevchenko wrote:

> On Tue, Jan 27, 2026 at 03:35:27PM +0200, Ilpo Järvinen wrote:
> > On Sat, 24 Jan 2026, Andy Shevchenko wrote:
> > > On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> 
> +Cc: printk people to check on printing from a serial driver routines.
> 
> ...
> 
> > > > +	/* Prevent triggering interrupt from RBR filling */
> > > > +	p->serial_out(p, UART_IER, 0);
> > > 
> > > Do we specifically use callbacks directly and not wrappers all over the change?
> > 
> > I guess it's just a habit, I suppose you meant using serial_port_in/out 
> > instead. I can try to change those.
> 
> Not (only) me. Jiri updated this driver (and many others) to use callbacks.
> That's why I added comments here and there about possible recursions.

Fair, this patch originated from a time way older than Jiri's conversion
(not an excuse, just stating how it came to be and I've not realized 
using an old way until you mentioned).

> > > > +	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
> > > > +	ndelay(p->frame_time);
> > > 
> > > Wouldn't be a problem on lowest baud rates (exempli gratia 110)?
> > 
> > Perhaps, but until somebody comes with an issue report related to 110, I'm 
> > wondering if this really is worth trying to address. Any suggestion how is 
> > welcome as well?
> 
> Polling work? Timer?

And how do I prevent others messing with the UART during that time? While 
IER is zeroed here (and I could make up->ier zero as well, I think), I 
can't hold port's lock if I do either of those.

And I can't take the tty_port's mutex here either because the caller 
is already holding port's lock (and it wouldn't prevent console writes 
anyway as that, I think, only takes port's lock).

Sadly THRE/TEMT are not trustworthy as they are set before all those 
non-data bits have been fully blasted on to the wire (we learned this with 
rs485 half-duplex scenarios).


Normal behavioral exceptation what I have here is that userspace is sane 
and won't do LCR write and tx at the same time but I don't know how to 
ensure that. Perhaps using now > last xmit timestamp + frame_time could 
avoid this unconditional delay.

> > > > +	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
> > > > +	do {
> > > > +		serial8250_clear_fifos(up);
> > > > +		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
> > > > +			break;
> > > > +		ndelay(p->frame_time);
> > > > +	} while (--retries);
> > > 
> > > read_poll_timeout_atomic() ? I assume it can't be used due to small frame time?
> > 
> > Frame time is in nanoseconds yes. I did consider 
> > read_poll_timeout_atomic() but it would have required nsec -> usec 
> > conversion so I left this as it is.
> 
> Yeah with the same issue on low baud rates. So far I think we need to consider
> 9600 as commonly used by the old HW (which may be connected to a modern PC with
> this new kernel running), so the frame time sounds like close to a millisecond.
> And this can be met in real life.
> 
> Maybe put TODO/FIXME around these ndelay() calls?

Seems reasonable, I'll add that.

I'm under impression that all LCR writes occur from contexts that are 
non-atomic by nature (except they are holding the port's lock, of course) 
so this should never delay an interrupt handler.

> > > > +	if (d->in_idle) {
> > > 
> > > > +		/*
> > > > +		 * FIXME: this deadlocks if port->lock is already held
> > > > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > > > +		 */
> > > 
> > > Hmm... That FIXME should gone since we have non-blocking consoles, no?
> > 
> > No, lockdep still gets angry if printing is used while holding port's 
> > lock.
> 
> Hmm... Let's ask PRINTK people about this. John, do we still have a gap
> with nbcon? Or did I misunderstand the scope of its use?
> 
> > What would be possible though, is to mark the port's lock critical section 
> > for print deferral (but it's outside the scope of this series). In case of 
> > serial, it would be justified to use deferred printing (which is only 
> > meant for special cases) because serial console and printing are related.
> > 
> > > > +		return;
> > > > +	}
> 
> 

-- 
 i.

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2662 bytes --]

On Mon, 26 Jan 2026, Greg Kroah-Hartman wrote:

> On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> > DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> > Existance of BUSY depends on uart_16550_compatible, if UART HW is
> > configured with 16550 compatible those registers can always be written.
> > 
> > There currently is dw8250_force_idle() which attempts to archive
> > non-BUSY state by disabling FIFO, however, the solution is unreliable
> > when Rx keeps getting more and more characters.
> > 
> > Create a sequence of operations to enforce that ensures UART cannot
> > keep BUSY asserted indefinitely. The new sequence relies on enabling
> > loopback mode temporarily to prevent incoming Rx characters keeping
> > UART BUSY.
> > 
> > Ensure no Tx in ongoing while the UART is switches into the loopback
> > mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding
> > DMA Tx pause/resume functions).
> > 
> > According to tests performed by Adriana Nicolae <adriana@arista.com>,
> > simply disabling FIFO or clearing FIFOs only once does not always
> > ensure BUSY is deasserted but up to two tries may be needed. This could
> > be related to ongoing Rx of a character (a guess, not known for sure).
> > Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary
> > number but using, e.g., p->fifosize seems overly large). Tests
> > performed by others did not exhibit similar challenge but it does not
> > seem harmful to leave the FIFO clearing loop in place for all DW UARTs
> > with BUSY functionality.
> > 
> > Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> > writes. In case of plain LCR writes, opportunistically try to update
> > LCR first and only invoke dw8250_idle_enter() if the write did not
> > succeed (it has been observed that in practice most LCR writes do
> > succeed without complications).
> > 
> > This issue was first reported by qianfan Zhao who put lots of debugging
> > effort into understanding the solution space.
> > 
> > Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround")
> > Fixes: 7d4008ebb1c9 ("tty: add a DesignWare 8250 driver")
> > Cc: <stable@vger.kernel.org>
> 
> Why is patch 6/6 only marked for stable?  If this is needed "now",
> shouldn't this be a separate patch?  Do you need all of the first 5 for
> this to work properly?

Some of those are really dependencies but I'll try to improve this 
situation for v2 and add a few more Fixes tag to the introducing commits.

> I can't take this series as-is because I don't know how to route it :(


-- 
 i.

Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[John Ogness]

On 2026-01-27, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > > +	if (d->in_idle) {
>> > 
>> > > +		/*
>> > > +		 * FIXME: this deadlocks if port->lock is already held
>> > > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
>> > > +		 */
>> > 
>> > Hmm... That FIXME should gone since we have non-blocking consoles, no?
>> 
>> No, lockdep still gets angry if printing is used while holding port's 
>> lock.
>
> Hmm... Let's ask PRINTK people about this. John, do we still have a gap
> with nbcon? Or did I misunderstand the scope of its use?

The 8250 has not yet been converted to a nbcon. I am still working on
it. Unfortunately I got side-tracked first fixing the broken 8250
console hardware-flow-control support. :-/

So the comment is correct. Once the driver converts to nbcon, the
dev_err() is fine.

Note that if the message is important, you could use a printk_deferred()
here with a FIXME to say to convert it to dev_err() once the 8250
supports nbcon.

John Ogness