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

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

On Friday 16 January 2009, Rafael J. Wysocki wrote:
> 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.

I've just sent the patch in the other thread.

Thanks,
Rafael