[PATCH] of/irq: improve error message on irq discovery process failure

[PATCH] of/irq: improve error message on irq discovery process failure

[Guilherme G. Piccoli]

On PowerPC machines some PCI slots might not have Level-triggered
interrupts capability (also know as Level Signaled Interrupts - LSI),
leading of_irq_parse_pci() to complain by presenting error messages
on the kernel log - in this case, the properties "interrupt-map" and
"interrupt-map-mask" are not present on the device's node on device
tree.

This patch introduces a different message for this specific case,
and it also reduces the level of the message from error to warning.
Before this patch, when an adapter was plugged in a slot without Level
interrupts capabilities, we saw generic error messages like this:

    [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-22

Now, with this applied, we see the following specific message:

    [19.947] pci 0014:60:00.0: of_irq_parse_pci() gave up. The slot of this
    device has no Level-triggered Interrupts capability.

No functional changes were introduced.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
---
 drivers/of/irq.c        | 5 ++++-
 drivers/of/of_pci_irq.c | 8 +++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 393fea8..1ad6882 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -275,7 +275,10 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 	of_node_put(ipar);
 	of_node_put(newpar);
 
-	return -EINVAL;
+	/* Positive non-zero return means no Level-triggered Interrupts
+	 * capability was found.
+	 */
+	return ENOENT;
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_raw);
 
diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 2306313..9b6f387 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -89,8 +89,14 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
 	laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
 	laddr[1] = laddr[2] = cpu_to_be32(0);
 	rc = of_irq_parse_raw(laddr, out_irq);
-	if (rc)
+
+	if (rc < 0) {
 		goto err;
+	} else if (rc > 0) {
+		dev_warn(&pdev->dev,
+			"of_irq_parse_pci() gave up. The slot of this device has no Level-triggered Interrupts capability.\n");
+		return -rc;
+	}
 	return 0;
 err:
 	dev_err(&pdev->dev, "of_irq_parse_pci() failed with rc=%d\n", rc);
-- 
2.1.0

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Rob Herring]

On Wed, Nov 9, 2016 at 8:05 AM, Guilherme G. Piccoli
<gpiccoli@linux.vnet.ibm.com> wrote:
> On PowerPC machines some PCI slots might not have Level-triggered
> interrupts capability (also know as Level Signaled Interrupts - LSI),
> leading of_irq_parse_pci() to complain by presenting error messages
> on the kernel log - in this case, the properties "interrupt-map" and
> "interrupt-map-mask" are not present on the device's node on device
> tree.
>
> This patch introduces a different message for this specific case,
> and it also reduces the level of the message from error to warning.
> Before this patch, when an adapter was plugged in a slot without Level
> interrupts capabilities, we saw generic error messages like this:
>
>     [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-22
>
> Now, with this applied, we see the following specific message:
>
>     [19.947] pci 0014:60:00.0: of_irq_parse_pci() gave up. The slot of this
>     device has no Level-triggered Interrupts capability.
>
> No functional changes were introduced.
>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
> ---
>  drivers/of/irq.c        | 5 ++++-
>  drivers/of/of_pci_irq.c | 8 +++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 393fea8..1ad6882 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -275,7 +275,10 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>         of_node_put(ipar);
>         of_node_put(newpar);
>
> -       return -EINVAL;
> +       /* Positive non-zero return means no Level-triggered Interrupts
> +        * capability was found.
> +        */
> +       return ENOENT;

It's not really a normal pattern to return positive errno values. You
should return a negative value and check for that specific error value
or perhaps move the print statement into this function.

Rob

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Mark Rutland]

On Wed, Nov 09, 2016 at 12:05:08PM -0200, Guilherme G. Piccoli wrote:
> On PowerPC machines some PCI slots might not have Level-triggered
> interrupts capability (also know as Level Signaled Interrupts - LSI),
> leading of_irq_parse_pci() to complain by presenting error messages
> on the kernel log - in this case, the properties "interrupt-map" and
> "interrupt-map-mask" are not present on the device's node on device
> tree.

If we don't have an interrupt-map on a PCI controller, why don't we
instead log a message regarding that being missing, and give up early?

That sounds like a more generically useful error message; it's also
possible that a DT author simply forgot to add the map, and the platform
has suitable interrupts wired up.

