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

PATCH Superseded [email protected] 2 participants 5 messages May 13 – May 16

Subsystems (3)

Sound ASoC Arm/Allwinner sunXi SoC support

lore

Share & tools

lore

[email protected]

author

2026-05-13 10:50 UTC #0 lore raw

PATCH 1/1 ASoC: sun4i-spdif: Use guard() for spin locks

From: bui duc phuc <[email protected]>

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

Signed-off-by bui duc phuc <[email protected]>

---
 sound/soc/sunxi/sun4i-spdif.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

sound/soc/sunxi/sun4i-spdif.c +2 -8

index c2ec19437cd7..ec00779182db 100644
@@ -427,10 +427,9 @@ 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);
+	guard(spinlock_irqsave)(&host->lock);
 
 	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

@@ -444,8 +443,6 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	status[4] = reg & 0xff;
 	status[5] = (reg >> 8) & 0x3;
 
-	spin_unlock_irqrestore(&host->lock, flags);
-
 	return 0;
 }

@@ -455,11 +452,10 @@ 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);
+	guard(spinlock_irqsave)(&host->lock);
 
 	reg = (u32)status[3] << 24;
 	reg |= (u32)status[2] << 16;

@@ -483,8 +479,6 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 			   SUN4I_SPDIF_TXCFG_CHSTMODE |
 			   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
 
-	spin_unlock_irqrestore(&host->lock, flags);
-
 	return chg0 || chg1;
 }

Signature · 1 line

2.43.0

[email protected]

AI review

2026-05-14 04:33 UTC #1 lore raw

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Hardware State / PM Violation leading to bus hang --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware State / PM Violation leading to bus hang
--

commit 0686001584788d984a0f16403183bcad78fe4176
Author: bui duc phuc <[email protected]>

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

This commit refactors the locking in the sun4i-spdif driver to use the guard() macro for spinlocks. It replaces manual spin_lock_irqsave() and spin_unlock_irqrestore() calls with scope-based cleanup.

Quoted 4 lines

sound/soc/sunxi/sun4i-spdif.c static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol, · line 434

Quoted 10 lines

sound/soc/sunxi/sun4i-spdif.c +1 -2

@@ -427,10 +427,9 @@ 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);
+	guard(spinlock_irqsave)(&host->lock);
 
 	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

While this issue isn't new to this patch, does this code risk a bus hang if the device is runtime-suspended?

The regmap configuration lacks a cache, and these ALSA kcontrols can be accessed by userspace at any time. If the audio stream is idle, the device will be runtime-suspended and its APB bus clock (apb_clk) will be disabled.

Accessing the physical registers with the clock disabled will trigger an asynchronous external abort.

Would it make sense to ensure the device is runtime-resumed (perhaps via pm_runtime_get_sync() outside the spinlock guard) before touching the hardware?

[ ... ]

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Bui Duc Phuc

author

2026-05-14 11:22 UTC #2 lore raw

Thanks for the review.

This seems to be pre-existing behavior rather than something introduced by this patch, since the current change only converts the locking to use guard() helpers and does not modify the runtime PM handling.

If this turns out to be a real issue, it would probably be better addressed separately from this cleanup patch.

Quoted 47 lines · On Thu, May 14, 2026 at 11:33 AM <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware State / PM Violation leading to bus hang
--
commit 0686001584788d984a0f16403183bcad78fe4176
Author: bui duc phuc <[email protected]>
ASoC: sun4i-spdif: Use guard() for spin locks
This commit refactors the locking in the sun4i-spdif driver to use the guard() macro for spinlocks. It replaces manual spin_lock_irqsave() and spin_unlock_irqrestore() calls with scope-based cleanup.
Quoted 4 lines
sound/soc/sunxi/sun4i-spdif.c static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol, · line 434
Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c +1 -2
@@ -427,10 +427,9 @@ 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);
+     guard(spinlock_irqsave)(&host->lock);
 
      regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
While this issue isn't new to this patch, does this code risk a bus hang if the device is runtime-suspended?
The regmap configuration lacks a cache, and these ALSA kcontrols can be accessed by userspace at any time. If the audio stream is idle, the device will be runtime-suspended and its APB bus clock (apb_clk) will be disabled.
Accessing the physical registers with the clock disabled will trigger an asynchronous external abort.
Would it make sense to ensure the device is runtime-resumed (perhaps via pm_runtime_get_sync() outside the spinlock guard) before touching the hardware?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Bui Duc Phuc

author

