[PATCH v2 0/3] ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

[PATCH v2 0/3] ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

[phucduc.bui]

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

Hi,

This series contains a few improvements for the sun4i-spdif driver,
including guard() conversions and ensuring the device is resumed
via runtime PM before kcontrol register accesses.


Link v1: 
https://lore.kernel.org/all/20260513105003.81880-1-phucduc.bui@gmail.com/

Change in v2: 
 - Switched from using guard() to scoped_guard()
 - Added runtime PM handling for kcontrol register accesses.
	
Best Regards,
Phuc

bui duc phuc (3):
  ASoC: sunxi: sun4i-spdif: Use guard() for spin locks
  ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register
    access
  ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

 sound/soc/sunxi/sun4i-spdif.c | 76 +++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] ASoC: sunxi: sun4i-spdif: Use guard() for spin locks

[phucduc.bui]

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

Clean up the code using guard() for spin locks.
Merely code refactoring, and no behavior change.

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

Change in v2: 
 - Switched from using guard() to scoped_guard()

 sound/soc/sunxi/sun4i-spdif.c | 62 ++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index c2ec19437cd7..89eccc83a130 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -427,24 +427,21 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
-	unsigned long flags;
 	unsigned int reg;
 
-	spin_lock_irqsave(&host->lock, flags);
+	scoped_guard(spinlock_irqsave, &host->lock) {
+		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
 
-	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
+		status[0] = reg & 0xff;
+		status[1] = (reg >> 8) & 0xff;
+		status[2] = (reg >> 16) & 0xff;
+		status[3] = (reg >> 24) & 0xff;
 
-	status[0] = reg & 0xff;
-	status[1] = (reg >> 8) & 0xff;
-	status[2] = (reg >> 16) & 0xff;
-	status[3] = (reg >> 24) & 0xff;
+		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA1, &reg);
 
-	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA1, &reg);
-
-	status[4] = reg & 0xff;
-	status[5] = (reg >> 8) & 0x3;
-
-	spin_unlock_irqrestore(&host->lock, flags);
+		status[4] = reg & 0xff;
+		status[5] = (reg >> 8) & 0x3;
+	}
 
 	return 0;
 }
@@ -455,35 +452,32 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
-	unsigned long flags;
 	unsigned int reg;
 	bool chg0, chg1;
 
-	spin_lock_irqsave(&host->lock, flags);
-
-	reg = (u32)status[3] << 24;
-	reg |= (u32)status[2] << 16;
-	reg |= (u32)status[1] << 8;
-	reg |= (u32)status[0];
+	scoped_guard(spinlock_irqsave, &host->lock) {
+		reg = (u32)status[3] << 24;
+		reg |= (u32)status[2] << 16;
+		reg |= (u32)status[1] << 8;
+		reg |= (u32)status[0];
 
-	regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA0,
-				 GENMASK(31,0), reg, &chg0);
+		regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA0,
+					 GENMASK(31, 0), reg, &chg0);
 
-	reg = (u32)status[5] << 8;
-	reg |= (u32)status[4];
+		reg = (u32)status[5] << 8;
+		reg |= (u32)status[4];
 
-	regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA1,
-				 GENMASK(9,0), reg, &chg1);
+		regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA1,
+					 GENMASK(9, 0), reg, &chg1);
 
-	reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
-	if (status[0] & IEC958_AES0_NONAUDIO)
-		reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
+		reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
+		if (status[0] & IEC958_AES0_NONAUDIO)
+			reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
 
-	regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
-			   SUN4I_SPDIF_TXCFG_CHSTMODE |
-			   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
-
-	spin_unlock_irqrestore(&host->lock, flags);
+		regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
+				   SUN4I_SPDIF_TXCFG_CHSTMODE |
+				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
+	}
 
 	return chg0 || chg1;
 }
-- 
2.43.0

[PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[phucduc.bui]

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

Accessing registers while the device is runtime-suspended
may lead to invalid hardware accesses on systems where the
APB bus clock is gated during runtime suspend.

Ensure the device is resumed before accessing registers.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun4i-spdif.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index 89eccc83a130..f54eb14c9ed8 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -428,6 +428,11 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
 	unsigned int reg;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
 
 	scoped_guard(spinlock_irqsave, &host->lock) {
 		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
@@ -443,6 +448,8 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 		status[5] = (reg >> 8) & 0x3;
 	}
 
+	pm_runtime_put(cpu_dai->dev);
+
 	return 0;
 }
 
@@ -453,8 +460,13 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
 	unsigned int reg;
+	int ret;
 	bool chg0, chg1;
 
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
+
 	scoped_guard(spinlock_irqsave, &host->lock) {
 		reg = (u32)status[3] << 24;
 		reg |= (u32)status[2] << 16;
@@ -479,6 +491,8 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
 	}
 
+	pm_runtime_put(cpu_dai->dev);
+
 	return chg0 || chg1;
 }
 
-- 
2.43.0

[PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

[phucduc.bui]

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

Enable the APB bus clock before the SPDIF module clock
during runtime resume, as register accesses depend on the
bus clock being enabled first.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index f54eb14c9ed8..102db1a2afbb 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
 
 static int sun4i_spdif_runtime_resume(struct device *dev)
 {
-	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
+	struct sun4i_spdif_dev *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(host->spdif_clk);
+	ret = clk_prepare_enable(host->apb_clk);
 	if (ret)
 		return ret;
-	ret = clk_prepare_enable(host->apb_clk);
+	ret = clk_prepare_enable(host->spdif_clk);
 	if (ret)
-		clk_disable_unprepare(host->spdif_clk);
+		clk_disable_unprepare(host->apb_clk);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v2 1/3] ASoC: sunxi: sun4i-spdif: Use guard() for spin locks

[sashiko-bot]

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

Pre-existing issues:
- [Critical] If `regmap_read()` fails in `sun4i_spdif_get_status()`, the local variable `reg` is left uninitialized, leading to a kernel stack information leak.
--

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

ASoC: sunxi: sun4i-spdif: Use guard() for spin locks

This commit refactors the sun4i-spdif driver to use the scoped_guard() macro
for managing spin locks. The changes are intended as refactoring only, with
no functional changes to the driver's behavior.

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index c2ec19437cd7a..89eccc83a1301 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -427,24 +427,21 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
>  	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
>  	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>  	u8 *status = ucontrol->value.iec958.status;
> -	unsigned long flags;
>  	unsigned int reg;
>  
> -	spin_lock_irqsave(&host->lock, flags);
> +	scoped_guard(spinlock_irqsave, &host->lock) {
> +		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

[Severity: Critical]
This is a pre-existing issue, but does this leak uninitialized kernel stack
memory to userspace?

The regmap_read() function can fail and return an error code without modifying
the reg variable. Since the return value isn't checked, the uninitialized
contents of reg are unpacked into the status array:

		status[0] = reg & 0xff;
		status[1] = (reg >> 8) & 0xff;
		status[2] = (reg >> 16) & 0xff;
		status[3] = (reg >> 24) & 0xff;

This array is subsequently copied back to userspace by the ALSA control
subsystem.

Should the return value of regmap_read() be checked, and an error returned
if it fails?

[ ... ]

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

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized stack memory is copied to userspace when regmap_read() fails in sun4i_spdif_get_status().
- [High] IEC958 status bits configured via ALSA kcontrols are unconditionally overwritten by hw_params.
--

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

ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

Accessing registers while the device is runtime-suspended may lead to invalid
hardware accesses. Ensure the device is resumed before accessing registers.

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index 89eccc83a1301..f54eb14c9ed86 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -428,6 +428,11 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
>  	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>  	u8 *status = ucontrol->value.iec958.status;
>  	unsigned int reg;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(cpu_dai->dev);
> +	if (ret)
> +		return ret;
>  
>  	scoped_guard(spinlock_irqsave, &host->lock) {
>  		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

[Severity: High]
This isn't a bug introduced by this patch, but does this code copy uninitialized
stack memory to userspace if regmap_read() fails?

The return value of regmap_read() isn't checked here. If it fails, reg remains
unmodified, containing whatever was on the kernel stack. The code then copies it
directly into status, which is part of the snd_ctl_elem_value structure
returned to userspace.

>  		status[0] = reg & 0xff;
>  		status[1] = (reg >> 8) & 0xff;
>  		status[2] = (reg >> 16) & 0xff;
>  		status[3] = (reg >> 24) & 0xff;

[ ... ]

> @@ -453,8 +460,13 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
>  	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>  	u8 *status = ucontrol->value.iec958.status;
>  	unsigned int reg;
> +	int ret;
>  	bool chg0, chg1;

[ ... ]

>  		reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
>  		if (status[0] & IEC958_AES0_NONAUDIO)
>  			reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
>  
>  		regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
>  				   SUN4I_SPDIF_TXCFG_CHSTMODE |
>  				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
>  	}

[Severity: High]
This is a pre-existing issue, but is the NONAUDIO bit set here unconditionally
overwritten later by sun4i_spdif_hw_params()?

It looks like sun4i_spdif_hw_params() builds a new register configuration from
scratch and uses regmap_write(host->regmap, SUN4I_SPDIF_TXCFG, reg_val). This
would destroy the NONAUDIO bit previously configured here by the user via
kcontrols, unless the stream has exactly 4 channels.

Can this break standard IEC958 passthrough (AC3/DTS) which uses 2 channels and
relies on the kcontrol to set the non-audio bit?

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

Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-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] This problem wasn't introduced by this patch, but the devres cleanup order for ALSA components triggers a kernel crash during module unload or driver unbind.
- [High] This problem wasn't introduced by this patch, but the driver incorrectly handles the 3-byte audio format `SNDRV_PCM_FORMAT_S20_3LE`, causing severe audio corruption.
--

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

ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

This commit reorders the clock enable sequence during runtime resume.
It enables the APB bus clock before the SPDIF module clock to ensure
register accesses do not fail.

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index f54eb14c9ed86..102db1a2afbb8 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
[ ... ]
>  static int sun4i_spdif_runtime_resume(struct device *dev)
>  {
> -	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
> +	struct sun4i_spdif_dev *host = dev_get_drvdata(dev);
>  	int ret;
>  
> -	ret = clk_prepare_enable(host->spdif_clk);
> +	ret = clk_prepare_enable(host->apb_clk);
>  	if (ret)
>  		return ret;
> -	ret = clk_prepare_enable(host->apb_clk);
> +	ret = clk_prepare_enable(host->spdif_clk);
>  	if (ret)
> -		clk_disable_unprepare(host->spdif_clk);
> +		clk_disable_unprepare(host->apb_clk);

[Severity: High]
This isn't a bug introduced by this patch, but the devres cleanup order for
ALSA components appears to trigger a kernel crash during module unload or
driver unbind.

In sun4i_spdif_probe(), the ALSA component and DMA engine are registered via
devm_snd_soc_register_component() and devm_snd_dmaengine_pcm_register().
The devres teardown runs after the driver's sun4i_spdif_remove() function
returns.

However, sun4i_spdif_remove() explicitly calls pm_runtime_disable() and
sun4i_spdif_runtime_suspend(), which disable the apb_clk bus clock.

When the devres teardown later unregisters the ASoC component, it forces any
active PCM streams to stop, invoking sun4i_spdif_trigger() with
SNDRV_PCM_TRIGGER_STOP. This calls sun4i_snd_txctrl_off(), which writes to
hardware registers via regmap.

Because the APB clock has already been disabled by the remove function, do
these regmap accesses hit unclocked hardware and result in a synchronous
external abort (kernel panic) on sunxi platforms?

[Severity: High]
This is a pre-existing issue, but does the driver incorrectly handle the
3-byte audio format SNDRV_PCM_FORMAT_S20_3LE, causing severe audio
corruption?

The driver advertises support for SNDRV_PCM_FMTBIT_S20_3LE in SUN4I_FORMATS,
which means 20-bit audio samples are tightly packed into exactly 3 bytes in
memory.

However, in sun4i_spdif_hw_params(), the DMA slave bus width
(dma_params_tx.addr_width) is left at DMA_SLAVE_BUSWIDTH_4_BYTES for this
format. This configures the dst_addr_width of the DMA controller to 4 bytes.

In the ALSA generic DMA engine code and sunxi-dma driver, the undefined
src_addr_width defaults to match the 4-byte destination width.

Because the memory buffer contains tightly packed 3-byte samples, does
commanding the DMA controller to perform 4-byte memory reads misalign the
stream immediately (reading part of the next sample on every fetch), leading
to complete data corruption and unusable audio output?

>  
>  	return ret;
>  }

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

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Chen-Yu Tsai]

