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

Message ID 8b88f8c2b766f36c71659deb0fce635154b2b39f.1523379766.git.gustavo.pimentel@synopsys.com
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • Adds pcitest tool support for MSI-X
Related show

Commit Message

Gustavo Pimentel April 10, 2018, 5:14 p.m.
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(-)

Comments

Kishon Vijay Abraham I April 17, 2018, 10:33 a.m. | #1
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.
> +/* 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.
>  
>  #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.
> +static int irq_type = IRQ_TYPE_MSIX;
> +module_param(irq_type, int, 0444);
> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
>  
>  enum pci_barno {
>  	BAR_0,
> @@ -103,7 +108,7 @@ struct pci_endpoint_test {
>  struct pci_endpoint_test_data {
>  	enum pci_barno test_reg_bar;
>  	size_t alignment;
> -	bool no_msi;
> +	int irq_type;
>  };
>  
>  static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  
>  static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>  {
> -	u32 val;
> +	u32 val = COMMAND_RAISE_LEGACY_IRQ;
>  
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -				 COMMAND_RAISE_LEGACY_IRQ);
> +	val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>  	val = wait_for_completion_timeout(&test->irq_raised,
>  					  msecs_to_jiffies(1000));
>  	if (!val)
> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>  static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>  				      u8 msi_num)
>  {
> -	u32 val;
> +	u32 val = COMMAND_RAISE_MSI_IRQ;
>  	struct pci_dev *pdev = test->pdev;
>  
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -				 msi_num << MSI_NUMBER_SHIFT |
> -				 COMMAND_RAISE_MSI_IRQ);
> +	val |= (msi_num << MSI_NUMBER_SHIFT);
> +	val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>  	val = wait_for_completion_timeout(&test->irq_raised,
>  					  msecs_to_jiffies(1000));
>  	if (!val)
> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>  	return false;
>  }
>  
> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test,
> +				       u16 msix_num)
> +{
> +	u32 val = COMMAND_RAISE_MSIX_IRQ;
> +	struct pci_dev *pdev = test->pdev;
> +
> +	val |= (msix_num << MSI_NUMBER_SHIFT);
> +	val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
> +	val = wait_for_completion_timeout(&test->irq_raised,
> +					  msecs_to_jiffies(1000));
> +	if (!val)
> +		return false;
> +
> +	if (test->last_irq - pdev->irq == msix_num - 1)
> +		return true;
> +
> +	return false;

I think you can have a single function for msix_irq and msi_irq.

Thanks
Kishon
Gustavo Pimentel April 17, 2018, 5:38 p.m. | #2
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?

>> +/* 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://patchwork.ozlabs.org/patch/896841/

>>  
>>  #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

>> +static int irq_type = IRQ_TYPE_MSIX;
>> +module_param(irq_type, int, 0444);
>> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
>>  
>>  enum pci_barno {
>>  	BAR_0,
>> @@ -103,7 +108,7 @@ struct pci_endpoint_test {
>>  struct pci_endpoint_test_data {
>>  	enum pci_barno test_reg_bar;
>>  	size_t alignment;
>> -	bool no_msi;
>> +	int irq_type;
>>  };
>>  
>>  static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
>> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>  
>>  static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>>  {
>> -	u32 val;
>> +	u32 val = COMMAND_RAISE_LEGACY_IRQ;
>>  
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> -				 COMMAND_RAISE_LEGACY_IRQ);
>> +	val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>>  	val = wait_for_completion_timeout(&test->irq_raised,
>>  					  msecs_to_jiffies(1000));
>>  	if (!val)
>> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>>  static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>>  				      u8 msi_num)
>>  {
>> -	u32 val;
>> +	u32 val = COMMAND_RAISE_MSI_IRQ;
>>  	struct pci_dev *pdev = test->pdev;
>>  
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> -				 msi_num << MSI_NUMBER_SHIFT |
>> -				 COMMAND_RAISE_MSI_IRQ);
>> +	val |= (msi_num << MSI_NUMBER_SHIFT);
>> +	val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>>  	val = wait_for_completion_timeout(&test->irq_raised,
>>  					  msecs_to_jiffies(1000));
>>  	if (!val)
>> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>>  	return false;
>>  }
>>  
>> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test,
>> +				       u16 msix_num)
>> +{
>> +	u32 val = COMMAND_RAISE_MSIX_IRQ;
>> +	struct pci_dev *pdev = test->pdev;
>> +
>> +	val |= (msix_num << MSI_NUMBER_SHIFT);
>> +	val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>> +	val = wait_for_completion_timeout(&test->irq_raised,
>> +					  msecs_to_jiffies(1000));
>> +	if (!val)
>> +		return false;
>> +
>> +	if (test->last_irq - pdev->irq == msix_num - 1)
>> +		return true;
>> +
>> +	return false;
> 
> I think you can have a single function for msix_irq and msi_irq.

Ok.

> 
> Thanks
> Kishon
>
Alan Douglas April 24, 2018, 6:59 a.m. | #3
Hi Gustavo,

On 10 April 2018 18:15 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
> +/* 12 bits for MSI number */
> +#define COMMAND_READ                    BIT(17)
> +#define COMMAND_WRITE                   BIT(18)
> +#define COMMAND_COPY                    BIT(19)
> 
>  #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");
> +static int irq_type = IRQ_TYPE_MSIX;
> +module_param(irq_type, int, 0444);
> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test
> (0
> +- Legacy, 1 - MSI, 2 - MSI-X)");
> 
>  enum pci_barno {
>  	BAR_0,
> @@ -103,7 +108,7 @@ struct pci_endpoint_test {  struct
> pci_endpoint_test_data {
>  	enum pci_barno test_reg_bar;
>  	size_t alignment;
> -	bool no_msi;
> +	int irq_type;
>  };
> 
>  static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct
> pci_endpoint_test *test,
> 
>  static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)  {
> -	u32 val;
> +	u32 val = COMMAND_RAISE_LEGACY_IRQ;
> 
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -				 COMMAND_RAISE_LEGACY_IRQ);
> +	val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>  	val = wait_for_completion_timeout(&test->irq_raised,
>  					  msecs_to_jiffies(1000));
>  	if (!val)
> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct
> pci_endpoint_test *test)  static bool pci_endpoint_test_msi_irq(struct
> pci_endpoint_test *test,
>  				      u8 msi_num)
>  {
> -	u32 val;
> +	u32 val = COMMAND_RAISE_MSI_IRQ;
>  	struct pci_dev *pdev = test->pdev;
> 
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -				 msi_num << MSI_NUMBER_SHIFT |
> -				 COMMAND_RAISE_MSI_IRQ);
> +	val |= (msi_num << MSI_NUMBER_SHIFT);
> +	val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>  	val = wait_for_completion_timeout(&test->irq_raised,
>  					  msecs_to_jiffies(1000));
>  	if (!val)
> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct
> pci_endpoint_test *test,
>  	return false;
>  }
> 
> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test,
> +				       u16 msix_num)
> +{
> +	u32 val = COMMAND_RAISE_MSIX_IRQ;
> +	struct pci_dev *pdev = test->pdev;
> +
> +	val |= (msix_num << MSI_NUMBER_SHIFT);
> +	val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
> +	val = wait_for_completion_timeout(&test->irq_raised,
> +					  msecs_to_jiffies(1000));
> +	if (!val)
> +		return false;
> +
> +	if (test->last_irq - pdev->irq == msix_num - 1)
> +		return true;
I think we should use pci_irq_vector() to convert from device vector to Linux IRQ.
It can be done in pci_endpoint_test_irqhandler()
(With MSI, pdev->irq will be updated during MSI init, but it is not for MSI-X.)

pci_irq_vector()  should also be used for devm_request_irq() and devm_free_irq()
so some changes required in probe and remove.

Regards,
Alan
Kishon Vijay Abraham I April 24, 2018, 7:19 a.m. | #4
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

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.
> 
>>> +/* 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://patchwork.ozlabs.org/patch/896841/

yeah. We have to make sure git bisect doesn't break functionality.
> 
>>>  
>>>  #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

yeah..

Thanks
Kishon
Gustavo Pimentel April 24, 2018, 10:57 a.m. | #5
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?

> yeah..
> 
> Thanks
> Kishon
> 

Regards,
Gustavo
Gustavo Pimentel April 24, 2018, 11:11 a.m. | #6
Hi Alan,

On 24/04/2018 07:59, Alan Douglas wrote:
> Hi Gustavo,
> 
> On 10 April 2018 18:15 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
>> +/* 12 bits for MSI number */
>> +#define COMMAND_READ                    BIT(17)
>> +#define COMMAND_WRITE                   BIT(18)
>> +#define COMMAND_COPY                    BIT(19)
>>
>>  #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");
>> +static int irq_type = IRQ_TYPE_MSIX;
>> +module_param(irq_type, int, 0444);
>> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test
>> (0
>> +- Legacy, 1 - MSI, 2 - MSI-X)");
>>
>>  enum pci_barno {
>>  	BAR_0,
>> @@ -103,7 +108,7 @@ struct pci_endpoint_test {  struct
>> pci_endpoint_test_data {
>>  	enum pci_barno test_reg_bar;
>>  	size_t alignment;
>> -	bool no_msi;
>> +	int irq_type;
>>  };
>>
>>  static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
>> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct
>> pci_endpoint_test *test,
>>
>>  static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)  {
>> -	u32 val;
>> +	u32 val = COMMAND_RAISE_LEGACY_IRQ;
>>
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> -				 COMMAND_RAISE_LEGACY_IRQ);
>> +	val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>>  	val = wait_for_completion_timeout(&test->irq_raised,
>>  					  msecs_to_jiffies(1000));
>>  	if (!val)
>> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct
>> pci_endpoint_test *test)  static bool pci_endpoint_test_msi_irq(struct
>> pci_endpoint_test *test,
>>  				      u8 msi_num)
>>  {
>> -	u32 val;
>> +	u32 val = COMMAND_RAISE_MSI_IRQ;
>>  	struct pci_dev *pdev = test->pdev;
>>
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> -				 msi_num << MSI_NUMBER_SHIFT |
>> -				 COMMAND_RAISE_MSI_IRQ);
>> +	val |= (msi_num << MSI_NUMBER_SHIFT);
>> +	val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>>  	val = wait_for_completion_timeout(&test->irq_raised,
>>  					  msecs_to_jiffies(1000));
>>  	if (!val)
>> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct
>> pci_endpoint_test *test,
>>  	return false;
>>  }
>>
>> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test,
>> +				       u16 msix_num)
>> +{
>> +	u32 val = COMMAND_RAISE_MSIX_IRQ;
>> +	struct pci_dev *pdev = test->pdev;
>> +
>> +	val |= (msix_num << MSI_NUMBER_SHIFT);
>> +	val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>> +	val = wait_for_completion_timeout(&test->irq_raised,
>> +					  msecs_to_jiffies(1000));
>> +	if (!val)
>> +		return false;
>> +
>> +	if (test->last_irq - pdev->irq == msix_num - 1)
>> +		return true;
> I think we should use pci_irq_vector() to convert from device vector to Linux IRQ.
> It can be done in pci_endpoint_test_irqhandler()
> (With MSI, pdev->irq will be updated during MSI init, but it is not for MSI-X.)
> 
> pci_irq_vector()  should also be used for devm_request_irq() and devm_free_irq()
> so some changes required in probe and remove.

We could try it, sounds good.
Let's see if Kishon also agrees.

> 
> Regards,
> Alan
> 

Regards,
Gustavo
Kishon Vijay Abraham I April 24, 2018, 11:43 a.m. | #7
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
Gustavo Pimentel April 26, 2018, 3:36 p.m. | #8
Hi Kishon,

On 24/04/2018 12:43, Kishon Vijay Abraham I wrote:
> 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?

Can you explain it? An sysfs entry where, on RC side? How this will work with
the irq allocation/deallocation in runtime?

> It will also help if a system has enumerated multiple PCI_ENDPOINT_TEST EP devices.

Can you elaborate more this idea?

> 
> Thanks
> Kishon
> 

Regards,
Gustavo

Patch

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
+/* 12 bits for MSI number */
+#define COMMAND_READ                    BIT(17)
+#define COMMAND_WRITE                   BIT(18)
+#define COMMAND_COPY                    BIT(19)
 
 #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");
+static int irq_type = IRQ_TYPE_MSIX;
+module_param(irq_type, int, 0444);
+MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
 
 enum pci_barno {
 	BAR_0,
@@ -103,7 +108,7 @@  struct pci_endpoint_test {
 struct pci_endpoint_test_data {
 	enum pci_barno test_reg_bar;
 	size_t alignment;
-	bool no_msi;
+	int irq_type;
 };
 
 static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
@@ -177,10 +182,10 @@  static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 
 static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
 {
-	u32 val;
+	u32 val = COMMAND_RAISE_LEGACY_IRQ;
 
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_RAISE_LEGACY_IRQ);
+	val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
 	val = wait_for_completion_timeout(&test->irq_raised,
 					  msecs_to_jiffies(1000));
 	if (!val)
@@ -192,12 +197,12 @@  static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
 static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
 				      u8 msi_num)
 {
-	u32 val;
+	u32 val = COMMAND_RAISE_MSI_IRQ;
 	struct pci_dev *pdev = test->pdev;
 
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 msi_num << MSI_NUMBER_SHIFT |
-				 COMMAND_RAISE_MSI_IRQ);
+	val |= (msi_num << MSI_NUMBER_SHIFT);
+	val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
 	val = wait_for_completion_timeout(&test->irq_raised,
 					  msecs_to_jiffies(1000));
 	if (!val)
@@ -209,6 +214,26 @@  static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
 	return false;
 }
 
+static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test,
+				       u16 msix_num)
+{
+	u32 val = COMMAND_RAISE_MSIX_IRQ;
+	struct pci_dev *pdev = test->pdev;
+
+	val |= (msix_num << MSI_NUMBER_SHIFT);
+	val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
+	val = wait_for_completion_timeout(&test->irq_raised,
+					  msecs_to_jiffies(1000));
+	if (!val)
+		return false;
+
+	if (test->last_irq - pdev->irq == msix_num - 1)
+		return true;
+
+	return false;
+}
+
 static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, size_t size)
 {
 	bool ret = false;
@@ -226,6 +251,7 @@  static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, size_t size)
 	size_t alignment = test->alignment;
 	u32 src_crc32;
 	u32 dst_crc32;
