| Message ID | 20180208053354.41725-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Commit | 560c0f978155282e36f33d21b2aeae28afb12df7 |
| Headers | show |
| Series | [kernel] powerpc/pci: Fix broken INTx configuration via OF | expand |
[+cc linux-pci] The original commit was merged via PCI, and I think it's a good idea to merge fixes to it the same way. I'll try to merge this in time for v4.16-rc1. On Wed, Feb 7, 2018 at 11:33 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") > correctly states that of_irq_parse_and_map_pci() does the same thing as > of_irq_parse_pci() does as it simply calls > of_irq_parse_pci() and irq_create_of_mapping(). > > However of_irq_parse_and_map_pci() not only returns 0 for success and > negative value for an error but also a positive virq value from > irq_create_of_mapping() which the mentioned commit ignores and > INTx config fails. > > This fixes the of_irq_parse_and_map_pci() return value handling. > > Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > Found it on POWER9 + powernv system - almost all devices suddenly lost > INTx support. > --- > arch/powerpc/kernel/pci-common.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index ae2ede4..acbb44f2 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -370,7 +370,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev) > memset(&oirq, 0xff, sizeof(oirq)); > #endif > /* Try to get a mapping from the device-tree */ > - if (!of_irq_parse_and_map_pci(pci_dev, 0, 0)) { > + virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); > + if (virq <= 0) { > u8 line, pin; > > /* If that fails, lets fallback to what is in the config > -- > 2.11.0 >
On Wed, Feb 7, 2018 at 11:33 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") > correctly states that of_irq_parse_and_map_pci() does the same thing as > of_irq_parse_pci() does as it simply calls > of_irq_parse_pci() and irq_create_of_mapping(). > > However of_irq_parse_and_map_pci() not only returns 0 for success and > negative value for an error but also a positive virq value from > irq_create_of_mapping() which the mentioned commit ignores and > INTx config fails. > > This fixes the of_irq_parse_and_map_pci() return value handling. > > Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > Found it on POWER9 + powernv system - almost all devices suddenly lost > INTx support. > --- > arch/powerpc/kernel/pci-common.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Thanks. Acked-by: Rob Herring <robh@kernel.org>
On Thu, Feb 08, 2018 at 01:20:04PM -0600, Bjorn Helgaas wrote: > [+cc linux-pci] > > The original commit was merged via PCI, and I think it's a good idea > to merge fixes to it the same way. I'll try to merge this in time for > v4.16-rc1. > > On Wed, Feb 7, 2018 at 11:33 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") > > correctly states that of_irq_parse_and_map_pci() does the same thing as > > of_irq_parse_pci() does as it simply calls > > of_irq_parse_pci() and irq_create_of_mapping(). > > > > However of_irq_parse_and_map_pci() not only returns 0 for success and > > negative value for an error but also a positive virq value from > > irq_create_of_mapping() which the mentioned commit ignores and > > INTx config fails. > > > > This fixes the of_irq_parse_and_map_pci() return value handling. > > > > Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > --- > > > > Found it on POWER9 + powernv system - almost all devices suddenly lost > > INTx support. > > --- > > arch/powerpc/kernel/pci-common.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > > index ae2ede4..acbb44f2 100644 > > --- a/arch/powerpc/kernel/pci-common.c > > +++ b/arch/powerpc/kernel/pci-common.c > > @@ -370,7 +370,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev) > > memset(&oirq, 0xff, sizeof(oirq)); > > #endif > > /* Try to get a mapping from the device-tree */ > > - if (!of_irq_parse_and_map_pci(pci_dev, 0, 0)) { > > + virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); > > + if (virq <= 0) { I don't understand how this fix works. We used to check the result of of_irq_parse_and_map_pci() and entered the block if it was zero. Now you enter the block if it is zero or less than zero, but: static int pci_read_irq_line(...) { unsigned int virq = 0; /* unnecessarily initialized, BTW */ virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); if (virq <= 0) { ... virq is unsigned, so "virq < 0" can never be true. So how does this change anything? Bjorn
On Thu, 2018-02-08 at 15:39 -0600, Bjorn Helgaas wrote: > I don't understand how this fix works. We used to check the result of > of_irq_parse_and_map_pci() and entered the block if it was zero. > > Now you enter the block if it is zero or less than zero, but: > > static int pci_read_irq_line(...) > { > unsigned int virq = 0; /* unnecessarily initialized, BTW */ > > virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); > if (virq <= 0) { > ... > > virq is unsigned, so "virq < 0" can never be true. So how does this > change anything? Yes it does: So the unsigned thing is a second bug in the original patch that Alexey isn't fixing, we need to fix it too. However, the actual bug Alexey is fixing is that we lost the actual value of virq. IE, without his fix, we test it for 0 but we don't actually return it if it's positive. So he fixes the normal case but there's still a bug in the error case, we need to make virq signed. Cheers, Ben.
On Fri, Feb 09, 2018 at 09:21:43AM +1100, Benjamin Herrenschmidt wrote: > On Thu, 2018-02-08 at 15:39 -0600, Bjorn Helgaas wrote: > > I don't understand how this fix works. We used to check the result of > > of_irq_parse_and_map_pci() and entered the block if it was zero. > > > > Now you enter the block if it is zero or less than zero, but: > > > > static int pci_read_irq_line(...) > > { > > unsigned int virq = 0; /* unnecessarily initialized, BTW */ > > > > virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); > > if (virq <= 0) { > > ... > > > > virq is unsigned, so "virq < 0" can never be true. So how does this > > change anything? > > Yes it does: > > So the unsigned thing is a second bug in the original patch that Alexey > isn't fixing, we need to fix it too. > > However, the actual bug Alexey is fixing is that we lost the actual > value of virq. IE, without his fix, we test it for 0 but we don't > actually return it if it's positive. Ah, I see, the bug is that we discarded the non-zero virq value when we actually need it. I'm going to wait for a new patch with a changelog that says that and doesn't test an unsigned value for < 0. > So he fixes the normal case but there's still a bug in the error case, > we need to make virq signed. I looked through the of_irq_parse_and_map_pci() path and I do not see a case where it can return a negative value. It either returns zero or one of these: virq = irq_find_mapping(...) virq = irq_create_mapping(...) Both of these functions return unsigned values.
On Thu, 2018-02-08 at 16:42 -0600, Bjorn Helgaas wrote: > On Fri, Feb 09, 2018 at 09:21:43AM +1100, Benjamin Herrenschmidt wrote: > > On Thu, 2018-02-08 at 15:39 -0600, Bjorn Helgaas wrote: > > > I don't understand how this fix works. We used to check the result of > > > of_irq_parse_and_map_pci() and entered the block if it was zero. > > > > > > Now you enter the block if it is zero or less than zero, but: > > > > > > static int pci_read_irq_line(...) > > > { > > > unsigned int virq = 0; /* unnecessarily initialized, BTW */ > > > > > > virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); > > > if (virq <= 0) { > > > ... > > > > > > virq is unsigned, so "virq < 0" can never be true. So how does this > > > change anything? > > > > Yes it does: > > > > So the unsigned thing is a second bug in the original patch that Alexey > > isn't fixing, we need to fix it too. > > > > However, the actual bug Alexey is fixing is that we lost the actual > > value of virq. IE, without his fix, we test it for 0 but we don't > > actually return it if it's positive. > > Ah, I see, the bug is that we discarded the non-zero virq value when > we actually need it. I'm going to wait for a new patch with a > changelog that says that and doesn't test an unsigned value for < 0. > > > So he fixes the normal case but there's still a bug in the error case, > > we need to make virq signed. > > I looked through the of_irq_parse_and_map_pci() path and I do not see > a case where it can return a negative value. It either returns zero > or one of these: > > virq = irq_find_mapping(...) > virq = irq_create_mapping(...) > > Both of these functions return unsigned values. Ok so the test is just wrong then. Aleey, can you respin ? Cheers, Ben.
On Thu, 2018-02-08 at 05:33:54 UTC, Alexey Kardashevskiy wrote: > Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") > correctly states that of_irq_parse_and_map_pci() does the same thing as > of_irq_parse_pci() does as it simply calls > of_irq_parse_pci() and irq_create_of_mapping(). > > However of_irq_parse_and_map_pci() not only returns 0 for success and > negative value for an error but also a positive virq value from > irq_create_of_mapping() which the mentioned commit ignores and > INTx config fails. > > This fixes the of_irq_parse_and_map_pci() return value handling. > > Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/560c0f978155282e36f33d21b2aeae cheers
On 09/02/18 15:00, Michael Ellerman wrote: > On Thu, 2018-02-08 at 05:33:54 UTC, Alexey Kardashevskiy wrote: >> Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") >> correctly states that of_irq_parse_and_map_pci() does the same thing as >> of_irq_parse_pci() does as it simply calls >> of_irq_parse_pci() and irq_create_of_mapping(). >> >> However of_irq_parse_and_map_pci() not only returns 0 for success and >> negative value for an error but also a positive virq value from >> irq_create_of_mapping() which the mentioned commit ignores and >> INTx config fails. >> >> This fixes the of_irq_parse_and_map_pci() return value handling. >> >> Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Applied to powerpc fixes, thanks. > > https://git.kernel.org/powerpc/c/560c0f978155282e36f33d21b2aeae there is a v2, this one is not 100% correct
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 09/02/18 15:00, Michael Ellerman wrote: >> On Thu, 2018-02-08 at 05:33:54 UTC, Alexey Kardashevskiy wrote: >>> Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") >>> correctly states that of_irq_parse_and_map_pci() does the same thing as >>> of_irq_parse_pci() does as it simply calls >>> of_irq_parse_pci() and irq_create_of_mapping(). >>> >>> However of_irq_parse_and_map_pci() not only returns 0 for success and >>> negative value for an error but also a positive virq value from >>> irq_create_of_mapping() which the mentioned commit ignores and >>> INTx config fails. >>> >>> This fixes the of_irq_parse_and_map_pci() return value handling. >>> >>> Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> Applied to powerpc fixes, thanks. >> >> https://git.kernel.org/powerpc/c/560c0f978155282e36f33d21b2aeae > > > there is a v2, this one is not 100% correct Yeah I just saw, I'll back this out. cheers
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index ae2ede4..acbb44f2 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -370,7 +370,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev) memset(&oirq, 0xff, sizeof(oirq)); #endif /* Try to get a mapping from the device-tree */ - if (!of_irq_parse_and_map_pci(pci_dev, 0, 0)) { + virq = of_irq_parse_and_map_pci(pci_dev, 0, 0); + if (virq <= 0) { u8 line, pin; /* If that fails, lets fallback to what is in the config
Commit 59f47eff03a0 ("powerpc/pci: Use of_irq_parse_and_map_pci() helper") correctly states that of_irq_parse_and_map_pci() does the same thing as of_irq_parse_pci() does as it simply calls of_irq_parse_pci() and irq_create_of_mapping(). However of_irq_parse_and_map_pci() not only returns 0 for success and negative value for an error but also a positive virq value from irq_create_of_mapping() which the mentioned commit ignores and INTx config fails. This fixes the of_irq_parse_and_map_pci() return value handling. Fixes: 59f47eff03a0 "powerpc/pci: Use of_irq_parse_and_map_pci() helper" Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Found it on POWER9 + powernv system - almost all devices suddenly lost INTx support. --- arch/powerpc/kernel/pci-common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)