> This patch introduces a different message for this specific case,
> and it also reduces the level of the message from error to warning.
> Before this patch, when an adapter was plugged in a slot without Level
> interrupts capabilities, we saw generic error messages like this:
> 
>     [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-22
> 
> Now, with this applied, we see the following specific message:
> 
>     [19.947] pci 0014:60:00.0: of_irq_parse_pci() gave up. The slot of this
>     device has no Level-triggered Interrupts capability.

Following my above example, this has gone from opaque to potentially
misleading.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Guilherme G. Piccoli]

On 11/09/2016 04:05 PM, Rob Herring wrote:
> On Wed, Nov 9, 2016 at 8:05 AM, Guilherme G. Piccoli
> <gpiccoli-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
>> On PowerPC machines some PCI slots might not have Level-triggered
>> interrupts capability (also know as Level Signaled Interrupts - LSI),
>> leading of_irq_parse_pci() to complain by presenting error messages
>> on the kernel log - in this case, the properties "interrupt-map" and
>> "interrupt-map-mask" are not present on the device's node on device
>> tree.
>>
>> This patch introduces a different message for this specific case,
>> and it also reduces the level of the message from error to warning.
>> Before this patch, when an adapter was plugged in a slot without Level
>> interrupts capabilities, we saw generic error messages like this:
>>
>>     [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-22
>>
>> Now, with this applied, we see the following specific message:
>>
>>     [19.947] pci 0014:60:00.0: of_irq_parse_pci() gave up. The slot of this
>>     device has no Level-triggered Interrupts capability.
>>
>> No functional changes were introduced.
>>
>> Signed-off-by: Guilherme G. Piccoli <gpiccoli-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>>  drivers/of/irq.c        | 5 ++++-
>>  drivers/of/of_pci_irq.c | 8 +++++++-
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 393fea8..1ad6882 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -275,7 +275,10 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>>         of_node_put(ipar);
>>         of_node_put(newpar);
>>
>> -       return -EINVAL;
>> +       /* Positive non-zero return means no Level-triggered Interrupts
>> +        * capability was found.
>> +        */
>> +       return ENOENT;
> 
> It's not really a normal pattern to return positive errno values. You
> should return a negative value and check for that specific error value
> or perhaps move the print statement into this function.

Thanks Rob! I thought about it, I had the option to differentiate the
errors through the value or the sign - ended up taking the wrong way it
seems heheh
I'll send a V2 with this change.

Printing the warning inside of_irq_parse_raw() would require more
changes to the logic, it was my first choice but I changed way during
the implementation.

Cheers,


Guilherme


> 
> Rob
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Benjamin Herrenschmidt]

On Wed, 2016-11-09 at 12:05 -0200, Guilherme G. Piccoli wrote:
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 393fea8..1ad6882 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -275,7 +275,10 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>         of_node_put(ipar);
>         of_node_put(newpar);
>  
> -       return -EINVAL;
> +       /* Positive non-zero return means no Level-triggered Interrupts
> +        * capability was found.
> +        */
> +       return ENOENT;
>  }
>  EXPORT_SYMBOL_GPL(of_irq_parse_raw);

I'm not fan. I'd rather it's -ENOENT and the callers can check for that
specific code rather than playing with the sign.

Cheers,
Ben.

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Benjamin Herrenschmidt]

On Wed, 2016-11-09 at 19:04 +0000, Mark Rutland wrote:
> 
> 
> If we don't have an interrupt-map on a PCI controller, why don't we
> instead log a message regarding that being missing, and give up
> early?

Why ? It's legit to not support LSIs.

> That sounds like a more generically useful error message; it's also
> possible that a DT author simply forgot to add the map, and the
> platform has suitable interrupts wired up.

But it's not necessarily an error...

> > This patch introduces a different message for this specific case,
> > and it also reduces the level of the message from error to warning.
> > Before this patch, when an adapter was plugged in a slot without
> Level
> > interrupts capabilities, we saw generic error messages like this:
> > 
> >     [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-
> 22
> > 
> > Now, with this applied, we see the following specific message:
> > 
> >     [19.947] pci 0014:60:00.0: of_irq_parse_pci() gave up. The slot
> of this
> >     device has no Level-triggered Interrupts capability.
> 
> Following my above example, this has gone from opaque to potentially
> misleading

I'm not sure. At least for some of our platforms this is the correct
message :-) Our Hypervisor doesn't allow LSIs on some slots.

I think it's not that misleading. It's obvious something is wrong with
LSIs, which you can easily figure out from there.

Cheers,
Ben.

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Mark Rutland]

