[PATCH 0/5] ASoC: fsl_asrc: Fix probe and resource management issues

[PATCH 0/5] ASoC: fsl_asrc: Fix probe and resource management issues

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all,

This series improves error handling and resource management in
the fsl_asrc probe function. It fixes deferred clock handling and
runtime PM usage count, ensures the IRQ is requested after device
initialization, and moves runtime PM cleanup to devm.

Compile-tested only.

Best regards,
Phuc

bui duc phuc (5):
  ASoC: fsl_asrc: Improve probe error handling
  ASoC: fsl_asrc: Fix handling of deferred spba clock
  ASoC: fsl_asrc: Fix runtime PM usage count on init failure
  ASoC: fsl_asrc: Request the IRQ after the device is initialized
  ASoC: fsl_asrc: Use devm for runtime PM cleanup

 sound/soc/fsl/fsl_asrc.c | 92 ++++++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH 1/5] ASoC: fsl_asrc: Improve probe error handling

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Use dev_err_probe() when retrieving clocks to avoid logging errors
for deferred probes. Also remove redundant error messages from
the probe function when failures are already reported deeper in
the call chain.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/fsl/fsl_asrc.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index faf348dd582d..055593d5d37b 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -1301,22 +1301,18 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, irq, fsl_asrc_isr, 0,
 			       dev_name(&pdev->dev), asrc);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to claim irq %u: %d\n", irq, ret);
+	if (ret)
 		return ret;
-	}
 
 	asrc->mem_clk = devm_clk_get(&pdev->dev, "mem");
-	if (IS_ERR(asrc->mem_clk)) {
-		dev_err(&pdev->dev, "failed to get mem clock\n");
-		return PTR_ERR(asrc->mem_clk);
-	}
+	if (IS_ERR(asrc->mem_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(asrc->mem_clk),
+				     "failed to get mem clock\n");
 
 	asrc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(asrc->ipg_clk)) {
-		dev_err(&pdev->dev, "failed to get ipg clock\n");
-		return PTR_ERR(asrc->ipg_clk);
-	}
+	if (IS_ERR(asrc->ipg_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(asrc->ipg_clk),
+				     "failed to get ipg clock\n");
 
 	asrc->spba_clk = devm_clk_get(&pdev->dev, "spba");
 	if (IS_ERR(asrc->spba_clk))
@@ -1325,10 +1321,9 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 	for (i = 0; i < ASRC_CLK_MAX_NUM; i++) {
 		sprintf(tmp, "asrck_%x", i);
 		asrc_priv->asrck_clk[i] = devm_clk_get(&pdev->dev, tmp);
-		if (IS_ERR(asrc_priv->asrck_clk[i])) {
-			dev_err(&pdev->dev, "failed to get %s clock\n", tmp);
-			return PTR_ERR(asrc_priv->asrck_clk[i]);
-		}
+		if (IS_ERR(asrc_priv->asrck_clk[i]))
+			return dev_err_probe(&pdev->dev, PTR_ERR(asrc_priv->asrck_clk[i]),
+					     "failed to get %s clock\n", tmp);
 	}
 
 	asrc_priv->soc = of_device_get_match_data(&pdev->dev);
@@ -1443,10 +1438,8 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 
 	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_asrc_component,
 					      fsl_asrc_dai, ARRAY_SIZE(fsl_asrc_dai));
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register ASoC DAI\n");
+	if (ret)
 		goto err_pm_get_sync;
-	}
 
 	ret = fsl_asrc_m2m_init(asrc);
 	if (ret) {
-- 
2.43.0

[PATCH 2/5] ASoC: fsl_asrc: Fix handling of deferred spba clock

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The current device tree binding does not describe the spba clock
as optional. However, the driver currently continues probing when
devm_clk_get() returns an error.

Return -EPROBE_DEFER when the clock provider is not ready, allowing
the driver to retry probing later. Preserve the existing behavior
for other errors.

Fixes: 13b8a97a760e ("ASoC: fsl_asrc: spba clock is needed by asrc device")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/fsl/fsl_asrc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 055593d5d37b..c91ffb5e0c5e 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -1315,8 +1315,11 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 				     "failed to get ipg clock\n");
 
 	asrc->spba_clk = devm_clk_get(&pdev->dev, "spba");
