Re: [PATCH v7 7/8] serial: qcom-geni: Enable PM runtime for serial driver
From: "Alexey Klimov" <alexey.klimov@linaro.org>
Date: 2025-08-12 10:05:29
Also in:
linux-arm-msm, linux-serial, lkml
(c/c Neil and Srini) On Mon Jul 21, 2025 at 6:45 PM BST, Praveen Talari wrote:
The GENI serial driver currently handles power resource management through calls to the statically defined geni_serial_resources_on() and geni_serial_resources_off() functions. This approach reduces modularity and limits support for platforms with diverse power management mechanisms, including resource managed by firmware. Improve modularity and enable better integration with platform-specific power management, introduce support for runtime PM. Use pm_runtime_resume_and_get() and pm_runtime_put_sync() within the qcom_geni_serial_pm() callback to control resource power state transitions based on UART power state changes. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Signed-off-by: Praveen Talari <redacted>
This breaks at least RB1 (QRB2210), maybe others. Currently broken on -master and on linux-next. Upon login prompt random parts of kernel seems to be off/failed and debugging led to udev being stuck: [ 85.369834] INFO: task kworker/u16:0:12 blocked for more than 42 seconds. [ 85.376699] Not tainted 6.17.0-rc1-00004-g53e760d89498 #9 [ 85.382660] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 85.390547] task:kworker/u16:0 state:D stack:0 pid:12 tgid:12 ppid:2 task_flags:0x4208060 flags:0x00000010 [ 85.401748] Workqueue: async async_run_entry_fn [ 85.406349] Call trace: [ 85.408828] __switch_to+0xe8/0x1a0 (T) [ 85.412724] __schedule+0x290/0x7c0 [ 85.416275] schedule+0x34/0x118 [ 85.419554] rpm_resume+0x14c/0x66c [ 85.423111] rpm_resume+0x2a4/0x66c [ 85.426647] rpm_resume+0x2a4/0x66c [ 85.430188] rpm_resume+0x2a4/0x66c [ 85.433722] __pm_runtime_resume+0x50/0x9c [ 85.437869] __driver_probe_device+0x58/0x120 [ 85.442287] driver_probe_device+0x3c/0x154 [ 85.446523] __driver_attach_async_helper+0x4c/0xc0 [ 85.451446] async_run_entry_fn+0x34/0xe0 [ 85.455504] process_one_work+0x148/0x290 [ 85.459565] worker_thread+0x2c4/0x3e0 [ 85.463368] kthread+0x118/0x1c0 [ 85.466651] ret_from_fork+0x10/0x20 [ 85.470337] INFO: task irq/92-4a8c000.:71 blocked for more than 42 seconds. [ 85.477351] Not tainted 6.17.0-rc1-00004-g53e760d89498 #9 [ 85.483323] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 85.491195] task:irq/92-4a8c000. state:D stack:0 pid:71 tgid:71 ppid:2 task_flags:0x208040 flags:0x00000010 [ 85.502290] Call trace: [ 85.504786] __switch_to+0xe8/0x1a0 (T) [ 85.508687] __schedule+0x290/0x7c0 [ 85.512231] schedule+0x34/0x118 [ 85.515504] __synchronize_irq+0x60/0xa0 [ 85.519483] disable_irq+0x3c/0x4c [ 85.522929] msm_pinmux_set_mux+0x3a8/0x44c [ 85.527167] pinmux_enable_setting+0x1c4/0x28c [ 85.531665] pinctrl_commit_state+0xa0/0x260 [ 85.535989] pinctrl_pm_select_default_state+0x4c/0xa0 [ 85.541182] geni_se_resources_on+0xd0/0x15c [ 85.545522] geni_serial_resource_state+0x8c/0xbc [ 85.550282] qcom_geni_serial_runtime_resume+0x24/0x3c [ 85.555470] pm_generic_runtime_resume+0x2c/0x44 [ 85.560139] __rpm_callback+0x48/0x1e0 [ 85.563949] rpm_callback+0x74/0x80 [ 85.567494] rpm_resume+0x39c/0x66c [ 85.571040] __pm_runtime_resume+0x50/0x9c [ 85.575193] handle_threaded_wake_irq+0x30/0x80 [ 85.579771] irq_thread_fn+0x2c/0xb0 [ 85.583443] irq_thread+0x16c/0x278 [ 85.587003] kthread+0x118/0x1c0 [ 85.590283] ret_from_fork+0x10/0x20 [ 85.593943] INFO: task (udev-worker):228 blocked for more than 42 seconds. [ 85.600873] Not tainted 6.17.0-rc1-00004-g53e760d89498 #9 [ 85.606846] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 85.614717] task:(udev-worker) state:D stack:0 pid:228 tgid:228 ppid:222 task_flags:0x400140 flags:0x00000818 [ 85.625823] Call trace: [ 85.628316] __switch_to+0xe8/0x1a0 (T) [ 85.632217] __schedule+0x290/0x7c0 [ 85.635765] schedule+0x34/0x118 [ 85.639044] async_synchronize_cookie_domain.part.0+0x50/0xa4 [ 85.644854] async_synchronize_full+0x78/0xa0 [ 85.649270] do_init_module+0x190/0x23c [ 85.653154] load_module+0x1708/0x1ca0 [ 85.656952] init_module_from_file+0x74/0xa0 [ 85.661273] __arm64_sys_finit_module+0x130/0x2f8 [ 85.666023] invoke_syscall+0x48/0x104 [ 85.669842] el0_svc_common.constprop.0+0xc0/0xe0 [ 85.674604] do_el0_svc+0x1c/0x28 [ 85.677973] el0_svc+0x2c/0x84 [ 85.681078] el0t_64_sync_handler+0xa0/0xe4 [ 85.685316] el0t_64_sync+0x198/0x19c [ 85.689032] INFO: task (udev-worker):229 blocked for more than 42 seconds. Usually wifi, all remoteprocs and anything that depends on lpass/pinctrl fail to probe. Reverting these: 86fa39dd6fb7 serial: qcom-geni: Enable Serial on SA8255p Qualcomm platforms 1afa70632c39 serial: qcom-geni: Enable PM runtime for serial driver resolves the regression. Couldn't say if we should go with reverting since 86fa39dd6fb7 adds support of serial on SA8255p and for clean revert both have to be reverted. Any thoughts? Best regards, Alexey
quoted hunk ↗ jump to hunk
--- v6 -> v7 From Bjorn: - used devm_pm_runtime_enable() instead of pm_runtime_enable() - updated commit text. v5 -> v6 - added reviewed-by tag in commit - added __maybe_unused to PM callback functions to avoid warnings of defined but not used --- drivers/tty/serial/qcom_geni_serial.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 81f385d900d0..aa08de659e34 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c@@ -1713,10 +1713,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport, old_state = UART_PM_STATE_OFF; if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) - geni_serial_resources_on(uport); + pm_runtime_resume_and_get(uport->dev); else if (new_state == UART_PM_STATE_OFF && old_state == UART_PM_STATE_ON) - geni_serial_resources_off(uport); + pm_runtime_put_sync(uport->dev); }@@ -1878,6 +1878,8 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) if (ret) return ret; + devm_pm_runtime_enable(port->se.dev); + ret = uart_add_one_port(drv, uport); if (ret) return ret;@@ -1909,6 +1911,22 @@ static void qcom_geni_serial_remove(struct platform_device *pdev) uart_remove_one_port(drv, &port->uport); } +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev) +{ + struct qcom_geni_serial_port *port = dev_get_drvdata(dev); + struct uart_port *uport = &port->uport; + + return geni_serial_resources_off(uport); +} + +static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev) +{ + struct qcom_geni_serial_port *port = dev_get_drvdata(dev); + struct uart_port *uport = &port->uport; + + return geni_serial_resources_on(uport); +} + static int qcom_geni_serial_suspend(struct device *dev) { struct qcom_geni_serial_port *port = dev_get_drvdata(dev);@@ -1952,6 +1970,8 @@ static const struct qcom_geni_device_data 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) };