[PATCH 1/2] powerpc: disable MSI using new interface if possible

[PATCH 1/2] powerpc: disable MSI using new interface if possible

[Nishanth Aravamudan]

On upcoming hardware, we have a PCI adapter with two functions, one of
which uses MSI and the other uses MSI-X. This adapter, when MSI is
disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
signals an MSI-X interrupt and triggers an EEH. We are working with the
vendor to ensure that the hardware is not at fault, but if we use the
"new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
automatically disable MSI-X and the adapter does not appear to signal
any stray MSI-X interrupt.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Cc: Milton Miller <miltonm@bga.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
---

This is effectively a PCI quirk, but I'm not sure how to do a
architecture-specific quirk (looking through the code now). If anyone
has any pointers, I'd appreciate it!

 arch/powerpc/platforms/pseries/msi.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 1164c34..9434576 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
 	if (!pdn)
 		return;
 
-	if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
-		pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+	/*
+	 * disabling MSI with the explicit interface also disables MSI-X
+	 */
+	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
+		/* 
+		 * may have failed due to lacking
+		 * "ibm,change-msix-capable" property
+		 */
+		if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
+			pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+		}
+	}
 }
 
 static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
-- 
1.7.1

[PATCH 2/2] powerpc/msi: clarify call to check_req_msi{,x}

[Nishanth Aravamudan]

The RTAS IRQ fixup code relies on a less-than-clear edge condition for
verifying a given device is not using MSI or MSI-X. Make that more clear
with a comment.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Cc: Milton Miller <miltonm@bga.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>

---
 arch/powerpc/platforms/pseries/msi.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 9434576..8c6dbbc 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -455,7 +455,12 @@ static void rtas_msi_pci_irq_fixup(struct pci_dev *pdev)
 		return;
 	}
 
-	/* No MSI -> MSIs can't have been assigned by fw, leave LSI */
+	/* 
+	 * No MSI -> MSIs can't have been assigned by fw, leave LSI
+	 * The calls below can only provide one of two responses:
+	 *   < 0: if the device doesn't request MSI or MSI-X
+	 *   0: if the device requests at least 1 MSI/MSI-X vector
+	 */
 	if (check_req_msi(pdev, 1) && check_req_msix(pdev, 1)) {
 		dev_dbg(&pdev->dev, "rtas_msi: no req#msi/x, nothing to do.\n");
 		return;
-- 
1.7.1

Re: [PATCH 1/2] powerpc: disable MSI using new interface if possible

[Nishanth Aravamudan]

On 03.03.2011 [11:39:23 -0800], Nishanth Aravamudan wrote:
> On upcoming hardware, we have a PCI adapter with two functions, one of
> which uses MSI and the other uses MSI-X. This adapter, when MSI is
> disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> signals an MSI-X interrupt and triggers an EEH. We are working with the
> vendor to ensure that the hardware is not at fault, but if we use the
> "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> automatically disable MSI-X and the adapter does not appear to signal
> any stray MSI-X interrupt.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>

Sorry, I fudged my git-send-email config, apologies that the Cc's above
weren't actually used in the original!

> ---
> 
> This is effectively a PCI quirk, but I'm not sure how to do a
> architecture-specific quirk (looking through the code now). If anyone
> has any pointers, I'd appreciate it!

If this would be preferred, I think I see now how it would be done and
can work on a different patch that only makes this change for this
device.

Thanks,
Nish

>  arch/powerpc/platforms/pseries/msi.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 1164c34..9434576 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
>  	if (!pdn)
>  		return;
> 
> -	if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
> -		pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> +	/*
> +	 * disabling MSI with the explicit interface also disables MSI-X
> +	 */
> +	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> +		/* 
> +		 * may have failed due to lacking
> +		 * "ibm,change-msix-capable" property
> +		 */
> +		if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
> +			pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> +		}
> +	}
>  }
> 
>  static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
> -- 
> 1.7.1
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [PATCH 1/2] powerpc: disable MSI using new interface if possible

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]

On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> On upcoming hardware, we have a PCI adapter with two functions, one of
> which uses MSI and the other uses MSI-X. This adapter, when MSI is
> disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> signals an MSI-X interrupt and triggers an EEH. We are working with the
> vendor to ensure that the hardware is not at fault, but if we use the
> "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> automatically disable MSI-X and the adapter does not appear to signal
> any stray MSI-X interrupt.

It seems this could also be a firmware bug, have we heard anything from
them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
disable MSI _and_ MSI-X (R1–7.3.10.5.1–1).

> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>

Cc: Me  :)

> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 1164c34..9434576 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
>  	if (!pdn)
>  		return;
>  
> -	if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
> -		pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> +	/*
> +	 * disabling MSI with the explicit interface also disables MSI-X
> +	 */
> +	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> +		/* 
> +		 * may have failed due to lacking
> +		 * "ibm,change-msix-capable" property
> +		 */
> +		if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
> +			pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> +		}
> +	}
>  }

This is probably a pretty safe change anyway, ie. use the newer API if
it is present. The comment is backward though, the call fails because
the new interface is not implemented, and that fact is signalled by the
absence of the property.

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] powerpc: disable MSI using new interface if possible

[Nishanth Aravamudan]

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > On upcoming hardware, we have a PCI adapter with two functions, one of
> > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > vendor to ensure that the hardware is not at fault, but if we use the
> > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > automatically disable MSI-X and the adapter does not appear to signal
> > any stray MSI-X interrupt.
> 
> It seems this could also be a firmware bug, have we heard anything from
> them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).

We're tracking that down too. I think the fact that the interrupt is
coming in is a hardware bug in this particular adapter.

I'm looking at PAPR again and I see what might be a contradiction:

7.3.10.5.1: "To removing all MSIs, set the Requested Number of
Interrupts to zero."

Table 71: "Function ... 1: Request to set to a new number of MSI or
MSI-X (platform choice) interrupts (including set to 0)"

It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
change only MSI or MSI-X and still be not a bug?

It does seem like it should disable both, but from what we've seen with
this adapter, it doesn't appear to.

> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > Cc: Milton Miller <miltonm@bga.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Paul Mackerras <paulus@samba.org>
> 
> Cc: Me  :)

Sorry! I was in a hurry to get this out the door, my fault. Note, you
don't show up per scripts/get_maintainer.pl :)

> > diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> > index 1164c34..9434576 100644
> > --- a/arch/powerpc/platforms/pseries/msi.c
> > +++ b/arch/powerpc/platforms/pseries/msi.c
> > @@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
> >     if (!pdn)
> >             return;
> >  
> > -   if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
> > -           pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> > +   /*
> > +    * disabling MSI with the explicit interface also disables MSI-X
> > +    */
> > +   if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> > +           /* 
> > +            * may have failed due to lacking
> > +            * "ibm,change-msix-capable" property
> > +            */
> > +           if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
> > +                   pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> > +           }
> > +   }
> >  }
> 
> This is probably a pretty safe change anyway, ie. use the newer API if
> it is present. The comment is backward though, the call fails because
> the new interface is not implemented, and that fact is signalled by the
> absence of the property.

Updated the comment, thanks:

On upcoming hardware, we have a PCI adapter with two functions, one of
which uses MSI and the other uses MSI-X. This adapter, when MSI is
disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
signals an MSI-X interrupt and triggers an EEH. We are working with the
vendor to ensure that the hardware is not at fault, but if we use the
"new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
automatically disable MSI-X and the adapter does not appear to signal
any stray MSI-X interrupt.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
---

 arch/powerpc/platforms/pseries/msi.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 1164c34..18ac801 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
 	if (!pdn)
 		return;
 
-	if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
-		pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+	/*
+	 * disabling MSI with the explicit interface also disables MSI-X
+	 */
+	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
+		/* 
+		 * may have failed because explicit interface is not
+		 * present
+		 */
+		if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
+			pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+		}
+	}
 }
 
 static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