2026-05-16 11:56 UTC #3 lore raw

Hi,

I am not entirely sure whether the issue is exactly as described in the Sashiko review.

However, I also looked at a similar case in the micfil_rate_set() function from the fsl_micfil driver:
https://elixir.bootlin.com/linux/v7.1-rc3/source/sound/soc/fsl/fsl_micfil.c

There, pm_runtime_resume_and_get() is also used to ensure that the device is runtime-resumed before accessing the registers.

To address the review comments and avoid mixing functional changes with the cleanup/refactoring using guard/scoped_guard, I am planning to split this into two separate patches:

Patch 1: replace spin_lock_irqsave() / spin_unlock_irqrestore() with scoped_guard() / guard()

+ scoped_guard(spinlock_irqsave, &host->lock) {

    regmap_read(...);

+ }

Patch 2: add runtime PM handling around register accesses

+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ return ret;

  scoped_guard(spinlock_irqsave, &host->lock) {

      regmap_read(...);

+ pm_runtime_put(dev);

This should make the review easier and clearly separate the functional change from the refactoring.
Does this approach look reasonable to you?
If so, please let me know and I will prepare the corresponding patches.

Best Regard,
Phuc

Quoted 60 lines · Bui Duc Phuc

Thanks for the review.
This seems to be pre-existing behavior rather than something introduced by this patch, since the current change only converts the locking to use guard() helpers and does not modify the runtime PM handling.
If this turns out to be a real issue, it would probably be better addressed separately from this cleanup patch.
Quoted 47 lines · On Thu, May 14, 2026 at 11:33 AM <[email protected]> wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware State / PM Violation leading to bus hang
--
commit 0686001584788d984a0f16403183bcad78fe4176
Author: bui duc phuc <[email protected]>
ASoC: sun4i-spdif: Use guard() for spin locks
This commit refactors the locking in the sun4i-spdif driver to use the guard() macro for spinlocks. It replaces manual spin_lock_irqsave() and spin_unlock_irqrestore() calls with scope-based cleanup.
Quoted 4 lines
sound/soc/sunxi/sun4i-spdif.c static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol, · line 434
Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c +1 -2
@@ -427,10 +427,9 @@ 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);
+     guard(spinlock_irqsave)(&host->lock);
 
      regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
While this issue isn't new to this patch, does this code risk a bus hang if the device is runtime-suspended?
The regmap configuration lacks a cache, and these ALSA kcontrols can be accessed by userspace at any time. If the audio stream is idle, the device will be runtime-suspended and its APB bus clock (apb_clk) will be disabled.
Accessing the physical registers with the clock disabled will trigger an asynchronous external abort.
Would it make sense to ensure the device is runtime-resumed (perhaps via pm_runtime_get_sync() outside the spinlock guard) before touching the hardware?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Bui Duc Phuc

author

2026-05-16 23:50 UTC #4 lore raw

Hi all

I would like to make a small correction to my previous email, and also confirm one additional point before preparing another patch.

Correction:

Quoted 3 lines

However, I also looked at a similar case in the micfil_rate_set() function from the fsl_micfil driver:
https://elixir.bootlin.com/linux/v7.1-rc3/source/sound/soc/fsl/fsl_micfil.c

In my previous email, I referred to the function as micfil_rate_set(), but the correct name is micfil_range_set().

Additional point for confirmation:

Currently, in sun4i_spdif_runtime_suspend(), the clocks are disabled in the following order:

clk_disable_unprepare(host->spdif_clk);
clk_disable_unprepare(host->apb_clk);

Meanwhile, in sun4i_spdif_runtime_resume(), the clocks are enabled in this order:

clk_prepare_enable(host->spdif_clk);
ret = clk_prepare_enable(host->apb_clk);

I checked the binding documentation here:

https://elixir.bootlin.com/linux/v7.1-rc3/source/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-spdif.yaml

It describes the clocks as:

properties:
clocks:
items:
- description: Bus Clock
- description: Module Clock

clock-names:
items:
- const: apb
- const: spdif

From my understanding, apb_clk is the bus clock, while spdif_clk is the module clock.

If that understanding is correct, then the suspend path seems reasonable since the module clock is disabled before the bus clock.

However, in the resume path, the module clock is currently enabled before the bus clock. It may make more sense to enable the bus clock first, followed by the module clock.

If my understanding is correct, I would like to prepare an additional patch to reorder the clock enable sequence.

Please let me know if this interpretation makes sense.

Best Regard,
Phuc