lkml.org 
[lkml]   [2023]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-07-04 16:30    [W:2.287 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site