[PATCH 0/4] pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[PATCH 0/4] pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

Hello there,

From recent feedback from Mani, he did not like PCITEST_IRQ_TYPE_AUTO,
thus rework the solution and let PCITEST_{READ,WRITE,COPY} set the IRQ
type themselves.


Kind regards,
Niklas

Niklas Cassel (4):
  Revert "misc: pci_endpoint_test: Add support for
    PCITEST_IRQ_TYPE_AUTO"
  misc: pci_endpoint_test: Fetch supported IRQ types in CAPS register
  misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type
    automatically
  selftests: pci_endpoint: Remove PCITEST_SET_IRQTYPE ioctls for
    read/write/copy

 drivers/misc/pci_endpoint_test.c              | 138 +++++++++---------
 include/uapi/linux/pcitest.h                  |   1 -
 .../pci_endpoint/pci_endpoint_test.c          |   9 --
 3 files changed, 68 insertions(+), 80 deletions(-)

-- 
2.48.1

[PATCH 1/4] Revert "misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO"

[Niklas Cassel]

This reverts commit c4aaa6b5b5ac14ea9b0abf1e0f4bf24c332a9b34.

The PCI endpoint maintainer did not like PCITEST_IRQ_TYPE_AUTO,
so let's remove this IRQ type and rework the implementation.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Hello PCI maintainers,
feel free to squash / drop the original commit from the branch.

 drivers/misc/pci_endpoint_test.c              | 25 ++++---------------
 include/uapi/linux/pcitest.h                  |  1 -
 .../pci_endpoint/pci_endpoint_test.c          | 12 ++++-----
 3 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d294850a35a12..849d730ba14dd 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -66,9 +66,6 @@
 
 #define PCI_ENDPOINT_TEST_CAPS			0x30
 #define CAP_UNALIGNED_ACCESS			BIT(0)
-#define CAP_MSI					BIT(1)
-#define CAP_MSIX				BIT(2)
-#define CAP_INTX				BIT(3)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
@@ -115,7 +112,6 @@ struct pci_endpoint_test {
 	struct miscdevice miscdev;
 	enum pci_barno test_reg_bar;
 	size_t alignment;
-	u32 ep_caps;
 	const char *name;
 };
 
@@ -806,23 +802,11 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	int ret;
 
 	if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
-	    req_irq_type > PCITEST_IRQ_TYPE_AUTO) {
+	    req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
 		return -EINVAL;
 	}
 
-	if (req_irq_type == PCITEST_IRQ_TYPE_AUTO) {
-		if (test->ep_caps & CAP_MSI)
-			req_irq_type = PCITEST_IRQ_TYPE_MSI;
-		else if (test->ep_caps & CAP_MSIX)
-			req_irq_type = PCITEST_IRQ_TYPE_MSIX;
-		else if (test->ep_caps & CAP_INTX)
-			req_irq_type = PCITEST_IRQ_TYPE_INTX;
-		else
-			/* fallback to MSI if no caps defined */
-			req_irq_type = PCITEST_IRQ_TYPE_MSI;
-	}
-
 	if (test->irq_type == req_irq_type)
 		return 0;
 
@@ -908,12 +892,13 @@ static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
 {
 	struct pci_dev *pdev = test->pdev;
 	struct device *dev = &pdev->dev;
+	u32 caps;
 
-	test->ep_caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
-	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", test->ep_caps);
+	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
+	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps);
 
 	/* CAP_UNALIGNED_ACCESS is set if the EP can do unaligned access */
-	if (test->ep_caps & CAP_UNALIGNED_ACCESS)
+	if (caps & CAP_UNALIGNED_ACCESS)
 		test->alignment = 0;
 }
 
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index d3aa8715a525e..304bf9be0f9a0 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -27,7 +27,6 @@
 #define PCITEST_IRQ_TYPE_INTX		0
 #define PCITEST_IRQ_TYPE_MSI		1
 #define PCITEST_IRQ_TYPE_MSIX		2
-#define PCITEST_IRQ_TYPE_AUTO		3
 
 #define PCITEST_FLAGS_USE_DMA	0x00000001
 
