[PATCH v2] serial: 8250_omap: fix wake irq cleared during suspend

[PATCH v2] serial: 8250_omap: fix wake irq cleared during suspend

[Kendall Willis]

The wake irq was cleared in shutdown(), which runs during the suspend
sequence, making it impossible to wake the system via UART.
Move wake irq setup to probe() and teardown to remove() so the irq
remains armed during suspend when the UART is a wakeup source.

Cc: stable@vger.kernel.org
Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
Signed-off-by: Kendall Willis <k-willis@ti.com>
---
Changes in v2:
- Move dev_pm_clear_wake_irq() before pm_runtime_put_sync() to avoid
  potential race condition.
- Link to v1: https://lore.kernel.org/r/20260824-uart-wakeirq-fix-v1-1-6d81a230a1c4@ti.com
---
 drivers/tty/serial/8250/8250_omap.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 3c7775df27effd309419b413c791b46688d4aacd..ceecb39fb82da255377931dbb5cf83fd57eaf164 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -711,12 +711,6 @@ static int omap_8250_startup(struct uart_port *port)
 	struct uart_8250_dma *dma = &priv->omap8250_dma;
 	int ret;
 
-	if (priv->wakeirq) {
-		ret = dev_pm_set_dedicated_wake_irq(port->dev, priv->wakeirq);
-		if (ret)
-			return ret;
-	}
-
 #ifdef CONFIG_PM
 	up->capabilities |= UART_CAP_RPM;
 #endif
@@ -787,7 +781,6 @@ static void omap_8250_shutdown(struct uart_port *port)
 	}
 
 	disable_irq_nosync(port->irq);
-	dev_pm_clear_wake_irq(port->dev);
 
 	serial8250_release_dma(up);
 	up->dma = NULL;
@@ -1588,6 +1581,12 @@ static int omap8250_probe(struct platform_device *pdev)
 
 	priv->wakeirq = irq_of_parse_and_map(np, 1);
 
+	if (priv->wakeirq) {
+		ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, priv->wakeirq);
+		if (ret)
+			goto err;
+	}
+
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to register 8250 port\n");
@@ -1603,6 +1602,7 @@ static int omap8250_probe(struct platform_device *pdev)
 
 	return 0;
 err:
