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

[PATCH v7 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

Patch Link (v6):
https://lore.kernel.org/all/20250623123054.472216-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 | 78 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

-- 
2.34.1

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

[Pan Chuang]

Modify devm_request_thread_irq() and devm_request_any_context_irq(), in
order to print an error message by default when the request fails.

Converting drivers to use this API has the following benefits:

  1.More than 2,000 lines of code can be saved by removing redundant error
  messages in drivers.

  2.Upper-layer functions can directly return error codes without missing
  debugging information.

  3.Having proper and consistent information about why the device cannot
  be used is useful.

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

diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
index eb16a58e0322..a1b934c1ecc8 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -31,7 +31,7 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
 }
 
 /**
- *	devm_request_threaded_irq - allocate an interrupt line for a managed device
+ *	__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
@@ -49,7 +49,7 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
  *	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,
+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)
@@ -78,10 +78,46 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq,
 
 	return 0;
 }
+
+/**
+ * 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
+ *
+ * This function extends __devm_request_threaded_irq by adding detailed error
+ * logging via dev_err_probe() when the underlying request fails. It ensures the
+ * interrupt is automatically freed on driver detach and provides contextual
+ * information (e.g., IRQ number, handler address, device name) in error messages.
+ *
+ * Return: 0 on success or a negative error number.
+ */
+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;
+
+	rc = __devm_request_threaded_irq(dev, irq, handler, thread_fn, irqflags,
+					 devname, dev_id);
+	if (rc < 0) {
+		return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",
+				     irq, handler, thread_fn,
+				     devname ? : dev_name(dev));
+	}
+
+	return 0;
+}
 EXPORT_SYMBOL(devm_request_threaded_irq);
 
 /**
- *	devm_request_any_context_irq - allocate an interrupt line for a managed device
+ *	__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
@@ -97,7 +133,7 @@ EXPORT_SYMBOL(devm_request_threaded_irq);
  *	If an IRQ allocated with this function needs to be freed
  *	separately, devm_free_irq() must be used.
  */
-int devm_request_any_context_irq(struct device *dev, unsigned int 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)
 {
@@ -124,6 +160,40 @@ 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
+ *
+ * This function extends __devm_request_any_context_irq by adding detailed error
+ * logging via dev_err_probe() when the underlying request fails. It ensures the
+ * interrupt is automatically freed on driver detach and provides contextual
+ * information (e.g., IRQ number, handler address, device name) in error messages.
+ *
+ * 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;
+
+	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 ? : dev_name(dev));
+	}
+
+	return rc;
+
+}
 EXPORT_SYMBOL(devm_request_any_context_irq);
 
 /**
-- 
2.34.1

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

[Jonathan Cameron]

On Mon, 28 Jul 2025 20:32:51 +0800
Pan Chuang <panchuang@vivo.com> wrote:

> Modify devm_request_thread_irq() and devm_request_any_context_irq(), in
> order to print an error message by default when the request fails.
> 
> Converting drivers to use this API has the following benefits:
> 
>   1.More than 2,000 lines of code can be saved by removing redundant error
>   messages in drivers.
> 
>   2.Upper-layer functions can directly return error codes without missing
>   debugging information.
> 
>   3.Having proper and consistent information about why the device cannot
>   be used is useful.
> 
> Signed-off-by: Pan Chuang <panchuang@vivo.com>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  kernel/irq/devres.c | 78 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
> index eb16a58e0322..a1b934c1ecc8 100644
> --- a/kernel/irq/devres.c
> +++ b/kernel/irq/devres.c
> @@ -31,7 +31,7 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
>  }
>  
>  /**
> - *	devm_request_threaded_irq - allocate an interrupt line for a managed device
> + *	__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
> @@ -49,7 +49,7 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
>   *	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,
> +static int __devm_request_threaded_irq(struct device *dev, unsigned int irq,
>  			      irq_handler_t handler, irq_handler_t thread_fn,

Parameters were previously aligned after (
Probably best to realign them to keep that style.

>  			      unsigned long irqflags, const char *devname,
>  			      void *dev_id)
> @@ -78,10 +78,46 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq,
>  
>  	return 0;
>  }
> +
> +/**
> + * 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
> + *
> + * This function extends __devm_request_threaded_irq by adding detailed error
> + * logging via dev_err_probe() when the underlying request fails. It ensures the
> + * interrupt is automatically freed on driver detach and provides contextual
> + * information (e.g., IRQ number, handler address, device name) in error messages.
> + *
> + * Return: 0 on success or a negative error number.
> + */
> +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;
> +
> +	rc = __devm_request_threaded_irq(dev, irq, handler, thread_fn, irqflags,
> +					 devname, dev_id);
> +	if (rc < 0) {

I'm in two minds about this.
The internals of __devm_request_threaded_irq() use if (rc)
so we know that will be fine here.

However, given you are passing it to dev_err_probe() locally being sure
that we don't have a value > 0 perhaps makes sense.

> +		return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",
> +				     irq, handler, thread_fn,
> +				     devname ? : dev_name(dev));
> +	}
> +
> +	return 0;
> +}
>  EXPORT_SYMBOL(devm_request_threaded_irq);

Same comments apply to remaining code.

Jonathan

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

[Thomas Gleixner]

On Mon, Jul 28 2025 at 20:32, Pan Chuang wrote:
> Modify devm_request_thread_irq() and devm_request_any_context_irq(), in
> order to print an error message by default when the request fails.

> Converting drivers to use this API has the following benefits:

No driver is converted. They all use the API already, no?

>   1.More than 2,000 lines of code can be saved by removing redundant error
>   messages in drivers.
>
>   2.Upper-layer functions can directly return error codes without missing
>   debugging information.
>
>   3.Having proper and consistent information about why the device cannot
>   be used is useful.

Please read

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

and do not start with _WHAT_ the patch does. Decribe the context and the
problem you are trying to solve and then briefly the solution.

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

This Signed-off-by chain is broken.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> ---
>  kernel/irq/devres.c | 78 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
> index eb16a58e0322..a1b934c1ecc8 100644
> --- a/kernel/irq/devres.c
> +++ b/kernel/irq/devres.c
> @@ -31,7 +31,7 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
>  }
>  
>  /**
> - *	devm_request_threaded_irq - allocate an interrupt line for a managed device
> + *	__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
> @@ -49,7 +49,7 @@ static int devm_irq_match(struct device *dev, void *res, void *data)
>   *	If an IRQ allocated with this function needs to be freed
>   *	separately, devm_free_irq() must be used.
>   */

This kernel doc is a pointless duplicate. Just leave it above the API
function. This is an internal function, which is split out to make the
error path simpler.

> -int devm_request_threaded_irq(struct device *dev, unsigned int irq,
> +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)
> @@ -78,10 +78,46 @@ int devm_request_threaded_irq(struct device *dev, unsigned int irq,
>  
>  	return 0;
>  }
> +
> +/**
> + * 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

Nice reformatting!

> + *
> + * This function extends __devm_request_threaded_irq by adding detailed error

This is irrelevant for the user and you take the real important
information away from the user and hide it in the pointless documenation
of the internal function.

> + * logging via dev_err_probe() when the underlying request fails. It ensures the
> + * interrupt is automatically freed on driver detach and provides contextual
> + * information (e.g., IRQ number, handler address, device name) in error messages.

Just add something like this to the original documentation:

 * 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.

I used 'handler functions' because 'handler address' is not what %pS
prints.

> + * Return: 0 on success or a negative error number.

Nice addition!

> + */
> +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;
> +
> +	rc = __devm_request_threaded_irq(dev, irq, handler, thread_fn, irqflags,
> +					 devname, dev_id);

Just move this up to the declaration

	int rc = __devm_request_threaded_irq(dev, irq, handler, thread_fn,
					     irqflags, devname, dev_id);

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

If you reverse the check, then you can spare an indentation level and
brackets.

        if (!rc)
        	return 0;

	return dev_err_probe(dev, rc, "request_irq(%u) %pS %pS %s\n",
			     irq, handler, thread_fn, devname ? : "");
                
There is no point to print dev_name(dev) again in case devname is NULL
as it is already printed by dev_err_probe() at the beginning, no?

Thanks,

        tglx