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

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE5696d

18 messages, 4 authors, 2011-03-10 · open the first message on its own page

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

From: Nishanth Aravamudan <hidden>
Date: 2011-03-03 19:39:33

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 <redacted>
Cc: Milton Miller <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
---

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}

From: Nishanth Aravamudan <hidden>
Date: 2011-03-03 19:39:33

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 <redacted>
Cc: Milton Miller <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>

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

From: Nishanth Aravamudan <hidden>
Date: 2011-03-03 19:44:19

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 <redacted>
Cc: Milton Miller <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>

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

quoted hunk

 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 [off-list ref]
IBM Linux Technology Center

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

From: Michael Ellerman <hidden>
Date: 2011-03-04 01:05:29

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 <redacted>
Cc: Milton Miller <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>

Cc: Me  :)

quoted hunk

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

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

From: Nishanth Aravamudan <hidden>
Date: 2011-03-04 01:41:10

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:

quoted

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.

quoted

Signed-off-by: Nishanth Aravamudan <redacted>
Cc: Milton Miller <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>

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 :)

quoted

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 <redacted>
---

 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

From: Michael Ellerman <hidden>
Date: 2011-03-04 03:01:24

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

quoted

On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:

quoted

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

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

From: Michael Ellerman <hidden>
Date: 2011-03-04 03:06:56

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

quoted

On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:

quoted

On upcoming hardware, we have a PCI adapter with two functions, one of

..

quoted

Signed-off-by: Nishanth Aravamudan <redacted>
Cc: Milton Miller <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>

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

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

From: Joe Perches <joe@perches.com>
Date: 2011-03-04 03:29:22

On Fri, 2011-03-04 at 14:06 +1100, Michael Ellerman wrote:

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:

quoted

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

quoted

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 [off-list ref] (supporter:LINUX FOR POWERPC...)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...)
Grant Likely [off-list ref] (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

From: Nishanth Aravamudan <hidden>
Date: 2011-03-04 07:25:01

On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:

quoted

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

quoted

On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:

quoted

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

From: Florian Mickler <hidden>
Date: 2011-03-04 11:22:38

On Thu, 03 Mar 2011 19:29:19 -0800
Joe Perches [off-list ref] wrote:

quoted

$ 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

From: Nishanth Aravamudan <hidden>
Date: 2011-03-08 05:34:21

On 03.03.2011 [23:24:44 -0800], Nishanth Aravamudan wrote:

On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:

quoted

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:

quoted

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

quoted

On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:

quoted

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.

quoted

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 [off-list ref]
IBM Linux Technology Center

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

From: Michael Ellerman <hidden>
Date: 2011-03-09 22:54:06

On Mon, 2011-03-07 at 21:34 -0800, Nishanth Aravamudan wrote:

On 03.03.2011 [23:24:44 -0800], Nishanth Aravamudan wrote:

quoted

On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:

quoted

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 <redacted>

cheers

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

From: Michael Ellerman <hidden>
Date: 2011-03-09 23:12:16

On Thu, 2011-03-03 at 19:29 -0800, Joe Perches wrote:

On Fri, 2011-03-04 at 14:06 +1100, Michael Ellerman wrote:

quoted

On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:

quoted

On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:

quoted

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 [off-list ref] (supporter:LINUX FOR POWERPC...)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...)
Grant Likely [off-list ref] (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

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

From: Joe Perches <joe@perches.com>
Date: 2011-03-10 00:28:27

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.

quoted

$ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
Benjamin Herrenschmidt [off-list ref] (supporter:LINUX FOR POWERPC...)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...)
Grant Likely [off-list ref] (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 [off-list ref] (supporter:LINUX FOR POWERPC...,commits:8/11=73%)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...,commits:4/11=36%)
Grant Likely [off-list ref] (maintainer:OPEN FIRMWARE AND...)
Michael Ellerman [off-list ref] (authored lines:481/481=100%,commits:11/11=100%)
Linas Vepstas [off-list ref] (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

From: Nishanth Aravamudan <hidden>
Date: 2011-03-10 00:46:22

On 09.03.2011 [16:28:23 -0800], Joe Perches wrote:

On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:

quoted

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.

quoted

$ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
Benjamin Herrenschmidt [off-list ref] (supporter:LINUX FOR POWERPC...)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...)
Grant Likely [off-list ref] (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 [off-list ref]
IBM Linux Technology Center

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

From: Michael Ellerman <hidden>
Date: 2011-03-10 03:35:21

On Wed, 2011-03-09 at 16:28 -0800, Joe Perches wrote:

On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:

quoted

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.

quoted

$ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
Benjamin Herrenschmidt [off-list ref] (supporter:LINUX FOR POWERPC...)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...)
Grant Likely [off-list ref] (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 [off-list ref] (supporter:LINUX FOR POWERPC...,commits:8/11=73%)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...,commits:4/11=36%)
Grant Likely [off-list ref] (maintainer:OPEN FIRMWARE AND...)
Michael Ellerman [off-list ref] (authored lines:481/481=100%,commits:11/11=100%)
Linas Vepstas [off-list ref] (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

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

From: Joe Perches <joe@perches.com>
Date: 2011-03-10 03:45:26

On Thu, 2011-03-10 at 14:35 +1100, Michael Ellerman wrote:

On Wed, 2011-03-09 at 16:28 -0800, Joe Perches wrote:

quoted

On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:

quoted

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.

quoted

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 [off-list ref] (supporter:LINUX FOR POWERPC...,commits:8/11=73%)
Paul Mackerras [off-list ref] (supporter:LINUX FOR POWERPC...,commits:4/11=36%)
Grant Likely [off-list ref] (maintainer:OPEN FIRMWARE AND...)
Michael Ellerman [off-list ref] (authored lines:481/481=100%,commits:11/11=100%)
Linas Vepstas [off-list ref] (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

From: Florian Mickler <hidden>
Date: 2011-03-10 07:04:21

Am 10.03.2011 04:50 schrieb "Joe Perches" [off-list ref]:

On Thu, 2011-03-10 at 14:35 +1100, Michael Ellerman wrote:

quoted

On Wed, 2011-03-09 at 16:28 -0800, Joe Perches wrote:

quoted

On Thu, 2011-03-10 at 10:12 +1100, Michael Ellerman wrote:

quoted

True my last patch may have been two years ago, but I _wrote the

entire

quoted

file_, and essentially no one else has ever touched it.

quoted

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

quoted

Benjamin Herrenschmidt [off-list ref] (supporter:LINUX FOR

POWERPC...,commits:8/11=73%)

quoted

Paul Mackerras [off-list ref] (supporter:LINUX FOR

POWERPC...,commits:4/11=36%)

quoted

Grant Likely [off-list ref] (maintainer:OPEN FIRMWARE

AND...)

quoted

Michael Ellerman [off-list ref] (authored

lines:481/481=100%,commits:11/11=100%)

quoted

Linas Vepstas [off-list ref] (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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help