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

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

[Pan Chuang]

There are over 700 calls to devm_request_threaded_irq() and more than 1000
calls to devm_request_irq() in the kernel. Currently, most drivers implement
repetitive and inconsistent error handling for these functions:

1. Over 2000 lines of code are dedicated to error messages
2. Analysis shows 519 unique error messages with 323 variants after normalization
3. 186 messages provide no useful debugging information
4. Only a small fraction deliver meaningful error context

As tglx pointed out:
  "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."

This patch implements a standardized error reporting approach[1]:

1. Renames existing functions to __devm_request_threaded_irq() and
   __devm_request_any_context_irq()
2. Creates new devm_request_threaded_irq() and devm_request_any_context_irq()
   functions that:
   a) Invoke the underscore-prefixed variants
   b) On error, call dev_err_probe() to provide consistent diagnostics

The new error format provides complete debugging context:
  "<device>: error -<errcode>: request_irq(<irq>) <handler> <thread_fn> <devname>"

Example from our QEMU testing:
  test_irq_device: error -EINVAL: request_irq(1001) test_handler+0x0/0x10 [test_irq] test_thread_fn+0x0/0x10 [test_irq] irq-1001-failure

Based on the v6, standardize coding style without logical change.
https://lore.kernel.org/all/20250728123251.384375-2-panchuang@vivo.com/

[1]https://lore.kernel.org/all/87qzy9tvso.ffs@tglx/

Pan Chuang (1):
  genirq/devres: Add dev_err_probe() in devm_request_threaded_irq() and
    devm_request_any_context_irq()

 kernel/irq/devres.c | 121 +++++++++++++++++++++++++++++---------------
 1 file changed, 81 insertions(+), 40 deletions(-)

-- 
2.34.1

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

[Pan Chuang]

The devm_request_threaded_irq() and devm_request_any_context_irq() functions
currently don't print any error when interrupt registration fails. This forces
each driver to implement redundant error logging - over 2,000 lines of error
messages exist across drivers. Additionally, when upper-layer functions
propagate these errors without logging, critical debugging information is lost.

Add automatic error logging to these functions via dev_err_probe(), printing
device name, IRQ number, handler addresses, and error code on failure.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 kernel/irq/devres.c | 121 +++++++++++++++++++++++++++++---------------
 1 file changed, 81 insertions(+), 40 deletions(-)

diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
index eb16a58e0322..4656ed12532c 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -30,29 +30,11 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
 	return this->irq == match->irq && this->dev_id == match->dev_id;
 }
 
-/**
- *	devm_request_threaded_irq - allocate an interrupt line for a managed device
- *	@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
- *
- *	Except for the extra @dev argument, this function takes the
- *	same arguments and performs the same function as
- *	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.
- */
-int devm_request_threaded_irq(struct device *dev, unsigned int irq,
-			      irq_handler_t handler, irq_handler_t thread_fn,
-			      unsigned long irqflags, const char *devname,
-			      void *dev_id)
+static int __devm_request_threaded_irq(struct device *dev, unsigned int irq,
+				       irq_handler_t handler,
+				       irq_handler_t thread_fn,
+				       unsigned long irqflags,
+				       const char *devname, void *dev_id)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -78,28 +60,50 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq,
 
 	return 0;
 }
-EXPORT_SYMBOL(devm_request_threaded_irq);
 
 /**
- *	devm_request_any_context_irq - allocate an interrupt line for a managed device
- *	@dev: device to request interrupt for
- *	@irq: Interrupt line to allocate
- *	@handler: Function to be called when the IRQ occurs
- *	@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
+ * devm_request_threaded_irq - allocate an interrupt line for a managed device with error logging
+ * @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
  *
- *	Except for the extra @dev argument, this function takes the
- *	same arguments and performs the same function as
- *	request_any_context_irq().  IRQs requested with this function will be
- *	automatically freed on driver detach.
+ * Except for the extra @dev argument, this function takes the same arguments
+ * and performs the same function as 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.
+ *
+ * When the request fails, an error message is printed with contextual
+ * information (device name, interrupt number, handler functions and
+ * error code). Don't add extra error messages at the call sites.
  *
- *	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_any_context_irq(struct device *dev, unsigned int irq,
-			      irq_handler_t handler, unsigned long irqflags,
-			      const char *devname, void *dev_id)
+int devm_request_threaded_irq(struct device *dev, unsigned int irq,
+			      irq_handler_t handler, irq_handler_t thread_fn,
+			      unsigned long irqflags, const char *devname,
+			      void *dev_id)
+{
+	int rc = __devm_request_threaded_irq(dev, irq, handler, thread_fn,
+					     irqflags, devname, dev_id);
+	if (!rc)
+		return 0;
+
+	return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",
+			     irq, handler, thread_fn, devname ? : "");
+}
+EXPORT_SYMBOL(devm_request_threaded_irq);
+
+static 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)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -124,6 +128,43 @@ int devm_request_any_context_irq(struct device *dev, unsigned int irq,
 
 	return rc;
 }
+
+/**
+ * devm_request_any_context_irq - allocate an interrupt line for a managed device with error logging
+ * @dev:	Device to request interrupt for
+ * @irq:	Interrupt line to allocate
+ * @handler:	Function to be called when the IRQ occurs
+ * @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
+ *
+ * Except for the extra @dev argument, this function takes the same arguments
+ * and performs the same function as request_any_context_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.
+ *
+ * When the request fails, an error message is printed with contextual
+ * information (device name, interrupt number, handler functions and
+ * error code). Don't add extra error messages at the call sites.
+ *
+ * On failure, it returns a negative value. On success, it returns either
+ * IRQC_IS_HARDIRQ or IRQC_IS_NESTED.
+ */
+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 ? : "");
+	}
+
+	return rc;
+}
 EXPORT_SYMBOL(devm_request_any_context_irq);
 
 /**
-- 
2.34.1

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

[Ahmad Fatoum]

Hi Pan,

On 29.07.25 10:14, Pan Chuang wrote:
> -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 devm_request_threaded_irq(struct device *dev, unsigned int irq,
> +			      irq_handler_t handler, irq_handler_t thread_fn,
> +			      unsigned long irqflags, const char *devname,
> +			      void *dev_id)
> +{
> +	int rc = __devm_request_threaded_irq(dev, irq, handler, thread_fn,
> +					     irqflags, devname, dev_id);
> +	if (!rc)
> +		return 0;
> +
> +	return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",

Compared to %ps, %pS also prints offset and size relative to the symbol.
This makes sense when you have an arbitrary program counter, but for
merely printing a function symbol, I'd suggest %ps as it's less noisy.

Cheers,
Ahmad

> +			     irq, handler, thread_fn, devname ? : "");
> +}
> +EXPORT_SYMBOL(devm_request_threaded_irq);
> +
> +static 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)
>  {
>  	struct irq_devres *dr;
>  	int rc;
> @@ -124,6 +128,43 @@ int devm_request_any_context_irq(struct device *dev, unsigned int irq,
>  
>  	return rc;
>  }
> +
> +/**
> + * devm_request_any_context_irq - allocate an interrupt line for a managed device with error logging
> + * @dev:	Device to request interrupt for
> + * @irq:	Interrupt line to allocate
> + * @handler:	Function to be called when the IRQ occurs
> + * @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
> + *
> + * Except for the extra @dev argument, this function takes the same arguments
> + * and performs the same function as request_any_context_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.
> + *
> + * When the request fails, an error message is printed with contextual
> + * information (device name, interrupt number, handler functions and
> + * error code). Don't add extra error messages at the call sites.
> + *
> + * On failure, it returns a negative value. On success, it returns either
> + * IRQC_IS_HARDIRQ or IRQC_IS_NESTED.
> + */
> +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 ? : "");
> +	}
> +
> +	return rc;
> +}
>  EXPORT_SYMBOL(devm_request_any_context_irq);
>  
>  /**


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Thomas Gleixner]

On Tue, Jul 29 2025 at 10:20, Ahmad Fatoum wrote:
> On 29.07.25 10:14, Pan Chuang wrote:
>> +	return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",
>
> Compared to %ps, %pS also prints offset and size relative to the symbol.
> This makes sense when you have an arbitrary program counter, but for
> merely printing a function symbol, I'd suggest %ps as it's less noisy.

Good point

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

[Thomas Gleixner]

On Tue, Jul 29 2025 at 16:14, Pan Chuang wrote:

> The devm_request_threaded_irq() and devm_request_any_context_irq() functions

devm_request_threaded_irq() and devm_request_any_context_irq() ....

The '()' notation already makes it clear that these are functions, so no
'The ... functions' is redundant.

> currently don't print any error when interrupt registration fails. This forces
> each driver to implement redundant error logging - over 2,000 lines of error
> messages exist across drivers. Additionally, when upper-layer functions
> propagate these errors without logging, critical debugging information is lost.
>
> Add automatic error logging to these functions via dev_err_probe(), printing
> device name, IRQ number, handler addresses, and error code on failure.

Again: %pS (or %ps) does NOT print the handler address. It prints the
symbol name. Feel free to ignore my review comments, but then accept
that I ignore your patches too.

> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Signed-off-by: Pan Chuang <panchuang@vivo.com>

This SOB chain is still incorrect. Again:

 https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

If anything is unclear, then please ask.

> +/**
> + * devm_request_any_context_irq - allocate an interrupt line for a managed device with error logging
> + * @dev:	Device to request interrupt for
> + * @irq:	Interrupt line to allocate
> + * @handler:	Function to be called when the IRQ occurs
> + * @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
> + *
> + * Except for the extra @dev argument, this function takes the same arguments
> + * and performs the same function as request_any_context_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.
> + *
> + * When the request fails, an error message is printed with contextual
> + * information (device name, interrupt number, handler functions and
> + * error code). Don't add extra error messages at the call sites.
> + *
> + * On failure, it returns a negative value. On success, it returns either
> + * IRQC_IS_HARDIRQ or IRQC_IS_NESTED.

As you touch this, can you please convert this to the proper

   Returns:

formatting?

Thanks,

        tglx

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

[Pan Chuang]

Hi tglx,

On Tue, Jul 29 2025 at 17:14, Thomas Gleixner wrote:
>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>> Signed-off-by: Pan Chuang <panchuang@vivo.com>
> This SOB chain is still incorrect. Again:
>
>   https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
> If anything is unclear, then please ask.

Could you please advise if this SOB chain is correct:

Co-developed-by: Yangtao Li <frank.li@vivo.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
Signed-off-by: Pan Chuang <panchuang@vivo.com>

Thank you for your time and guidance.

Thanks,

    PanChuang

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

[Thomas Gleixner]

On Tue, Jul 29 2025 at 19:48, Pan Chuang wrote:
> On Tue, Jul 29 2025 at 17:14, Thomas Gleixner wrote:
>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>> Signed-off-by: Pan Chuang <panchuang@vivo.com>
>> This SOB chain is still incorrect. Again:
>>
>>   https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>>
>> If anything is unclear, then please ask.
>
> Could you please advise if this SOB chain is correct:
>
> Co-developed-by: Yangtao Li <frank.li@vivo.com>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Signed-off-by: Pan Chuang <panchuang@vivo.com>

If you are supposed to be listed as author of the patch in git, then
yes.

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

[Markus Elfring]

…
> This patch implements a standardized error reporting approach[1]:
…

Would it become helpful to move any information from the cover letter
to the (single) patch?
How do you think about to avoid the presentation of duplicate data?

Regards,
Markus

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

[Pan Chuang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 733 bytes --]

Hi, Markus
On Wed, Jul 30, 2025 at 2:35 AM, Markus Elfring wrote:
> …
>> This patch implements a standardized error reporting approach[1]:
> …
>
> Would it become helpful to move any information from the cover letter
> to the (single) patch?
> How do you think about to avoid the presentation of duplicate data?
Thank you for your suggestion.

To ensure commit message conciseness, we only describe:

    The context and problem being addressed.

    A brief summary of the solution.

Regarding duplicate error messages:

    The failure probability is low, so temporary duplication is acceptable for now.

    We will remove the random printks from the drivers, once the core change 
    has hit upstream.

Thanks,

	PanChuang