Message-ID: <20230710134135.00007ba7@Huawei.com>
Date:   Mon, 10 Jul 2023 13:41:35 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     Yangtao Li <frank.li@...o.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 01/21] genirq/devres: Add
 devm_request_threaded_irq_probe() and devm_request_irq_probe()

On Mon, 10 Jul 2023 14:25:24 +0200
Geert Uytterhoeven <geert@...ux-m68k.org> wrote:

> Hi Yangtao,
> 
> On Mon, Jul 10, 2023 at 12:05 PM Yangtao Li <frank.li@...o.com> wrote:
> > There are more than 700 calls to devm_request_threaded_irq method and
> > more than 1000 calls to devm_request_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 2000 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.
> >
> > So add devm_request_threaded_irq_probe() and devm_request_irq_probe(),
> > 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@...utronix.de>
> > Cc: Krzysztof Kozlowski <krzk@...nel.org>
> > Cc: "Uwe Kleine-König" <u.kleine-koenig@...gutronix.de>
> > Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > Cc: AngeloGioacchino Del Regno  <angelogioacchino.delregno@...labora.com>
> > Signed-off-by: Yangtao Li <frank.li@...o.com>  
> 
> Thanks for your patch!
> 
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -201,6 +201,21 @@ extern void free_percpu_nmi(unsigned int irq, void __percpu *percpu_dev_id);
> >
> >  struct device;
> >
> > +extern int __must_check
> > +devm_request_threaded_irq_probe(struct device *dev, unsigned int irq,
> > +                             irq_handler_t handler, irq_handler_t thread_fn,
> > +                             unsigned long irqflags, const char *devname,
> > +                             void *dev_id, const char *info);
> > +
> > +static inline int __must_check
> > +devm_request_irq_probe(struct device *dev, unsigned int irq,
> > +                      irq_handler_t handler, unsigned long irqflags,
> > +                      const char *devname, void *dev_id, const char *info)
> > +{
> > +       return devm_request_threaded_irq_probe(dev, irq, handler, NULL, irqflags,
> > +                                        devname, dev_id, info);
> > +}
> > +
> >  extern int __must_check
> >  devm_request_threaded_irq(struct device *dev, unsigned int irq,
> >                           irq_handler_t handler, irq_handler_t thread_fn,
> > diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
> > index f6e5515ee077..43a40d6e2e0b 100644
> > --- a/kernel/irq/devres.c
> > +++ b/kernel/irq/devres.c
> > @@ -79,6 +79,46 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq,
> >  }
> >  EXPORT_SYMBOL(devm_request_threaded_irq);
> >
> > +/**
> > + *     devm_request_threaded_irq_probe - allocate an interrupt line for a managed device
It's awkward for the short description to be identical to devm_request_threaded_irq
but I can't think of a sensible and short way to differentiate it.

> > + *     @dev: device to request interrupt for
> > + *     @irq: Interrupt line to allocate
> > + *     @handler: Function to be called when the IRQ occurs
> > + *     @thread_fn: function to be called in a threaded interrupt context. NULL
> > + *                 for devices which handle everything in @handler
> > + *     @irqflags: Interrupt type flags
> > + *     @devname: An ascii name for the claiming device, dev_name(dev) if NULL
> > + *     @dev_id: A cookie passed back to the handler function
> > + *     @info: Optional additional error log
> > + *
> > + *     This is a variant of the devm_request_threaded_irq function.
> > + *     It will print an error message by default when the request fails,
> > + *     and the consumer can add a special error msg.
> > + *
> > + *     Except for the extra @dev argument, this function takes the  
> 
> ... extra @dev and @info arguments, ...
> 
> > + *     same arguments and performs the same function as
> > + *     request_threaded_irq().  IRQs requested with this function will be  
> 
> But probably you want to compare with devm_request_threaded_irq()
> instead of with request_threaded_irq()?
> I.e. "... extra @info argument..." above.
> 
> > + *     automatically freed on driver detach.
> > + *
> > + *     If an IRQ allocated with this function needs to be freed
> > + *     separately, devm_free_irq() must be used.
> > + */
> > +int devm_request_threaded_irq_probe(struct device *dev, unsigned int irq,
> > +                                   irq_handler_t handler, irq_handler_t thread_fn,
> > +                                   unsigned long irqflags, const char *devname,
> > +                                   void *dev_id, const char *info)
> > +{
> > +       int rc;
> > +
> > +       rc = devm_request_threaded_irq(dev, irq, handler, NULL, irqflags, devname, dev_id);
> > +       if (rc)
> > +               return dev_err_probe(dev, rc, "Failed to request %sinterrupt %u %s %s\n",
> > +                                    thread_fn ? "threaded " : "", irq, devname ? : dev_name(dev),
> > +                                    info ? : "");
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(devm_request_threaded_irq_probe);
> > +
> >  /**
> >   *     devm_request_any_context_irq - allocate an interrupt line for a managed device
> >   *     @dev: device to request interrupt for  
> 
> With the above fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>

Likewise, lgtm
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

I'd aim to get this upstream in one cycle with a few example drover changes
but if you plan to do mass changes, leave them for the following cycle to
make it really easy for maintainers to pick them up.

Jonathan


> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Powered by blists - more mailing lists