linux-kernel - Re: [PATCH v7 1/1] genirq/devres: Add dev_err_probe() in devm_request_threaded_irq() and devm_request_any_context

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250728161050.000018f2@huawei.com>
Date: Mon, 28 Jul 2025 16:10:50 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Pan Chuang <panchuang@...o.com>
CC: <tglx@...utronix.de>, <linux-kernel@...r.kernel.org>,
	<miquel.raynal@...tlin.com>, <u.kleine-koenig@...gutronix.de>,
	<angeg.delregno@...labora.com>, <krzk@...nel.org>, <a.fatoum@...gutronix.de>,
	<frank.li@...o.com>
Subject: Re: [PATCH v7 1/1] genirq/devres: Add dev_err_probe() in
 devm_request_threaded_irq() and devm_request_any_context_irq()

On Mon, 28 Jul 2025 20:32:51 +0800
Pan Chuang <panchuang@...o.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@...o.com>
> Signed-off-by: Yangtao Li <frank.li@...o.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