On Fri, May 22, 2026 at 12:54 PM <phucduc.bui@gmail.com> wrote:
>
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Accessing registers while the device is runtime-suspended
> may lead to invalid hardware accesses on systems where the
> APB bus clock is gated during runtime suspend.

Did you actually reproduce the issue, or did you add the patch simply
because Sashiko mentioned it?

On sunxi, either it will hang the system because the bus transaction
got ignored, or it won't as something else enabled the clock.

And when you do add patches due to Sashiko raising an issue, please
do mention it in the commit message.

> Ensure the device is resumed before accessing registers.
>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-spdif.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index 89eccc83a130..f54eb14c9ed8 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -428,6 +428,11 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
>         struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>         u8 *status = ucontrol->value.iec958.status;
>         unsigned int reg;
> +       int ret;
> +
> +       ret = pm_runtime_resume_and_get(cpu_dai->dev);
> +       if (ret)
> +               return ret;
>
>         scoped_guard(spinlock_irqsave, &host->lock) {
>                 regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
> @@ -443,6 +448,8 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
>                 status[5] = (reg >> 8) & 0x3;
>         }
>
> +       pm_runtime_put(cpu_dai->dev);
> +
>         return 0;
>  }
>
> @@ -453,8 +460,13 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
>         struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>         u8 *status = ucontrol->value.iec958.status;
>         unsigned int reg;
> +       int ret;
>         bool chg0, chg1;
>
> +       ret = pm_runtime_resume_and_get(cpu_dai->dev);
> +       if (ret)
> +               return ret;
> +
>         scoped_guard(spinlock_irqsave, &host->lock) {
>                 reg = (u32)status[3] << 24;
>                 reg |= (u32)status[2] << 16;
> @@ -479,6 +491,8 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
>                                    SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
>         }
>
> +       pm_runtime_put(cpu_dai->dev);
> +
>         return chg0 || chg1;
>  }
>
> --
> 2.43.0
>

Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