-- 
1.7.1

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > On upcoming hardware, we have a PCI adapter with two functions, one of
> > > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > > vendor to ensure that the hardware is not at fault, but if we use the
> > > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > > automatically disable MSI-X and the adapter does not appear to signal
> > > any stray MSI-X interrupt.
> > 
> > It seems this could also be a firmware bug, have we heard anything from
> > them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> > disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).
> 
> We're tracking that down too. I think the fact that the interrupt is
> coming in is a hardware bug in this particular adapter.
> 
> I'm looking at PAPR again and I see what might be a contradiction:
> 
> 7.3.10.5.1: "To removing all MSIs, set the Requested Number of
> Interrupts to zero."
> 
> Table 71: "Function ... 1: Request to set to a new number of MSI or
> MSI-X (platform choice) interrupts (including set to 0)"
> 
> It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
> change only MSI or MSI-X and still be not a bug?

Yeah I guess you could read it that way, though I think that would be a
bug.

The idea is that it chooses for you whether it uses MSI or MSI-X. So the
only sane semantic is that when deconfiguring it deconfigures either,
ie. both, kinds.

Looking closer at your patch, now I don't understand :)

+       /*
+        * disabling MSI with the explicit interface also disables MSI-X
+        */
+       if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {


So we first disable using function 3, which should:

        3: Request to set to a new number of MSI interrupts (including set to 0)

Which does not mention MSI-X at all, implying it has no effect on them.
Which contradicts what you see, and the comment in the code?

So I think I'm not sure what's going on here :)

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > On upcoming hardware, we have a PCI adapter with two functions, one of
..
> > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > > Cc: Milton Miller <miltonm@bga.com>
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Paul Mackerras <paulus@samba.org>
> > 
> > Cc: Me  :)
> 
> Sorry! I was in a hurry to get this out the door, my fault. Note, you
> don't show up per scripts/get_maintainer.pl :)

No worries, though I will remember never to use get_maintainer.pl, it is
obviously utterly broken:

$ git log --pretty=format:"%an %ae" arch/powerpc/platforms/pseries/msi.c
Andre Detsch adetsch@br.ibm.com
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au

And the patch from Andre was also signed off by me.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Joe Perches]

On Fri, 2011-03-04 at 14:06 +1100, Michael Ellerman wrote:
> On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> > On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > > Cc: Me  :)
> > Sorry! I was in a hurry to get this out the door, my fault. Note, you
> > don't show up per scripts/get_maintainer.pl :)
> No worries, though I will remember never to use get_maintainer.pl, it is
> obviously utterly broken:

Blah, blah, stupid tool doesn't work exactly as I want,
and it doesn't credit me for my over 2 year old patches,
therefore it's not only stupid, it's broken...

$ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...)
Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...)
Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
linux-kernel@vger.kernel.org (open list)
devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)

If you don't like how it currently works, suggest
improvements.

> $ git log --pretty=format:"%an %ae" arch/powerpc/platforms/pseries/msi.c
> Andre Detsch adetsch@br.ibm.com
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> 
> And the patch from Andre was also signed off by me.

How nice for you.  Last patch from you was
2 years ago.

$ git log --format="%an %ae %ad" arch/powerpc/platforms/pseries/msi.c
Andre Detsch adetsch@br.ibm.com Wed Nov 4 13:03:19 2009 -0200
Michael Ellerman michael@ellerman.id.au Thu Mar 5 14:44:26 2009 +0000
Michael Ellerman michael@ellerman.id.au Tue Feb 17 00:21:56 2009 +0000
Michael Ellerman michael@ellerman.id.au Tue Feb 17 00:18:49 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:33 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:32 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:31 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:31 2009 +0000
Michael Ellerman michael@ellerman.id.au Tue Oct 23 14:23:44 2007 +1000
Michael Ellerman michael@ellerman.id.au Thu Sep 20 16:36:50 2007 +1000
Michael Ellerman michael@ellerman.id.au Thu Sep 20 16:36:48 2007 +1000
Michael Ellerman michael@ellerman.id.au Tue May 8 12:58:35 2007 +1000