diff --git a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
index ac26481d29d9d..fdf4bc6aa9d2a 100644
--- a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
+++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
@@ -181,8 +181,8 @@ TEST_F(pci_ep_data_transfer, READ_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
@@ -200,8 +200,8 @@ TEST_F(pci_ep_data_transfer, WRITE_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
@@ -219,8 +219,8 @@ TEST_F(pci_ep_data_transfer, COPY_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
-- 
2.48.1

[PATCH 2/4] misc: pci_endpoint_test: Fetch supported IRQ types in CAPS register

[Niklas Cassel]

Fetch supported IRQ types in CAPS register and save them in struct
pci_endpoint_test.

The supported IRQ types will be used in follow-up commit.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 849d730ba14dd..3c04121d24733 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -66,6 +66,9 @@
 
 #define PCI_ENDPOINT_TEST_CAPS			0x30
 #define CAP_UNALIGNED_ACCESS			BIT(0)
+#define CAP_MSI					BIT(1)
+#define CAP_MSIX				BIT(2)
+#define CAP_INTX				BIT(3)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
@@ -112,6 +115,7 @@ struct pci_endpoint_test {
 	struct miscdevice miscdev;
 	enum pci_barno test_reg_bar;
 	size_t alignment;
+	u32 ep_caps;
 	const char *name;
 };
 
@@ -892,13 +896,12 @@ static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
 {
 	struct pci_dev *pdev = test->pdev;
 	struct device *dev = &pdev->dev;
-	u32 caps;
 
-	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
-	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps);
+	test->ep_caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
+	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", test->ep_caps);
 
 	/* CAP_UNALIGNED_ACCESS is set if the EP can do unaligned access */
-	if (caps & CAP_UNALIGNED_ACCESS)
+	if (test->ep_caps & CAP_UNALIGNED_ACCESS)
 		test->alignment = 0;
 }
 
-- 
2.48.1

[PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

The test cases for read/write/copy currently do:
1) ioctl(PCITEST_SET_IRQTYPE, MSI)
2) ioctl(PCITEST_{READ,WRITE,COPY})

This design is quite bad for a few reasons:
-It assumes that all PCI EPCs support MSI.
-One ioctl should be sufficient for these test cases.

Modify the PCITEST_{READ,WRITE,COPY} ioctls to set IRQ type automatically,
overwriting the currently configured IRQ type. It there are no IRQ types
supported in the CAPS register, fall back to MSI IRQs. This way the
implementation is no worse than before this commit.

Any test case that requires a specific IRQ type, e.g. MSIX_TEST, will do
an explicit PCITEST_SET_IRQTYPE ioctl at the start of the test case, thus
it is safe to always overwrite the configured IRQ type.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 126 +++++++++++++++++--------------
 1 file changed, 68 insertions(+), 58 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3c04121d24733..cfaeeea7642ac 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -464,6 +464,62 @@ static int pci_endpoint_test_validate_xfer_params(struct device *dev,
 	return 0;
 }
 
+static int pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
+{
+	pci_endpoint_test_release_irq(test);
+	pci_endpoint_test_free_irq_vectors(test);
+
+	return 0;
+}
+
+static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
+				      int req_irq_type)
+{
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
+	    req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
+		dev_err(dev, "Invalid IRQ type option\n");
+		return -EINVAL;
+	}
+
+	if (test->irq_type == req_irq_type)
+		return 0;
+
+	pci_endpoint_test_release_irq(test);
+	pci_endpoint_test_free_irq_vectors(test);
+
+	ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
+	if (ret)
+		return ret;
+
+	ret = pci_endpoint_test_request_irq(test);
+	if (ret) {
+		pci_endpoint_test_free_irq_vectors(test);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pci_endpoint_test_set_auto_irq(struct pci_endpoint_test *test,
+					  int *irq_type)
+{
+	if (test->ep_caps & CAP_MSI)
+		*irq_type = PCITEST_IRQ_TYPE_MSI;
+	else if (test->ep_caps & CAP_MSIX)
+		*irq_type = PCITEST_IRQ_TYPE_MSIX;
+	else if (test->ep_caps & CAP_INTX)
+		*irq_type = PCITEST_IRQ_TYPE_INTX;
+	else
+		/* fallback to MSI if no caps defined */
+		*irq_type = PCITEST_IRQ_TYPE_MSI;
+
+	return pci_endpoint_test_set_irq(test, *irq_type);
+}
+
 static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
 				   unsigned long arg)
 {
@@ -483,7 +539,7 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
 	dma_addr_t orig_dst_phys_addr;
 	size_t offset;
 	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
+	int irq_type;
 	u32 src_crc32;
 	u32 dst_crc32;
 	int ret;
@@ -504,11 +560,9 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
 	if (use_dma)
 		flags |= FLAG_USE_DMA;
 
-	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
-	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		return -EINVAL;
-	}
+	ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
+	if (ret)
+		return ret;
 
 	orig_src_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_src_addr) {
@@ -616,7 +670,7 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
 	dma_addr_t orig_phys_addr;
 	size_t offset;
 	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
+	int irq_type;
 	size_t size;
 	u32 crc32;
 	int ret;
@@ -637,11 +691,9 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
 	if (use_dma)
 		flags |= FLAG_USE_DMA;
 
-	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
-	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		return -EINVAL;
-	}
+	ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
+	if (ret)
+		return ret;
 
 	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_addr) {
@@ -714,7 +766,7 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
 	dma_addr_t orig_phys_addr;
 	size_t offset;
 	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
+	int irq_type;
 	u32 crc32;
 	int ret;
 
@@ -734,11 +786,9 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
 	if (use_dma)
 		flags |= FLAG_USE_DMA;
 
-	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
-	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		return -EINVAL;
-	}
+	ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
+	if (ret)
+		return ret;
 
 	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_addr) {
@@ -790,46 +840,6 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
 	return ret;
 }
 
-static int pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
-{
-	pci_endpoint_test_release_irq(test);
-	pci_endpoint_test_free_irq_vectors(test);
-
-	return 0;
-}
-
-static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
-				      int req_irq_type)
-{
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	int ret;
-
-	if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
-	    req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		return -EINVAL;
-	}
-
-	if (test->irq_type == req_irq_type)
-		return 0;
-
-	pci_endpoint_test_release_irq(test);
-	pci_endpoint_test_free_irq_vectors(test);
-
-	ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
-	if (ret)
-		return ret;
-
-	ret = pci_endpoint_test_request_irq(test);
-	if (ret) {
-		pci_endpoint_test_free_irq_vectors(test);
-		return ret;
-	}
-
-	return 0;
-}
-
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 				    unsigned long arg)
 {
-- 
2.48.1

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Manivannan Sadhasivam]

