From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(), and snd_soc_pm_ops is associated with the soc_driver (defined in sound/soc/soc-core.c), and there is no parent-child relationship between the soc_driver and the DA7213 codec driver, the power management subsystem does not enforce a specific suspend/resume order between the DA7213 driver and the soc_driver.
Because of this, the different codec component functionalities, called from snd_soc_resume() to reconfigure various functions, can race with the DA7213 resume function, leading to misapplied configuration. This occasionally results in clipped sound.
Fix this by moving the regmap cache operations into struct snd_soc_component_driver::{suspend, resume}. This ensures the proper configuration sequence is handled by the ASoC subsystem.
Cc: stable@vger.kernel.org Fixes: 431e040065c8 ("ASoC: da7213: Add suspend to RAM support") Signed-off-by: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com --- sound/soc/codecs/da7213.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c index c1657f348ad9..051806982bbe 100644 --- a/sound/soc/codecs/da7213.c +++ b/sound/soc/codecs/da7213.c @@ -2124,11 +2124,31 @@ static int da7213_probe(struct snd_soc_component *component) return 0; }
+static int da7213_suspend(struct snd_soc_component *component) +{ + struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component); + + regcache_cache_only(da7213->regmap, true); + regcache_mark_dirty(da7213->regmap); + + return 0; +} + +static int da7213_resume(struct snd_soc_component *component) +{ + struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component); + + regcache_cache_only(da7213->regmap, false); + return regcache_sync(da7213->regmap); +} + static const struct snd_soc_component_driver soc_component_dev_da7213 = { .probe = da7213_probe, .set_bias_level = da7213_set_bias_level, .controls = da7213_snd_controls, .num_controls = ARRAY_SIZE(da7213_snd_controls), + .suspend = da7213_suspend, + .resume = da7213_resume, .dapm_widgets = da7213_dapm_widgets, .num_dapm_widgets = ARRAY_SIZE(da7213_dapm_widgets), .dapm_routes = da7213_audio_map, @@ -2228,27 +2248,19 @@ static int da7213_runtime_suspend(struct device *dev) { struct da7213_priv *da7213 = dev_get_drvdata(dev);
- regcache_cache_only(da7213->regmap, true); - regcache_mark_dirty(da7213->regmap); - regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies); - - return 0; + return regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies); }
static int da7213_runtime_resume(struct device *dev) { struct da7213_priv *da7213 = dev_get_drvdata(dev); - int ret;
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies); - if (ret < 0) - return ret; - regcache_cache_only(da7213->regmap, false); - return regcache_sync(da7213->regmap); + return regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies); }
-static DEFINE_RUNTIME_DEV_PM_OPS(da7213_pm, da7213_runtime_suspend, - da7213_runtime_resume, NULL); +static const struct dev_pm_ops da7213_pm = { + RUNTIME_PM_OPS(da7213_runtime_suspend, da7213_runtime_resume, NULL) +};
static const struct i2c_device_id da7213_i2c_id[] = { { "da7213" },
On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(), and snd_soc_pm_ops is associated with the soc_driver (defined in sound/soc/soc-core.c), and there is no parent-child relationship between the soc_driver and the DA7213 codec driver, the power management subsystem does not enforce a specific suspend/resume order between the DA7213 driver and the soc_driver.
The theory here is that the power management core uses the device instantiation order for both suspend and resume (reversed on suspend) so the fact that we use probe deferral to make sure that the card components are ready should ensure that the card suspends before anything in the card. If that is no longer the case then we need to ensure that all drivers have system PM ops which trigger the card, this won't be a driver specific issue.
static int da7213_runtime_resume(struct device *dev) { struct da7213_priv *da7213 = dev_get_drvdata(dev);
- int ret;
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
- if (ret < 0)
return ret;- regcache_cache_only(da7213->regmap, false);
- return regcache_sync(da7213->regmap);
- return regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
}
This seems obviously buggy, we just power on the device and don't sync the register state. If the device actually lost power during a runtime suspend then we'll end up having a bad time. There was also no mention of runtime PM in the patch description...
Hi, Mark,
On 10/29/25 16:37, Mark Brown wrote:
On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(), and snd_soc_pm_ops is associated with the soc_driver (defined in sound/soc/soc-core.c), and there is no parent-child relationship between the soc_driver and the DA7213 codec driver, the power management subsystem does not enforce a specific suspend/resume order between the DA7213 driver and the soc_driver.
The theory here is that the power management core uses the device instantiation order for both suspend and resume (reversed on suspend) so the fact that we use probe deferral to make sure that the card components are ready should ensure that the card suspends before anything in the card. If that is no longer the case then we need to ensure that all drivers have system PM ops which trigger the card, this won't be a driver specific issue.
I also saw the behavior described in this commit with the rz-ssi.c driver as well. The fix there was commit c1b0f5183a44 ("ASoC: renesas: rz-ssi: Use NOIRQ_SYSTEM_SLEEP_PM_OPS()").
In case of this this codec, I saw the code in da7213_runtime_resume() and soc_resume_deferred() racing each other on system resume.
static int da7213_runtime_resume(struct device *dev) { struct da7213_priv *da7213 = dev_get_drvdata(dev);
- int ret;
- ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
- if (ret < 0)
return ret;- regcache_cache_only(da7213->regmap, false);
- return regcache_sync(da7213->regmap);
- return regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies); }
This seems obviously buggy, we just power on the device and don't sync the register state.
You're right! I'll revisit this.
If the device actually lost power during a runtime suspend then we'll end up having a bad time. There was also no mention of runtime PM in the patch description...
I had no issues with runtime PM, but only with suspend to RAM, when this function was called though struct dev_pm_ops::resume = pm_runtime_force_resume().
Would keeping the regcache_cache_only() + regcache_sync() here along with populating the struct snd_soc_component_driver::{suspend, resume} be an acceptable solution for you? I think that will work as well.
Thank you for your review, Claudiu
On Wed, Oct 29, 2025 at 05:22:26PM +0200, claudiu beznea wrote:
On 10/29/25 16:37, Mark Brown wrote:
The theory here is that the power management core uses the device instantiation order for both suspend and resume (reversed on suspend) so the fact that we use probe deferral to make sure that the card components are ready should ensure that the card suspends before anything in the card. If that is no longer the case then we need to ensure that all drivers have system PM ops which trigger the card, this won't be a driver specific issue.
I also saw the behavior described in this commit with the rz-ssi.c driver as well. The fix there was commit c1b0f5183a44 ("ASoC: renesas: rz-ssi: Use NOIRQ_SYSTEM_SLEEP_PM_OPS()").
In case of this this codec, I saw the code in da7213_runtime_resume() and soc_resume_deferred() racing each other on system resume.
So I guess we need the more general fix then, with everything calling into the core to suspend the cards.
If the device actually lost power during a runtime suspend then we'll end up having a bad time. There was also no mention of runtime PM in the patch description...
I had no issues with runtime PM, but only with suspend to RAM, when this function was called though struct dev_pm_ops::resume = pm_runtime_force_resume().
Calling regulator_disable() doesn't guarantee the regulator will be disabled, the constraints or other devices can ensure that the device retains power so there's no effect on the actual hardware.
Would keeping the regcache_cache_only() + regcache_sync() here along with populating the struct snd_soc_component_driver::{suspend, resume} be an acceptable solution for you? I think that will work as well.
I'm not sure what you're intending to populate the component with there.
Hi, Mark,
On 10/29/25 18:14, Mark Brown wrote:
Would keeping the regcache_cache_only() + regcache_sync() here along with populating the struct snd_soc_component_driver::{suspend, resume} be an acceptable solution for you? I think that will work as well.
I'm not sure what you're intending to populate the component with there.
Sorry for the late reply, I took the chance and prepared a new version showing what I intended to say here. v2 posted here:
https://lore.kernel.org/all/20251104114914.2060603-1-claudiu.beznea.uj@bp.re...
Thank you, Claudiu
On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(), and snd_soc_pm_ops is associated with the soc_driver (defined in sound/soc/soc-core.c), and there is no parent-child relationship between the soc_driver and the DA7213 codec driver, the power management subsystem does not enforce a specific suspend/resume order between the DA7213 driver and the soc_driver.
Oh, also:
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
On 10/29/25 16:37, Mark Brown wrote:
On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(), and snd_soc_pm_ops is associated with the soc_driver (defined in sound/soc/soc-core.c), and there is no parent-child relationship between the soc_driver and the DA7213 codec driver, the power management subsystem does not enforce a specific suspend/resume order between the DA7213 driver and the soc_driver.
Oh, also:
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
I messed this up. I'll be more careful next time.
Thank you, Claudiu
On 29/10/2025 2:11 pm, Claudiu wrote:
From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(), and snd_soc_pm_ops is associated with the soc_driver (defined in sound/soc/soc-core.c), and there is no parent-child relationship between the soc_driver and the DA7213 codec driver, the power management subsystem does not enforce a specific suspend/resume order between the DA7213 driver and the soc_driver.
Because of this, the different codec component functionalities, called from snd_soc_resume() to reconfigure various functions, can race with the DA7213 resume function, leading to misapplied configuration. This occasionally results in clipped sound.
Fix this by moving the regmap cache operations into struct snd_soc_component_driver::{suspend, resume}. This ensures the proper configuration sequence is handled by the ASoC subsystem.
Cc: stable@vger.kernel.org Fixes: 431e040065c8 ("ASoC: da7213: Add suspend to RAM support") Signed-off-by: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
sound/soc/codecs/da7213.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
The commit title starts "ASoC: codecs:", which implies that this patch affects multiple codecs. But it only changes one file.
The commit title prefix should be "ASoC: da7213:", same as the commit you reference in the Fixes: tag.
linux-stable-mirror@lists.linaro.org
-
Claudiu -
Claudiu Beznea
-
claudiu beznea
-
Mark Brown -
Richard Fitzgerald