| Message ID | 1480946356-16778-1-git-send-email-gpiccoli@linux.vnet.ibm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Dec 5, 2016 at 7:59 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), > 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 device's node in the device > tree. > > This patch introduces a different message for this specific case, > and also reduces its level from error to warning. Besides, we warn > (once) that possibly some PCI slots on the system have no level > triggered interrupts available. > We changed some error return codes too on function of_irq_parse_raw() > in order other failure's cases can be presented in a more precise way. > > Before this patch, when an adapter was plugged in a slot without level > interrupts capabilitiy on PowerPC, we saw a generic error message > 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: > > [16.154] pci 0014:60:00.1: of_irq_parse_pci: no interrupt-map found, > INTx interrupts not available > > Finally, we standardize the error path in of_irq_parse_raw() by always > taking the fail path instead of returning directly from the loop. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- > > v2: > * Changed function return code to always return negative values; Are you sure this is safe? This is tricky because of differing values of NO_IRQ (0 or -1). > * Improved/simplified warning outputs; > * Changed some return codes and some error paths in of_irq_parse_raw() > in order to be more precise/consistent; This too could have some side effects on callers. Not saying don't do these changes, just need some assurances this has been considered. 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 12/05/2016 12:28 PM, Rob Herring wrote: > On Mon, Dec 5, 2016 at 7:59 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), >> 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 device's node in the device >> tree. >> >> This patch introduces a different message for this specific case, >> and also reduces its level from error to warning. Besides, we warn >> (once) that possibly some PCI slots on the system have no level >> triggered interrupts available. >> We changed some error return codes too on function of_irq_parse_raw() >> in order other failure's cases can be presented in a more precise way. >> >> Before this patch, when an adapter was plugged in a slot without level >> interrupts capabilitiy on PowerPC, we saw a generic error message >> 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: >> >> [16.154] pci 0014:60:00.1: of_irq_parse_pci: no interrupt-map found, >> INTx interrupts not available >> >> Finally, we standardize the error path in of_irq_parse_raw() by always >> taking the fail path instead of returning directly from the loop. >> >> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> >> --- >> >> v2: >> * Changed function return code to always return negative values; > > Are you sure this is safe? This is tricky because of differing values > of NO_IRQ (0 or -1). Thanks Rob, but this is purely bad wording from myself. I'm sorry - I meant to say that I changed only my positive return code (that was suggested to be removed in the prior revision) to negative return code! So, I changed only code I added myself in v1 =) > >> * Improved/simplified warning outputs; >> * Changed some return codes and some error paths in of_irq_parse_raw() >> in order to be more precise/consistent; > > This too could have some side effects on callers. > > Not saying don't do these changes, just need some assurances this has > been considered. Thanks for your attention. I performed a quick investigation before changing this, all the places that use the return values are just getting "true/false" information from that, meaning they just are comparing to 0 basically. So change -EINVAL to -ENOENT wouldn't hurt any user of these return values, it'll only become more informative IMHO. Now, regarding the only error path that was changed: for some reason, this was the only place in which we didn't goto fail label in case of failure - it was added by a legacy commit from Ben, dated from 2006: 006b64de60 ("[POWERPC] Make OF irq map code detect more error cases"). Then it was carried by Grant Likely's commit 7dc2e1134a ("of/irq: merge irq mapping code"), 6-year old commit. I wasn't able to imagine a scenario in which changing this would break something; I believe the change improve consistency, but I'd remove it if you or somebody else thinks it worth be removed. 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 Mon, Dec 5, 2016 at 1:01 PM, Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> wrote: > On 12/05/2016 12:28 PM, Rob Herring wrote: >> On Mon, Dec 5, 2016 at 7:59 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), >>> 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 device's node in the device >>> tree. >>> >>> This patch introduces a different message for this specific case, >>> and also reduces its level from error to warning. Besides, we warn >>> (once) that possibly some PCI slots on the system have no level >>> triggered interrupts available. >>> We changed some error return codes too on function of_irq_parse_raw() >>> in order other failure's cases can be presented in a more precise way. >>> >>> Before this patch, when an adapter was plugged in a slot without level >>> interrupts capabilitiy on PowerPC, we saw a generic error message >>> 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: >>> >>> [16.154] pci 0014:60:00.1: of_irq_parse_pci: no interrupt-map found, >>> INTx interrupts not available >>> >>> Finally, we standardize the error path in of_irq_parse_raw() by always >>> taking the fail path instead of returning directly from the loop. >>> >>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> >>> --- >>> >>> v2: >>> * Changed function return code to always return negative values; >> >> Are you sure this is safe? This is tricky because of differing values >> of NO_IRQ (0 or -1). > > Thanks Rob, but this is purely bad wording from myself. I'm sorry - I > meant to say that I changed only my positive return code (that was > suggested to be removed in the prior revision) to negative return code! > > So, I changed only code I added myself in v1 =) > > >> >>> * Improved/simplified warning outputs; >>> * Changed some return codes and some error paths in of_irq_parse_raw() >>> in order to be more precise/consistent; >> >> This too could have some side effects on callers. >> >> Not saying don't do these changes, just need some assurances this has >> been considered. > > Thanks for your attention. I performed a quick investigation before > changing this, all the places that use the return values are just > getting "true/false" information from that, meaning they just are > comparing to 0 basically. So change -EINVAL to -ENOENT wouldn't hurt any > user of these return values, it'll only become more informative IMHO. > > Now, regarding the only error path that was changed: for some reason, > this was the only place in which we didn't goto fail label in case of > failure - it was added by a legacy commit from Ben, dated from 2006: > 006b64de60 ("[POWERPC] Make OF irq map code detect more error cases"). > Then it was carried by Grant Likely's commit 7dc2e1134a ("of/irq: merge > irq mapping code"), 6-year old commit. > I wasn't able to imagine a scenario in which changing this would break > something; I believe the change improve consistency, but I'd remove it > if you or somebody else thinks it worth be removed. Okay. It's a bit late for 4.10 now and want this to be in -next for a while, so I'll queue it after the merge window. 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 12/09/2016 02:25 PM, Rob Herring wrote: > On Mon, Dec 5, 2016 at 1:01 PM, Guilherme G. Piccoli > <gpiccoli@linux.vnet.ibm.com> wrote: >> On 12/05/2016 12:28 PM, Rob Herring wrote: >>> On Mon, Dec 5, 2016 at 7:59 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), >>>> 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 device's node in the device >>>> tree. >>>> >>>> This patch introduces a different message for this specific case, >>>> and also reduces its level from error to warning. Besides, we warn >>>> (once) that possibly some PCI slots on the system have no level >>>> triggered interrupts available. >>>> We changed some error return codes too on function of_irq_parse_raw() >>>> in order other failure's cases can be presented in a more precise way. >>>> >>>> Before this patch, when an adapter was plugged in a slot without level >>>> interrupts capabilitiy on PowerPC, we saw a generic error message >>>> 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: >>>> >>>> [16.154] pci 0014:60:00.1: of_irq_parse_pci: no interrupt-map found, >>>> INTx interrupts not available >>>> >>>> Finally, we standardize the error path in of_irq_parse_raw() by always >>>> taking the fail path instead of returning directly from the loop. >>>> >>>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> >>>> --- >>>> >>>> v2: >>>> * Changed function return code to always return negative values; >>> >>> Are you sure this is safe? This is tricky because of differing values >>> of NO_IRQ (0 or -1). >> >> Thanks Rob, but this is purely bad wording from myself. I'm sorry - I >> meant to say that I changed only my positive return code (that was >> suggested to be removed in the prior revision) to negative return code! >> >> So, I changed only code I added myself in v1 =) >> >> >>> >>>> * Improved/simplified warning outputs; >>>> * Changed some return codes and some error paths in of_irq_parse_raw() >>>> in order to be more precise/consistent; >>> >>> This too could have some side effects on callers. >>> >>> Not saying don't do these changes, just need some assurances this has >>> been considered. >> >> Thanks for your attention. I performed a quick investigation before >> changing this, all the places that use the return values are just >> getting "true/false" information from that, meaning they just are >> comparing to 0 basically. So change -EINVAL to -ENOENT wouldn't hurt any >> user of these return values, it'll only become more informative IMHO. >> >> Now, regarding the only error path that was changed: for some reason, >> this was the only place in which we didn't goto fail label in case of >> failure - it was added by a legacy commit from Ben, dated from 2006: >> 006b64de60 ("[POWERPC] Make OF irq map code detect more error cases"). >> Then it was carried by Grant Likely's commit 7dc2e1134a ("of/irq: merge >> irq mapping code"), 6-year old commit. >> I wasn't able to imagine a scenario in which changing this would break >> something; I believe the change improve consistency, but I'd remove it >> if you or somebody else thinks it worth be removed. > > Okay. It's a bit late for 4.10 now and want this to be in -next for a > while, so I'll queue it after the merge window. > OK, perfect! Thanks Rob 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
diff --git a/drivers/of/irq.c b/drivers/of/irq.c index 393fea8..9deee86 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -104,7 +104,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) const __be32 *match_array = initial_match_array; const __be32 *tmp, *imap, *imask, dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 }; u32 intsize = 1, addrsize, newintsize = 0, newaddrsize = 0; - int imaplen, match, i; + int imaplen, match, i, rc = -EINVAL; #ifdef DEBUG of_print_phandle_args("of_irq_parse_raw: ", out_irq); @@ -134,7 +134,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) pr_debug("of_irq_parse_raw: ipar=%s, size=%d\n", of_node_full_name(ipar), intsize); if (out_irq->args_count != intsize) - return -EINVAL; + goto fail; /* Look for this #address-cells. We have to implement the old linux * trick of looking for the parent here as some device-trees rely on it @@ -153,8 +153,10 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) pr_debug(" -> addrsize=%d\n", addrsize); /* Range check so that the temporary buffer doesn't overflow */ - if (WARN_ON(addrsize + intsize > MAX_PHANDLE_ARGS)) + if (WARN_ON(addrsize + intsize > MAX_PHANDLE_ARGS)) { + rc = -EFAULT; goto fail; + } /* Precalculate the match array - this simplifies match loop */ for (i = 0; i < addrsize; i++) @@ -240,10 +242,11 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) newintsize, newaddrsize); /* Check for malformed properties */ - if (WARN_ON(newaddrsize + newintsize > MAX_PHANDLE_ARGS)) - goto fail; - if (imaplen < (newaddrsize + newintsize)) + if (WARN_ON(newaddrsize + newintsize > MAX_PHANDLE_ARGS) + || (imaplen < (newaddrsize + newintsize))) { + rc = -EFAULT; goto fail; + } imap += newaddrsize + newintsize; imaplen -= newaddrsize + newintsize; @@ -271,11 +274,13 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) ipar = newpar; newpar = NULL; } + rc = -ENOENT; /* No interrupt-map found */ + fail: of_node_put(ipar); of_node_put(newpar); - return -EINVAL; + return rc; } 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..c175d9c 100644 --- a/drivers/of/of_pci_irq.c +++ b/drivers/of/of_pci_irq.c @@ -93,7 +93,15 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq goto err; return 0; err: - dev_err(&pdev->dev, "of_irq_parse_pci() failed with rc=%d\n", rc); + if (rc == -ENOENT) { + dev_warn(&pdev->dev, + "%s: no interrupt-map found, INTx interrupts not available\n", + __func__); + pr_warn_once("%s: possibly some PCI slots don't have level triggered interrupts capability\n", + __func__); + } else { + dev_err(&pdev->dev, "%s: failed with rc=%d\n", __func__, rc); + } return rc; } EXPORT_SYMBOL_GPL(of_irq_parse_pci);
On PowerPC machines some PCI slots might not have level triggered interrupts capability (also know as level signaled interrupts), 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 device's node in the device tree. This patch introduces a different message for this specific case, and also reduces its level from error to warning. Besides, we warn (once) that possibly some PCI slots on the system have no level triggered interrupts available. We changed some error return codes too on function of_irq_parse_raw() in order other failure's cases can be presented in a more precise way. Before this patch, when an adapter was plugged in a slot without level interrupts capabilitiy on PowerPC, we saw a generic error message 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: [16.154] pci 0014:60:00.1: of_irq_parse_pci: no interrupt-map found, INTx interrupts not available Finally, we standardize the error path in of_irq_parse_raw() by always taking the fail path instead of returning directly from the loop. Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- v2: * Changed function return code to always return negative values; * Improved/simplified warning outputs; * Changed some return codes and some error paths in of_irq_parse_raw() in order to be more precise/consistent; drivers/of/irq.c | 19 ++++++++++++------- drivers/of/of_pci_irq.c | 10 +++++++++- 2 files changed, 21 insertions(+), 8 deletions(-)