On Tue, Mar 18, 2025 at 11:33:34AM +0100, Niklas Cassel wrote:
> The test cases for read/write/copy currently do:
> 1) ioctl(PCITEST_SET_IRQTYPE, MSI)
> 2) ioctl(PCITEST_{READ,WRITE,COPY})
> 
> This design is quite bad for a few reasons:
> -It assumes that all PCI EPCs support MSI.
> -One ioctl should be sufficient for these test cases.
> 
> Modify the PCITEST_{READ,WRITE,COPY} ioctls to set IRQ type automatically,
> overwriting the currently configured IRQ type. It there are no IRQ types
> supported in the CAPS register, fall back to MSI IRQs. This way the
> implementation is no worse than before this commit.
> 
> Any test case that requires a specific IRQ type, e.g. MSIX_TEST, will do
> an explicit PCITEST_SET_IRQTYPE ioctl at the start of the test case, thus
> it is safe to always overwrite the configured IRQ type.
> 

I don't quite understand this sentence. What if users wants to use a specific
IRQ type like MSI-X if the platform supports both MSI/MSI-X? That's why I wanted
to honor 'test->irq_type' if already set before READ,WRITE,COPY testcases.

Everything else looks good to me.

- Mani

> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/misc/pci_endpoint_test.c | 126 +++++++++++++++++--------------
>  1 file changed, 68 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3c04121d24733..cfaeeea7642ac 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -464,6 +464,62 @@ static int pci_endpoint_test_validate_xfer_params(struct device *dev,
>  	return 0;
>  }
>  
> +static int pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
> +{
> +	pci_endpoint_test_release_irq(test);
> +	pci_endpoint_test_free_irq_vectors(test);
> +
> +	return 0;
> +}
> +
> +static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
> +				      int req_irq_type)
> +{
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
> +	    req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
> +		dev_err(dev, "Invalid IRQ type option\n");
> +		return -EINVAL;
> +	}
> +
> +	if (test->irq_type == req_irq_type)
> +		return 0;
> +
> +	pci_endpoint_test_release_irq(test);
> +	pci_endpoint_test_free_irq_vectors(test);
> +
> +	ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_endpoint_test_request_irq(test);
> +	if (ret) {
> +		pci_endpoint_test_free_irq_vectors(test);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pci_endpoint_test_set_auto_irq(struct pci_endpoint_test *test,
> +					  int *irq_type)
> +{
> +	if (test->ep_caps & CAP_MSI)
> +		*irq_type = PCITEST_IRQ_TYPE_MSI;
> +	else if (test->ep_caps & CAP_MSIX)
> +		*irq_type = PCITEST_IRQ_TYPE_MSIX;
> +	else if (test->ep_caps & CAP_INTX)
> +		*irq_type = PCITEST_IRQ_TYPE_INTX;
> +	else
> +		/* fallback to MSI if no caps defined */
> +		*irq_type = PCITEST_IRQ_TYPE_MSI;
> +
> +	return pci_endpoint_test_set_irq(test, *irq_type);
> +}
> +
>  static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
>  				   unsigned long arg)
>  {
> @@ -483,7 +539,7 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
>  	dma_addr_t orig_dst_phys_addr;
>  	size_t offset;
>  	size_t alignment = test->alignment;
> -	int irq_type = test->irq_type;
> +	int irq_type;
>  	u32 src_crc32;
>  	u32 dst_crc32;
>  	int ret;
> @@ -504,11 +560,9 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
>  	if (use_dma)
>  		flags |= FLAG_USE_DMA;
>  
> -	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
> -	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
> -		dev_err(dev, "Invalid IRQ type option\n");
> -		return -EINVAL;
> -	}
> +	ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
> +	if (ret)
> +		return ret;
>  
>  	orig_src_addr = kzalloc(size + alignment, GFP_KERNEL);
>  	if (!orig_src_addr) {
> @@ -616,7 +670,7 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
>  	dma_addr_t orig_phys_addr;
>  	size_t offset;
>  	size_t alignment = test->alignment;
> -	int irq_type = test->irq_type;
> +	int irq_type;
>  	size_t size;
>  	u32 crc32;
>  	int ret;
> @@ -637,11 +691,9 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
>  	if (use_dma)
>  		flags |= FLAG_USE_DMA;
>  
> -	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
> -	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
> -		dev_err(dev, "Invalid IRQ type option\n");
> -		return -EINVAL;
> -	}
> +	ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
> +	if (ret)
> +		return ret;
>  
>  	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
>  	if (!orig_addr) {
> @@ -714,7 +766,7 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
>  	dma_addr_t orig_phys_addr;
>  	size_t offset;
>  	size_t alignment = test->alignment;
> -	int irq_type = test->irq_type;
> +	int irq_type;
>  	u32 crc32;
>  	int ret;
>  
> @@ -734,11 +786,9 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
>  	if (use_dma)
>  		flags |= FLAG_USE_DMA;
>  
> -	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
> -	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
> -		dev_err(dev, "Invalid IRQ type option\n");
> -		return -EINVAL;
> -	}
> +	ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
> +	if (ret)
> +		return ret;
>  
>  	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
>  	if (!orig_addr) {
> @@ -790,46 +840,6 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
>  	return ret;
>  }
>  
> -static int pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
> -{
> -	pci_endpoint_test_release_irq(test);
> -	pci_endpoint_test_free_irq_vectors(test);
> -
> -	return 0;
> -}
> -
> -static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
> -				      int req_irq_type)
> -{
> -	struct pci_dev *pdev = test->pdev;
> -	struct device *dev = &pdev->dev;
> -	int ret;
> -
> -	if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
> -	    req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
> -		dev_err(dev, "Invalid IRQ type option\n");
> -		return -EINVAL;
> -	}
> -
> -	if (test->irq_type == req_irq_type)
> -		return 0;
> -
> -	pci_endpoint_test_release_irq(test);
> -	pci_endpoint_test_free_irq_vectors(test);
> -
> -	ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
> -	if (ret)
> -		return ret;
> -
> -	ret = pci_endpoint_test_request_irq(test);
> -	if (ret) {
> -		pci_endpoint_test_free_irq_vectors(test);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  				    unsigned long arg)
>  {
> -- 
> 2.48.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

Hello Mani,

On Thu, Mar 20, 2025 at 08:57:32PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 18, 2025 at 11:33:34AM +0100, Niklas Cassel wrote:
> > The test cases for read/write/copy currently do:
> > 1) ioctl(PCITEST_SET_IRQTYPE, MSI)
> > 2) ioctl(PCITEST_{READ,WRITE,COPY})
> > 
> > This design is quite bad for a few reasons:
> > -It assumes that all PCI EPCs support MSI.
> > -One ioctl should be sufficient for these test cases.
> > 
> > Modify the PCITEST_{READ,WRITE,COPY} ioctls to set IRQ type automatically,
> > overwriting the currently configured IRQ type. It there are no IRQ types
> > supported in the CAPS register, fall back to MSI IRQs. This way the
> > implementation is no worse than before this commit.
> > 
> > Any test case that requires a specific IRQ type, e.g. MSIX_TEST, will do
> > an explicit PCITEST_SET_IRQTYPE ioctl at the start of the test case, thus
> > it is safe to always overwrite the configured IRQ type.
> > 
> 
> I don't quite understand this sentence.

