| 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
| ||
|
Message-ID: <2afd89be-713e-4075-b91b-36ec7fb6072d@wanadoo.fr>
Date: Wed, 30 Jul 2025 18:48:40 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Pan Chuang <panchuang@...o.com>, tglx@...utronix.de
Cc: kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org,
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
Subject: Re: [PATCH v9 1/1] genirq/devres: Add dev_err_probe() in
devm_request_threaded_irq() and devm_request_any_context_irq()
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@...o.com>
> Signed-off-by: Yangtao Li <frank.li@...o.com>
> Signed-off-by: Pan Chuang <panchuang@...o.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
>
> /**
Powered by blists - more mailing lists