[PATCH 0/2] soundwire: improve pm_runtime handling

[PATCH 0/2] soundwire: improve pm_runtime handling

[Bard Liao]

This patchset improves the pm_runtime behavior in rare corner cases
identified by the Intel CI in the last 6 months.

a) in stress-tests, it's not uncommon to see the following type of
warnings when the codec reports as ATTACHED

    "rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
    sdw:0:025d:0711:00 but parent (sdw-master-0) is not active"

This warning was not correlated with any functional issue, but it
exposed a design issue on when to enable pm_runtime. The recommended
practice in the pm_runtime documentation is to keep the devices in
'suspended' mode and mark them as 'active' when they are really
functional.

Pierre-Louis Bossart (2):
  soundwire: intel_auxdevice: enable pm_runtime earlier on startup
  soundWire: intel_auxdevice: resume 'sdw-master' on startup and system
    resume

 drivers/soundwire/intel_auxdevice.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/2] soundwire: intel_auxdevice: enable pm_runtime earlier on startup

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

As soon as the bus starts, physical peripheral devices may report as
ATTACHED and set their status with pm_runtime_set_active() in their
update_status()/io_init().

This is problematic with the existing code, since the parent
pm_runtime status is changed to "active" after starting the bus. This
creates a time window where the pm_runtime framework can report an
issue, e.g.

"rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
sdw:0:025d:0711:00 but parent (sdw-master-0) is not active"

This patch enables runtime_pm earlier to make sure the auxiliary
device is pm_runtime active after powering-up, but before starting the
bus.

This problem was exposed by recent changes in the timing of the bus
reset, but was present in this driver since we introduced pm_runtime
support.

Closes: https://github.com/thesofproject/linux/issues/4328
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel_auxdevice.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 0daa6ca9a224..f51c776eeeff 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -248,13 +248,6 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 
 	sdw_intel_debugfs_init(sdw);
 
-	/* start bus */
-	ret = sdw_intel_start_bus(sdw);
-	if (ret) {
-		dev_err(dev, "bus start failed: %d\n", ret);
-		goto err_power_up;
-	}
-
 	/* Enable runtime PM */
 	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME)) {
 		pm_runtime_set_autosuspend_delay(dev,
@@ -266,6 +259,13 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 		pm_runtime_enable(dev);
 	}
 
+	/* start bus */
+	ret = sdw_intel_start_bus(sdw);
+	if (ret) {
+		dev_err(dev, "bus start failed: %d\n", ret);
+		goto err_pm_runtime;
+	}
+
 	clock_stop_quirks = sdw->link_res->clock_stop_quirks;
 	if (clock_stop_quirks & SDW_INTEL_CLK_STOP_NOT_ALLOWED) {
 		/*
@@ -293,12 +293,17 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 	 * with a delay. A more complete solution would require the
 	 * definition of Master properties.
 	 */
-	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE))
+	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE)) {
+		pm_runtime_mark_last_busy(dev);
 		pm_runtime_idle(dev);
+	}
 
 	sdw->startup_done = true;
 	return 0;
 
+err_pm_runtime:
+	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME))
+		pm_runtime_disable(dev);
 err_power_up:
 	sdw_intel_link_power_down(sdw);
 err_init:
-- 
2.25.1

Re: [PATCH 1/2] soundwire: intel_auxdevice: enable pm_runtime earlier on startup

[Charles Keepax]

On Thu, Aug 03, 2023 at 02:52:19PM +0800, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> As soon as the bus starts, physical peripheral devices may report as
> ATTACHED and set their status with pm_runtime_set_active() in their
> update_status()/io_init().
> 
> This is problematic with the existing code, since the parent
> pm_runtime status is changed to "active" after starting the bus. This
> creates a time window where the pm_runtime framework can report an
> issue, e.g.
> 
> "rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
> sdw:0:025d:0711:00 but parent (sdw-master-0) is not active"
> 
> This patch enables runtime_pm earlier to make sure the auxiliary
> device is pm_runtime active after powering-up, but before starting the
> bus.
> 
> This problem was exposed by recent changes in the timing of the bus
> reset, but was present in this driver since we introduced pm_runtime
> support.
> 
> Closes: https://github.com/thesofproject/linux/issues/4328
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---

Doesn't seem to cause any problems on my SoundWire devices, so
very loosely:

Tested-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

[PATCH 2/2] soundWire: intel_auxdevice: resume 'sdw-master' on startup and system resume

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The SoundWire bus is handled with a dedicated device, which is placed
between the Intel auxiliary device and peripheral devices, e.g.