What I was trying to say is that it is okay if PCITEST_{READ,WRITE,COPY}
ioctls always overwrite the configured IRQ type, because all test cases
in tools/testing/selftests/pci_endpoint/pci_endpoint_test.c that require
a specific IRQ type, e.g.:

- TEST_F(pci_ep_basic, LEGACY_IRQ_TEST)
  will call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_INTX);
  before calling pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0);

- TEST_F(pci_ep_basic, MSI_TEST)
  will call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
  before calling pci_ep_ioctl(PCITEST_MSI, i);

- TEST_F(pci_ep_basic, MSIX_TEST)
  will call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSIX);
  before calling pci_ep_ioctl(PCITEST_MSIX, i);

Thus, all test cases will still work, even if PCITEST_{READ,WRITE,COPY}
overwrites the configured IRQ type.


> What if users wants to use a specific
> IRQ type like MSI-X if the platform supports both MSI/MSI-X? That's why I wanted
> to honor 'test->irq_type' if already set before READ,WRITE,COPY testcases.

As I said, I don't think you can have the cake and eat it too ;)

Let me explain:
If you simply run pcitest, it will execute the test cases in the following
order:
1) pci_ep_bar.BARx.BAR_TEST
2) pci_ep_basic.CONSECUTIVE_BAR_TEST
3) pci_ep_basic.LEGACY_IRQ_TEST
4) pci_ep_basic.MSI_TEST
5) pci_ep_basic.MSIX_TEST
6) pci_ep_data_transfer.memcpy.READ_TEST
7) pci_ep_data_transfer.memcpy.WRITE_TEST
8) pci_ep_data_transfer.memcpy.COPY_TEST

Thus, when you reach 6) (READ_TEST), irq_type will already be set, and
READ_TEST will use whatever IRQ type that happened to be the last one that was
executed successfully. That unpredictability doesn't sound like very good
design to me.


To me, it sounds like what you want is actually what is already queued up on
pci/next.

Because with that, READ_TEST / WRITE_TEST / COPY_TEST test cases will always
call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
(If you want the behavior that the READ_TEST / WRITE_TEST / COPY_TEST case
should automatically figure out which IRQ type to use.)

If you want to use a specific IRQ type for READ_TEST / WRITE_TEST / COPY_TEST,
it should be trivial to write a new test case variant (or new test case), that
does SET_IRQ(WHICH_EVER_IRQ_TYPE_YOU_WANT); (depending on the test case variant)
followed by the ioctl() for the test itself.

This determinism is not possible if you move the "automatic IRQ selection"
logic to be within the PCITEST_{READ,WRITE,COPY} ioctls. (As this series does.)


I'm travelling to a conference this weekend, and will be busy all next week.

I've sent two proposals, what is currently queued up on pci/next, and this
series.

As you might guess, I think that IRQ_TYPE_AUTO is the most elegant solution
with regard to the existing design.

But, if you want to make changes before the merge window opens, feel free to:
-Take over this series; or
-Write your own series on top of what is on pci/next; or
-Keep pci/next as it is.


I'm really (honestly) happy with whatever solution, as long as we, once again,
handle EPCs that only support INTx, or only support MSI-X.

(Because ever since your patch series that migrated pcitest to selftests,
READ_TEST / WRITE_TEST / COPY_TEST test cases unconditionally use MSI, which
is a regression for EPCs that only support INTx, or only support MSI-X,
which is the whole reason why I wrote this series.)


Kind regards,
Niklas

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Manivannan Sadhasivam]

On Fri, Mar 21, 2025 at 02:27:34PM +0100, Niklas Cassel wrote:
> Hello Mani,
> 
> On Thu, Mar 20, 2025 at 08:57:32PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Mar 18, 2025 at 11:33:34AM +0100, Niklas Cassel wrote:
> > > The test cases for read/write/copy currently do:
> > > 1) ioctl(PCITEST_SET_IRQTYPE, MSI)
> > > 2) ioctl(PCITEST_{READ,WRITE,COPY})
> > > 
> > > This design is quite bad for a few reasons:
> > > -It assumes that all PCI EPCs support MSI.
> > > -One ioctl should be sufficient for these test cases.
> > > 
> > > Modify the PCITEST_{READ,WRITE,COPY} ioctls to set IRQ type automatically,
> > > overwriting the currently configured IRQ type. It there are no IRQ types
> > > supported in the CAPS register, fall back to MSI IRQs. This way the
> > > implementation is no worse than before this commit.
> > > 
> > > Any test case that requires a specific IRQ type, e.g. MSIX_TEST, will do
> > > an explicit PCITEST_SET_IRQTYPE ioctl at the start of the test case, thus
> > > it is safe to always overwrite the configured IRQ type.
> > > 
> > 
> > I don't quite understand this sentence.
> 
> What I was trying to say is that it is okay if PCITEST_{READ,WRITE,COPY}
> ioctls always overwrite the configured IRQ type, because all test cases
> in tools/testing/selftests/pci_endpoint/pci_endpoint_test.c that require
> a specific IRQ type, e.g.:
> 
> - TEST_F(pci_ep_basic, LEGACY_IRQ_TEST)
>   will call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_INTX);
>   before calling pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0);
> 
> - TEST_F(pci_ep_basic, MSI_TEST)
>   will call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
>   before calling pci_ep_ioctl(PCITEST_MSI, i);
> 
> - TEST_F(pci_ep_basic, MSIX_TEST)
>   will call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSIX);
>   before calling pci_ep_ioctl(PCITEST_MSIX, i);
> 
> Thus, all test cases will still work, even if PCITEST_{READ,WRITE,COPY}
> overwrites the configured IRQ type.
> 

Right, but those test cases are IRQ_TYPE specific. So setting the IRQ_TYPE
before executing the test is paramount.

> 
> > What if users wants to use a specific
> > IRQ type like MSI-X if the platform supports both MSI/MSI-X? That's why I wanted
> > to honor 'test->irq_type' if already set before READ,WRITE,COPY testcases.
> 
> As I said, I don't think you can have the cake and eat it too ;)
> 
> Let me explain:
> If you simply run pcitest, it will execute the test cases in the following
> order:
> 1) pci_ep_bar.BARx.BAR_TEST
> 2) pci_ep_basic.CONSECUTIVE_BAR_TEST
> 3) pci_ep_basic.LEGACY_IRQ_TEST
> 4) pci_ep_basic.MSI_TEST
> 5) pci_ep_basic.MSIX_TEST
> 6) pci_ep_data_transfer.memcpy.READ_TEST
> 7) pci_ep_data_transfer.memcpy.WRITE_TEST
> 8) pci_ep_data_transfer.memcpy.COPY_TEST
> 
> Thus, when you reach 6) (READ_TEST), irq_type will already be set, and
> READ_TEST will use whatever IRQ type that happened to be the last one that was
> executed successfully. That unpredictability doesn't sound like very good
> design to me.
> 

Not 'unpredictable'. As you correctly said, the test will end up using the 'IRQ
type that happened to be the last one that was executed successfully'. To me,
this is equivalent to having the IRQ_TYPE_AUTO set as the test will use the
IRQ_TYPE that is supported by the platform.

But having said that, I now tend to agree that there is value in keeping
IRQ_TYPE_AUTO also. More below.

> 
> To me, it sounds like what you want is actually what is already queued up on
> pci/next.
> 
> Because with that, READ_TEST / WRITE_TEST / COPY_TEST test cases will always
> call pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
> (If you want the behavior that the READ_TEST / WRITE_TEST / COPY_TEST case
> should automatically figure out which IRQ type to use.)
> 
> If you want to use a specific IRQ type for READ_TEST / WRITE_TEST / COPY_TEST,
> it should be trivial to write a new test case variant (or new test case), that
> does SET_IRQ(WHICH_EVER_IRQ_TYPE_YOU_WANT); (depending on the test case variant)
> followed by the ioctl() for the test itself.
> 

I think this new testcase could be simplified if we keep the IRQ_TYPE_AUTO.

> This determinism is not possible if you move the "automatic IRQ selection"
> logic to be within the PCITEST_{READ,WRITE,COPY} ioctls. (As this series does.)
> 
> 
> I'm travelling to a conference this weekend, and will be busy all next week.
> 
> I've sent two proposals, what is currently queued up on pci/next, and this
> series.
> 
> As you might guess, I think that IRQ_TYPE_AUTO is the most elegant solution
> with regard to the existing design.
> 

Now, I tend to agree.

> But, if you want to make changes before the merge window opens, feel free to:
> -Take over this series; or
> -Write your own series on top of what is on pci/next; or
> -Keep pci/next as it is.
> 
> 
> I'm really (honestly) happy with whatever solution, as long as we, once again,
> handle EPCs that only support INTx, or only support MSI-X.
> 

We will keep your old series as it is.

> (Because ever since your patch series that migrated pcitest to selftests,
> READ_TEST / WRITE_TEST / COPY_TEST test cases unconditionally use MSI, which
> is a regression for EPCs that only support INTx, or only support MSI-X,
> which is the whole reason why I wrote this series.)
> 

IMO, the regression could be simply fixed if you have removed the ASSERT_EQ from
READ/WRITE/COPY testcases.

But anyway, all good now. Thanks a lot for your patience in educating me :)
Really appreciated.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

On 22 March 2025 03:24:50 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>On Fri, Mar 21, 2025 at 02:27:34PM +0100, Niklas Cassel wrote:
>> 
>> I'm really (honestly) happy with whatever solution, as long as we, once again,
>> handle EPCs that only support INTx, or only support MSI-X.
>> 
>
>We will keep your old series as it is.
>
>> (Because ever since your patch series that migrated pcitest to selftests,
>> READ_TEST / WRITE_TEST / COPY_TEST test cases unconditionally use MSI, which
>> is a regression for EPCs that only support INTx, or only support MSI-X,
>> which is the whole reason why I wrote this series.)
>> 
>
>IMO, the regression could be simply fixed if you have removed the ASSERT_EQ from
>READ/WRITE/COPY testcases.
>
>But anyway, all good now. Thanks a lot for your patience in educating me :)
>Really appreciated.

Don't say it like that :)

I understand where you were coming from.
I think if we designed the API today, we would have kept it as one ioctl, and user space could have provided the IRQ type (and/or auto) as a struct member in the struct supplied in the ioctl.

But, considering that we already have PCITEST_SET_IRQTYPE as a separate ioctl, I think what is currently queued fits the current design the best, even if it is not a "real" IRQ type.


I still need to send a patch that fixes the kdoc.

BTW, can all Qcom platform raise INTx interrupts in EP mode?

Bjorn did not like that I added intx_capable to epc_features without having any platform that sets it to true. I'm quite sure that many platforms can raise INTx interrupts.
If all Qcom platforms can raise INTx interrupts,
then I could set intx_capable to true in epc_features in qcom-ep.c, to address Bjorns comment.


Kind regards,
Niklas

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Krzysztof Wilczyński]

Hello,

> >> I'm really (honestly) happy with whatever solution, as long as we, once again,
> >> handle EPCs that only support INTx, or only support MSI-X.
> >> 
> >
> >We will keep your old series as it is.
> >
> >> (Because ever since your patch series that migrated pcitest to selftests,
> >> READ_TEST / WRITE_TEST / COPY_TEST test cases unconditionally use MSI, which
> >> is a regression for EPCs that only support INTx, or only support MSI-X,
> >> which is the whole reason why I wrote this series.)
> >> 
> >
> >IMO, the regression could be simply fixed if you have removed the ASSERT_EQ from
> >READ/WRITE/COPY testcases.
> >
> >But anyway, all good now. Thanks a lot for your patience in educating me :)
> >Really appreciated.
> 
> Don't say it like that :)
> 
> I understand where you were coming from.
> I think if we designed the API today, we would have kept it as one ioctl, and user space could have provided the IRQ type (and/or auto) as a struct member in the struct supplied in the ioctl.

Nobody would object to a refactor, I believe.  Especially, where it makes
sense to fix some old technical debt.

[...]
> I still need to send a patch that fixes the kdoc.

Feel free to let me know what kernel-doc you want added there.  I will, in
the mean time, go ahead and add something.

Thank you!

	Krzysztof

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

Hello Krzysztof,

On Sun, Mar 23, 2025 at 08:34:49PM +0900, Krzysztof Wilczyński wrote:
> [...]
> > I still need to send a patch that fixes the kdoc.
> 
> Feel free to let me know what kernel-doc you want added there.  I will, in
> the mean time, go ahead and add something.

We already have:

 * @msi_capable: indicate if the endpoint function has MSI capability          
 * @msix_capable: indicate if the endpoint function has MSI-X capability

How about:

intx_capable: indicate if the endpoint can raise INTx interrupts


Kind regards,
Niklas

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Damien Le Moal]

On 3/25/25 03:20, Niklas Cassel wrote:
> Hello Krzysztof,
> 
> On Sun, Mar 23, 2025 at 08:34:49PM +0900, Krzysztof Wilczyński wrote:
>> [...]
>>> I still need to send a patch that fixes the kdoc.
>>
>> Feel free to let me know what kernel-doc you want added there.  I will, in
>> the mean time, go ahead and add something.
> 
> We already have:
> 
>  * @msi_capable: indicate if the endpoint function has MSI capability          
>  * @msix_capable: indicate if the endpoint function has MSI-X capability
> 
> How about:
> 
> intx_capable: indicate if the endpoint can raise INTx interrupts

It feels like we should just have a @irq_capability field that combines
PCI_IRQ_xxx flags to indicate the type(s) of IRQ supported by the controller.
That would be way cleaner than one boolean per type :)

> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Krzysztof Wilczyński]

Hello,

[...]
> > > I still need to send a patch that fixes the kdoc.
> > 
> > Feel free to let me know what kernel-doc you want added there.  I will, in
> > the mean time, go ahead and add something.
> 
> We already have:
> 
>  * @msi_capable: indicate if the endpoint function has MSI capability          
>  * @msix_capable: indicate if the endpoint function has MSI-X capability
> 
> How about:
> 
> intx_capable: indicate if the endpoint can raise INTx interrupts

Sounds good.  Thank you!

	Krzysztof

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

Hello Mani,

On 22 March 2025 01:31:44 GMT-04:00, Niklas Cassel <cassel@kernel.org> wrote:
>
>BTW, can all Qcom platform raise INTx interrupts in EP mode?
>
>Bjorn did not like that I added intx_capable to epc_features without having any platform that sets it to true. I'm quite sure that many platforms can raise INTx interrupts.
>If all Qcom platforms can raise INTx interrupts,
>then I could set intx_capable to true in epc_features in qcom-ep.c, to address Bjorns comment.

Can all Qcom platforms raise INTx in EP mode?


Kind regards,
Niklas

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Manivannan Sadhasivam]

On Wed, Mar 26, 2025 at 10:39:50AM -0400, Niklas Cassel wrote:
> 
> 
> Hello Mani,
> 
> On 22 March 2025 01:31:44 GMT-04:00, Niklas Cassel <cassel@kernel.org> wrote:
> >
> >BTW, can all Qcom platform raise INTx interrupts in EP mode?
> >
> >Bjorn did not like that I added intx_capable to epc_features without having any platform that sets it to true. I'm quite sure that many platforms can raise INTx interrupts.
> >If all Qcom platforms can raise INTx interrupts,
> >then I could set intx_capable to true in epc_features in qcom-ep.c, to address Bjorns comment.
> 
> Can all Qcom platforms raise INTx in EP mode?
> 

Yes, all Qcom platforms support INTx. But if we start setting the flag to true,
there is no need to set it to false as that would be the default value. So let's
just set 'true' for INTx capable platforms and assume others as not supported. I
know that you had added justification in the commit message, but I think we'd
have to drop the below commit:

PCI: dw-rockchip: Endpoint mode cannot raise INTx interrupts

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Niklas Cassel]

On Wed, Mar 26, 2025 at 09:47:18PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 26, 2025 at 10:39:50AM -0400, Niklas Cassel wrote:
> > 
> > Can all Qcom platforms raise INTx in EP mode?
> > 
> 
> Yes, all Qcom platforms support INTx. But if we start setting the flag to true,
> there is no need to set it to false as that would be the default value. So let's
> just set 'true' for INTx capable platforms and assume others as not supported. I
> know that you had added justification in the commit message, but I think we'd
> have to drop the below commit:
> 
> PCI: dw-rockchip: Endpoint mode cannot raise INTx interrupts

Well, with that logic, we should also remove the following:

$ git grep "msi_capable = false"
drivers/pci/controller/dwc/pcie-tegra194.c:     .msi_capable = false,

$ git grep "msix_capable = false"
drivers/pci/controller/dwc/pci-dra7xx.c:        .msix_capable = false,
drivers/pci/controller/dwc/pci-imx6.c:  .msix_capable = false,
drivers/pci/controller/dwc/pci-imx6.c:  .msix_capable = false,
drivers/pci/controller/dwc/pcie-artpec6.c:      .msix_capable = false,
drivers/pci/controller/dwc/pcie-qcom-ep.c:      .msix_capable = false,
drivers/pci/controller/dwc/pcie-rcar-gen4.c:    .msix_capable = false,
drivers/pci/controller/dwc/pcie-tegra194.c:     .msix_capable = false,
drivers/pci/controller/dwc/pcie-uniphier-ep.c:          .msix_capable = false,
drivers/pci/controller/dwc/pcie-uniphier-ep.c:          .msix_capable = false,
drivers/pci/controller/pcie-rcar-ep.c:  .msix_capable = false,
drivers/pci/controller/pcie-rockchip-ep.c:      .msix_capable = false,

Feel free to send patches that removes all:
{msi_capable,msix_capable,intx_capable}=false;

I will be happy to help out with reviews.

However, I'm slightly leaning towards thinking that there actually is some
value in _explicitly_ seeing that something is not supported.


Kind regards,
Niklas

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Bjorn Helgaas]

