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 +, Mark Rutland wrote:
>> On Fri, Nov 11, 2016 at 08:30:43AM +1100, Benjamin Herrenschmidt
>> wrote:
>>>
>>> On Wed, 2016-11-09 at 19:04 +, 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

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

[Benjamin Herrenschmidt]

On Fri, 2016-11-11 at 16:32 +, Mark Rutland wrote:
> On Fri, Nov 11, 2016 at 08:30:43AM +1100, Benjamin Herrenschmidt
> wrote:
> > 
> > On Wed, 2016-11-09 at 19:04 +, 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

[Mark Rutland]

On Fri, Nov 11, 2016 at 08:30:43AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2016-11-09 at 19:04 +, 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 Wed, 2016-11-09 at 19:04 +, 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

[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

[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.

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
>  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 
>> ---
>>  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
> 

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
 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 
> ---
>  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