Re: [PATCH 2/3] ASoC: mxs-saif: Drop redundant probe error messages

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Removing error logs degrades debugging context, as the underlying functions do not log their specific failures.

Pre-existing issues:
- [High] Clock divider and OF clock provider leak on probe failure, leading to a kernel panic (use-after-unmap) upon subsequent accesses.
- [High] The global `mxs_saif` array retains a dangling pointer when `mxs_saif_probe` fails or the driver is unbound, leading to a potential Use-After-Free.
--

--- Patch [2]: [PATCH 2/3] ASoC: mxs-saif: Drop redundant probe error messages ---
commit d8455dc02bf0f7c00ea57b01724fbc7b2b394e9d
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: mxs-saif: Drop redundant probe error messages

This commit removes error logging messages from the probe function's failure
paths. The commit message states these are redundant because the underlying
functions log the errors themselves.

> commit d8455dc02bf0f7c00ea57b01724fbc7b2b394e9d
> Author: bui duc phuc <phucduc.bui@gmail.com>
> Date:   Wed Aug 12 10:14:17 2026 +0000
> 
>     ASoC: mxs-saif: Drop redundant probe error messages
>     
>     The functions called here don't log the error themselves, but the
>     error is already reported deeper in the call chain, so the dev_err()
>     calls are redundant and can be removed.

[Severity: Low]
Are we sure the underlying functions actually log these errors?

Looking at devm_request_irq(), devm_snd_soc_register_component(), and
mxs_pcm_platform_register(), they do not appear to print error messages when
they fail, except in very specific edge cases.

While the driver core will log a generic failure code on probe exit, does
removing these specific error logs cause the driver to fail silently at these
initialization steps, making it difficult to debug which step failed?

> diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
> index b877c978a04c6..a77cd516a1bfd 100644
> --- a/sound/soc/mxs/mxs-saif.c
> +++ b/sound/soc/mxs/mxs-saif.c
> @@ -841,10 +841,8 @@ static int mxs_saif_probe(struct platform_device *pdev)
>  	saif->dev = &pdev->dev;
>  	ret = devm_request_irq(&pdev->dev, irq, mxs_saif_irq, 0,
>  			       dev_name(&pdev->dev), saif);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request irq\n");
> +	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does returning here leave a dangling pointer
in the global mxs_saif array?

Earlier in mxs_saif_probe(), the newly allocated saif context is stored
globally:

mxs_saif_probe()
    mxs_saif[saif->id] = saif;

If the probe function fails here, or if the driver is manually unbound via
sysfs, devres will automatically free the saif structure. However, the driver
never clears mxs_saif[saif->id].

If a user is actively playing audio and the driver is unbound, the ALSA core
unregisters the component but keeps the PCM file descriptor open. When the
stream is subsequently closed, the machine driver's hw_free callback invokes
mxs_saif_put_mclk(), which will dereference the freed mxs_saif pointer.

>  
>  	platform_set_drvdata(pdev, saif);
>  
> @@ -857,16 +855,12 @@ static int mxs_saif_probe(struct platform_device *pdev)
>  
>  	ret = devm_snd_soc_register_component(&pdev->dev, &mxs_saif_component,
>  					      &mxs_saif_dai, 1);
> -	if (ret) {
> -		dev_err(&pdev->dev, "register DAI failed\n");
> +	if (ret)
>  		return ret;

[Severity: High]
This is also a pre-existing issue, but does returning here cause a
use-after-unmap panic if the clock divider was initialized?

When mxs_saif_probe() calls mxs_saif_mclk_init() for saif0, it registers a
clock divider and an OF clock provider. These are not devm-managed and require
explicit unregistration.

The registered clock divider is passed a register pointer mapped via devres:

mxs_saif_mclk_init()
    clk = clk_register_divider(..., saif->base + SAIF_CTRL, ...);

If devm_snd_soc_register_component() fails and we return directly, devres
will automatically unmap saif->base.

Because the clock framework objects are never unregistered, they remain in the
system. Any subsequent access to this clock, such as reading debugfs or a
machine driver interacting with the clock, will dereference the now-unmapped
register pointer and cause a synchronous external abort.

>  
>  	ret = mxs_pcm_platform_register(&pdev->dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "register PCM failed: %d\n", ret);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	return 0;
>  }

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