[PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Alexey Kardashevskiy]

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

Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Bjorn Helgaas]

[+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
>

Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Rob Herring]

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>

Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Bjorn Helgaas]

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

Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Benjamin Herrenschmidt]

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.

Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Bjorn Helgaas]

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.

Re: [PATCH kernel] powerpc/pci: Fix broken INTx configuration via OF

[Benjamin Herrenschmidt]

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.

Re: [kernel] powerpc/pci: Fix broken INTx configuration via OF

[Michael Ellerman]

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

Re: [kernel] powerpc/pci: Fix broken INTx configuration via OF

[Alexey Kardashevskiy]

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

Re: [kernel] powerpc/pci: Fix broken INTx configuration via OF

[Michael Ellerman]

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