[Chen-Yu Tsai]

On Fri, May 22, 2026 at 12:54 PM <phucduc.bui@gmail.com> wrote:
>
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Enable the APB bus clock before the SPDIF module clock
> during runtime resume, as register accesses depend on the
> bus clock being enabled first.

That does not even matter here. Access will only happen once the runtime
PM callbacks return.

> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index f54eb14c9ed8..102db1a2afbb 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
>
>  static int sun4i_spdif_runtime_resume(struct device *dev)
>  {
> -       struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
> +       struct sun4i_spdif_dev *host = dev_get_drvdata(dev);
>         int ret;
>
> -       ret = clk_prepare_enable(host->spdif_clk);
> +       ret = clk_prepare_enable(host->apb_clk);
>         if (ret)
>                 return ret;
> -       ret = clk_prepare_enable(host->apb_clk);
> +       ret = clk_prepare_enable(host->spdif_clk);
>         if (ret)
> -               clk_disable_unprepare(host->spdif_clk);
> +               clk_disable_unprepare(host->apb_clk);
>
>         return ret;
>  }
> --
> 2.43.0
>

Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

[Bui Duc Phuc]

Hi Chen-yu,

Thanks for your feedback

On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > Enable the APB bus clock before the SPDIF module clock
> > during runtime resume, as register accesses depend on the
> > bus clock being enabled first.
>
> That does not even matter here. Access will only happen once the runtime
> PM callbacks return.
>

