[PATCH v2 0/5] ASoC: rockchip: Reorder clock enable sequence

[PATCH v2 0/5] ASoC: rockchip: Reorder clock enable sequence

[phucduc.bui]

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

Hi all,

This series reorders the runtime resume clock enable sequence in the 
Rockchip SPDIF and PDM drivers to enable the bus clock before the 
functional controller clock.

It also updates the SPDIF DT binding clock descriptions to match the 
actual clock usage in the driver.

Additionally, this v2 adds two new patches addressing issues reported 
by the Sashiko AI Review tool regarding regcache sync failure handling 
and runtime PM resume status validation.

Testing:
  - Patch 1: Verified (dt_binding_check passed).
  - Patches 2 to 5: Compile tested only. Please help test if you have 
    the relevant Rockchip hardware.

Changes in v2:
  - Include two new patches to handle runtime PM resume and regcache sync 
    failures based on Sashiko AI Review.
  - Update commit message based on Krzysztof's review
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.


Best Regards,
Phuc

bui duc phuc (5):
  ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions
  ASoC: rockchip: spdif: Reorder clock enable sequence
  ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence
  ASoC: rockchip: spdif: Restore regcache cache-only mode on sync
    failure
  ASoC: rockchip: rockchip_pdm: Handle runtime PM resume failures in
    set_fmt

 .../bindings/sound/rockchip-spdif.yaml           |  2 +-
 sound/soc/rockchip/rockchip_pdm.c                | 16 ++++++++++------
 sound/soc/rockchip/rockchip_spdif.c              | 11 ++++++-----
 3 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[phucduc.bui]

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

Update the binding descriptions to match the actual clock usage, where
'mclk' is the controller clock and 'hclk' is the bus clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v2:
  - Update commit message based on Krzysztof's review

 Documentation/devicetree/bindings/sound/rockchip-spdif.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
index 502907dd28b3..b174d7498029 100644
--- a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
+++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
@@ -45,8 +45,8 @@ properties:
 
   clocks:
     items:
-      - description: clock for SPDIF bus
       - description: clock for SPDIF controller
+      - description: clock for SPDIF bus
 
   clock-names:
     items:
-- 
2.43.0

[PATCH v2 2/5] ASoC: rockchip: spdif: Reorder clock enable sequence

[phucduc.bui]

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

Enable the 'hclk' bus clock before the 'mclk' controller clock during
runtime resume.
The bus clock provides the register access interface, so enable it before
the controller clock. This also makes the resume sequence the reverse of
the suspend sequence, which keeps the clock ordering consistent.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.
Changes in v2:
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.

 sound/soc/rockchip/rockchip_spdif.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
index 581624f2682e..8de5b76cfe79 100644
--- a/sound/soc/rockchip/rockchip_spdif.c
+++ b/sound/soc/rockchip/rockchip_spdif.c
@@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
 	struct rk_spdif_dev *spdif = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(spdif->mclk);