-	if (IS_ERR(asrc->spba_clk))
+	if (IS_ERR(asrc->spba_clk)) {
+		if (PTR_ERR(asrc->spba_clk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
 		dev_warn(&pdev->dev, "failed to get spba clock\n");
+	}
 
 	for (i = 0; i < ASRC_CLK_MAX_NUM; i++) {
 		sprintf(tmp, "asrck_%x", i);
-- 
2.43.0

[PATCH 3/5] ASoC: fsl_asrc: Fix runtime PM usage count on init failure

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Balance the runtime PM usage count when fsl_asrc_init() fails
after pm_runtime_get_sync() succeeds.

Fixes: 286d658477a4 ("ASoC: fsl_asrc: register m2m platform device")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/fsl/fsl_asrc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index c91ffb5e0c5e..f7ac60724c6c 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -1432,6 +1432,7 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 	ret = fsl_asrc_init(asrc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to init asrc %d\n", ret);
+		pm_runtime_put_noidle(&pdev->dev);
 		goto err_pm_get_sync;
 	}
 
-- 
2.43.0

[PATCH 4/5] ASoC: fsl_asrc: Request the IRQ after the device is initialized

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Request the IRQ after fsl_asrc_init(), while the device is still
runtime active, so that the interrupt handler cannot run before
the device is initialized.

If requesting the IRQ fails, balance the runtime PM usage count
before proceeding with cleanup.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/fsl/fsl_asrc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index f7ac60724c6c..c97f814d7358 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -1299,11 +1299,6 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	ret = devm_request_irq(&pdev->dev, irq, fsl_asrc_isr, 0,
-			       dev_name(&pdev->dev), asrc);
-	if (ret)
-		return ret;
-
 	asrc->mem_clk = devm_clk_get(&pdev->dev, "mem");
 	if (IS_ERR(asrc->mem_clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(asrc->mem_clk),
@@ -1436,6 +1431,14 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 		goto err_pm_get_sync;
 	}
 
+	/* Request the IRQ only once the device is initialized */
+	ret = devm_request_irq(&pdev->dev, irq, fsl_asrc_isr, 0,
+			       dev_name(&pdev->dev), asrc);
+	if (ret) {
+		pm_runtime_put_noidle(&pdev->dev);
+		goto err_pm_get_sync;
+	}
+
 	ret = pm_runtime_put_sync(&pdev->dev);
 	if (ret < 0 && ret != -ENOSYS)
 		goto err_pm_get_sync;
-- 
2.43.0

[PATCH 5/5] ASoC: fsl_asrc: Use devm for runtime PM cleanup

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

On unbind, fsl_asrc_remove() gates the clocks before devres
unregisters the component and frees the IRQ, while the component
may still be in use.

Move the cleanup to devm. The action that disables the clocks is
registered before devm_pm_runtime_enable(), so it runs last, after
runtime PM is disabled, as in fsl_asrc_remove().

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/fsl/fsl_asrc.c | 48 ++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index c97f814d7358..09e09164a106 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -1258,6 +1258,15 @@ static int fsl_asrc_m2m_pair_resume(struct fsl_asrc_pair *pair)
 static int fsl_asrc_runtime_resume(struct device *dev);
 static int fsl_asrc_runtime_suspend(struct device *dev);
 
+/* devm action: runtime suspend the device if it is not runtime suspended yet */
+static void fsl_asrc_runtime_suspend_action(void *data)
+{
+	struct device *dev = data;
+
+	if (!pm_runtime_status_suspended(dev))
+		fsl_asrc_runtime_suspend(dev);
+}
+
 static int fsl_asrc_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1413,22 +1422,34 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, asrc);
 	spin_lock_init(&asrc->lock);
-	pm_runtime_enable(&pdev->dev);
+
+	/*
+	 * Ensure the device is suspended after the IRQ and component
+	 * have been released.
+	 */
+	ret = devm_add_action(&pdev->dev, fsl_asrc_runtime_suspend_action,
+			      &pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = fsl_asrc_runtime_resume(&pdev->dev);
 		if (ret)
-			goto err_pm_disable;
+			return ret;
 	}
 
 	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret < 0)
-		goto err_pm_get_sync;
+		return ret;
 
 	ret = fsl_asrc_init(asrc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to init asrc %d\n", ret);
 		pm_runtime_put_noidle(&pdev->dev);
-		goto err_pm_get_sync;
+		return ret;
 	}
 
 	/* Request the IRQ only once the device is initialized */
@@ -1436,32 +1457,25 @@ static int fsl_asrc_probe(struct platform_device *pdev)
 			       dev_name(&pdev->dev), asrc);
 	if (ret) {
 		pm_runtime_put_noidle(&pdev->dev);
-		goto err_pm_get_sync;
+		return ret;
 	}
 
 	ret = pm_runtime_put_sync(&pdev->dev);
 	if (ret < 0 && ret != -ENOSYS)
-		goto err_pm_get_sync;
+		return ret;
 
 	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_asrc_component,
 					      fsl_asrc_dai, ARRAY_SIZE(fsl_asrc_dai));
 	if (ret)