I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access
registers within the current ⁠runtime_resume⁠ path, so the order might
not trigger a failure right now.

However, if we look at the peer driver for the same Sunxi SoC family,
⁠sun4i-i2s.c⁠:
Links:
https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:

1. Enable bus clock
2. Access and restore/sync I2S registers
3. Enable module clock

Since both IP blocks belong to the same Sunxi platform and share similar
bus/module clock relationships, shouldn't we maintain architectural
consistency across these drivers?

Enforcing the "bus clock before module clock" order keeps the dependency
ordering aligned with the actual hardware roles, where the bus clock is
required for register access while the module clock drives the functional
audio path.

Wouldn't keeping this order also make the runtime PM behavior more
consistent and easier to follow across the Sunxi audio drivers?

Best Regards,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Bui Duc Phuc]

Hi Chen-Yu,

On Sat, May 23, 2026 at 2:19 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> And when you do add patches due to Sashiko raising an issue, please
> do mention it in the commit message.
>

As mentioned in the v1 discussion , this issue was originally reported
by Sashiko.
I'll add the Reported-by tag in the next revision.
v1 links:
https://lore.kernel.org/all/20260513105003.81880-1-phucduc.bui@gmail.com/

> Did you actually reproduce the issue, or did you add the patch simply
> because Sashiko mentioned it?
>
Since I lack Sunxi hardware, I couldn't reproduce it or perform runtime testing.
But I did compile-test the patch.
The patch aims to fix unsafe register accesses that occur before ensuring the
device is runtime-resumed.

> On sunxi, either it will hang the system because the bus transaction
> got ignored, or it won't as something else enabled the clock.
>

If Sunxi's PM design already guarantees safe access here,
feel free to reject the patch.

Best Regards,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Chen-Yu Tsai]

On Sat, May 23, 2026 at 4:55 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> Hi Chen-Yu,
>
> On Sat, May 23, 2026 at 2:19 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > And when you do add patches due to Sashiko raising an issue, please
> > do mention it in the commit message.
> >
>
> As mentioned in the v1 discussion , this issue was originally reported
> by Sashiko.
> I'll add the Reported-by tag in the next revision.
> v1 links:
> https://lore.kernel.org/all/20260513105003.81880-1-phucduc.bui@gmail.com/
>
> > Did you actually reproduce the issue, or did you add the patch simply
> > because Sashiko mentioned it?
> >
> Since I lack Sunxi hardware, I couldn't reproduce it or perform runtime testing.
> But I did compile-test the patch.
> The patch aims to fix unsafe register accesses that occur before ensuring the
> device is runtime-resumed.

When you submit a patch, it is expected that you already tested it.
If you only compile tested it, please remember to say so in the
footer (or mark the patch as RFT) so that others can test for you
and the maintainer knows the status.

And if possible, provide a scheme to test it.

> > On sunxi, either it will hang the system because the bus transaction
> > got ignored, or it won't as something else enabled the clock.
> >
>
> If Sunxi's PM design already guarantees safe access here,
> feel free to reject the patch.

I can't say that it does. But since the only control that SPDIF gives
is the IEC958 status, and that doesn't appear in the standard mixer apps,
it's unlikely that a _user_ will trigger it. Plus the control was added
after the basic structure of the driver was done, so there is definitely
some possibility of a crash.

But what you wrote in the commit message doesn't match the actual hardware
behavior, like I wrote.


ChenYu

Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

[Chen-Yu Tsai]

