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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
Signed-off-by: Yangtao Li <frank.li@xxxxxxxx>
Signed-off-by: Pan Chuang <panchuang@xxxxxxxx>
---
  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

/**





[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux