[PATCH v3] serial: 8250: always disable IRQ during THRE test

[PATCH v3] serial: 8250: always disable IRQ during THRE test

[Alban Bedel]

From: Peng Zhang <zhangpeng.00@bytedance.com>

commit 039d4926379b ("serial: 8250: Toggle IER bits on only after irq
has been set up") moved IRQ setup before the THRE test, so the interrupt
handler can run during the test and race with its IIR reads. This can
produce wrong THRE test results and cause spurious registration of the
serial8250_backup_timeout timer. Unconditionally disable the IRQ for the
short duration of the test and re-enable it afterwards to avoid the race.

Cc: stable@vger.kernel.org
Fixes: 039d4926379b ("serial: 8250: Toggle IER bits on only after irq has been set up")
Signed-off-by: Peng Zhang <zhangpeng.00@bytedance.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Signed-off-by: Alban Bedel <alban.bedel@lht.dlh.de>
Tested-by: Maximilian Lueer <maximilian.lueer@lht.dlh.de>
---
Changelog:

v2: Replaced disable_irq_nosync() with disable_irq() to prevent interrupts
    that are currently being handled
v3: Added changelog
---

Commit 039d4926379b ("serial: 8250: Toggle IER bits on only after irq
has been set up") introduced a bug which showed in some of our devices
after updating them to kernel 5.15.169. These devices have an I2C
touch screen which is behind an UART to I2C bridge. This bug lead to
an ever growing latency on the bus, delaying the touch events by up to
several seconds.

Looking for other solutions than reverting commit 039d4926379b I found
Peng Zhang's patch, backported it to 5.15 and could confirm that it
solve the high delay issue.

As this a quiet sever regression I'm taking the liberty to re-submit
Peng Zhang's patch in the hope it will be considered for inclusion. I
added the changelog requested by greg k-h's patch email bot after the
last submission.

Also please note that commit 039d4926379b was backported as far back as
5.10, so quiet a few stable kernels are affected. This patch doesn't
apply as-is on older kernels, I can provide a patch for 5.15 if desired.

Alban Bedel
---
 drivers/tty/serial/8250/8250_port.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 719faf92aa8ae..f1740cc911431 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2147,8 +2147,7 @@ static void serial8250_THRE_test(struct uart_port *port)
 	if (up->port.flags & UPF_NO_THRE_TEST)
 		return;
 
-	if (port->irqflags & IRQF_SHARED)
-		disable_irq_nosync(port->irq);
+	disable_irq(port->irq);
 
 	/*
 	 * Test for UARTs that do not reassert THRE when the transmitter is idle and the interrupt
@@ -2170,8 +2169,7 @@ static void serial8250_THRE_test(struct uart_port *port)
 		serial_port_out(port, UART_IER, 0);
 	}
 
-	if (port->irqflags & IRQF_SHARED)
-		enable_irq(port->irq);
+	enable_irq(port->irq);
 
 	/*
 	 * If the interrupt is not reasserted, or we otherwise don't trust the iir, setup a timer to
-- 
2.39.5

Re: [PATCH v3] serial: 8250: always disable IRQ during THRE test

[Jiri Slaby]

On 09. 02. 26, 12:32, Alban Bedel wrote:
> From: Peng Zhang <zhangpeng.00@bytedance.com>
> 
> commit 039d4926379b ("serial: 8250: Toggle IER bits on only after irq
> has been set up") moved IRQ setup before the THRE test, so the interrupt
> handler can run during the test and race with its IIR reads. This can
> produce wrong THRE test results and cause spurious registration of the
> serial8250_backup_timeout timer. Unconditionally disable the IRQ for the
> short duration of the test and re-enable it afterwards to avoid the race.
> 
> Cc: stable@vger.kernel.org
> Fixes: 039d4926379b ("serial: 8250: Toggle IER bits on only after irq has been set up")
> Signed-off-by: Peng Zhang <zhangpeng.00@bytedance.com>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> Signed-off-by: Alban Bedel <alban.bedel@lht.dlh.de>
> Tested-by: Maximilian Lueer <maximilian.lueer@lht.dlh.de>
> ---
> Changelog:
> 
> v2: Replaced disable_irq_nosync() with disable_irq() to prevent interrupts
>      that are currently being handled

This made me to check, why/how this is possible. It appears to be, but 
only thanks to:
   205d300aea75 serial: 8250: change lock order in serial8250_do_startup()

And the change should be documented in the commit log. Especially to 
avoid stable backports to trees without 205d300aea75.

thanks,
-- 
js
suse labs

Re: [PATCH v3] serial: 8250: always disable IRQ during THRE test

[Andy Shevchenko]

On Thu, Feb 12, 2026 at 09:41:08AM +0100, Jiri Slaby wrote:
> On 09. 02. 26, 12:32, Alban Bedel wrote:

> > commit 039d4926379b ("serial: 8250: Toggle IER bits on only after irq
> > has been set up") moved IRQ setup before the THRE test, so the interrupt
> > handler can run during the test and race with its IIR reads. This can
> > produce wrong THRE test results and cause spurious registration of the
> > serial8250_backup_timeout timer. Unconditionally disable the IRQ for the
> > short duration of the test and re-enable it afterwards to avoid the race.

...

> > v2: Replaced disable_irq_nosync() with disable_irq() to prevent interrupts
> >      that are currently being handled
> 
> This made me to check, why/how this is possible. It appears to be, but only
> thanks to:
>   205d300aea75 serial: 8250: change lock order in serial8250_do_startup()
> 
> And the change should be documented in the commit log. Especially to avoid
> stable backports to trees without 205d300aea75.

There is a de facto tag Depends-on: which might be used here.

-- 
With Best Regards,
Andy Shevchenko