On Sat, May 23, 2026 at 3:11 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> Hi Chen-yu,
>
> Thanks for your feedback
>
> On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > Enable the APB bus clock before the SPDIF module clock
> > > during runtime resume, as register accesses depend on the
> > > bus clock being enabled first.
> >
> > That does not even matter here. Access will only happen once the runtime
> > PM callbacks return.
> >
>
> I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access
> registers within the current ⁠runtime_resume⁠ path, so the order might
> not trigger a failure right now.
>
> However, if we look at the peer driver for the same Sunxi SoC family,
> ⁠sun4i-i2s.c⁠:
> Links:
> https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
> In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:
>
> 1. Enable bus clock
> 2. Access and restore/sync I2S registers
> 3. Enable module clock
>
> Since both IP blocks belong to the same Sunxi platform and share similar
> bus/module clock relationships, shouldn't we maintain architectural
> consistency across these drivers?
>
> Enforcing the "bus clock before module clock" order keeps the dependency
> ordering aligned with the actual hardware roles, where the bus clock is
> required for register access while the module clock drives the functional
> audio path.
>
> Wouldn't keeping this order also make the runtime PM behavior more
> consistent and easier to follow across the Sunxi audio drivers?

This should be your primary motivation for the patch, i.e. what you
put in the commit message as the reason for this patch. What you
currently have doesn't make sense, as I already mentioned.

Some background though, sunxi is done mostly by volunteers, so we're
not overly concerned with rigidness or aligning different drivers,
unless they share a common library, such as all the clk or pinctrl
drivers.


ChenYu

Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

[Chen-Yu Tsai]

On Sun, May 24, 2026 at 9:41 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Sat, May 23, 2026 at 3:11 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
> >
> > Hi Chen-yu,
> >
> > Thanks for your feedback
> >
> > On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > > Enable the APB bus clock before the SPDIF module clock
> > > > during runtime resume, as register accesses depend on the
> > > > bus clock being enabled first.
> > >
> > > That does not even matter here. Access will only happen once the runtime
> > > PM callbacks return.
> > >
> >
> > I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access
> > registers within the current ⁠runtime_resume⁠ path, so the order might
> > not trigger a failure right now.
> >
> > However, if we look at the peer driver for the same Sunxi SoC family,
> > ⁠sun4i-i2s.c⁠:
> > Links:
> > https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
> > In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:
> >
> > 1. Enable bus clock
> > 2. Access and restore/sync I2S registers
> > 3. Enable module clock
> >
> > Since both IP blocks belong to the same Sunxi platform and share similar
> > bus/module clock relationships, shouldn't we maintain architectural
> > consistency across these drivers?
> >
> > Enforcing the "bus clock before module clock" order keeps the dependency
> > ordering aligned with the actual hardware roles, where the bus clock is
> > required for register access while the module clock drives the functional
> > audio path.
> >
> > Wouldn't keeping this order also make the runtime PM behavior more
> > consistent and easier to follow across the Sunxi audio drivers?
>
> This should be your primary motivation for the patch, i.e. what you
> put in the commit message as the reason for this patch. What you
> currently have doesn't make sense, as I already mentioned.
>
> Some background though, sunxi is done mostly by volunteers, so we're
> not overly concerned with rigidness or aligning different drivers,
> unless they share a common library, such as all the clk or pinctrl
> drivers.

Oh, and you could also add that the resume order should (normally) be
the reverse of the suspend order.

ChenYu

Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

[Bui Duc Phuc]

Hi Chen-yu,

> Oh, and you could also add that the resume order should (normally) be
> the reverse of the suspend order.

Thank you for the feedback and clarification.

I can certainly rewrite the commit message around the idea that the
resume ordering should normally mirror the suspend ordering.

However, I am still slightly unsure about which ordering should be
considered the preferred one in this case. If the reasoning is purely
based on suspend/resume symmetry, I wonder if changing the suspend
path itself to disable the APB clock before the SPDIF module clock
would be an option, which would then naturally lead to enabling the
SPDIF module clock first during resume.

