Messages in this thread | | | Date | Tue, 4 Jul 2023 16:22:27 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v3 2/5] thermal/drivers/armada: convert to use devm_request_threaded_irq_emsg() |
| |
Hello Miquel,
On Tue, Jul 04, 2023 at 10:46:08AM +0200, Miquel Raynal wrote: > Hi Yangtao, > > frank.li@vivo.com wrote on Mon, 3 Jul 2023 17:04:51 +0800: > > > There are more than 700 calls to the devm_request_threaded_irq method. > > Most drivers only request one interrupt resource, and these error > > messages are basically the same. If error messages are printed > > everywhere, more than 1000 lines of code can be saved by removing the > > msg in the driver. > > > > And tglx point out that: > > > > If we actually look at the call sites of > > devm_request_threaded_irq() then the vast majority of them print more or > > less lousy error messages. A quick grep/sed/awk/sort/uniq revealed > > > > 519 messages total (there are probably more) > > > > 352 unique messages > > > > 323 unique messages after lower casing > > > > Those 323 are mostly just variants of the same patterns with > > slight modifications in formatting and information provided. > > > > 186 of these messages do not deliver any useful information, > > e.g. "no irq", " > > > > The most useful one of all is: "could request wakeup irq: %d" > > > > So there is certainly an argument to be made that this particular > > function should print a well formatted and informative error message. > > > > It's not a general allocator like kmalloc(). It's specialized and in the > > vast majority of cases failing to request the interrupt causes the > > device probe to fail. So having proper and consistent information why > > the device cannot be used _is_ useful. > > > > Let's use devm_request_threaded_irq_emsg(), which ensure that all error > > handling branches print error information. In this way, when this function > > fails, the upper-layer functions can directly return an error code without > > missing debugging information. Otherwise, the error message will be > > printed redundantly or missing. > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > > --- > > drivers/thermal/armada_thermal.c | 13 +++++-------- > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > > index 9f6dc4fc9112..a5e140643f00 100644 > > --- a/drivers/thermal/armada_thermal.c > > +++ b/drivers/thermal/armada_thermal.c > > @@ -913,15 +913,12 @@ static int armada_thermal_probe(struct platform_device *pdev) > > > > /* The overheat interrupt feature is not mandatory */ > > if (irq > 0) { > > - ret = devm_request_threaded_irq(&pdev->dev, irq, > > - armada_overheat_isr, > > - armada_overheat_isr_thread, > > - 0, NULL, priv); > > - if (ret) { > > - dev_err(&pdev->dev, "Cannot request threaded IRQ %d\n", > > - irq); > > + ret = devm_request_threaded_irq_emsg(&pdev->dev, irq, > > + armada_overheat_isr, > > + armada_overheat_isr_thread, > > + 0, NULL, priv, NULL); > > + if (ret) > > I don't see a patch renaming this helper with s/emsg//, do you plan to > keep it like that? I bet nobody outside of this series will notice the > new helper and will continue to add error messages because it kind > of feels "right" to do so. > > I would rather prefer returning to the original function name which > everybody knows/uses.
See https://lore.kernel.org/lkml/87h6qpyzkd.ffs@tglx for why there is a new function name.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |