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

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 <[email protected]>

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/[email protected]?part=1