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

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(+)
  

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?
  

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
>
  

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
  

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
  

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
  

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