diff mbox series

driver core: platform: Add an error message to platform_get_irq*()

Message ID 20181222072452.186726-1-swboyd@chromium.org (mailing list archive)
State New, archived
Headers show
Series driver core: platform: Add an error message to platform_get_irq*() | expand

Commit Message

Stephen Boyd Dec. 22, 2018, 7:24 a.m. UTC 
A grep of the kernel shows that many drivers print an error message if
they fail to get the irq they're looking for. Furthermore, those drivers
all decide to print the device name, or not, and the irq they were
requesting, or not, etc. Let's consolidate all these error messages into
the API itself, allowing us to get rid of the error messages in each
driver.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Rob Herring wrote:
> Shouldn't platform_get_irq (or what it calls) print the error message
> (like we do for kmalloc), rather than every driver? We could get rid
> of lots of error strings that way. I guess there are cases where no
> irq is not an error and we wouldn't want to always print an error. In
> some cases like that, we have 2 versions of the function.

Yes it should. Just by coincidence, I got around to implementing this
patch yesterday after I bemoaned this on the list a few months ago[1].

> Not what you're addressing here exactly, but what I'd like to see is
> the ability to print the exact locations generating errors in the
> first place. That would require wrapping all the error code
> assignments and returns (or at least the common sources). 

That could be done with some macro magic to add __line__ and __FILE__
into a definition of the "important" functions. In that sense nothing
would really need to change besides the implementation, right?

I also started working on a cocci script to fix up the call sites to
drop the extra prints, but I'm really bad at writing semantic patches so
please help improve the below. My simple grep shows that we can remove
~500 error prints out of the 1500 places the platform_get_irq()
functions are called.

	@@
	expression ret;
	struct platform_device *E;
	@@

	ret =
	(
	platform_get_irq(E, ...)
	|
	platform_get_irq_byname(E, ...)
	);

	if ( \( ret < 0 \| ret <= 0 \) )
	{
	(
	-if (ret != -EPROBE_DEFER)
	-{ ...
	-dev_err(...);
	-... }
	|
	...
	-dev_err(...);
	)
	...
	}

[1] https://lkml.kernel.org/r/153911433511.119890.17831207059115471972@swboyd.mtv.corp.google.com

 drivers/base/platform.c | 52 +++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

Comments

Russell King (Oracle) Dec. 22, 2018, 10:33 a.m. UTC | #1 
On Fri, Dec 21, 2018 at 11:24:52PM -0800, Stephen Boyd wrote:
> A grep of the kernel shows that many drivers print an error message if
> they fail to get the irq they're looking for. Furthermore, those drivers
> all decide to print the device name, or not, and the irq they were
> requesting, or not, etc. Let's consolidate all these error messages into
> the API itself, allowing us to get rid of the error messages in each
> driver.
...
> +error:
> +	if (warn)
> +		dev_err(&dev->dev, "IRQ%d not found\n", num);

Please don't use the notation IRQn - this is normally used when
referring to interrupt numbers (such as those seen in
/proc/interrupts) rather than a per-device interrupt index.
Grep for IRQ% in drivers/ for many examples.

dev_err(&dev->dev, "IRQ index %u not found: %d\n", num, ret);

would be better - note also the use of %u for unsigned integers.
Using %d for them is IMHO sloppy coding.
Stephen Boyd Dec. 28, 2018, 9:53 p.m. UTC | #2 
Quoting Russell King - ARM Linux (2018-12-22 02:33:20)
> On Fri, Dec 21, 2018 at 11:24:52PM -0800, Stephen Boyd wrote:
> > A grep of the kernel shows that many drivers print an error message if
> > they fail to get the irq they're looking for. Furthermore, those drivers
> > all decide to print the device name, or not, and the irq they were
> > requesting, or not, etc. Let's consolidate all these error messages into
> > the API itself, allowing us to get rid of the error messages in each
> > driver.
> ...
> > +error:
> > +     if (warn)
> > +             dev_err(&dev->dev, "IRQ%d not found\n", num);
> 
> Please don't use the notation IRQn - this is normally used when
> referring to interrupt numbers (such as those seen in
> /proc/interrupts) rather than a per-device interrupt index.
> Grep for IRQ% in drivers/ for many examples.
> 
> dev_err(&dev->dev, "IRQ index %u not found: %d\n", num, ret);

Sure! I'll use that one.

> 
> would be better - note also the use of %u for unsigned integers.
> Using %d for them is IMHO sloppy coding.
> 

Ok.
diff mbox series

Patch

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4..75ceda9f452a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -79,23 +79,18 @@  struct resource *platform_get_resource(struct platform_device *dev,
 }
 EXPORT_SYMBOL_GPL(platform_get_resource);
 
-/**
- * platform_get_irq - get an IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- */
-int platform_get_irq(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn)
 {
+	int ret = -ENXIO;
+
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
-		return -ENXIO;
+		goto error;
 	return dev->archdata.irqs[num];
 #else
 	struct resource *r;
 	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
-		int ret;
-
 		ret = of_irq_get(dev->dev.of_node, num);
 		if (ret > 0 || ret == -EPROBE_DEFER)
 			return ret;
@@ -104,11 +99,11 @@  int platform_get_irq(struct platform_device *dev, unsigned int num)
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
 	if (has_acpi_companion(&dev->dev)) {
 		if (r && r->flags & IORESOURCE_DISABLED) {
-			int ret;
-
 			ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
-			if (ret)
+			if (ret > 0 || ret == -EPROBE_DEFER)
 				return ret;
+			if (ret)
+				goto error;
 		}
 	}
 
@@ -122,13 +117,32 @@  int platform_get_irq(struct platform_device *dev, unsigned int num)
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
-		if (!irqd)
-			return -ENXIO;
+		if (!irqd) {
+			ret = -ENXIO;
+			goto error;
+		}
+
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
 #endif
+error:
+	if (warn)
+		dev_err(&dev->dev, "IRQ%d not found\n", num);
+
+	return ret;
+}
+
+/**
+ * platform_get_irq - get an IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ */
+int platform_get_irq(struct platform_device *dev, unsigned int num)
+{
+	return __platform_get_irq(dev, num, true);
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
 
@@ -142,7 +156,7 @@  int platform_irq_count(struct platform_device *dev)
 {
 	int ret, nr = 0;
 
-	while ((ret = platform_get_irq(dev, nr)) >= 0)
+	while ((ret = __platform_get_irq(dev, nr, false)) >= 0)
 		nr++;
 
 	if (ret == -EPROBE_DEFER)
@@ -195,7 +209,11 @@  int platform_get_irq_byname(struct platform_device *dev, const char *name)
 	}
 
 	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	dev_err(&dev->dev, "IRQ %s not found\n", name);
+	return -ENXIO;
 }
 EXPORT_SYMBOL_GPL(platform_get_irq_byname);