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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
