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

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

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