Messages in this thread | | | Date | Tue, 13 Jan 2009 11:34:46 +0900 | From | Kenji Kaneshige <> | Subject | Re: [PATCH 5/8] PCI PCIe portdrv: Fix allocation of interrupts |
| |
Rafael J. Wysocki wrote: > On Thursday 08 January 2009, Kenji Kaneshige wrote: >> Rafael J. Wysocki wrote: >>> On Thursday 08 January 2009, Kenji Kaneshige wrote: >>>> Rafael J. Wysocki wrote: >>>>> From: Rafael J. Wysocki <rjw@sisk.pl> >>>>> >>>>> If MSI-X interrupt mode is used by the PCI Express port driver, too >>>>> many vectors are allocated and it is not ensured that the right >>>>> vectors will be used for various services. Namely, the PCI Express >>>>> specification states that both PCI Express native PME and PCI Express >>>>> hotplug will always use the same MSI or MSI-X message for signalling >>>>> interrupts, which implies that the same vector will be used by both >>>>> of them. Also, the VC service does not use interrupts at all. >>>>> Moreover, is not clear which of the vectors allocated by >>>>> pci_enable_msix() will be used for PME and hotplug and which of them >>>>> will be used for AER if all of these services are configured. >>>>> >>>>> For these reasons, rework the allocation of interrupts for PCI >>>>> Express ports so that at most two vectors are allocated and ensure >>>>> that the right vector will be used for the right purpose. >>>>> >>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> >>>>> --- >>>>> drivers/pci/pcie/portdrv.h | 1 >>>>> drivers/pci/pcie/portdrv_core.c | 155 +++++++++++++++++++++++++++++----------- >>>>> 2 files changed, 117 insertions(+), 39 deletions(-) >>>>> >>>>> Index: linux-2.6/drivers/pci/pcie/portdrv.h >>>>> =================================================================== >>>>> --- linux-2.6.orig/drivers/pci/pcie/portdrv.h >>>>> +++ linux-2.6/drivers/pci/pcie/portdrv.h >>>>> @@ -25,6 +25,7 @@ >>>>> #define PCIE_CAPABILITIES_REG 0x2 >>>>> #define PCIE_SLOT_CAPABILITIES_REG 0x14 >>>>> #define PCIE_PORT_DEVICE_MAXSERVICES 4 >>>>> +#define PORT_MSI_VECTOR_MASK 0x1f >>>>> >>>>> #define get_descriptor_id(type, service) (((type - 4) << 4) | service) >>>>> >>>>> Index: linux-2.6/drivers/pci/pcie/portdrv_core.c >>>>> =================================================================== >>>>> --- linux-2.6.orig/drivers/pci/pcie/portdrv_core.c >>>>> +++ linux-2.6/drivers/pci/pcie/portdrv_core.c >>>>> @@ -30,55 +30,120 @@ static void release_pcie_device(struct d >>>>> } >>>>> >>>>> /** >>>>> + * fix_up_vectors - ensure the right ordering of MSI-X interrupt vectors >>>>> + * @dev: PCI Express port that is going to use the vectors >>>>> + * @vectors: Array of interrupt vectors to check (2 entries) >>>>> + * >>>>> + * Return value: >>>>> + * 0 on success, error code if the values read from config registers are not as >>>>> + * expected >>>>> + * >>>>> + * If this function is called, we are going to use two interrupt vectors which >>>>> + * may be different, but we have to make sure they are in the right order such >>>>> + * that the vector to be used for PME and hotplug signalling is the first one. >>>>> + * >>>>> + * NOTE: The assumption here is that MSI message offset (with respect to the >>>>> + * base Message Data) equal to N corresponds to index N in the array of vectors >>>>> + * returned by pci_enable_msix(). >>>>> + */ >>>> I've posted the similar patch recently, which doesn't have this >>>> assumption. >>> Actually, this assumption is in agreement with PCI Express Base Specification >>> 2.0, which very clearly states in Section 7.8.2 that: >>> >>> "[...] Interrupt Message Number – This field indicates which >>> MSI/MSI-X vector is used for the interrupt message generated in >>> association with any of the status bits of this Capability structure. >>> >>> [...] >>> >>> For MSI-X, the value in this field indicates which MSI-X Table >>> entry is used to generate the interrupt message. The entry must >>> be one of the first 32 entries even if the Function implements >>> more than 32 entries. For a given MSI-X implementation, the >>> entry must remain constant." >>> >> Though I might be misunderstanding something, my understanding is >> that this implies that there can be more MSI-X table entries than >> number of interrupts, and PME/HotPlug or AER interrupts can be >> mapped to other than first two entries. > > We are requesting two vectors with my patch and they will be assigned to the > first two entries in the MSI-X table. If my understanding of the MSI-X code is > corrtect, there won't be more MSI-X table entries set up for the port. > >> If my understanding is correct, your patch would not work if PME/HotPlug or >> AER interrupts were mapped to other than first two. > > In this case it's going to fall back to MSI w/ one vector, because we're going > to find that offset > 1 in one of the tests in fix_up_vectors(). > >> Regardless of my understanding is correct or not, I have another >> concern. I think there are two interrupts for PCIe port service >> currently as your patch indicates. But new interrupts can be added >> in the future. With the assumption, PME/HotPlug and/or AER interrupts >> would not work if the first several entries are mapped to other new >> interrupts. Therefore, with the assumption, we need to fix MSI-X >> initialization code whenever new interrupts are defined. > > In that case we'll need to allocate more vectors, so we'll need to change the > code anyway. Also, we'll have to add the tests for the new interrupts to > the code, because there will have to be new registers defined to read the > MSI offsets or MSI-X table indices from. >
I'm sorry for delay. I've just resumed.
Yes, we need to change the code if we use new port service that uses new interrupt. But there would be a delay between hardware update and kernel update. I think currently supported MSI-X interrupts should keep working with the new hardware even before the kernel supports new port service.
> I very much prefer to keep the code as simple as possible as long as we can.
I agree with this.
Thanks, Kenji Kaneshige
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |