| Message ID | 1478700308-25481-1-git-send-email-gpiccoli@linux.vnet.ibm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09/2016 04:05 PM, Rob Herring wrote: > 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. 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 linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)