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

On 11/09/2016 04:05 PM, Rob Herring wrote:

On Wed, Nov 9, 2016 at 8:05 AM, Guilherme G. Piccoli
[off-list ref] wrote:

quoted

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