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

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