powerpc: disable MSI using new interface if possible

Submitted by Nishanth Aravamudan on March 4, 2011, 1:41 a.m.

Details

Message ID 1299202862-10682-1-git-send-email-nacc@us.ibm.com
State Accepted, archived
Headers show

Commit Message

Nishanth Aravamudan March 4, 2011, 1:41 a.m.
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(-)

Comments

Michael Ellerman March 4, 2011, 3:01 a.m.
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
Michael Ellerman March 4, 2011, 3:06 a.m.
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
Joe Perches March 4, 2011, 3:29 a.m.
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.
Nishanth Aravamudan March 4, 2011, 7:24 a.m.
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
florian@mickler.org March 4, 2011, 11:13 a.m.
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
Nishanth Aravamudan March 8, 2011, 5:34 a.m.
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
Michael Ellerman March 9, 2011, 10:54 p.m.
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
Michael Ellerman March 9, 2011, 11:12 p.m.
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
Joe Perches March 10, 2011, 12:28 a.m.
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
Nishanth Aravamudan March 10, 2011, 12:46 a.m.
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
Michael Ellerman March 10, 2011, 3:35 a.m.
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
Joe Perches March 10, 2011, 3:45 a.m.
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.
florian@mickler.org March 10, 2011, 7:04 a.m.
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

Patch hide | download patch | download mbox

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)