On Fri, Nov 11, 2016 at 08:30:43AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2016-11-09 at 19:04 +0000, Mark Rutland wrote:
> > 
> > If we don't have an interrupt-map on a PCI controller, why don't we
> > instead log a message regarding that being missing, and give up
> > early?
> 
> Why ? It's legit to not support LSIs.

Sure; I had envisioned a message like:

	pr_info("%s: no interrupt-map, INTx interrupts not possible\n",
		pci_controller_name);

... Which tells the user exaclty what we know, and doesn't imply either
an error or the actual absence of HW support.

> > That sounds like a more generically useful error message; it's also
> > possible that a DT author simply forgot to add the map, and the
> > platform has suitable interrupts wired up.
> 
> But it's not necessarily an error...

Sure, "error" was a misnomer.

> > > This patch introduces a different message for this specific case,
> > > and it also reduces the level of the message from error to warning.
> > > Before this patch, when an adapter was plugged in a slot without
> > Level
> > > interrupts capabilities, we saw generic error messages like this:
> > > 
> > >     [54.239] pci 002d:70:00.0: of_irq_parse_pci() failed with rc=-
> > 22
> > > 
> > > Now, with this applied, we see the following specific message:
> > > 
> > >     [19.947] pci 0014:60:00.0: of_irq_parse_pci() gave up. The slot
> > of this
> > >     device has no Level-triggered Interrupts capability.
> > 
> > Following my above example, this has gone from opaque to potentially
> > misleading
> 
> I'm not sure. At least for some of our platforms this is the correct
> message :-) Our Hypervisor doesn't allow LSIs on some slots.
> 
> I think it's not that misleading. It's obvious something is wrong with
> LSIs, which you can easily figure out from there.

As above, I think it's clearer to log that there's no interrupt-map for
the controller. 

Orthogonal to that, "INTx" is a more generally understood name for the
legacy wired PCI interrupts, and is probably preferable in generic code.

Thanks,
Mark.

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Benjamin Herrenschmidt]

On Fri, 2016-11-11 at 16:32 +0000, Mark Rutland wrote:
> On Fri, Nov 11, 2016 at 08:30:43AM +1100, Benjamin Herrenschmidt
> wrote:
> > 
> > On Wed, 2016-11-09 at 19:04 +0000, Mark Rutland wrote:
> > > 
> > > 
> > > If we don't have an interrupt-map on a PCI controller, why don't
> > > we
> > > instead log a message regarding that being missing, and give up
> > > early?
> > 
> > Why ? It's legit to not support LSIs.
> 
> Sure; I had envisioned a message like:
> 
> 	pr_info("%s: no interrupt-map, INTx interrupts not possible\n",
> 		pci_controller_name);
> 
> ... Which tells the user exaclty what we know, and doesn't imply
> either
> an error or the actual absence of HW support.

Works for me.

Cheers,
Ben.

Re: [PATCH] of/irq: improve error message on irq discovery process failure

[Guilherme G. Piccoli]

On 11/11/2016 05:12 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2016-11-11 at 16:32 +0000, Mark Rutland wrote:
>> On Fri, Nov 11, 2016 at 08:30:43AM +1100, Benjamin Herrenschmidt
>> wrote:
>>>
>>> On Wed, 2016-11-09 at 19:04 +0000, Mark Rutland wrote:
>>>>
>>>>
>>>> If we don't have an interrupt-map on a PCI controller, why don't
>>>> we
>>>> instead log a message regarding that being missing, and give up
>>>> early?
>>>
>>> Why ? It's legit to not support LSIs.
>>
>> Sure; I had envisioned a message like:
>>
>> 	pr_info("%s: no interrupt-map, INTx interrupts not possible\n",
>> 		pci_controller_name);
>>
>> ... Which tells the user exaclty what we know, and doesn't imply
>> either
>> an error or the actual absence of HW support.
> 
> Works for me.
> 
> Cheers,
> Ben.
> 

Thanks very much Ben and Mark, for the good suggestions you gave me.
I was talking on IRC today with Mark, about showing a message once per
device (and not in all its functions) about LSI being not available in
that slot, besides the simple/small messages per function, saying
interrupt-map wasn't found.

I'm working on V2, but will delay a bit to send - I'm out next 20 days.
When I'm back, I'll send the improved version. Oh, I removed positive
return value - never will use it again, noticed isn't that popular heheh

Thanks,


Guilherme

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html