soundwire_intel.link.0/sdw-master-0/sdw:0:025d:0711:01

The functionality of this 'sdw-master' device is limited, specifically
for pm_runtime the ASoC framework will not rely on
pm_runtime_get_sync() since it does not register any components. It
will only change status thanks to the parent-child relationship which
guarantees that the 'sdw-master' device will be pm_runtime resumed
before any peripheral device.

However on startup and system resume it's possible that only the
auxiliary device is pm_runtime active, and the peripheral will only
become active during its io_init routine, leading to another
occurrence of the error reported by the pm_runtime framework:

rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
sdw:0:025d:0711:00 but parent (sdw-master-0) is not active

This patch suggests aligning the sdw-master device status to that of
the auxiliary device. The difference between the two is completely
notional and their pm_status shouldn't be different during the startup
and system resume steps.

This problem was exposed by recent changes in the timing of the bus
reset, but was present in this driver since we introduced pm_runtime
support.

Closes: https://github.com/thesofproject/linux/issues/4328
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel_auxdevice.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index f51c776eeeff..91c86b46a5a1 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -257,6 +257,8 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 
 		pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
+
+		pm_runtime_resume(bus->dev);
 	}
 
 	/* start bus */
@@ -294,6 +296,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 	 * definition of Master properties.
 	 */
 	if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE)) {
+		pm_runtime_mark_last_busy(bus->dev);
 		pm_runtime_mark_last_busy(dev);
 		pm_runtime_idle(dev);
 	}
@@ -557,6 +560,8 @@ static int __maybe_unused intel_resume(struct device *dev)
 		pm_runtime_mark_last_busy(dev);
 		pm_runtime_enable(dev);
 
+		pm_runtime_resume(bus->dev);
+
 		link_flags = md_flags >> (bus->link_id * 8);
 
 		if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE))
@@ -592,6 +597,7 @@ static int __maybe_unused intel_resume(struct device *dev)
 	 * counters and delay the pm_runtime suspend by several
 	 * seconds, by when all enumeration should be complete.
 	 */
+	pm_runtime_mark_last_busy(bus->dev);
 	pm_runtime_mark_last_busy(dev);
 
 	return 0;
-- 
2.25.1

Re: [PATCH 2/2] soundWire: intel_auxdevice: resume 'sdw-master' on startup and system resume

[Charles Keepax]

On Thu, Aug 03, 2023 at 02:52:20PM +0800, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The SoundWire bus is handled with a dedicated device, which is placed
> between the Intel auxiliary device and peripheral devices, e.g.
> 
> soundwire_intel.link.0/sdw-master-0/sdw:0:025d:0711:01
> 
> The functionality of this 'sdw-master' device is limited, specifically
> for pm_runtime the ASoC framework will not rely on
> pm_runtime_get_sync() since it does not register any components. It
> will only change status thanks to the parent-child relationship which
> guarantees that the 'sdw-master' device will be pm_runtime resumed
> before any peripheral device.
> 
> However on startup and system resume it's possible that only the
> auxiliary device is pm_runtime active, and the peripheral will only
> become active during its io_init routine, leading to another
> occurrence of the error reported by the pm_runtime framework:
> 
> rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
> sdw:0:025d:0711:00 but parent (sdw-master-0) is not active
> 
> This patch suggests aligning the sdw-master device status to that of
> the auxiliary device. The difference between the two is completely
> notional and their pm_status shouldn't be different during the startup
> and system resume steps.
> 
> This problem was exposed by recent changes in the timing of the bus
> reset, but was present in this driver since we introduced pm_runtime
> support.
> 
> Closes: https://github.com/thesofproject/linux/issues/4328
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---

Tested-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH 0/2] soundwire: improve pm_runtime handling

[Vinod Koul]

On Thu, 03 Aug 2023 14:52:18 +0800, Bard Liao wrote:
> This patchset improves the pm_runtime behavior in rare corner cases
> identified by the Intel CI in the last 6 months.
> 
> a) in stress-tests, it's not uncommon to see the following type of
> warnings when the codec reports as ATTACHED
> 
>     "rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
>     sdw:0:025d:0711:00 but parent (sdw-master-0) is not active"
> 
> [...]

Applied, thanks!

[1/2] soundwire: intel_auxdevice: enable pm_runtime earlier on startup
      commit: 3d71f43f8a59a62c6ab832d4e73a69bae22e13b7
[2/2] soundWire: intel_auxdevice: resume 'sdw-master' on startup and system resume
      commit: f9031288110589c8f565683a182f194110338d65

Best regards,
-- 
~Vinod