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

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>
---
 sound/soc/sunxi/sun4i-spdif.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
  

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 <phucduc.bui@gmail.com>

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.

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -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?

[ ... ]
  

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.


On Thu, May 14, 2026 at 11:33 AM <sashiko-bot@kernel.org> 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 <phucduc.bui@gmail.com>
>
> 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.
>
> > diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> > --- a/sound/soc/sunxi/sun4i-spdif.c
> > +++ b/sound/soc/sunxi/sun4i-spdif.c
> > @@ -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/20260513105003.81880-1-phucduc.bui@gmail.com?part=1
  

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




On Thu, May 14, 2026 at 6:22 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> 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.
>
>
> On Thu, May 14, 2026 at 11:33 AM <sashiko-bot@kernel.org> 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 <phucduc.bui@gmail.com>
> >
> > 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.
> >
> > > diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> > > --- a/sound/soc/sunxi/sun4i-spdif.c
> > > +++ b/sound/soc/sunxi/sun4i-spdif.c
> > > @@ -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/20260513105003.81880-1-phucduc.bui@gmail.com?part=1
  

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:

> 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