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 <redacted>
---
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(-)
@@ -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){u8line,pin;/* If that fails, lets fallback to what is in the config
[+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 [off-list ref] wrote:
quoted hunk
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 <redacted>
---
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(-)
@@ -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){u8line,pin;/* If that fails, lets fallback to what is in the config--
From: Rob Herring <robh@kernel.org> Date: 2018-02-08 19:47:05
On Wed, Feb 7, 2018 at 11:33 PM, Alexey Kardashevskiy [off-list ref] 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 <redacted>
---
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(-)
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 [off-list ref] wrote:
quoted
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 <redacted>
---
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(-)
@@ -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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-02-08 22:22:01
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:
quoted
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.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-02-08 22:51:03
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:
quoted
On Thu, 2018-02-08 at 15:39 -0600, Bjorn Helgaas wrote:
quoted
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.
quoted
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.
From: Michael Ellerman <hidden> Date: 2018-02-09 04:00:37
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 <redacted>
On Thu, 2018-02-08 at 05:33:54 UTC, Alexey Kardashevskiy wrote:
quoted
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 <redacted>
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-02-09 05:34:08
Alexey Kardashevskiy [off-list ref] writes:
On 09/02/18 15:00, Michael Ellerman wrote:
quoted
On Thu, 2018-02-08 at 05:33:54 UTC, Alexey Kardashevskiy wrote:
quoted
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 <redacted>