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

Hi Daniel,

Thank you for your feedback.

>
> Is there a tool that told you to fix these? How did you reached the conclusion that
> this patch is needed.
>

My main goal was to understand what the underlying core code is
actually doing, so I tend
to trace the call chain and read deeper into the implementation when
reviewing these issues.
I use Bootlin Elixir for this, as it is convenient to access anytime
and anywhere,
even from my phone :

https://elixir.bootlin.com/linux/v7.1.8/C/ident/platform_get_irq_optional

I don't use a tool to detect bugs or generate these patches. The
patches are mainly the result of
reading the code, tracing the actual paths, and then finding issues or
improvements along the way.
Also, through discussions with other maintainers during the review
process, I can sometimes
identify additional issues or areas that could be improved.
For example, this discussion led me to another improvement:

https://lore.kernel.org/all/CAABR9nG2vYKhMKeSxFstCRcHb8Z2FzDTBuQ3PY261Lipx7FfJw@mail.gmail.com/

https://lore.kernel.org/all/[email protected]/#t

> > 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.
>
> Point here is not really that the error is not propagated up in the call chain
> but more like if you remove the error messages the user wont' really know exactly
> where the failure happened.

I understand your concern.
I think this comes down to perspective on how much value the outer
message adds.
Let me give a concrete example :

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

Here, devm_request_irq() internally calls devm_request_result()

https://elixir.bootlin.com/linux/v7.2-rc7/source/kernel/irq/devres.c#L33

 which already uses dev_err_probe():
--------------------------
dev_err_probe(dev, rc, "request_irq(%u) %ps %ps %s\n",
                         irq, handler, thread_fn, devname ? : "");
-------------------------

Since this lower-level message already logs the exact IRQ number,
handler, thread_fn,
devname, and error code, my thought was that the outer dev_err() might
be redundant
without adding extra context.

That said, I recognize this can be a matter of maintainer preference.
If you feel keeping the explicit dev_err() at the driver level is
still clearer for tracing,
I am completely fine dropping this change from the patch series.

Best regards,
Phuc