diff mbox series

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

Message ID 20250623123054.472216-2-panchuang@vivo.com
State New
Headers show
Series [v6,01/24] genirq/devres: Add devm_request_threaded_irq_probe() and devm_request_irq_probe() | expand

Commit Message

Pan Chuang June 23, 2025, 12:30 p.m. UTC 
From: Yangtao Li <frank.li@vivo.com>

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@linutronix.de>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 include/linux/interrupt.h | 15 ++++++++++++++
 kernel/irq/devres.c       | 43 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

Comments

Thomas Gleixner June 24, 2025, 8:13 a.m. UTC | #1 
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
diff mbox series

Patch

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 51b6484c0493..5c39ff7f030c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -205,6 +205,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 eb16a58e0322..40e3862b0e80 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -80,6 +80,49 @@  int devm_request_threaded_irq(struct device *dev, unsigned int irq,
 }
 EXPORT_SYMBOL(devm_request_threaded_irq);
 
+/**
+ * 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.
+ *
+ * If an IRQ allocated with this function needs to be freed
+ * separately, devm_free_irq() must be used.
+ *
+ * Return: 0 on success or a negative error number.
+ */
+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_GPL(devm_request_threaded_irq_probe);
+
 /**
  *	devm_request_any_context_irq - allocate an interrupt line for a managed device
  *	@dev: device to request interrupt for