Messages in this thread | | | Date | Tue, 4 Jul 2023 16:29:37 +0200 | From | Miquel Raynal <> | Subject | Re: [PATCH v3 2/5] thermal/drivers/armada: convert to use devm_request_threaded_irq_emsg() |
| |
Hi Uwe,
u.kleine-koenig@pengutronix.de wrote on Tue, 4 Jul 2023 16:22:27 +0200:
> 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.
Yes of course, I fully understand Thomas' concerns, but I am questioning the usefulness of creating such helper if it's not the default. People will continue to use [devm_]request_threaded_irq() without noticing the new helper. If we want to make this change useful, I believe we should: - target all users - at the end of the series: atomically include the error message in request_threaded_irq() and rename all callers of the _verbose variant which will eventually vanish.
Otherwise this is a lot of noise for very little progress, IMHO :)
Thanks, Miquèl
| |