Message-ID: <875xfcpc9l.ffs@tglx>
Date: Mon, 28 Jul 2025 17:26:30 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Pan Chuang <panchuang@...o.com>, linux-kernel@...r.kernel.org
Cc: miquel.raynal@...tlin.com, Jonathan.Cameron@...wei.com,
 u.kleine-koenig@...gutronix.de, angeg.delregno@...labora.com,
 krzk@...nel.org, a.fatoum@...gutronix.de, frank.li@...o.com, Pan Chuang
 <panchuang@...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, 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@...o.com>
> Signed-off-by: Yangtao Li <frank.li@...o.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

Powered by blists - more mailing lists