From the hardware perspective, I originally assumed the APB clock acts
as the prerequisite for register access while the module clock mainly
drives the functional audio logic, which is why the "bus before module"
ordering initially seemed more intuitive to me.

So I would like to better understand which ordering is considered the
preferred model for the Sunxi audio blocks here: preserving the existing
suspend/resume symmetry, or enforcing the bus/module dependency ordering.

Thanks,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Bui Duc Phuc]

Hi Chenyu,

> When you submit a patch, it is expected that you already tested it.
> If you only compile tested it, please remember to say so in the
> footer (or mark the patch as RFT) so that others can test for you
> and the maintainer knows the status.
>
> And if possible, provide a scheme to test it.
>

Thanks for the guidance.
I’ll clearly mention the test status next time.

>
> I can't say that it does. But since the only control that SPDIF gives
> is the IEC958 status, and that doesn't appear in the standard mixer apps,
> it's unlikely that a _user_ will trigger it. Plus the control was added
> after the basic structure of the driver was done, so there is definitely
> some possibility of a crash.
>
> But what you wrote in the commit message doesn't match the actual hardware
> behavior, like I wrote.

Thanks for the clarification.
I'll update the commit message to something like:
"The kcontrols may access hardware registers while the
device is runtime-suspended.
Ensure the device is resumed before touching the registers."

Best Regards,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Bui Duc Phuc]

Hi ,

I did some additional investigation into the regmap core functions
currently used by this driver, such as _regmap_update_bits():

https://elixir.bootlin.com/linux/v7.1-rc5/source/drivers/base/regmap/regmap.c#L3249

and _regmap_read():
https://elixir.bootlin.com/linux/v7.1-rc5/source/drivers/base/regmap/regmap.c#L2822

as well as the current sun4i_spdif_regmap_config definition:
https://elixir.bootlin.com/linux/v7.1-rc5/source/sound/soc/sunxi/sun4i-spdif.c#L526

Based on the current regmap core implementation, I think it might be
cleaner to leverage the existing regmap suspend protection mechanism:

if (map->cache_only)
return -EBUSY;

This would inherently prevent direct hardware register access and help
avoid potential crashes during suspend. To support this properly, we would
likely also need to enable a basic cache type (e.g. REGCACHE_FLAT) in
sun4i_spdif_regmap_config, since the driver currently
does not define a .cache_type.

A patch in this direction would look roughly like this:

static int sun4i_spdif_runtime_suspend(struct device *dev)
{
  struct sun4i_spdif_dev *host = dev_get_drvdata(dev);

  clk_disable_unprepare(host->spdif_clk);
+regcache_cache_only(host->regmap, true);
 clk_disable_unprepare(host->apb_clk);

}

static int sun4i_spdif_runtime_resume(struct device *dev)
{
   struct sun4i_spdif_dev *host = dev_get_drvdata(dev);

   clk_prepare_enable(host->apb_clk);
+ regcache_cache_only(host->regmap, false);

   clk_prepare_enable(host->spdif_clk);
}

This approach would not only address the current kcontrol-related issue,
but could also provide a more generic safeguard against other runtime
suspend race conditions that we may not have considered yet.

However, one downside is that this solution depends on the current internal
behavior of the regmap core. In addition, if this driver starts using
.volatile_reg in the future, the current regmap implementation may no
longer provide sufficient protection during suspend.

For example, in the first branch of _regmap_update_bits(),
volatile register accesses bypass the cache_only check entirely and can
still hit the hardware bus directly, which could still lead to system crashes.

Because of that, the explicit pm_runtime resume handling proposed
in the current patch may still be the more robust solution in the long term.

Given our previous discussion about making the pm_runtime handling
more consistent between I2S and SPDIF:

https://lore.kernel.org/all/CAABR9nEFGOX5GnQ9qpJY-T-92dA_kDcVS+qBz1ior590G_x6gw@mail.gmail.com/

What are your thoughts on this direction?
Or would it be safer and simpler to keep the current patch as-is?

Best Regards,
Phuc