On Wed, Mar 26, 2025 at 08:22:08PM +0100, Niklas Cassel wrote:
> On Wed, Mar 26, 2025 at 09:47:18PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Mar 26, 2025 at 10:39:50AM -0400, Niklas Cassel wrote:
> > > 
> > > Can all Qcom platforms raise INTx in EP mode?
> > 
> > Yes, all Qcom platforms support INTx. But if we start setting the
> > flag to true, there is no need to set it to false as that would be
> > the default value. So let's just set 'true' for INTx capable
> > platforms and assume others as not supported. I know that you had
> > added justification in the commit message, but I think we'd have
> > to drop the below commit:
> > 
> > PCI: dw-rockchip: Endpoint mode cannot raise INTx interrupts
> 
> Well, with that logic, we should also remove the following:
> 
> $ git grep "msi_capable = false"
> drivers/pci/controller/dwc/pcie-tegra194.c:     .msi_capable = false,
> 
> $ git grep "msix_capable = false"
> drivers/pci/controller/dwc/pci-dra7xx.c:        .msix_capable = false,
> drivers/pci/controller/dwc/pci-imx6.c:  .msix_capable = false,
> drivers/pci/controller/dwc/pci-imx6.c:  .msix_capable = false,
> drivers/pci/controller/dwc/pcie-artpec6.c:      .msix_capable = false,
> drivers/pci/controller/dwc/pcie-qcom-ep.c:      .msix_capable = false,
> drivers/pci/controller/dwc/pcie-rcar-gen4.c:    .msix_capable = false,
> drivers/pci/controller/dwc/pcie-tegra194.c:     .msix_capable = false,
> drivers/pci/controller/dwc/pcie-uniphier-ep.c:          .msix_capable = false,
> drivers/pci/controller/dwc/pcie-uniphier-ep.c:          .msix_capable = false,
> drivers/pci/controller/pcie-rcar-ep.c:  .msix_capable = false,
> drivers/pci/controller/pcie-rockchip-ep.c:      .msix_capable = false,
> 
> Feel free to send patches that removes all:
> {msi_capable,msix_capable,intx_capable}=false;
> 
> I will be happy to help out with reviews.
> 
> However, I'm slightly leaning towards thinking that there actually is some
> value in _explicitly_ seeing that something is not supported.

IMO, this value is pretty weak.  I think we should only mention
features that *are* supported.  The universe of possibly supported
features is unbounded.  Suppose new hardware adds a new feature X.
Obviously we have to mark hardware that supports X.  But I do not want
to go back and mark all the old hardware as "not supporting X".

Bjorn

Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Manivannan Sadhasivam]

On Wed, Mar 26, 2025 at 08:22:08PM +0100, Niklas Cassel wrote:
> On Wed, Mar 26, 2025 at 09:47:18PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Mar 26, 2025 at 10:39:50AM -0400, Niklas Cassel wrote:
> > > 
> > > Can all Qcom platforms raise INTx in EP mode?
> > > 
> > 
> > Yes, all Qcom platforms support INTx. But if we start setting the flag to true,
> > there is no need to set it to false as that would be the default value. So let's
> > just set 'true' for INTx capable platforms and assume others as not supported. I
> > know that you had added justification in the commit message, but I think we'd
> > have to drop the below commit:
> > 
> > PCI: dw-rockchip: Endpoint mode cannot raise INTx interrupts
> 
> Well, with that logic, we should also remove the following:
> 
> $ git grep "msi_capable = false"
> drivers/pci/controller/dwc/pcie-tegra194.c:     .msi_capable = false,
> 
> $ git grep "msix_capable = false"
> drivers/pci/controller/dwc/pci-dra7xx.c:        .msix_capable = false,
> drivers/pci/controller/dwc/pci-imx6.c:  .msix_capable = false,
> drivers/pci/controller/dwc/pci-imx6.c:  .msix_capable = false,
> drivers/pci/controller/dwc/pcie-artpec6.c:      .msix_capable = false,
> drivers/pci/controller/dwc/pcie-qcom-ep.c:      .msix_capable = false,
> drivers/pci/controller/dwc/pcie-rcar-gen4.c:    .msix_capable = false,
> drivers/pci/controller/dwc/pcie-tegra194.c:     .msix_capable = false,
> drivers/pci/controller/dwc/pcie-uniphier-ep.c:          .msix_capable = false,
> drivers/pci/controller/dwc/pcie-uniphier-ep.c:          .msix_capable = false,
> drivers/pci/controller/pcie-rcar-ep.c:  .msix_capable = false,
> drivers/pci/controller/pcie-rockchip-ep.c:      .msix_capable = false,
> 
> Feel free to send patches that removes all:
> {msi_capable,msix_capable,intx_capable}=false;
> 

Sure. Will do once I get some spare cycles.

> I will be happy to help out with reviews.
> 
> However, I'm slightly leaning towards thinking that there actually is some
> value in _explicitly_ seeing that something is not supported.
> 

No. We should only set 'true' for capable controllers as 'false' is implied.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 4/4] selftests: pci_endpoint: Remove PCITEST_SET_IRQTYPE ioctls for read/write/copy

[Niklas Cassel]

The test cases for read/write/copy currently do:
1) ioctl(PCITEST_SET_IRQTYPE)
2) ioctl(PCITEST_{READ,WRITE,COPY})

The PCITEST_{READ,WRITE,COPY} ioctls now configure the IRQ type to use
themselves, ignoring any value configured by the user via
ioctl(PCITEST_SET_IRQTYPE).

Since PCITEST_{READ,WRITE,COPY} ioctls will ignore the IRQ type set by the
user, remove the ioctl(PCITEST_SET_IRQTYPE), as it has no effect on the
result of the test case.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 tools/testing/selftests/pci_endpoint/pci_endpoint_test.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
index fdf4bc6aa9d2a..e788ab4eb003e 100644
--- a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
+++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
@@ -181,9 +181,6 @@ TEST_F(pci_ep_data_transfer, READ_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
-
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
 		pci_ep_ioctl(PCITEST_READ, &param);
@@ -200,9 +197,6 @@ TEST_F(pci_ep_data_transfer, WRITE_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
-
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
 		pci_ep_ioctl(PCITEST_WRITE, &param);
@@ -219,9 +213,6 @@ TEST_F(pci_ep_data_transfer, COPY_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
-
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
 		pci_ep_ioctl(PCITEST_COPY, &param);
-- 
2.48.1