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

[sashiko-bot@kernel.org]

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