[PATCH] soundwire: qcom: force runtime suspend across system sleep

[PATCH] soundwire: qcom: force runtime suspend across system sleep

[Gopi Chandra Sekhar Reddy Kanala via B4 Relay]

From: Gopi Chandra Sekhar Reddy Kanala <gopi.kanala@oss.qualcomm.com>

The controller only implements runtime PM callbacks and relies on
runtime suspend to stop the bus clock, gate hclk and arm the wake IRQ.
That is not enough across system suspend: from the prepare phase until
the complete phase the PM core holds a runtime PM usage count on every
device, so a controller that is runtime active at that point stays
active until the system resumes. This happens when suspend is entered
within the 3 second autosuspend delay after bus activity, or when a
peripheral driver resumes its device from its own system suspend
callback, as the SDCA class function driver does to quiesce its
interrupts.

The bus clock then keeps running and hclk stays enabled for the whole
suspend. On Qualcomm platforms the LPASS clock votes behind it are held
as well, which keeps CX and DDR from collapsing in s2idle.

Use pm_runtime_force_suspend() and pm_runtime_force_resume() as the
system sleep callbacks, so the runtime suspend path always runs at
system suspend and is undone on resume. Peripherals are children of the
bus, so they are suspended before the controller and resumed after it.

Signed-off-by: Gopi Chandra Sekhar Reddy Kanala <gopi.kanala@oss.qualcomm.com>
---
Tested on Glymur CRD (WSA884x speakers on two controllers) with
next-20261110. When s2idle is entered within the autosuspend delay
after playback, both controllers now stop the bus clock and gate hclk
during suspend, and playback works after resume.
---
 drivers/soundwire/qcom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index a4cb62897192..6884ddbe69b3 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -1902,6 +1902,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
 }
 
 static const struct dev_pm_ops swrm_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
 	SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL)
 };
 

---
base-commit: 793807ca13bf3072fc6d16ce6d90e9ab9beff2a5
change-id: 20261112-qcom-swr-system-sleep-e42144d69ecb

Best regards,
-- 
Gopi Chandra Sekhar Reddy Kanala <gopi.kanala@oss.qualcomm.com>

Re: [PATCH] soundwire: qcom: force runtime suspend across system sleep

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] soundwire: qcom: bus reset on force resume skips slave and
  stream re-init
- [Medium] soundwire: qcom: pm_runtime_force_suspend() leaves wake_irq
  active with RPM off
- [Low] soundwire: qcom: missing pm_ptr() and modern PM macros on
  swrm_dev_pm_ops
- [Low] soundwire: qcom: missing Fixes: trailer for commit 74e79da9fd46
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] soundwire: qcom: clock underflow and missing PM cleanup in
  qcom_swrm_remove() [1]
--

commit 2c857f79ae3cadb1038a8f3f2f64e481a9560e45
Author: Gopi Chandra Sekhar Reddy Kanala <gopi.kanala@oss.qualcomm.com>

> soundwire: qcom: force runtime suspend across system sleep
>
> The controller only implements runtime PM callbacks and relies on
> runtime suspend to stop the bus clock, gate hclk and arm the wake IRQ.
> That is not enough across system suspend: from the prepare phase until
> the complete phase the PM core holds a runtime PM usage count on every
> device, so a controller that is runtime active at that point stays
> active until the system resumes. This happens when suspend is entered
> within the 3 second autosuspend delay after bus activity, or when a
> peripheral driver resumes its device from its own system suspend
> callback, as the SDCA class function driver does to quiesce its
> interrupts.
>
> The bus clock then keeps running and hclk stays enabled for the whole
> suspend. On Qualcomm platforms the LPASS clock votes behind it are held
> as well, which keeps CX and DDR from collapsing in s2idle.
>
> Use pm_runtime_force_suspend() and pm_runtime_force_resume() as the
> system sleep callbacks, so the runtime suspend path always runs at
> system suspend and is undone on resume. Peripherals are children of the
> bus, so they are suspended before the controller and resumed after it.
>
> Signed-off-by: Gopi Chandra Sekhar Reddy Kanala <gopi.kanala@oss.qualcomm.com>

