Re: [PATCH v2 2/5] ASoC: rockchip: spdif: Reorder clock enable sequence

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Reading channel status bytes via a u16 pointer causes unaligned memory accesses and endianness bugs.
- [High] The MCLK domain reset bit is asserted but never de-asserted, which poisons the regmap cache and causes unintended resets on resume.
--

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

ASoC: rockchip: spdif: Reorder clock enable sequence

This commit reorders the clock enable sequence during runtime resume for the
rockchip spdif driver. It enables the hclk bus clock before the mclk
controller clock, keeping the clock ordering consistent with the suspend
sequence.

> diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
> index 581624f2682ef..8de5b76cfe791 100644
> --- a/sound/soc/rockchip/rockchip_spdif.c
> +++ b/sound/soc/rockchip/rockchip_spdif.c
> @@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
[ ... ]
> -	ret = clk_prepare_enable(spdif->hclk);
> +	ret = clk_prepare_enable(spdif->mclk);
>  	if (ret) {
> -		clk_disable_unprepare(spdif->mclk);
> -		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
> +		clk_disable_unprepare(spdif->hclk);
> +		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
>  		return ret;
>  	}
>  
>  	regcache_cache_only(spdif->regmap, false);
>  	regcache_mark_dirty(spdif->regmap);
>  
>  	ret = regcache_sync(spdif->regmap);

[Severity: High]
This is a pre-existing issue, but does this regcache_sync() call cause
unintended resets due to a poisoned regmap cache?

In rk_spdif_hw_params(), the driver sets the SPDIF_CFGR_CLR_EN bit to clear
the MCLK domain logic:

sound/soc/rockchip/rockchip_spdif.c:rk_spdif_hw_params()
	...
	regmap_update_bits(spdif->regmap, SPDIF_CFGR, SPDIF_CFGR_CLR_MASK,
			   SPDIF_CFGR_CLR_EN);
	udelay(1);

	ret = regmap_update_bits(spdif->regmap, SPDIF_CFGR,
	...

This bit is never explicitly de-asserted.

Because SPDIF_CFGR is a cached register, the cache permanently stores the
value with the clear bit set.

During pm_runtime_resume(), does writing this cached value back to the
hardware unintentionally re-trigger the MCLK domain reset and corrupt the
hardware state?


Also, looking elsewhere in the same file, there is another pre-existing issue
in rk_spdif_hw_params():

sound/soc/rockchip/rockchip_spdif.c:rk_spdif_hw_params()
	...
	ret = snd_pcm_create_iec958_consumer_hw_params(params, cs, sizeof(cs));
	if (ret < 0)
		return ret;

	fc = (u16 *)cs;
	for (i = 0; i < CS_BYTE / 2; i++)
		regmap_write(spdif->regmap, SPDIF_CHNSRn(i), CS_FRAME(fc[i]));
	...

[Severity: High]
This isn't a bug introduced by this patch, but does casting the u8 array to a
u16 pointer cause unaligned memory accesses and endianness bugs?

The 6-byte cs array is cast to a u16 pointer and directly dereferenced.

Can this trigger alignment faults or undefined behavior depending on the
architecture and compiler optimizations?

Additionally, since it performs a host-endian read of the byte stream, does
this cause the IEC958 channel status bits to be incorrectly swapped when
running on big-endian architectures?

Could get_unaligned_le16() be used here instead?

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