CC'ing inactive non named maintainers via git
history also draws complaints btw.

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Nishanth Aravamudan]

On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:
> On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> > On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > > On upcoming hardware, we have a PCI adapter with two functions, one of
> > > > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > > > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > > > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > > > vendor to ensure that the hardware is not at fault, but if we use the
> > > > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > > > automatically disable MSI-X and the adapter does not appear to signal
> > > > any stray MSI-X interrupt.
> > > 
> > > It seems this could also be a firmware bug, have we heard anything from
> > > them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> > > disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).
> > 
> > We're tracking that down too. I think the fact that the interrupt is
> > coming in is a hardware bug in this particular adapter.
> > 
> > I'm looking at PAPR again and I see what might be a contradiction:
> > 
> > 7.3.10.5.1: "To removing all MSIs, set the Requested Number of
> > Interrupts to zero."
> > 
> > Table 71: "Function ... 1: Request to set to a new number of MSI or
> > MSI-X (platform choice) interrupts (including set to 0)"
> > 
> > It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
> > change only MSI or MSI-X and still be not a bug?
> 
> Yeah I guess you could read it that way, though I think that would be a
> bug.
> 
> The idea is that it chooses for you whether it uses MSI or MSI-X. So the
> only sane semantic is that when deconfiguring it deconfigures either,
> ie. both, kinds.

I agree with you that is how it should be :) I'm asking the firmware
folks to make sure I'm not misunderstanding the underlying issue.

> Looking closer at your patch, now I don't understand :)
> 
> +       /*
> +        * disabling MSI with the explicit interface also disables MSI-X
> +        */
> +       if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> 
> 
> So we first disable using function 3, which should:
> 
>         3: Request to set to a new number of MSI interrupts (including set to 0)
> 
> Which does not mention MSI-X at all, implying it has no effect on them.
> Which contradicts what you see, and the comment in the code?

Thanks for the thorough review!

Per PAPR 2.4 from Power.org, look at the page before that table, page
169:

"Specifying Function 3 (MSI) also disables MSI-X for the specified IOA
function, and likewise specifying Function 4 (MSI-X) disables MSI for
the IOA function....Specifying the Requested Number of Interrupts to
zero for either Function 3 or 4 removes all MSI & MSI-X interrupts from
the IOA function."

So I'm relying on this aspect of PAPR being enforced by the firmware,
which I think it is in my testing.

Thanks,
Nish

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Florian Mickler]

On Thu, 03 Mar 2011 19:29:19 -0800
Joe Perches <joe@perches.com> wrote:

> > $ git log --pretty=format:"%an %ae" arch/powerpc/platforms/pseries/msi.c
> > Andre Detsch adetsch@br.ibm.com
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > 
> > And the patch from Andre was also signed off by me.
> 
> How nice for you.  Last patch from you was
> 2 years ago.
> 

The problem is, that it get's pretty slow if you don't do a time
cut-off... i don't know the complexity but i guess it's linear in
(number of files) * (number of commits) * (avg size of diffs) ... 
(if you compute the diffstat) 

What I'll probably try as soon as I have another few hours to spare
for get_maintainer.pl, is to implement a fall back to older history if
the data basis is not stable enough. 


> 
> CC'ing inactive non named maintainers via git
> history also draws complaints btw.
> 
Yes.  I guess, my current version could use an staggered approach where
it does more digging if the results are too volatile... but first
try to be fast ...

Regards,
Flo

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Nishanth Aravamudan]

