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

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

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---.
> > 
> > > 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.
> > 
> > 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,
> > (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.
> 
> (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.

Thanks,
Rafael