serial: 8250_port: Update runtime PM flow

PATCH In review Andy Shevchenko 2 participants 6 messages Sep 24 – Sep 25 TTY layer and serial drivers

Share & tools

Andy Shevchenko

author

2026-09-24 12:25 UTC #0 lore raw

v2 cover serial: 8250_port: Update runtime PM flow

There are two changes, one is a straightforward drop of the duplicate runtime PM call (which is idempotent and hence it's harmless to call, but practically no need to do so) and the other addresses long standing problem with potentially sleeping PM calls on some system in IRQ context. Also the latter might lead to unneeded resume-suspend cycle when IRQ is shared and interrupt is not ours. This mini-series to update runtime PM flow to make sure this won't happen.

In v2:
- removed now unused local variable (Greg)

v1: <[email protected]>

Andy Shevchenko (2):
serial: 8250_port: Remove redundant pm_runtime_mark_last_busy() call
serial: 8250_port: properly handle runtime PM in IRQ

 drivers/tty/serial/8250/8250_port.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Signature · 1 line

2.50.1

Andy Shevchenko

author

2026-09-24 12:25 UTC #1 lore raw

PATCH v2 1/2 serial: 8250_port: Remove redundant pm_runtime_mark_last_busy() call

The pm_runtime_mark_last_busy() call is redundant in the wrapper function as pm_runtime_put_autosuspend() already calls pm_runtime_mark_last_busy() internally to update the last access time of the device before queuing autosuspend.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 2 --
 1 file changed, 2 deletions(-)

drivers/tty/serial/8250/8250_port.c +0 -2

index 38fa45e74a37..c0bcc0742a60 100644
@@ -526,7 +526,6 @@ void serial8250_rpm_put(struct uart_8250_port *p)
 {
 	if (!(p->capabilities & UART_CAP_RPM))
 		return;
-	pm_runtime_mark_last_busy(p->port.dev);
 	pm_runtime_put_autosuspend(p->port.dev);
 }
 EXPORT_SYMBOL_GPL(serial8250_rpm_put);

@@ -667,7 +666,6 @@ static void serial8250_rpm_put_tx(struct uart_8250_port *p)
 	rpm_active = xchg(&p->rpm_tx_active, 0);
 	if (!rpm_active)
 		return;
-	pm_runtime_mark_last_busy(p->port.dev);
 	pm_runtime_put_autosuspend(p->port.dev);
 }

Signature · 1 line

2.50.1

[email protected]

AI review

2026-09-24 12:28 UTC #2 lore raw

Re: [PATCH v2 1/2] serial: 8250_port: Remove redundant pm_runtime_mark_last_busy() call

Sashiko has reviewed this patch and found no issues. It looks great!

Quoted 6 lines

The pm_runtime_mark_last_busy() call is redundant in the wrapper function as pm_runtime_put_autosuspend() already calls pm_runtime_mark_last_busy() internally to update the last access time of the device before queuing autosuspend.
Signed-off-by Andy Shevchenko <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Andy Shevchenko

author

2026-09-24 12:25 UTC #3 lore raw

PATCH v2 2/2 serial: 8250_port: properly handle runtime PM in IRQ

In reply to Andy Shevchenko

We can't and basically don't need to call runtime PM in IRQ handler. If IRQ is ours, the device must be powered on. Otherwise check if the device is powered off and return immediately.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/tty/serial/8250/8250_port.c +12 -3

index c0bcc0742a60..1e62460b19ab 100644
@@ -1878,13 +1878,22 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq);
 
 static int serial8250_default_handle_irq(struct uart_port *port)
 {
-	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned int iir;
+	int pm_status;
+	int ret;
 
-	guard(serial8250_rpm)(up);
+	/* if driver suspended, return, probably shared interrupt */
+	pm_status = pm_runtime_get_if_active(port->dev);
+	if (!pm_status)
+		return 0;
 
 	iir = serial_port_in(port, UART_IIR);
-	return serial8250_handle_irq(port, iir);
+	ret = serial8250_handle_irq(port, iir);
+
+	if (pm_status > 0)
+		pm_runtime_put_autosuspend(port->dev);
+
+	return ret;
 }
 
 /*

Signature · 1 line

2.50.1

[email protected]

AI review

2026-09-24 12:40 UTC #4 lore raw

Re: [PATCH v2 2/2] serial: 8250_port: properly handle runtime PM in IRQ

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] serial: 8250: missing port->dev NULL check in serial8250_default_handle_irq() --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: missing port->dev NULL check in serial8250_default_handle_irq() --

commit 128116d52224b04215dd65b2f4f799e8c14bdec7
Author: Andy Shevchenko <[email protected]>

serial: 8250_port: properly handle runtime PM in IRQ

This commit updates the 8250 driver IRQ handler to explicitly check the runtime PM status before processing the interrupt. It returns immediately if the device is powered off, assuming it is a shared interrupt.

Quoted 8 lines

drivers/tty/serial/8250/8250_port.c +0 -0

index c0bcc0742a60c..1e62460b19ab9 100644
@@ -1878,13 +1878,22 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq);
 
 static int serial8250_default_handle_irq(struct uart_port *port)
 {

[ ... ]

drivers/tty/serial/8250/8250_port.c EXPORT_SYMBOL_GPL(serial8250_handle_irq); · line 1886

Quoted 2 lines

drivers/tty/serial/8250/8250_port.c +2 -0

+	/* if driver suspended, return, probably shared interrupt */
+	pm_status = pm_runtime_get_if_active(port->dev);

[Severity: High]
Will this trigger a kernel panic if the port has no associated device? Some ports, such as legacy ISA serial ports, might have a NULL port->dev. Calling pm_runtime_get_if_active() with a NULL pointer would unconditionally dereference it.

drivers/tty/serial/8250/8250_port.c EXPORT_SYMBOL_GPL(serial8250_handle_irq); · line 1894

Quoted 9 lines

drivers/tty/serial/8250/8250_port.c +6 -1

+	if (!pm_status)
+		return 0;
 
 	iir = serial_port_in(port, UART_IIR);
-	return serial8250_handle_irq(port, iir);
+	ret = serial8250_handle_irq(port, iir);
+
+	if (pm_status > 0)
+		pm_runtime_put_autosuspend(port->dev);

[Severity: High]
Similarly, does this put operation need a check to ensure port->dev is valid before passing it to pm_runtime_put_autosuspend()?

Quoted 3 lines

drivers/tty/serial/8250/8250_port.c +2 -0
+
+	return ret;
 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Andy Shevchenko

author

2026-09-25 17:00 UTC #5 lore raw

Re: [PATCH v2 2/2] serial: 8250_port: properly handle runtime PM in IRQ

Quoted 3 lines · On Thu, Sep 24, 2026 at 12:40:41PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: missing port->dev NULL check in serial8250_default_handle_irq() --

Isn't this a preexisting issue since serial8250_rpm_get() is implemented this way
https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/tty/serial/8250/8250_port.c#L517
?

Yes, only for platforms with UART_CAP_RPM, but it doesn't matter, they have no special handling for the port in this case. (And pm_runtime_get_if_active() actually works properly on those which do not set that flag as we only check pm_status for 0 and don't care for the other cases.)

Quoted 30 lines

commit 128116d52224b04215dd65b2f4f799e8c14bdec7
Author: Andy Shevchenko <[email protected]>
serial: 8250_port: properly handle runtime PM in IRQ
This commit updates the 8250 driver IRQ handler to explicitly check the runtime PM status before processing the interrupt. It returns immediately if the device is powered off, assuming it is a shared interrupt.
Quoted 8 lines
drivers/tty/serial/8250/8250_port.c +0 -0
index c0bcc0742a60c..1e62460b19ab9 100644
@@ -1878,13 +1878,22 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq);
 
 static int serial8250_default_handle_irq(struct uart_port *port)
 {
[ ... ]
drivers/tty/serial/8250/8250_port.c EXPORT_SYMBOL_GPL(serial8250_handle_irq); · line 1886
Quoted 2 lines
drivers/tty/serial/8250/8250_port.c +2 -0
+	/* if driver suspended, return, probably shared interrupt */
+	pm_status = pm_runtime_get_if_active(port->dev);
[Severity: High]
Will this trigger a kernel panic if the port has no associated device? Some ports, such as legacy ISA serial ports, might have a NULL port->dev. Calling pm_runtime_get_if_active() with a NULL pointer would unconditionally dereference it.
Quoted 3 lines
drivers/tty/serial/8250/8250_port.c +2 -0
+	if (!pm_status)
+		return 0;

Quoted 10 lines

drivers/tty/serial/8250/8250_port.c EXPORT_SYMBOL_GPL(serial8250_handle_irq); · line 1894
Quoted 6 lines
drivers/tty/serial/8250/8250_port.c +4 -1
 	iir = serial_port_in(port, UART_IIR);
-	return serial8250_handle_irq(port, iir);
+	ret = serial8250_handle_irq(port, iir);
+
+	if (pm_status > 0)
+		pm_runtime_put_autosuspend(port->dev);
[Severity: High]
Similarly, does this put operation need a check to ensure port->dev is valid before passing it to pm_runtime_put_autosuspend()?

Same as above. We check pm_status. It can't be positive if above case failed already.

Quoted 2 lines

Quoted 2 lines
drivers/tty/serial/8250/8250_port.c +1 -0
+	return ret;
 }

Signature · 2 lines

With Best Regards,
Andy Shevchenko