-		goto err_pm_get_sync;
+		return ret;
 
 	ret = fsl_asrc_m2m_init(asrc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to init m2m device %d\n", ret);
-		goto err_pm_get_sync;
+		return ret;
 	}
 
 	return 0;
-
-err_pm_get_sync:
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		fsl_asrc_runtime_suspend(&pdev->dev);
-err_pm_disable:
-	pm_runtime_disable(&pdev->dev);
-	return ret;
 }
 
 static void fsl_asrc_remove(struct platform_device *pdev)
@@ -1469,10 +1483,6 @@ static void fsl_asrc_remove(struct platform_device *pdev)
 	struct fsl_asrc *asrc = dev_get_drvdata(&pdev->dev);
 
 	fsl_asrc_m2m_exit(asrc);
-
-	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		fsl_asrc_runtime_suspend(&pdev->dev);
 }
 
 static int fsl_asrc_runtime_resume(struct device *dev)
-- 
2.43.0

Re: [PATCH 2/5] ASoC: fsl_asrc: Fix handling of deferred spba clock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] ASoC: fsl_asrc: stale register states in
  fsl_asrc_config_pair() [1]
--

commit 62ba61794f6dac52b1de47eef34f329de32edee9
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl_asrc: Fix handling of deferred spba clock

This commit updates fsl_asrc_probe() to return -EPROBE_DEFER when
devm_clk_get() for the spba clock defers, while preserving a warning for
other error codes.

> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 055593d5d37b6..c91ffb5e0c5e8 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -1315,8 +1315,11 @@ static int fsl_asrc_probe(struct platform_device *pdev)
>  				     "failed to get ipg clock\n");
>  
>  	asrc->spba_clk = devm_clk_get(&pdev->dev, "spba");
> -	if (IS_ERR(asrc->spba_clk))
> +	if (IS_ERR(asrc->spba_clk)) {
> +		if (PTR_ERR(asrc->spba_clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
>  		dev_warn(&pdev->dev, "failed to get spba clock\n");
> +	}

[Severity: Medium]
This problem wasn't introduced by this patch, but can [1]
fsl_asrc_config_pair() leave hardware registers with stale values across a
warm reboot?

Earlier in fsl_asrc_probe(), devm_regmap_init_mmio() initializes the regmap
cache to all-zero defaults without issuing a hardware soft-reset first:

sound/soc/fsl/fsl_asrc.c:fsl_asrc_probe() {
	...
	asrc->regmap = devm_regmap_init_mmio(&pdev->dev, regs, &fsl_asrc_regmap_config);
	...
}

When fsl_asrc_config_pair() later calls regmap_update_bits() on REG_ASRCNCR,
REG_ASRCDR, REG_ASRMCR1, or REG_ASRCFG with a target field value of 0 (for
example, a 24-bit audio width or 1:1 clock ratio), regmap_update_bits() sees
0 in the cache and skips the register write, leaving the hardware running
with non-zero dividers and modes from the prior boot.

[1] https://sashiko.dev/#/bug/linux-75f0b96f-de73-4139-881e-4c69d6455f22

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112105116.32274-1-phucduc.bui@gmail.com?part=2

Re: [PATCH 3/5] ASoC: fsl_asrc: Fix runtime PM usage count on init failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ASoC: fsl_asrc: wrong Fixes tag and pm_runtime_get_sync() in
  commit msg
--

commit 503d1e3d04da11b4fe4bd7121c2929541b45d6b1
Author: bui duc phuc <phucduc.bui@gmail.com>

> ASoC: fsl_asrc: Fix runtime PM usage count on init failure
>
> Balance the runtime PM usage count when fsl_asrc_init() fails
> after pm_runtime_get_sync() succeeds.
>
> Fixes: 286d658477a4 ("ASoC: fsl_asrc: register m2m platform device")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>

[Severity: Low]
Commit 286d658477a4 ("ASoC: fsl_asrc: register m2m platform device") only
added fsl_asrc_m2m_init() at the end of fsl_asrc_probe() and did not touch
fsl_asrc_init() or its error path.  The unbalanced runtime PM usage count on
fsl_asrc_init() failure was introduced by commit cab04ab5900f ("ASoC:
fsl_asrc: Don't use devm_regmap_init_mmio_clk"), so the Fixes tag should
point to that commit instead.

Also, fsl_asrc_probe() acquires the reference via pm_runtime_resume_and_get()
rather than pm_runtime_get_sync() since commit d05040741afe ("ASoC: fsl_asrc:
using pm_runtime_resume_and_get to simplify the code").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112105116.32274-1-phucduc.bui@gmail.com?part=3

Re: [PATCH 5/5] ASoC: fsl_asrc: Use devm for runtime PM cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ASoC: fsl_asrc: premature suspend action registration in
  fsl_asrc_probe()
--

commit d552655304748475ea350f20bd450d9524c4e316
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl_asrc: Use devm for runtime PM cleanup

This commit replaces manual runtime PM cleanup in fsl_asrc_probe() and
fsl_asrc_remove() with devm_add_action() and devm_pm_runtime_enable().

> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index c97f814d7358b..09e09164a1061 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -1258,6 +1258,15 @@ static int fsl_asrc_m2m_pair_resume(struct fsl_asrc_pair *pair)
>  static int fsl_asrc_runtime_resume(struct device *dev);
>  static int fsl_asrc_runtime_suspend(struct device *dev);
>  
> +/* devm action: runtime suspend the device if it is not runtime suspended yet */
> +static void fsl_asrc_runtime_suspend_action(void *data)
> +{
> +	struct device *dev = data;
> +
> +	if (!pm_runtime_status_suspended(dev))
> +		fsl_asrc_runtime_suspend(dev);
> +}
> +
>  static int fsl_asrc_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1413,22 +1422,34 @@ static int fsl_asrc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, asrc);
>  	spin_lock_init(&asrc->lock);
> -	pm_runtime_enable(&pdev->dev);
> +
> +	/*
> +	 * Ensure the device is suspended after the IRQ and component
> +	 * have been released.
> +	 */
> +	ret = devm_add_action(&pdev->dev, fsl_asrc_runtime_suspend_action,
> +			      &pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
>  	if (!pm_runtime_enabled(&pdev->dev)) {
>  		ret = fsl_asrc_runtime_resume(&pdev->dev);
>  		if (ret)
> -			goto err_pm_disable;
> +			return ret;
>  	}

[Severity: Medium]
If CONFIG_PM is disabled and fsl_asrc_runtime_resume() fails here, does
devres unwind end up calling fsl_asrc_runtime_suspend() while the clocks are
still disabled?

When CONFIG_PM is disabled, pm_runtime_enabled() and
pm_runtime_status_suspended() both return false.  If
fsl_asrc_runtime_resume() fails, it cleans up any clocks it enabled and
returns an error, which now triggers fsl_asrc_runtime_suspend_action().
fsl_asrc_runtime_suspend() then reads the volatile REG_ASRCFG register over
MMIO with all ASRC clocks gated (which can cause an external abort or bus
hang on i.MX) and calls clk_disable_unprepare() on clocks that were never
enabled.

Registering fsl_asrc_runtime_suspend_action() after the !pm_runtime_enabled()
block (or using devm_add_action_or_reset() after devm_pm_runtime_enable()
with the !pm_runtime_enabled() resume handled first) would avoid suspending
an unresumed device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112105116.32274-1-phucduc.bui@gmail.com?part=5