[Severity: Low]
This fixes the missing system sleep PM handling from when runtime PM support
was added.  Should it include:

  Fixes: 74e79da9fd46 ("soundwire: qcom: add runtime pm support")

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index a4cb62897192e..6884ddbe69b32 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -1902,6 +1902,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops swrm_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>  	SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL)
>  };
>  

[Severity: High]
Does forcing swrm_runtime_suspend() and swrm_runtime_resume() across system
sleep skip slave and stream re-initialization when
ctrl->clock_stop_not_supported is true?

SoundWire slave drivers such as wsa881x only define runtime PM callbacks.
When system suspend is entered within the autosuspend window, the PM core
holds a runtime PM reference on the slaves, so they remain active and
slave->status stays SDW_SLAVE_ATTACHED.  On resume, swrm_runtime_resume()
resets the controller and bus without calling sdw_clear_slave_status()
first:

drivers/soundwire/qcom.c:swrm_runtime_resume() {
	if (ctrl->clock_stop_not_supported) {
		reinit_completion(&ctrl->enumeration);
		ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01);
		...
		qcom_swrm_init(ctrl);
		...
		qcom_swrm_get_device_status(ctrl);
		sdw_handle_slave_status(&ctrl->bus, ctrl->status);
	}
}

Because slave->status is still SDW_SLAVE_ATTACHED when the slave
re-enumerates, sdw_handle_slave_status() breaks out early without calling
sdw_initialize_slave() or notifying the codec that it was reset.  If a PCM
stream was suspended while open, SWRM_COMP_SW_RESET also wipes the port
registers while *stream_prepared in qcom_snd_sdw_prepare() remains true,
breaking playback after resume.

[Severity: Medium]
Can qcom_swrm_wake_irq_handler() leave ctrl->wake_irq disabled without
resuming the controller if an in-band wake fires while runtime PM is
disabled by pm_runtime_force_suspend()?

pm_runtime_force_suspend() disables runtime PM on ctrl->dev in dpm_suspend()
before swrm_runtime_suspend() enables ctrl->wake_irq, and device IRQs stay
enabled until dpm_suspend_noirq() and are re-enabled in dpm_resume_noirq()
before pm_runtime_force_resume() runs.  If a peripheral asserts the
level-high wake line during those windows, qcom_swrm_wake_irq_handler() gets
-EACCES from pm_runtime_get_sync(ctrl->dev), treats it as success, and calls
disable_irq_nosync(ctrl->wake_irq):

drivers/soundwire/qcom.c:qcom_swrm_wake_irq_handler() {
	ret = pm_runtime_get_sync(ctrl->dev);
	if (ret < 0 && ret != -EACCES) {
		...
		return ret;
	}

	if (ctrl->wake_irq > 0) {
		if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq)))
			disable_irq_nosync(ctrl->wake_irq);
	}
}

When dev->power.needs_force_resume is not set (for example when ctrl->dev or
its child master device was already runtime-suspended before system sleep),
pm_runtime_force_resume() does not call swrm_runtime_resume(), leaving the
controller runtime-suspended with the wake unserviced and ctrl->wake_irq
disabled.

[Severity: Low]
This isn't a bug, but could swrm_dev_pm_ops use DEFINE_RUNTIME_DEV_PM_OPS()
(or SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS()) together with
pm_ptr(&swrm_dev_pm_ops) on qcom_swrm_driver instead of the deprecated
SET_*_PM_OPS() macros and __maybe_unused annotations?

[Severity: High]
This problem wasn't introduced by this patch, but can qcom_swrm_remove()
underflow ctrl->hclk [1] when the device is unbound while
runtime-suspended?

swrm_runtime_suspend() calls clk_disable_unprepare(ctrl->hclk), and
qcom_swrm_remove() unconditionally calls clk_disable_unprepare(ctrl->hclk)
again without waking the device or calling pm_runtime_disable():

drivers/soundwire/qcom.c:qcom_swrm_remove() {
	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);

	sdw_bus_master_delete(&ctrl->bus);
	clk_disable_unprepare(ctrl->hclk);
}

Unbinding while runtime-suspended triggers a clock enable_count underflow
warning and leaves runtime PM enabled on the device.

[1] https://sashiko.dev/#/bug/linux-e26e04cd-3dec-4a3d-94e6-f45403c4bb57

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-qcom-swr-system-sleep-v1-1-a61a83f6475d@oss.qualcomm.com?part=1