* [PATCH] ASoC: sun4i-spdif: Use guard() for spin locks
@ 2026-05-13 10:50 phucduc.bui
[not found] ` <20260514043314.62222C2BCB7@smtp.kernel.org>
0 siblings, 1 reply; 3+ messages in thread
From: phucduc.bui @ 2026-05-13 10:50 UTC (permalink / raw)
To: Mark Brown
Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Chen-Yu Tsai,
Jernej Skrabec, Samuel Holland, Marcus Cooper, Chen Ni,
linux-sound, linux-sunxi, linux-arm-kernel, linux-kernel,
bui duc phuc
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(-)
diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index c2ec19437cd7..ec00779182db 100644
--- 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, ®);
@@ -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;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: sun4i-spdif: Use guard() for spin locks
[not found] ` <CAABR9nGhwtBu9Yhbn-BTD1Scykp2h6eQbtQtT13DcnekUdum1A@mail.gmail.com>
@ 2026-05-16 11:56 ` Bui Duc Phuc
2026-05-16 23:50 ` Bui Duc Phuc
0 siblings, 1 reply; 3+ messages in thread
From: Bui Duc Phuc @ 2026-05-16 11:56 UTC (permalink / raw)
To: sashiko-reviews, Mark Brown
Cc: linux-sunxi, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, Marcus Cooper,
Chen Ni, linux-sound, linux-arm-kernel, linux-kernel
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, ®);
> >
> > 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: sun4i-spdif: Use guard() for spin locks
2026-05-16 11:56 ` Bui Duc Phuc
@ 2026-05-16 23:50 ` Bui Duc Phuc
0 siblings, 0 replies; 3+ messages in thread
From: Bui Duc Phuc @ 2026-05-16 23:50 UTC (permalink / raw)
To: sashiko-reviews, Mark Brown
Cc: linux-sunxi, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, Marcus Cooper,
Chen Ni, linux-sound, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-16 23:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-13 10:50 [PATCH] ASoC: sun4i-spdif: Use guard() for spin locks phucduc.bui
[not found] ` <20260514043314.62222C2BCB7@smtp.kernel.org>
[not found] ` <CAABR9nGhwtBu9Yhbn-BTD1Scykp2h6eQbtQtT13DcnekUdum1A@mail.gmail.com>
2026-05-16 11:56 ` Bui Duc Phuc
2026-05-16 23:50 ` Bui Duc Phuc
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox