Re: [PATCH v3 3/9] ASoC: loongson: Add Loongson-2K0300 I2S controller support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing hardware quirk reset sequence on system resume.
- [Medium] Read-Modify-Write on a volatile control register can unintentionally re-enable transient hardware states during soft reset.

Pre-existing issues:
- [High] Illegal overriding of platform device name during probe leading to Use-After-Free and sysfs corruption.
--

commit b9499f3634cc6b7a7bad0f418f80a05c56aa0954
Author: Binbin Zhou <zhoubinbin@loongson.cn>

ASoC: loongson: Add Loongson-2K0300 I2S controller support

The Loongson-2K0300 I2S interface differs significantly from the
Loongson-2K1000. Although both utilize external DMA controllers, the
Loongson-2K0300 does not require additional registers for routing
configuration.

Due to hardware design flaw, an extra controller reset sequence is
required during probe.

> diff --git a/sound/soc/loongson/loongson_i2s_plat.c b/sound/soc/loongson/loongson_i2s_plat.c
> index ac054b6ce6322..82d95c6644ef0 100644
> --- a/sound/soc/loongson/loongson_i2s_plat.c
> +++ b/sound/soc/loongson/loongson_i2s_plat.c

[ ... ]

> @@ -87,11 +108,17 @@ static int loongson_i2s_plat_probe(struct platform_device *pdev)
>  	if (IS_ERR(i2s_clk))
>  		return dev_err_probe(dev, PTR_ERR(i2s_clk), "clock property invalid\n");
>  	i2s->clk_rate = clk_get_rate(i2s_clk);
> +	i2s->rev_id = plat_config->rev_id;
>  
>  	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>  	dev_set_name(dev, LS_I2S_DRVNAME);

[Severity: High]
This is a pre-existing issue, but does calling dev_set_name() on an already
registered platform device cause a use-after-free?

Calling dev_set_name() here frees the existing kobj->name without renaming
the underlying sysfs directory.

Could this corrupt the kobject state and lead to kernel warnings upon device
removal?

>  	dev_set_drvdata(dev, i2s);
>  
> +	if (i2s->rev_id == 1) {
> +		regmap_update_bits(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_RESET, I2S_CTRL_RESET);

[Severity: High]
Since this controller requires a reset due to a hardware flaw, does it also
need this reset applied in the resume path?

When the system suspends, power to the controller is typically lost. Upon
resume, the shared i2s_resume() function is called:

sound/soc/loongson/loongson_i2s.c:i2s_resume() {
	struct loongson_i2s *i2s = dev_get_drvdata(dev);

	regcache_cache_only(i2s->regmap, false);

	return regcache_sync(i2s->regmap);
}

Because LS_I2S_CTRL is typically a volatile register, regcache_sync() will
not automatically restore it. Will this leave the hardware design flaw
unmitigated after a suspend/resume cycle?

[Severity: Medium]
Does using regmap_update_bits() on a volatile control register risk
preserving unintended active states during the soft reset?

Since this performs a read-modify-write, if the bootloader or previous
uninitialized state left other functional bits active, they might be
logically preserved and kept enabled during the reset sequence.

Would it be safer to use regmap_write() to assert the reset while explicitly
ensuring all other functional bits are zeroed out?

> +		fsleep(200);
> +	}
> +
>  	ret = devm_snd_soc_register_component(dev, &loongson_i2s_edma_component,
>  					      &loongson_i2s_dai, 1);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782439646.git.zhoubinbin@loongson.cn?part=3