On 03.03.2011 [23:24:44 -0800], Nishanth Aravamudan wrote:
> On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:
> > On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> > > On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > > > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > > > On upcoming hardware, we have a PCI adapter with two functions, one of
> > > > > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > > > > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > > > > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > > > > vendor to ensure that the hardware is not at fault, but if we use the
> > > > > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > > > > automatically disable MSI-X and the adapter does not appear to signal
> > > > > any stray MSI-X interrupt.
> > > > 
> > > > It seems this could also be a firmware bug, have we heard anything from
> > > > them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> > > > disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).
> > > 
> > > We're tracking that down too. I think the fact that the interrupt is
> > > coming in is a hardware bug in this particular adapter.
> > > 
> > > I'm looking at PAPR again and I see what might be a contradiction:
> > > 
> > > 7.3.10.5.1: "To removing all MSIs, set the Requested Number of
> > > Interrupts to zero."
> > > 
> > > Table 71: "Function ... 1: Request to set to a new number of MSI or
> > > MSI-X (platform choice) interrupts (including set to 0)"
> > > 
> > > It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
> > > change only MSI or MSI-X and still be not a bug?
> > 
> > Yeah I guess you could read it that way, though I think that would be a
> > bug.
> > 
> > The idea is that it chooses for you whether it uses MSI or MSI-X. So the
> > only sane semantic is that when deconfiguring it deconfigures either,
> > ie. both, kinds.
> 
> I agree with you that is how it should be :) I'm asking the firmware
> folks to make sure I'm not misunderstanding the underlying issue.

It would appear that if a device does support both MSI and MSI-X and the
old (non-explicit) interface is used, only one of MSI or MSI-X is
guaranteed to be disabled, with preference given to MSI. Now, it turns
out that there is also a firmware dragon here (because this particular
device is misbehaving), but we can also fix it by using the new
interface, which shouldn't cause any harm, given the fallback.

> > Looking closer at your patch, now I don't understand :)
> > 
> > +       /*
> > +        * disabling MSI with the explicit interface also disables MSI-X
> > +        */
> > +       if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> > 
> > 
> > So we first disable using function 3, which should:
> > 
> >         3: Request to set to a new number of MSI interrupts (including set to 0)
> > 
> > Which does not mention MSI-X at all, implying it has no effect on them.
> > Which contradicts what you see, and the comment in the code?
> 
> Thanks for the thorough review!
> 
> Per PAPR 2.4 from Power.org, look at the page before that table, page
> 169:
> 
> "Specifying Function 3 (MSI) also disables MSI-X for the specified IOA
> function, and likewise specifying Function 4 (MSI-X) disables MSI for
> the IOA function....Specifying the Requested Number of Interrupts to
> zero for either Function 3 or 4 removes all MSI & MSI-X interrupts from
> the IOA function."
> 
> So I'm relying on this aspect of PAPR being enforced by the firmware,
> which I think it is in my testing.

Given all that, do I have your Ack? :)

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

On Mon, 2011-03-07 at 21:34 -0800, Nishanth Aravamudan wrote:
> On 03.03.2011 [23:24:44 -0800], Nishanth Aravamudan wrote:
> > On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:
> > > Looking closer at your patch, now I don't understand :)
> > > 
> > > +       /*
> > > +        * disabling MSI with the explicit interface also disables MSI-X
> > > +        */
> > > +       if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> > > 
> > > 
> > > So we first disable using function 3, which should:
> > > 
> > >         3: Request to set to a new number of MSI interrupts (including set to 0)
> > > 
> > > Which does not mention MSI-X at all, implying it has no effect on them.
> > > Which contradicts what you see, and the comment in the code?
> > 
> > Thanks for the thorough review!
> > 
> > Per PAPR 2.4 from Power.org, look at the page before that table, page
> > 169:
> > 
> > "Specifying Function 3 (MSI) also disables MSI-X for the specified IOA
> > function, and likewise specifying Function 4 (MSI-X) disables MSI for
> > the IOA function....Specifying the Requested Number of Interrupts to
> > zero for either Function 3 or 4 removes all MSI & MSI-X interrupts from
> > the IOA function."
> > 
> > So I'm relying on this aspect of PAPR being enforced by the firmware,
> > which I think it is in my testing.
> 
> Given all that, do I have your Ack? :)