+	u32 val = COMMAND_COPY;
 
 	if (size > SIZE_MAX - alignment)
 		goto err;
@@ -281,8 +307,9 @@  static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, size_t size)
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE,
 				 size);
 
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 1 << MSI_NUMBER_SHIFT | COMMAND_COPY);
+	val |= (1 << MSI_NUMBER_SHIFT);
+	val |= (irq_type << IRQ_TYPE_SHIFT);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
 
 	wait_for_completion(&test->irq_raised);
 
@@ -314,6 +341,7 @@  static bool pci_endpoint_test_write(struct pci_endpoint_test *test, size_t size)
 	size_t offset;
 	size_t alignment = test->alignment;
 	u32 crc32;
+	u32 val = COMMAND_READ;
 
 	if (size > SIZE_MAX - alignment)
 		goto err;
@@ -348,8 +376,9 @@  static bool pci_endpoint_test_write(struct pci_endpoint_test *test, size_t size)
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
 
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 1 << MSI_NUMBER_SHIFT | COMMAND_READ);
+	val |= (1 << MSI_NUMBER_SHIFT);
+	val |= (irq_type << IRQ_TYPE_SHIFT);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
 
 	wait_for_completion(&test->irq_raised);
 
@@ -375,6 +404,7 @@  static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t size)
 	size_t offset;
 	size_t alignment = test->alignment;
 	u32 crc32;
