Re: [PATCH v7 7/8] serial: qcom-geni: Enable PM runtime for serial driver

["Alexey Klimov" <alexey.klimov@linaro.org>]

(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 <quic_ptalari@quicinc.com>


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





> ---
> 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)
>  };
>