Re: [PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The return value of `fsi_clk_init()` is ignored in `fsi_probe()`, which breaks probe deferral and leads to a kernel panic from dereferencing error pointers.
--

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

ASoC: renesas: fsi: refactor clock initialization

This commit moves fsi_clk_init() to probe and uses devm_clk_get_optional()
for optional clocks. It allows probe to succeed when optional clocks are
missing, while set_rate() validates clock availability.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 55a11f1fe8aa1..8c46d68069581 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -973,35 +983,42 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
>  	return ret;
>  }
>  
> -static int fsi_clk_init(struct device *dev,
> -			struct fsi_priv *fsi,
> -			int xck,
> -			int ick,
> -			int div,
> -			int (*set_rate)(struct device *dev,
> -					struct fsi_priv *fsi))
> +static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
>  {
[ ... ]
>  	clock->own = devm_clk_get(dev, NULL);
>  	if (IS_ERR(clock->own))
>  		return -EINVAL;

Does returning -EINVAL here mask probe deferral? If devm_clk_get() returns
-EPROBE_DEFER, returning -EINVAL will permanently fail the driver probe
rather than deferring it.

[ ... ]
> @@ -1997,6 +2001,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->running_streams = 0;
>  	fsi_port_info_init(fsi, &info.port_a);
>  	fsi_handler_init(fsi, &info.port_a);
> +	fsi_clk_init(&pdev->dev, fsi);

Is it safe to ignore the return value of fsi_clk_init() here?

If devm_clk_get_optional() fails and returns an error pointer like
-EPROBE_DEFER, those error pointers will be stored in the clock fields
instead of NULL.

Later, when fsi_clk_set_rate_external() checks if the clocks are missing:

    if (!xck || !ick) {

This check will pass because the error pointers are not NULL. The error
pointers will then be passed to common clock framework APIs like
clk_get_rate(), which will dereference them and cause a crash.

Should the return value of fsi_clk_init() be checked and propagated?

>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIA stream probe failed\n");
> @@ -2011,6 +2016,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->running_streams = 0;
>  	fsi_port_info_init(fsi, &info.port_b);
>  	fsi_handler_init(fsi, &info.port_b);
> +	fsi_clk_init(&pdev->dev, fsi);

Would the same return value check be needed here as well?

>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");

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