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

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

[Birk Skyum]

A hibernation image is restored after a power cycle, which resets every
serial engine. The kernel that loads the image only sets up the engines
it uses itself, and on laptops the Bluetooth UART is opened from the
root file system, so that kernel never touches it.

The restored kernel still has port->setup set from before, so the
startup in uart_resume_port() skips qcom_geni_serial_port_setup() and
the port runs on an engine whose FIFOs, packing and transfer mode were
never configured. On a Lenovo Yoga Slim 7x (X1E80100) the WCN7850's
Bluetooth controller then never comes back: every setup attempt of
hci_qca's error recovery ends in "command 0xfc00 tx timeout" and
"Reading QCA version information failed (-110)".

Clear port->setup in a restore callback of its own, so the next startup
sets the engine up again. The other system sleep callbacks are
unchanged.

Signed-off-by: Birk Skyum <birk.skyum@pm.me>
---
Tested on a Lenovo Yoga Slim 7x (X1E80100) with 7.3 and the other
hibernation fixes for this laptop, which are in review:

  pinctrl: qcom: Restore the pin configuration after hibernation (v4)
  https://lore.kernel.org/all/20261021210425.57066-1-birk.skyum@pm.me/
  PCI: qcom: Power the controller down when hibernating (v2), on top of
  the v6 series "PCI: qcom: Implement shutdown() to avoid SMMU/NoC
  errors on reboot"
  https://lore.kernel.org/all/179181074907.22199.10556821368232442334@pm.me/
  soc: qcom: smp2p: Start over after a restore from hibernation
  https://lore.kernel.org/all/179158127177.70185.5691100900886332545@pm.me/
  Bluetooth: hci_qca: Reset the in-band sleep state on power off (v2)
  https://lore.kernel.org/all/20261014180201.57109-1-birk.skyum@pm.me/

The test kernel had a switch for this change, so both ways ran on the
same build, each a real hibernation with a power-off from a fresh boot:

  without   restored after 112 s, Bluetooth never comes back
  with      restored after 89 s, hci_qca's recovery sets the controller
            up about 18 s after the restore and a scan finds devices

pm_test=core and test_resume pass with it as well; there the port is
restored without a power cycle and is simply set up a second time.

7.3 does not have the console pm_runtime_resume() that tty-next adds to
the resume callback. The port here is not a console and this laptop has
no serial console, so console ports were not exercised. For those, the
kernel that loads the image sets the engine up itself when it
registers the console, so clearing port->setup only repeats that at the
next startup.

Built with W=1 and sparse on tty-next with hibernation on, with
hibernation off, and without system sleep.

 drivers/tty/serial/qcom_geni_serial.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index bfdc155c1..4e5b0e247 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);
+}
+
 #if IS_ENABLED(CONFIG_SERIAL_QCOM_GENI_CONSOLE)
 static const struct qcom_geni_device_data qcom_geni_console_data = {
 	.console = true,
@@ -2168,7 +2182,12 @@ static const struct qcom_geni_device_data sa8255p_qcom_geni_uart_data = {
 static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
 	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
 			   qcom_geni_serial_runtime_resume, NULL)
-	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
+	.suspend = pm_sleep_ptr(qcom_geni_serial_suspend),
+	.resume = pm_sleep_ptr(qcom_geni_serial_resume),
+	.freeze = pm_sleep_ptr(qcom_geni_serial_suspend),
+	.thaw = pm_sleep_ptr(qcom_geni_serial_resume),
+	.poweroff = pm_sleep_ptr(qcom_geni_serial_suspend),
+	.restore = pm_sleep_ptr(qcom_geni_serial_restore),
 };
 
 static void qcom_geni_serial_sys_shutdown(struct platform_device *pdev)
-- 
2.56.0

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

[sashiko-bot]

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