Re: [PATCH 5/8] PCI PCIe portdrv: Fix allocation of interrupts

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

Rafael J. Wysocki wrote:
> On Thursday 15 January 2009, Kenji Kaneshige wrote:
>> Hidetoshi Seto wrote:
>>> Rafael J. Wysocki wrote:
>>>> On Wednesday 14 January 2009, Rafael J. Wysocki wrote:
>>>>> On Wednesday 14 January 2009, Kenji Kaneshige wrote:
>>>> [...]
>>>>>> I'm sorry but I don't understand what the problem is.
>>>>>> Do you mean pci_disable_msix() doesn't work on some platforms?
>>>>> No, I don't.  It was just confusion on my side, sorry.
>>>>>
>>>>> Please have a look at the new version of the patch I sent yesterday
>>>>> (http://marc.info/?l=linux-pci&m=123185510828181&w=4).
>>>> BTW, in your patch the first dummy pci_enable_msix() allocates just one
>>>> vector, which means that the contents of both
>>>> msix_entries[idx_hppme].entry and msix_entries[idx_aer].entry will be the same,
>>>> if my reading of the spec (PCI 3.0 in this case) is correct.
>>> According to PCI 3.0 implementation note "Handling MSI-X Vector Shortage,"
>>> it seems your reading is not correct.
>>>
>>> Assume that the port have 4 entries([0-3]) in MSI-X table, and that entry[2]
>>> for hotplug/PME and entry[3] for AER, and that kernel only allocates 2 vector.
>>> Spec says that the port could be designed for software to configure entries
>>> assigning vectors{A,B} to multiple entries as ABAB, AABB, ABBB etc. 
>>>
>>> So if there is just one vector, it could be AAAA.
> 
> Our pci_enable_msix() doesn't do that.  It will always do A---.
> 
>> BTW, I don't think pci_enable_msix() allows this kind of configuration.
>> With the dummy pci_enable_msix() in my patch, it would be A---, I think.
> 
> And that exactly is why I'm not sure it's correct.
> 
> Namely, if only the first entry is configured, the device is only able to use
> one vector, represented by this entry, for any purpose.  Now, for instance, for
> PCIE_CAPABILITIES_REG, there are two possibilities:
> (1) the value in the register always points to the _valid_ entry in the MSI-X
>     table and that would be the first one,
> (2) the value in the register may point to an _invalid_ entry (1 - 3).
> 
> You seem to assume that (2) is the case, but I'm not sure (that should follow
> from the PCI Express spec, but it clearly doesn't, at least I couldn't find
> any pointer in the spec).  IMO it wouldn't make sense, because the port
> wouldn't have been able to generate interrupts for this service if only one
> vector had been configured.

I don't understand what "valid" and "invalid" means. And I don't
imagine how the PCI Express chipset judges "valid" or "invalid".
If those mean "unmasked", "masked" respectively, we need to unmask
MSI-X before configuring MSI-X table entries. It doesn't look
correct behavior, I think.

After all, I think what we need to do at least for reading
Interrupt Message Number is that setting "MSI Enable"/"MSI-X Enable"
fields to 0b/1b. But it might exist some other things for enabling
MSI-X, quirk handling for example. And if we enable MSI-X by hand in
in port service driver, it would add the possibility of the race
condition between port service driver and existing pci_enable_msix().
That's why I used dummy pci_enable_msix() for reading Interrupt
Message Number. As you pointed out before, it's a little ugly, but
I think it's the simplest and most reliable way so far.

> 
> Still, even though (2) is the case, but both PCIE_CAPABILITIES_REG and
> PCI_ERR_ROOT_STATUS just happen to point to the same entry, which very well may
> be possible, the second pci_enable_msix() in your patch will fail.
> 

Oh, I understood. As you pointed out, my patch doesn't take into
account the case that HP/PME and AER share the same MSI-X entry.
This need to be fixed. Thanks!

> In any case, I think we should
> (a) get the number of the port's MSI-X table entries _first_, without enabling
>     MSI-X,
> (b) allocate as many MSI-X vectors as indicated by this number, even though
>     some of them may not be used,
> (c) use PCIE_CAPABILITIES_REG and PCI_ERR_ROOT_STATUS to check
>     which vector has been allocated to which service.
> 

This will cause unused vectors. We need to note that there can be
multiple PCIe ports in the system. So the number of unused vectors
can become large.

Thanks,
Kenji Kaneshige