Re: [PATCH v1 3/3] serial: fsl_lpuart: Enable TTY wakeup for wakeup-source ports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: fsl_lpuart: suspend state mismatch in lpuart_uport_is_active()
--

commit 5f417acc28861452465313313b5ef22d373f3ae3
Author: Stefano Radaelli <stefano.r@variscite.com>

serial: fsl_lpuart: Enable TTY wakeup for wakeup-source ports

This commit sets tty_wakeup_default in lpuart_probe() from the wakeup-source
device tree property so the serial core enables TTY wakeup at registration
time.

> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index c8575c9652035..35397152a39b3 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2905,6 +2905,7 @@ static int lpuart_probe(struct platform_device *pdev)
>  		sport->port.ops = &lpuart_pops;
>  	sport->port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_FSL_LPUART_CONSOLE);
>  	sport->port.flags = UPF_BOOT_AUTOCONF;
> +	sport->port.tty_wakeup_default = of_property_read_bool(np, "wakeup-source");

[Severity: High]
Does enabling tty_wakeup_default at registration time cause a suspend and
resume state mismatch between the serial core and lpuart_uport_is_active()
when the TTY device is not open in userspace?

With tty_wakeup_default set, serial_core_add_one_port() enables wakeup on
tty_dev during probe before any process opens the port (when port->tty is
NULL and tty_port_initialized(port) is false).

If the system suspends while the port is closed, lpuart_suspend() calls
uart_suspend_port(), which checks device_may_wakeup(tty_dev) without checking
tty_port_initialized(port):

drivers/tty/serial/serial_core.c:uart_suspend_port() {
	...
	tty_dev = tty_port_find_device(port);
	if (tty_dev && device_may_wakeup(tty_dev)) {
		enable_irq_wake(uport->irq);
		put_device(tty_dev);
		return 0;
	}
	put_device(tty_dev);
	...
}

Because uart_suspend_port() returns early here, it leaves uport->suspended as
0 and skips console_suspend(uport->cons) as well as uart_change_pm(state,
UART_PM_STATE_OFF).

However, lpuart_suspend() then checks lpuart_uport_is_active(sport), which
requires both a non-NULL port->tty and tty_port_initialized(port):

drivers/tty/serial/fsl_lpuart.c:lpuart_uport_is_active() {
	...
	tty = tty_port_tty_get(port);
	if (tty) {
		tty_dev = tty->dev;
		may_wake = tty_dev && device_may_wakeup(tty_dev);
		tty_kref_put(tty);
	}

	if ((tty_port_initialized(port) && may_wake) ||
	    (!console_suspend_enabled && uart_console(&sport->port)))
		return true;

	return false;
}

When the port is not open in userspace and console_suspend_enabled is true,
lpuart_uport_is_active(sport) returns false, causing lpuart_suspend() to take
the else-if branch and disable ipg_clk and baud_clk while the console is
still active and unsuspended:

drivers/tty/serial/fsl_lpuart.c:lpuart_suspend() {
	...
	} else if (pm_runtime_active(sport->port.dev)) {
		lpuart_disable_clks(sport);
		pm_runtime_disable(sport->port.dev);
		pm_runtime_set_suspended(sport->port.dev);
	}
}

Wouldn't any printk() during suspend or early resume then call
lpuart32_console_write() or lpuart_console_write() while the LPUART clocks
are disabled, triggering an unclocked MMIO access (bus hang or synchronous
external abort)?

Similarly, on resume, lpuart_resume() skips lpuart32_hw_setup(),
lpuart_hw_setup(), and lpuart_console_fixup() because
lpuart_uport_is_active(sport) is false, while uart_resume_port() also returns
early without restoring console termios or calling console_resume() because
!uport->suspended && device_may_wakeup(tty_dev) is true:

drivers/tty/serial/serial_core.c:uart_resume_port() {
	...
	if (!uport->suspended && device_may_wakeup(tty_dev)) {
		if (irqd_is_wakeup_set(irq_get_irq_data((uport->irq))))
			disable_irq_wake(uport->irq);
		put_device(tty_dev);
		return 0;
	}
	...
}

If the LPUART loses power during deep sleep while the console TTY is closed,
doesn't this leave the console hardware uninitialized after resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793719239.git.stefano.r@variscite.com?part=3