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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 30/07/2025 à 08:25, Pan Chuang a écrit :
> The devm_request_threaded_irq() and devm_request_any_context_irq() 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 functions, and error code on failure.
> 
> 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>
> ---
>   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..dcbb9d0cd736 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.
> + *
> + * 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;

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

CJ

>   
>   /**