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

> [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?
>

I checked all error paths in the called functions, and they already
report the corresponding errors,
either directly or deeper in the call chain.
Therefore, removing the additional dev_err() calls does not make these
failures silent.

> [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;
> >  }
>

I confirmed that both issues are pre-existing and are not introduced
by this patch.
They are unrelated to the error-message cleanup in this series.