+	u32 val = COMMAND_WRITE;
 
 	if (size > SIZE_MAX - alignment)
 		goto err;
@@ -403,8 +433,9 @@  static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t size)
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
 
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 1 << MSI_NUMBER_SHIFT | COMMAND_WRITE);
+	val |= (1 << MSI_NUMBER_SHIFT);
+	val |= (irq_type << IRQ_TYPE_SHIFT);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
 
 	wait_for_completion(&test->irq_raised);
 
@@ -438,6 +469,9 @@  static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	case PCITEST_MSI:
 		ret = pci_endpoint_test_msi_irq(test, arg);
 		break;
+	case PCITEST_MSIX:
+		ret = pci_endpoint_test_msix_irq(test, arg);
+		break;
 	case PCITEST_WRITE:
 		ret = pci_endpoint_test_write(test, arg);
 		break;
@@ -490,7 +524,7 @@  static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	if (data) {
 		test_reg_bar = data->test_reg_bar;
 		test->alignment = data->alignment;
-		no_msi = data->no_msi;
+		irq_type = data->irq_type;
 	}
 
 	init_completion(&test->irq_raised);
@@ -510,11 +544,24 @@  static int pci_endpoint_test_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	if (!no_msi) {
+	switch (irq_type) {
+	case IRQ_TYPE_LEGACY:
+		break;
+	case IRQ_TYPE_MSI:
 		irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI);
 		if (irq < 0)
 			dev_err(dev, "failed to get MSI interrupts\n");
 		test->num_irqs = irq;
+		break;
+	case IRQ_TYPE_MSIX:
+		irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX);
+		if (irq < 0)
+			dev_err(dev, "Failed to get MSI-X interrupts\n");
+		test->num_irqs = irq;
+		break;
+	default:
+		dev_err(dev, "Invalid IRQ type selected\n");
+		goto err_disable_msi;
 	}
 
 	err = devm_request_irq(dev, pdev->irq, pci_endpoint_test_irqhandler,
@@ -529,8 +576,9 @@  static int pci_endpoint_test_probe(struct pci_dev *pdev,
 				       pci_endpoint_test_irqhandler,
 				       IRQF_SHARED, DRV_MODULE_NAME, test);
 		if (err)
-			dev_err(dev, "failed to request IRQ %d for MSI %d\n",
-				pdev->irq + i, i + 1);
+			dev_err(dev, "Failed to request IRQ %d for MSI%s %d\n",
+				pdev->irq + i,
+				irq_type == IRQ_TYPE_MSIX ? "-X" : "", i + 1);
 	}
 
 	for (bar = BAR_0; bar <= BAR_5; bar++) {
@@ -596,6 +644,7 @@  static int pci_endpoint_test_probe(struct pci_dev *pdev,
 
 err_disable_msi:
 	pci_disable_msi(pdev);
+	pci_disable_msix(pdev);
 	pci_release_regions(pdev);
 
 err_disable_pdev:
@@ -627,6 +676,7 @@  static void pci_endpoint_test_remove(struct pci_dev *pdev)
 	for (i = 0; i < test->num_irqs; i++)
 		devm_free_irq(&pdev->dev, pdev->irq + i, test);
 	pci_disable_msi(pdev);
+	pci_disable_msix(pdev);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 }