Re: [PATCH v6 01/24] genirq/devres: Add devm_request_threaded_irq_probe() and devm_request_irq_probe()

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Jun 23 2025 at 20:30, Pan Chuang wrote:
> +/**
> + * devm_request_threaded_irq_probe - request irq for a managed device with error msg (recommended in probe)
> + * @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 @info argument, this function takes the
> + * same arguments and performs the same function as
> + * devm_request_threaded_irq(). IRQs requested with this function will be
> + * automatically freed on driver detach.

I really have to ask why you need the extra info argument. It's not
providing any really value.

What's important when the interrupt request fails?

    1) The device, which is identifiable by @devname and/or dev_name(@dev)

    2) The interrupt number

    3) The error code

If you want to be more expressive then you can also print out the
handler function symbols, which makes it even more useful to map back
into the affected driver.

As you cited correctly I said back then:

  "So there is certainly an argument to be made that this particular
   function should print a well formatted and informative error
   message."

This particular function is: devm_request_threaded_irq().

IOW, I did not ask you should go an create a new one, right?

The extra @info argument is just proliferating the nonsensical
information which driver developers put into the output. I just looked
through a couple of your thermal patches (hint, I had to chase them
manually because you failed to thread them properly). While most of them
provide NULL, the two adding 'alarm' are really not providing any useful
information.

The point is, that the information is aimed at developers and not to be
helpful for Joe User to identify/fix a problem. If the driver
developer/maintainer is not able to track back the information (name,
number, error code) to the driver in question, then the added "foo" info
is not going to make him any smarter.

If that happens, then being able to map it back to the driver is only 1%
of the problem analysis, simply because you need to decode the
underlying problem (DT, vector exhaustion, memory exhaustion ....) to be
able to address it.

So just make devm_request.*irq() emit a uniform and informative message
on fail and you can go and remove all the homebrewn useless error prints
from the drivers.

It does not matter at all if there are duplicated error messages for a
release or two until all drivers have been cleaned up. This is not the
common case and only rarely triggered. So what?

> +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);

This is just wrong as you fail to hand in thread_fn.

Q: How was this code ever tested?
A: Not at all.

> Could you please consider merging the entire series into your branch? 

You're seriously asking that for something which is so obviously broken?

> +	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 ? : "");

This is wrong too because devm_request_threaded_irq() allows you to hand
in both a hard interrupt and a threaded handler and either one of them
can be NULL.

So what you want to add in devm_request_threaded_irq() is something like
this:

	if (rc < 0) {
		return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",
                        	     handler, thread_fn, devname ? : "");
        }

dev_err_probe() already prefixes the caller string with 'error ', so
there is no need for a lenghty 'failed to request ....' novel.

Thanks,

        tglx