+	ret = clk_prepare_enable(spdif->hclk);
 	if (ret) {
-		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
+		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(spdif->hclk);
+	ret = clk_prepare_enable(spdif->mclk);
 	if (ret) {
-		clk_disable_unprepare(spdif->mclk);
-		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
+		clk_disable_unprepare(spdif->hclk);
+		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0

Re: [PATCH v2 2/5] ASoC: rockchip: spdif: Reorder clock enable sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Reading channel status bytes via a u16 pointer causes unaligned memory accesses and endianness bugs.
- [High] The MCLK domain reset bit is asserted but never de-asserted, which poisons the regmap cache and causes unintended resets on resume.
--

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

ASoC: rockchip: spdif: Reorder clock enable sequence

This commit reorders the clock enable sequence during runtime resume for the
rockchip spdif driver. It enables the hclk bus clock before the mclk
controller clock, keeping the clock ordering consistent with the suspend
sequence.

> diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
> index 581624f2682ef..8de5b76cfe791 100644
> --- a/sound/soc/rockchip/rockchip_spdif.c
> +++ b/sound/soc/rockchip/rockchip_spdif.c
> @@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
[ ... ]
> -	ret = clk_prepare_enable(spdif->hclk);
> +	ret = clk_prepare_enable(spdif->mclk);
>  	if (ret) {
> -		clk_disable_unprepare(spdif->mclk);
> -		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
> +		clk_disable_unprepare(spdif->hclk);
> +		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
>  		return ret;
>  	}
>  
>  	regcache_cache_only(spdif->regmap, false);
>  	regcache_mark_dirty(spdif->regmap);
>  
>  	ret = regcache_sync(spdif->regmap);

[Severity: High]
This is a pre-existing issue, but does this regcache_sync() call cause
unintended resets due to a poisoned regmap cache?

In rk_spdif_hw_params(), the driver sets the SPDIF_CFGR_CLR_EN bit to clear
the MCLK domain logic:

sound/soc/rockchip/rockchip_spdif.c:rk_spdif_hw_params()
	...
	regmap_update_bits(spdif->regmap, SPDIF_CFGR, SPDIF_CFGR_CLR_MASK,
			   SPDIF_CFGR_CLR_EN);
	udelay(1);

	ret = regmap_update_bits(spdif->regmap, SPDIF_CFGR,
	...

This bit is never explicitly de-asserted.

Because SPDIF_CFGR is a cached register, the cache permanently stores the
value with the clear bit set.

During pm_runtime_resume(), does writing this cached value back to the
hardware unintentionally re-trigger the MCLK domain reset and corrupt the
hardware state?


Also, looking elsewhere in the same file, there is another pre-existing issue
in rk_spdif_hw_params():

sound/soc/rockchip/rockchip_spdif.c:rk_spdif_hw_params()
	...
	ret = snd_pcm_create_iec958_consumer_hw_params(params, cs, sizeof(cs));
	if (ret < 0)
		return ret;

	fc = (u16 *)cs;
	for (i = 0; i < CS_BYTE / 2; i++)
		regmap_write(spdif->regmap, SPDIF_CHNSRn(i), CS_FRAME(fc[i]));
	...

[Severity: High]
This isn't a bug introduced by this patch, but does casting the u8 array to a
u16 pointer cause unaligned memory accesses and endianness bugs?

The 6-byte cs array is cast to a u16 pointer and directly dereferenced.

Can this trigger alignment faults or undefined behavior depending on the
architecture and compiler optimizations?

Additionally, since it performs a host-endian read of the byte stream, does
this cause the IEC958 channel status bits to be incorrectly swapped when
running on big-endian architectures?

Could get_unaligned_le16() be used here instead?

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

[PATCH v2 3/5] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[phucduc.bui]

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

Enable the 'hclk' bus clock before the 'clk' controller clock during
runtime resume.
The bus clock provides the register access interface, so enable it before
the controller clock. This also makes the resume sequence the reverse of
the suspend sequence, which keeps the clock ordering consistent.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.
Changes in v2:
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.

 sound/soc/rockchip/rockchip_pdm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
index c69cdd6f2499..8f78f7bc1806 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -422,16 +422,16 @@ static int rockchip_pdm_runtime_resume(struct device *dev)
 	struct rk_pdm_dev *pdm = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(pdm->clk);
+	ret = clk_prepare_enable(pdm->hclk);
 	if (ret) {
-		dev_err(pdm->dev, "clock enable failed %d\n", ret);
+		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pdm->hclk);
+	ret = clk_prepare_enable(pdm->clk);
 	if (ret) {
-		clk_disable_unprepare(pdm->clk);
-		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
+		clk_disable_unprepare(pdm->hclk);
+		dev_err(pdm->dev, "clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0

Re: [PATCH v2 3/5] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If `get_pdm_clk()` falls back to the signoff clock rate, it leaves `clk_out` at 0, resulting in a division by zero panic in `rockchip_pdm_hw_params()`.
--

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

ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

Enable the 'hclk' bus clock before the 'clk' controller clock during
runtime resume. This ensures the bus clock is available for register access
before the controller is clocked, keeping the resume sequence symmetrical
with suspend.

> diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
> index c69cdd6f24994..8f78f7bc1806c 100644
> --- a/sound/soc/rockchip/rockchip_pdm.c
> +++ b/sound/soc/rockchip/rockchip_pdm.c

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential division by zero
in this file when the clock tree cannot synthesize an exact frequency match.

In get_pdm_clk(), if the code falls back to the signoff clock rate, it
updates *clk_src but appears to leave *clk_out at 0:

sound/soc/rockchip/rockchip_pdm.c:get_pdm_clk() {
...
	if (!clk) {
		clk = clk_round_rate(pdm->clk, PDM_SIGNOFF_CLK_RATE);
		*clk_src = clk;
	}
...
}

This 0 value is then passed into rational_best_approximation() as the
given_numerator, which evaluates it and sets m = 0. Later in
rockchip_pdm_hw_params(), the driver divides by m:

sound/soc/rockchip/rockchip_pdm.c:rockchip_pdm_hw_params() {
...
	rational_best_approximation(clk_out, clk_src,
				    GENMASK(16 - 1, 0),
				    GENMASK(16 - 1, 0),
				    &m, &n);
...
	clk_div = n / m;
...
}

Could an unprivileged userspace application requesting a standard audio
format trigger this division by zero and panic the kernel? Should *clk_out
be updated in the fallback path, or should rockchip_pdm_hw_params() check
for m == 0 before dividing?

[ ... ]

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

[PATCH v2 4/5] ASoC: rockchip: spdif: Restore regcache cache-only mode on sync failure

[phucduc.bui]

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

If regcache_sync() fails during runtime resume, the driver disables the
clocks and returns an error. However, the regmap cache-only mode is left
disabled.
Restore cache-only mode in the error path so subsequent register accesses
continue to use the cache while the device is inactive.

Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260522103713.6C09D1F000E9@smtp.kernel.org/
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.

 sound/soc/rockchip/rockchip_spdif.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
index 8de5b76cfe79..7f15bc7f8f35 100644
--- a/sound/soc/rockchip/rockchip_spdif.c
+++ b/sound/soc/rockchip/rockchip_spdif.c
@@ -94,6 +94,7 @@ static int rk_spdif_runtime_resume(struct device *dev)
 
 	ret = regcache_sync(spdif->regmap);
 	if (ret) {
+		regcache_cache_only(spdif->regmap, true);
 		clk_disable_unprepare(spdif->mclk);
 		clk_disable_unprepare(spdif->hclk);
 	}
-- 
2.43.0

Re: [PATCH v2 4/5] ASoC: rockchip: spdif: Restore regcache cache-only mode on sync failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The error paths in `i2s_runtime_resume`, `i2s_tdm_runtime_resume`, and `rockchip_sai_runtime_resume` fail to restore `regcache_cache_only(..., true)` when `regcache_sync()` fails.
- [High] The regmap is initialized with `cache_only = false` but the device is placed in `RPM_SUSPENDED` state at probe completion without setting `cache_only = true`.
--

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

ASoC: rockchip: spdif: Restore regcache cache-only mode on sync failure

If regcache_sync() fails during runtime resume, the driver disables the
clocks and returns an error. This restores cache-only mode in the error
path so subsequent register accesses continue to use the cache while the
device is inactive.

Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260522103713.6C09D1F000E9@smtp.kernel.org/

> diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
> --- a/sound/soc/rockchip/rockchip_spdif.c
> +++ b/sound/soc/rockchip/rockchip_spdif.c
> @@ -94,6 +94,7 @@ static int rk_spdif_runtime_resume(struct device *dev)
>  
>  	ret = regcache_sync(spdif->regmap);
>  	if (ret) {
> +		regcache_cache_only(spdif->regmap, true);
>  		clk_disable_unprepare(spdif->mclk);
>  		clk_disable_unprepare(spdif->hclk);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while this change fixes the
issue in rockchip_spdif.c, the exact same error path exists in other
Rockchip ASoC drivers.

For example, in sound/soc/rockchip/rockchip_i2s.c:i2s_runtime_resume():

    ret = regcache_sync(i2s->regmap);
    if (ret)
        clk_disable_unprepare(i2s->mclk);

Because the error path drops the device back to the suspended state
without restoring cache-only mode, subsequent regmap accesses (like
reading from debugfs /sys/kernel/debug/regmap/) will bypass the -EBUSY
check in _regmap_read() and attempt to read directly from memory-mapped
registers. Accessing registers in a powered-off domain causes the AHB bus
to hang, resulting in an imprecise external abort and kernel panic.

Similar paths exist in rockchip_i2s_tdm.c:i2s_tdm_runtime_resume() and
rockchip_sai.c:rockchip_sai_runtime_resume(). Should these drivers be
updated as well to fully resolve the issue across the subsystem?

[Severity: High]
This is also a pre-existing issue, but does rk_spdif_probe() leave the
device exposed to a similar abort before the first resume?

In sound/soc/rockchip/rockchip_spdif.c:rk_spdif_probe():

    devm_pm_runtime_enable(&pdev->dev);

    if (!pm_runtime_enabled(&pdev->dev)) {

The regmap is initialized with cache_only = false by default. When runtime
PM is enabled, the device is placed in the RPM_SUSPENDED state and the
hardware power domain can be powered off.

Because config->pm_runtime is not set to true in the regmap configuration,
if a regmap read occurs (e.g., via debugfs) before the device is first
resumed via ALSA, wouldn't regmap attempt to read physical registers while
the power domain is off? Should rk_spdif_probe() explicitly call
regcache_cache_only(spdif->regmap, true) before completing?

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

[PATCH v2 5/5] ASoC: rockchip: rockchip_pdm: Handle runtime PM resume failures in set_fmt

[phucduc.bui]

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

rockchip_pdm_set_fmt() calls pm_runtime_get_sync() before accessing
hardware registers, but ignores its return value.
If the runtime resume fails, the function continues to perform register
accesses while the device state is undefined.
Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() and
return early on failure to avoid unpowered register accesses.

Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260522110302.349421F000E9@smtp.kernel.org/
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only. Please help test if you have 
    the relevant Rockchip hardware.

 sound/soc/rockchip/rockchip_pdm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
index 8f78f7bc1806..115e90d3bbfe 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -321,6 +321,7 @@ static int rockchip_pdm_set_fmt(struct snd_soc_dai *cpu_dai,
 {
 	struct rk_pdm_dev *pdm = to_info(cpu_dai);
 	unsigned int mask = 0, val = 0;
+	int ret;
 
 	mask = PDM_CKP_MSK;
 	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
@@ -334,7 +335,10 @@ static int rockchip_pdm_set_fmt(struct snd_soc_dai *cpu_dai,
 		return -EINVAL;
 	}
 
-	pm_runtime_get_sync(cpu_dai->dev);
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
+
 	regmap_update_bits(pdm->regmap, PDM_CLK_CTRL, mask, val);
 	pm_runtime_put(cpu_dai->dev);
 
-- 
2.43.0

Re: [PATCH v2 5/5] ASoC: rockchip: rockchip_pdm: Handle runtime PM resume failures in set_fmt

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbalanced clock disable in `rockchip_pdm_remove` for `pdm->clk` causes an enable count underflow, leading to a kernel warning.
--

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

ASoC: rockchip: rockchip_pdm: Handle runtime PM resume failures in set_fmt

This patch replaces pm_runtime_get_sync() with pm_runtime_resume_and_get()
in rockchip_pdm_set_fmt() and adds an early return on failure. This ensures
register accesses are not performed when the device state is undefined.

Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260522110302.349421F000E9@smtp.kernel.org/
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>

> diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
> index 8f78f7bc1806c..115e90d3bbfe0 100644
> --- a/sound/soc/rockchip/rockchip_pdm.c
> +++ b/sound/soc/rockchip/rockchip_pdm.c

[ ... ]

> static void rockchip_pdm_remove(struct platform_device *pdev)
> {
> 	struct rk_pdm_dev *pdm = dev_get_drvdata(&pdev->dev);
> 
> 	pm_runtime_disable(&pdev->dev);
> 	if (!pm_runtime_status_suspended(&pdev->dev))
> 		rockchip_pdm_runtime_suspend(&pdev->dev);
> 
> 	clk_disable_unprepare(pdm->clk);

[Severity: High]
This is a pre-existing issue, but does this unconditional disable cause an
enable count underflow for pdm->clk?

In rockchip_pdm_probe(), pdm->clk is acquired but not enabled. It appears to
only be enabled dynamically via rockchip_pdm_runtime_resume(). During
rockchip_pdm_remove(), the driver correctly calls rockchip_pdm_runtime_suspend()
if the device is active, which disables the clock and drops its reference count
to 0.

However, calling clk_disable_unprepare() again immediately after would push
the reference count below zero, triggering a Common Clock Framework warning
in clk_core_disable(). Could this be repeatedly triggered via sysfs driver
unbind to cause a denial of service if panic_on_warn is set?

> 	clk_disable_unprepare(pdm->hclk);
> }

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