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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday 16 January 2009, Hidetoshi Seto wrote:
> Rafael J. Wysocki wrote:
> > On Thursday 15 January 2009, Rafael J. Wysocki wrote:
> >> On Thursday 15 January 2009, 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---.
> 
> Just above the implementation note, the spec says:
>   "Software is permitted to configure multiple MSI-X Table entries
>    with the same vector, and this may indeed be necessary when fewer
>    vectors are allocated than requested."
> while "software" refers to either system software or device driver software.
> 
> So, yes, the our current implementation of system software (=Linux kernel)
> doesn't do that.
> However I'd like to note that doing that by "software" is not prohibited
> in PCI 3.0.
> 
> >>>> 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).
> 
> The "invalid entry" is not defined.

s/invalid/unused/ (or masked permanently)

> >>> 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.
> >>>
> >>> 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.
> >>>
> >>> In any case, I think we should
> >>> (a) get the number of the port's MSI-X table entries _first_, without enabling
> >>>     MSI-X,
> 
> We cannot do this because both of PCIE_CAPABILITIES_REG and PCI_ERR_ROOT_STATUS
> will indicate the number for MSI, not for MSI-X without enabling MSI-X.

Yes, we can.  We don't read PCIE_CAPABILITIES_REG and PCI_ERR_ROOT_STATUS at
this point yet and the number of entries in the MSI-X table is constant
(read-only), so we can read it even before enabling MSI-X.  Actually, our MSI-X
code does that already anyway.

> >>> (b) allocate as many MSI-X vectors as indicated by this number, even though
> >>>     some of them may not be used,

(b) should be: call pci_enable_msix() with the last argument equal to the
number of entries in the MSI-X table or 32, whichever is smaller.

> >>> (c) use PCIE_CAPABILITIES_REG and PCI_ERR_ROOT_STATUS to check
> >>>     which vector has been allocated to which service.
> >> (d) mask the unused vectors.
> > 
> > However, it's probably simpler to do something like in your patch, although
> > I don't like the dummy enabling of MSI-X at all.
> 
> How about this?
> 
>   #define PCIE_MSIX_ENTRY_HPPME MAGIC_NUMBER_1
>   #define PCIE_MSIX_ENTRY_AER   MAGIC_NUMBER_2
> 
>   struct msix_entry msix_entries[] =
>                         {{0, PCIE_MSIX_ENTRY_HPPME}, {0, PCIE_MSIX_ENTRY_AER}};
>   status = pci_enable_msix(dev, msix_entries, nvec);
> 
> And modify pci_enable_msix() to handle these magic numbers.

Quite frankly, I prefer the procedure described above in (a) - (d).  I'll try
to implement it and we'll see how it looks like.

Thanks,
Rafael