Re: [PATCH v9 1/1] genirq/devres: Add dev_err_probe() in devm_request_threaded_irq() and devm_request_any_context_irq()

[PanChuang]

Hi, Christophe

在 2025/7/31 0:48, Christophe JAILLET 写道:
> Le 30/07/2025 à 08:25, Pan Chuang a écrit :
> > + * Return: IRQC_IS_HARDIRQ or IRQC_IS_NESTED on success, or a 
> > negative error
> > + * number.
> > + */
> > +int devm_request_any_context_irq(struct device *dev, unsigned int irq,
> > +                 irq_handler_t handler, unsigned long irqflags,
> > +                 const char *devname, void *dev_id)
> > +{
> > +    int rc = __devm_request_any_context_irq(dev, irq, handler, 
> > irqflags,
> > +                        devname, dev_id);
> > +    if (rc < 0) {
> > +        return dev_err_probe(dev, rc, "request_irq(%u) %ps %s\n",
> > +                     irq, handler, devname ? : "");
> > +    }
>
> Extra { } should be removed.
>
> From my PoV, it would look more logical to have the same logic in 
> devm_request_threaded_irq() and in devm_request_any_context_irq().
>
> Why "if (!rc) SUCCESS" in one case, and "if (rc < 0) FAILURE" in the 
> other case?
>
> Personally, I would change in devm_request_threaded_irq() above to have
>     if (rc)
>         return dev_err_probe();
>     return 0;
Thanks for your suggestion.

The return value of __devm_request_any_context_irq on success is either 
IRQC_IS_HARDIRQ or IRQC_IS_NESTED (>= 0).

Therefore, we cannot directly use "if (rc)" to check for errors.

> > +
> > +    return rc;
> > +}
> >   EXPORT_SYMBOL(devm_request_any_context_irq);
>
> On version 5 of the patch, there was a comment related to using 
> EXPORT_SYMBOL_GPL(), does it still make sense?
> (no strong opinion from me, just noted that and wondered if done on 
> purpose)
Since it is an existing export, I will keep it as-is.

Thanks,

    Pan Chuang