Re: [RFC 06/10] misc: pci_endpoint_test: Add MSI-X support

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On Tuesday 24 April 2018 04:27 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 24/04/2018 08:19, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Tuesday 17 April 2018 11:08 PM, Gustavo Pimentel wrote:
>>> Hi Kishon,
>>>
>>> On 17/04/2018 11:33, Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>>
>>>> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>>>>> Adds the MSI-X support and updates driver documentation accordingly.
>>>>>
>>>>> Changes the driver parameter in order to allow the interruption type
>>>>> selection.
>>>>>
>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>> ---
>>>>>  Documentation/misc-devices/pci-endpoint-test.txt |   3 +
>>>>>  drivers/misc/pci_endpoint_test.c                 | 102 +++++++++++++++++------
>>>>>  2 files changed, 79 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt
>>>>> index 4ebc359..fdfa0f6 100644
>>>>> --- a/Documentation/misc-devices/pci-endpoint-test.txt
>>>>> +++ b/Documentation/misc-devices/pci-endpoint-test.txt
>>>>> @@ -10,6 +10,7 @@ The PCI driver for the test device performs the following tests
>>>>>  	*) verifying addresses programmed in BAR
>>>>>  	*) raise legacy IRQ
>>>>>  	*) raise MSI IRQ
>>>>> +	*) raise MSI-X IRQ
>>>>>  	*) read data
>>>>>  	*) write data
>>>>>  	*) copy data
>>>>> @@ -25,6 +26,8 @@ ioctl
>>>>>   PCITEST_LEGACY_IRQ: Tests legacy IRQ
>>>>>   PCITEST_MSI: Tests message signalled interrupts. The MSI number
>>>>>  	      to be tested should be passed as argument.
>>>>> + PCITEST_MSIX: Tests message signalled interrupts. The MSI-X number
>>>>> +	      to be tested should be passed as argument.
>>>>>   PCITEST_WRITE: Perform write tests. The size of the buffer should be passed
>>>>>  		as argument.
>>>>>   PCITEST_READ: Perform read tests. The size of the buffer should be passed
>>>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>>>> index 37db0fc..a7d9354 100644
>>>>> --- a/drivers/misc/pci_endpoint_test.c
>>>>> +++ b/drivers/misc/pci_endpoint_test.c
>>>>> @@ -42,11 +42,16 @@
>>>>>  #define PCI_ENDPOINT_TEST_COMMAND	0x4
>>>>>  #define COMMAND_RAISE_LEGACY_IRQ	BIT(0)
>>>>>  #define COMMAND_RAISE_MSI_IRQ		BIT(1)
>>>>> -#define MSI_NUMBER_SHIFT		2
>>>>> -/* 6 bits for MSI number */
>>>>> -#define COMMAND_READ                    BIT(8)
>>>>> -#define COMMAND_WRITE                   BIT(9)
>>>>> -#define COMMAND_COPY                    BIT(10)
>>>>> +#define COMMAND_RAISE_MSIX_IRQ		BIT(2)
>>>>> +#define IRQ_TYPE_SHIFT			3
>>>>> +#define IRQ_TYPE_LEGACY			0
>>>>> +#define IRQ_TYPE_MSI			1
>>>>> +#define IRQ_TYPE_MSIX			2
>>>>> +#define MSI_NUMBER_SHIFT		5
>>>>
>>>> Now that you are anyways fixing this, add a new register entry for MSI numbers.
>>>> Let's not keep COMMAND and MSI's together.
>>>
>>> What you suggest?
>>
>> #define PCI_ENDPOINT_TEST_COMMAND	0x4
>> #define COMMAND_RAISE_LEGACY_IRQ	BIT(0)
>> #define COMMAND_RAISE_MSI_IRQ		BIT(1)
>> #define COMMAND_RAISE_MSIX_IRQ		BIT(2)
>> #define COMMAND_READ                    BIT(3)
>> #define COMMAND_WRITE                   BIT(4)
>> #define COMMAND_COPY                    BIT(5)
>>
>> #define PCI_ENDPOINT_TEST_STATUS	0x8
>> #define STATUS_READ_SUCCESS             BIT(0)
>> #define STATUS_READ_FAIL                BIT(1)
>> #define STATUS_WRITE_SUCCESS            BIT(2)
>> #define STATUS_WRITE_FAIL               BIT(3)
>> #define STATUS_COPY_SUCCESS             BIT(4)
>> #define STATUS_COPY_FAIL                BIT(5)
>> #define STATUS_IRQ_RAISED               BIT(6)
>> #define STATUS_SRC_ADDR_INVALID         BIT(7)
>> #define STATUS_DST_ADDR_INVALID         BIT(8)
>>
>> #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0xc
>> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
>>
>> #define PCI_ENDPOINT_TEST_LOWER_DST_ADDR	0x14
>> #define PCI_ENDPOINT_TEST_UPPER_DST_ADDR	0x18
>>
>> #define PCI_ENDPOINT_TEST_SIZE		0x1c
>> #define PCI_ENDPOINT_TEST_CHECKSUM	0x20
>>
>> #define PCI_ENDPOINT_TEST_MSI_NUMBER	0x24
> 
> Ok. I will do it.
> 
>>
>> We should try not to modify either the existing register offsets or the bit
>> fields within these registers in the future as EP and RC will be running on
>> different systems and it is possible one of them might not have the updated
>> kernel.
> 
> I totally agree.
> 
>>>
>>>>> +/* 12 bits for MSI number */
>>>>> +#define COMMAND_READ                    BIT(17)
>>>>> +#define COMMAND_WRITE                   BIT(18)
>>>>> +#define COMMAND_COPY                    BIT(19)
>>>>
>>>> This change should be done along with the pci-epf-test in a single patch.
>>>
>>> To be clear, you're saying is this patch should be just be squashed into the
>>> patch number 8 [1], because there is a lot of dependencies namely the defines,
>>> that is used on the alter functions.
>>>
>>> [1] -> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_896841_&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=8urVwHCybXa1XMxsEbwHZAzzaEI_HJGXqmWgXpXb9TY&s=MRVr2YPY2Bk_WNFOxBfU4FGrFReTKdPhfzNDLiVxDbs&e=
>>
>> yeah. We have to make sure git bisect doesn't break functionality.
> 
> Ok, it'll be squashed.
> 
>>>
>>>>>  
>>>>>  #define PCI_ENDPOINT_TEST_STATUS	0x8
>>>>>  #define STATUS_READ_SUCCESS             BIT(0)
>>>>> @@ -73,9 +78,9 @@ static DEFINE_IDA(pci_endpoint_test_ida);
>>>>>  #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
>>>>>  					    miscdev)
>>>>>  
>>>>> -static bool no_msi;
>>>>> -module_param(no_msi, bool, 0444);
>>>>> -MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test");
>>>>
>>>> Let's not remove this just to make sure existing users doesn't get affected.
>>>
>>> Hum, by making an internal conversion? Like this
>>> no_msi = false <=> irq_type = 1
>>> no_msi = true <=> irq_type = 0
>>
> Disregard previous comment, it doesn't make sense. I don't know where my head was.
> 
> It will be like this on probe:
> 
> if (no_msi)
> 	irq_type = IRQ_TYPE_LEGACY;
> 
> However since we are breaking the compatibility on terms of MSI/MSI-X
> bits/registers (discussion on the top), it makes sense to keep the compatibility
> on this parameter?

This is userspace compatibility, so lets not break it.
Btw can we have a sysfs entry per device for defining irq_type. Having a sysfs
entry might be helpful instead of insmod/rmmod with different irq_type values?
It will also help if a system has enumerated multiple PCI_ENDPOINT_TEST EP devices.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html