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.

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 | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

2.50.1

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.

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

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);
 }
 

2.50.1

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.

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

index c0bcc0742a60..01703a707c22 100644
@@ -1880,11 +1880,21 @@ 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;
 }
 
 /*

2.50.1

CC drivers/tty/serial/8250/8250_port.o
drivers/tty/serial/8250/8250_port.c: In function ‘serial8250_default_handle_irq’:
drivers/tty/serial/8250/8250_port.c:1897:32: error: unused variable ‘up’ [-Werror=unused-variable]
1897 | struct uart_8250_port *up = up_to_u8250p(port);
| ^~
cc1: all warnings being treated as errors

:(

...

Ouch, rebase issue I believe. Thanks for catching this, I will fix in v2!

With Best Regards,
Andy Shevchenko