Indeed. Thanks for clarifying it.

Acked-by: Michael Ellerman <michael@ellerman.id.au>

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

On Thu, 2011-03-03 at 19:29 -0800, Joe Perches wrote:
> On Fri, 2011-03-04 at 14:06 +1100, Michael Ellerman wrote:
> > On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> > > On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > > > Cc: Me  :)
> > > Sorry! I was in a hurry to get this out the door, my fault. Note, you
> > > don't show up per scripts/get_maintainer.pl :)
> > No worries, though I will remember never to use get_maintainer.pl, it is
> > obviously utterly broken:
> 
> Blah, blah, stupid tool doesn't work exactly as I want,
> and it doesn't credit me for my over 2 year old patches,
> therefore it's not only stupid, it's broken...

I do not give two shits about being given "credit" for this horrible
code, but if someone changes it I may have input.

True my last patch may have been two years ago, but I _wrote the entire
file_, and essentially no one else has ever touched it.

> $ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
> Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...)
> Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...)
> Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
> linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> linux-kernel@vger.kernel.org (open list)
> devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
> 
> If you don't like how it currently works, suggest
> improvements.

I think I was suggesting that I should be in that list :)

In the life of this file there have been 553 lines changed (added or
deleted), of which I have written 551, and I signed off the other 2.

So I guess I'm suggesting that when someone has written a large number
of the changes to a file they should be CC'ed. I don't know what the cut
off for "large number" should be, but anything > 50% would seem
reasonable (in terms of lines, not commits).

> How nice for you.  Last patch from you was 2 years ago.

Last patch from _anyone_ was 2 years ago. As much as I may have tried to
banish that code from my mind I can still remember some of the details,
even after 2 years :)

> CC'ing inactive non named maintainers via git
> history also draws complaints btw.

Yeah I can imagine. But that is a case of someone who is no longer
interested getting one extra email, ie. a false positive, whereas in
this case it's someone who does care _not_ seeing the patch, ie. a false
negative.

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Joe Perches]

On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:
> True my last patch may have been two years ago, but I _wrote the entire
> file_, and essentially no one else has ever touched it.
> 
> > $ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...)
> > Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...)
> > Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
> > linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> > linux-kernel@vger.kernel.org (open list)
> > devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
> > 
> > If you don't like how it currently works, suggest
> > improvements.
> 
> I think I was suggesting that I should be in that list :)

$ ./scripts/get_maintainer.pl -f --git-blame arch/powerpc/platforms/pseries/msi.c
Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...,commits:8/11=73%)
Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...,commits:4/11=36%)
Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
Michael Ellerman <michael@ellerman.id.au> (authored lines:481/481=100%,commits:11/11=100%)
Linas Vepstas <linas@austin.ibm.com> (commits:1/11=9%)
linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
linux-kernel@vger.kernel.org (open list)
devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)

cheers, Joe

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Nishanth Aravamudan]

On 09.03.2011 [16:28:23 -0800], Joe Perches wrote:
> On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:
> > True my last patch may have been two years ago, but I _wrote the entire
> > file_, and essentially no one else has ever touched it.
> > 
> > > $ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
> > > Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...)
> > > Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...)
> > > Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
> > > linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> > > linux-kernel@vger.kernel.org (open list)
> > > devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
> > > 
> > > If you don't like how it currently works, suggest
> > > improvements.
> > 
> > I think I was suggesting that I should be in that list :)
> 
> $ ./scripts/get_maintainer.pl -f --git-blame arch/powerpc/platforms/pseries/msi.c

Ah thanks, I didn't realize that flag existed, as I'd not needed to use
it before.

-Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

On Wed, 2011-03-09 at 16:28 -0800, Joe Perches wrote:
> On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:
> > True my last patch may have been two years ago, but I _wrote the entire
> > file_, and essentially no one else has ever touched it.
> > 
> > > $ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
> > > Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...)
> > > Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...)
> > > Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
> > > linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> > > linux-kernel@vger.kernel.org (open list)
> > > devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
> > > 
> > > If you don't like how it currently works, suggest
> > > improvements.
> > 
> > I think I was suggesting that I should be in that list :)
> 
> $ ./scripts/get_maintainer.pl -f --git-blame arch/powerpc/platforms/pseries/msi.c
> Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...,commits:8/11=73%)
> Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...,commits:4/11=36%)
> Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
> Michael Ellerman <michael@ellerman.id.au> (authored lines:481/481=100%,commits:11/11=100%)
> Linas Vepstas <linas@austin.ibm.com> (commits:1/11=9%)
> linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> linux-kernel@vger.kernel.org (open list)
> devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)

That's awesome. But it's not the default, so hardly anyone will ever use
it :)

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Joe Perches]

On Thu, 2011-03-10 at 14:35 +1100, Michael Ellerman wrote:
> On Wed, 2011-03-09 at 16:28 -0800, Joe Perches wrote:
> > On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:
> > > True my last patch may have been two years ago, but I _wrote the entire
> > > file_, and essentially no one else has ever touched it.
> > > > If you don't like how it currently works, suggest
> > > > improvements.
> > > I think I was suggesting that I should be in that list :)
> > $ ./scripts/get_maintainer.pl -f --git-blame arch/powerpc/platforms/pseries/msi.c
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...,commits:8/11=73%)
> > Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...,commits:4/11=36%)
> > Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
> > Michael Ellerman <michael@ellerman.id.au> (authored lines:481/481=100%,commits:11/11=100%)
> > Linas Vepstas <linas@austin.ibm.com> (commits:1/11=9%)
> > linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> > linux-kernel@vger.kernel.org (open list)
> > devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
> That's awesome. But it's not the default, so hardly anyone will ever use
> it :)

It's not the default for a few reasons:

o It can take a _long_ time to run
o Linus Torvalds is still the "author of record"
  for all the files in the initial commit
o For files with activity, it doesn't add very
  much useful as old commit authors do also tend
  to show up in the new commits.
o It can show maintainers that are no longer
  active.

Re: [PATCH] powerpc: disable MSI using new interface if possible

[Florian Mickler]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Am 10.03.2011 04:50 schrieb "Joe Perches" <joe@perches.com>:
>
> On Thu, 2011-03-10 at 14:35 +1100, Michael Ellerman wrote:
> > On Wed, 2011-03-09 at 16:28 -0800, Joe Perches wrote:
> > > On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:
> > > > True my last patch may have been two years ago, but I _wrote the
entire
> > > > file_, and essentially no one else has ever touched it.
> > > > > If you don't like how it currently works, suggest
> > > > > improvements.
> > > > I think I was suggesting that I should be in that list :)
> > > $ ./scripts/get_maintainer.pl -f --git-blame
arch/powerpc/platforms/pseries/msi.c
> > > Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR
POWERPC...,commits:8/11=73%)
> > > Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR
POWERPC...,commits:4/11=36%)
> > > Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE
AND...)
> > > Michael Ellerman <michael@ellerman.id.au> (authored
lines:481/481=100%,commits:11/11=100%)
> > > Linas Vepstas <linas@austin.ibm.com> (commits:1/11=9%)
> > > linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
> > > linux-kernel@vger.kernel.org (open list)
> > > devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
> > That's awesome. But it's not the default, so hardly anyone will ever use
> > it :)
>
> It's not the default for a few reasons:
>
> o It can take a _long_ time to run
> o Linus Torvalds is still the "author of record"
>  for all the files in the initial commit
> o For files with activity, it doesn't add very
>  much useful as old commit authors do also tend
>  to show up in the new commits.
> o It can show maintainers that are no longer
>  active.
>
Michael, maybe I'm missing something,  but why don't you have a MAINTAINERS
entry for that file? That would fix it for you.

Regards, Flo

[-- Attachment #2: Type: text/html, Size: 2787 bytes --]