Re: [PATCH] serial: qcom-geni: Set the serial engine up again after hibernation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: qcom-geni: unsafe console re-setup in qcom_geni_serial_restore()
--

commit 987848026d2292ba1e5b2bfaa3e913a58fce2b92
Author: Birk Skyum <birk.skyum@pm.me>

serial: qcom-geni: Set the serial engine up again after hibernation

This commit adds a hibernation restore callback that clears port->setup
before calling qcom_geni_serial_resume() so that the serial engine is
re-initialized when the port starts up after a hibernation power cycle.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index bfdc155c128da..4e5b0e2473aa2 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -2131,6 +2131,20 @@ static int qcom_geni_serial_resume(struct device *dev)
>  	return ret;
>  }
>  
> +/*
> + * A hibernation image is restored after a power cycle, which reset the serial
> + * engine. The kernel that loaded the image only set up the engines it used
> + * itself, so set this one up again when the port starts.
> + */
> +static int qcom_geni_serial_restore(struct device *dev)
> +{
> +	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> +
> +	port->setup = false;
> +
> +	return qcom_geni_serial_resume(dev);
> +}

[Severity: High]
Can clearing port->setup here and deferring qcom_geni_serial_port_setup() to
qcom_geni_serial_startup() cause ordering and concurrency issues for console
ports during hibernation restore?

When qcom_geni_serial_restore() calls qcom_geni_serial_resume() ->
uart_resume_port() in drivers/tty/serial/serial_core.c, the console
restoration block runs before ops->startup(uport):

drivers/tty/serial/serial_core.c:uart_resume_port() {
	if (uart_console(uport)) {
		...
		if (console_suspend_enabled)
			uart_change_pm(state, UART_PM_STATE_ON);
		uport->ops->set_termios(uport, &termios, NULL);
		if (!console_suspend_enabled && uport->ops->start_rx) {
			guard(uart_port_lock_irq)(uport);
			uport->ops->start_rx(uport);
		}
		if (console_suspend_enabled)
			console_resume(uport->cons);
	}

	if (tty_port_suspended(port)) {
		...
		if (console_suspend_enabled || !uart_console(uport)) {
			struct tty_struct *tty = port->tty;

			ret = ops->startup(uport);
}

If the kernel that loaded the hibernation image did not initialize the
console serial engine (for instance, when the driver was built as a module
not loaded before restore, when the boot kernel command line did not enable
the console, or on sa8255p where the power domain is powered off during
quiesce), uport->ops->set_termios(), uport->ops->start_rx(), and
console_resume() will run before qcom_geni_serial_port_setup() configures
FIFO packing, watermarks, and FIFO mode. Furthermore, if
tty_port_suspended(port) is false (no userspace tty open on the console port
at hibernation time) or console_suspend_enabled is false, ops->startup(uport)
is not called by uart_resume_port() at all.

Conversely, when qcom_geni_serial_startup() does run on a console port with
port->setup == false (either inside uart_resume_port() immediately after
console_resume() has re-enabled NBCON console output, or later when userspace
opens the console tty device), it invokes qcom_geni_serial_port_setup()
without holding uport->lock (which serves as the NBCON device_lock in
qcom_geni_serial_console_device_lock()) or NBCON console ownership:

drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_startup() {
	if (!port->setup) {
		ret = qcom_geni_serial_port_setup(uport);
		if (ret) {
			pm_runtime_put_sync(uport->dev);
			return ret;
		}
	}
	...
	uart_port_lock_irq(uport);
	qcom_geni_serial_start_rx(uport);
	uart_port_unlock_irq(uport);
}

Inside qcom_geni_serial_port_setup(), qcom_geni_serial_stop_rx(),
qcom_geni_serial_poll_tx_done() (which can issue M_GENI_CMD_ABORT),
geni_se_config_packing(), geni_se_init() (which writes FORCE_DEFAULT and
clears all M/S IRQ status bits via geni_se_irq_clear()), and
geni_se_select_mode() can then race with concurrent NBCON console writes in
__qcom_geni_serial_console_write().

Should console ports instead be re-initialized in the restore path before
uart_resume_port() (after runtime resume), or while holding the appropriate
port/console lock before console_resume()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027203136.95971-1-birk.skyum@pm.me?part=1