[PATCH 0/3] soundwire: pm runtime improvements
This series provides a solution to solve a corner case issue where the manager device may become pm_runtime active, but without ALSA/ASoC requesting any functionality from the peripherals. In this case, the hardware peripheral device will report as ATTACHED and its initialization routine will be executed. If this initialization routine initiates any sort of deferred processing, there is a possibility that the manager could suspend without the peripheral suspend sequence being invoked: from the pm_runtime framework perspective, the peripheral is *already* suspended.
Pierre-Louis Bossart (3): soundwire: intel: prevent pm_runtime resume prior to system suspend soundwire: intel: disable WAKEEN in pm_runtime resume soundwire: bus: pm_runtime_request_resume on peripheral attachment
drivers/soundwire/bus.c | 12 ++++++++++++ drivers/soundwire/intel.c | 6 ++++++ 2 files changed, 18 insertions(+)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
commit e38f9ff63e6d ("ACPI: scan: Do not add device IDs from _CID if _HID is not valid") exposes a race condition on a TGL RVP device leading to a timeout.
The detailed analysis shows the RT711 codec driver scheduling a jack detection workqueue while attaching during a spurious pm_runtime resume, and the work function happens to be scheduled after the manager device is suspended.
The direct link between this ACPI patch and a spurious pm_runtime resume is not obvious; the most likely explanation is that a change in the ACPI device linked list management modifies the order in which the pm_runtime device status is checked and exposes a race condition that was probably present for a very long time, but was not identified.
We already have a check in the .prepare stage, where we will resume to full power from specific clock-stop modes. In all other cases, we don't need to resume to full power by default. Adding the SMART_SUSPEND flag prevents the spurious resume from happening.
BugLink: https://github.com/thesofproject/linux/issues/3459 Fixes: 029bfd1cd53cd ("soundwire: intel: conditionally exit clock stop mode on system suspend") 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.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 63101f1ba271..32e5fdb823c4 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1293,6 +1293,9 @@ static int intel_link_probe(struct auxiliary_device *auxdev, /* use generic bandwidth allocation algorithm */ sdw->cdns.bus.compute_params = sdw_compute_params;
+ /* avoid resuming from pm_runtime suspend if it's not required */ + dev_pm_set_driver_flags(dev, DPM_FLAG_SMART_SUSPEND); + ret = sdw_bus_master_add(bus, dev, dev->fwnode); if (ret) { dev_err(dev, "sdw_bus_master_add fail: %d\n", ret);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
When the manager device is pm_runtime resumed, we see a series of spurious wakes and attempts to resume the same device:
soundwire_intel.link.0: intel_resume_runtime: start soundwire_intel.link.0: intel_link_power_up: powering up all links soundwire_intel.link.0: intel_link_power_up: first link up, programming SYNCPRD soundwire_intel.link.0: intel_shim_wake: WAKEEN disabled for link 0 soundwire_intel.link.0: intel_link_process_wakeen_event: pm_request_resume start soundwire_intel.link.0: intel_link_process_wakeen_event: pm_request_resume done soundwire_intel.link.0: intel_shim_wake: WAKEEN disabled for link 0 soundwire_intel.link.0: intel_link_process_wakeen_event: pm_request_resume start soundwire_intel.link.0: intel_link_process_wakeen_event: pm_request_resume done
This sequence does not break anything but is totally unnecessary.
Currently the wakes are only disabled after the peripheral generates a wake, e.g. for jack detection.
If the resume is initiated by the host drivers as a result of userspace actions (play/record typically), we need to disable wake detection as well. Doing so prevents the spurious wakes and calls to pm_request_resume().
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.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 32e5fdb823c4..4b458e5e7336 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1831,6 +1831,9 @@ static int __maybe_unused intel_resume_runtime(struct device *dev) return 0; }
+ /* unconditionally disable WAKEEN interrupt */ + intel_shim_wake(sdw, false); + link_flags = md_flags >> (bus->link_id * 8); multi_link = !(link_flags & SDW_INTEL_MASTER_DISABLE_MULTI_LINK);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
In typical use cases, the peripheral becomes pm_runtime active as a result of the ALSA/ASoC framework starting up a DAI. The parent/child hierarchy guarantees that the manager device will be fully resumed beforehand.
There is however a corner case where the manager device may become pm_runtime active, but without ALSA/ASoC requesting any functionality from the peripherals. In this case, the hardware peripheral device will report as ATTACHED and its initialization routine will be executed. If this initialization routine initiates any sort of deferred processing, there is a possibility that the manager could suspend without the peripheral suspend sequence being invoked: from the pm_runtime framework perspective, the peripheral is *already* suspended.
To avoid such disconnects between hardware state and pm_runtime state, this patch adds an asynchronous pm_request_resume() upon successful attach/initialization which will result in the proper resume/suspend sequence to be followed on the peripheral side.
BugLink: https://github.com/thesofproject/linux/issues/3459 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/bus.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 354d3f89366f..8b7a680f388e 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -1838,6 +1838,18 @@ int sdw_handle_slave_status(struct sdw_bus *bus, __func__, slave->dev_num);
complete(&slave->initialization_complete); + + /* + * If the manager became pm_runtime active, the peripherals will be + * restarted and attach, but their pm_runtime status may remain + * suspended. If the 'update_slave_status' callback initiates + * any sort of deferred processing, this processing would not be + * cancelled on pm_runtime suspend. + * To avoid such zombie states, we queue a request to resume. + * This would be a no-op in case the peripheral was being resumed + * by e.g. the ALSA/ASoC framework. + */ + pm_request_resume(&slave->dev); } }
On 20-04-22, 10:32, Bard Liao wrote:
This series provides a solution to solve a corner case issue where the manager device may become pm_runtime active, but without ALSA/ASoC requesting any functionality from the peripherals. In this case, the hardware peripheral device will report as ATTACHED and its initialization routine will be executed. If this initialization routine initiates any sort of deferred processing, there is a possibility that the manager could suspend without the peripheral suspend sequence being invoked: from the pm_runtime framework perspective, the peripheral is *already* suspended.
Applied, thanks
participants (2)
-
Bard Liao -
Vinod Koul