+	dev_pm_clear_wake_irq(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	flush_work(&priv->qos_work);
@@ -1625,6 +1625,7 @@ static void omap8250_remove(struct platform_device *pdev)
 	omap_8250_shutdown(&up->port);
 	serial8250_unregister_port(priv->line);
 	priv->line = -ENODEV;
+	dev_pm_clear_wake_irq(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	flush_work(&priv->qos_work);

---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260824-uart-wakeirq-fix-33f470ac7245

Best regards,
-- 
Kendall Willis <k-willis@ti.com>

Re: [PATCH v2] serial: 8250_omap: fix wake irq cleared during suspend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The IRQ handler `omap8250_irq()` calls `pm_runtime_get_noresume()` and subsequently accesses hardware registers without checking if the device is active.
- [High] `omap8250_remove()` ignores the failure of `pm_runtime_resume_and_get()`, leading to hardware access on a suspended device and unbalanced runtime PM usage counters.
- [High] Unchecked return value of `pm_runtime_get_sync()` leads to potentially unsafe hardware access in `omap8250_probe()`.
--

commit bc6e22f3c6331f4f33e45d32f550a8d1ce7dad46
Author: Kendall Willis <k-willis@ti.com>

serial: 8250_omap: fix wake irq cleared during suspend

This commit moves the wake irq setup from omap_8250_startup() to
omap8250_probe() and teardown from omap_8250_shutdown() to omap8250_remove().
This ensures the wake irq remains armed during the suspend sequence when the
UART is acting as a wakeup source.

> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c

[ ... ]

> @@ -1588,6 +1581,12 @@ static int omap8250_probe(struct platform_device *pdev)
>  
>  	priv->wakeirq = irq_of_parse_and_map(np, 1);
>  
> +	if (priv->wakeirq) {
> +		ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, priv->wakeirq);
> +		if (ret)
> +			goto err;
> +	}
> +
>  	ret = serial8250_register_8250_port(&up);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "unable to register 8250 port\n");
> @@ -1603,6 +1602,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  
>  	return 0;
>  err:
> +	dev_pm_clear_wake_irq(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_sync(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but I noticed an unchecked return value earlier
in omap8250_probe():

drivers/tty/serial/8250/8250_omap.c:omap8250_probe() {
    ...
    pm_runtime_get_sync(&pdev->dev);
    omap_serial_fill_features_erratas(&up, priv);
    ...
}

Does this risk a bus fault if pm_runtime_get_sync() fails? If the device
fails to resume due to PM domain issues, omap_serial_fill_features_erratas()
will proceed to perform readl operations on the unclocked UART memory space.

>  	flush_work(&priv->qos_work);
> @@ -1625,6 +1625,7 @@ static void omap8250_remove(struct platform_device *pdev)
>  	omap_8250_shutdown(&up->port);
>  	serial8250_unregister_port(priv->line);
>  	priv->line = -ENODEV;
> +	dev_pm_clear_wake_irq(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_sync(&pdev->dev);
>  	flush_work(&priv->qos_work);

[Severity: High]
This is a pre-existing issue, but is it safe to proceed with hardware teardown
if we fail to resume the device in omap8250_remove()?

drivers/tty/serial/8250/8250_omap.c:omap8250_remove() {
    ...
    err = pm_runtime_resume_and_get(&pdev->dev);
    if (err)
        dev_err(&pdev->dev, "Failed to resume hardware\n");

    up = serial8250_get_port(priv->line);
    omap_8250_shutdown(&up->port);
    ...
}

Executing omap_8250_shutdown() on an unclocked device could cause a bus fault.

Additionally, unconditionally calling pm_runtime_put_sync() at the end of
omap8250_remove() after a failed pm_runtime_resume_and_get() might cause the
usage counter to underflow.

[Severity: High]
This is another pre-existing issue, but is there a risk of invalid hardware
access in the interrupt handler?

drivers/tty/serial/8250/8250_omap.c:omap8250_irq() {
    ...
    pm_runtime_get_noresume(port->dev);
    ...
    lsr = serial_port_in(port, UART_LSR);
    ...
}

If the interrupt fires while the device is in a runtime-suspended state (for
instance, via a shared line or spurious interrupt), pm_runtime_get_noresume()
increments the usage counter but does not wake the device. Could the
unconditional read of UART_LSR result in invalid register reads returning
0xffffffff on unclocked hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-uart-wakeirq-fix-v2-1-0b9fba03d9b6@ti.com?part=1

Re: [PATCH v2] serial: 8250_omap: fix wake irq cleared during suspend

[Sebastian Andrzej Siewior]

On 2026-08-25 17:06:07 [-0500], Kendall Willis wrote:
> The wake irq was cleared in shutdown(), which runs during the suspend
> sequence, making it impossible to wake the system via UART.
> Move wake irq setup to probe() and teardown to remove() so the irq
> remains armed during suspend when the UART is a wakeup source.

The omap-serial has the same problem, doesn't it?
I would still like to see an ACK from Tony here.

> Cc: stable@vger.kernel.org
> Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
> Signed-off-by: Kendall Willis <k-willis@ti.com>

Sebastian

Re: [PATCH v2] serial: 8250_omap: fix wake irq cleared during suspend

[Tony Lindgren]

On Wed, Aug 26, 2026 at 04:53:07PM +0200, Sebastian Andrzej Siewior wrote:
> On 2026-08-25 17:06:07 [-0500], Kendall Willis wrote:
> > The wake irq was cleared in shutdown(), which runs during the suspend
> > sequence, making it impossible to wake the system via UART.
> > Move wake irq setup to probe() and teardown to remove() so the irq
> > remains armed during suspend when the UART is a wakeup source.
> 
> The omap-serial has the same problem, doesn't it?

Yes likely.

> I would still like to see an ACK from Tony here.

Maybe there was some deferred probe type issue with getting the wakeirq
earlier.

In any case if there was an issue getting the wakeirq it should be handled
nowadays with -EPROBE_DEFER.

Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>