[PATCH 0/7] pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[PATCH 0/7] pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Niklas Cassel]

Hello all,

For the PCITEST_WRITE, PCITEST_READ, and PCITEST_COPY test cases,
tools/testing/selftests/pci_endpoint/pci_endpoint_test.c unconditionally
uses MSIs, even for EPC drivers that do not support MSI.
(E.g. an EPC could support only INTx, or only MSI-X.)

Thus, improve tools/testing/selftests/pci_endpoint/pci_endpoint_test.c to
use any supported IRQ type (by introducing a new IRQ type
PCITEST_IRQ_TYPE_AUTO).

Note that it is only the PCITEST_WRITE, PCITEST_READ, and PCITEST_COPY test
cases that will use this new IRQ type. (PCITEST_MSI, PCITEST_MSIX, and
PCITEST_LEGACY_IRQ actually test a specific IRQ type, so these test cases
must still use a specific IRQ type.)


Kind regards,
Niklas


Niklas Cassel (7):
  PCI: endpoint: pcitest: Add IRQ_TYPE_* defines to UAPI header
  misc: pci_endpoint_test: Use IRQ_TYPE_* defines from UAPI header
  selftests: pci_endpoint: Use IRQ_TYPE_* defines from UAPI header
  PCI: endpoint: Add intx_capable to epc_features
  PCI: dw-rockchip: EP mode cannot raise INTx interrupts
  PCI: endpoint: pci-epf-test: Expose supported IRQ types in CAPS
    register
  misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

 drivers/misc/pci_endpoint_test.c              | 69 +++++++++++--------
 drivers/pci/controller/dwc/pcie-dw-rockchip.c |  2 +
 drivers/pci/endpoint/functions/pci-epf-test.c | 12 ++++
 include/linux/pci-epc.h                       |  1 +
 include/uapi/linux/pcitest.h                  |  6 ++
 .../pci_endpoint/pci_endpoint_test.c          | 24 +++----
 6 files changed, 75 insertions(+), 39 deletions(-)

-- 
2.48.1

[PATCH 1/7] PCI: endpoint: pcitest: Add IRQ_TYPE_* defines to UAPI header

[Niklas Cassel]

These IRQ_TYPE_* defines are used by both drivers/misc/pci_endpoint_test.c
and tools/testing/selftests/pci_endpoint/pci_endpoint_test.c.

Considering that both the misc driver and the selftest already includes
the pcitest.h UAPI header, it makes sense for these IRQ_TYPE_* defines
to be located in the pcitest.h UAPI header.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 include/uapi/linux/pcitest.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index acd261f49866..304bf9be0f9a 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -23,6 +23,11 @@
 #define PCITEST_BARS		_IO('P', 0xa)
 #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
 
+#define PCITEST_IRQ_TYPE_UNDEFINED	-1
+#define PCITEST_IRQ_TYPE_INTX		0
+#define PCITEST_IRQ_TYPE_MSI		1
+#define PCITEST_IRQ_TYPE_MSIX		2
+
 #define PCITEST_FLAGS_USE_DMA	0x00000001
 
 struct pci_endpoint_test_xfer_param {
-- 
2.48.1

[PATCH 2/7] misc: pci_endpoint_test: Use IRQ_TYPE_* defines from UAPI header

[Niklas Cassel]

Use the IRQ_TYPE_* defines from the UAPI header rather than duplicating
these defines in the driver itself.

No functional change.

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

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index e3d9638b7a1b..849d730ba14d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -28,11 +28,6 @@
 
 #define DRV_MODULE_NAME				"pci-endpoint-test"
 
-#define IRQ_TYPE_UNDEFINED			-1
-#define IRQ_TYPE_INTX				0
-#define IRQ_TYPE_MSI				1
-#define IRQ_TYPE_MSIX				2
-
 #define PCI_ENDPOINT_TEST_MAGIC			0x0
 
 #define PCI_ENDPOINT_TEST_COMMAND		0x4
@@ -157,7 +152,7 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test)
 	struct pci_dev *pdev = test->pdev;
 
 	pci_free_irq_vectors(pdev);
-	test->irq_type = IRQ_TYPE_UNDEFINED;
+	test->irq_type = PCITEST_IRQ_TYPE_UNDEFINED;
 }
 
 static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test,
@@ -168,7 +163,7 @@ static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test,
 	struct device *dev = &pdev->dev;
 
 	switch (type) {
-	case IRQ_TYPE_INTX:
+	case PCITEST_IRQ_TYPE_INTX:
 		irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
 		if (irq < 0) {
 			dev_err(dev, "Failed to get Legacy interrupt\n");
@@ -176,7 +171,7 @@ static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test,
 		}
 
 		break;
-	case IRQ_TYPE_MSI:
+	case PCITEST_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");
@@ -184,7 +179,7 @@ static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test,
 		}
 
 		break;
-	case IRQ_TYPE_MSIX:
+	case PCITEST_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");
@@ -233,16 +228,16 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 
 fail:
 	switch (test->irq_type) {
-	case IRQ_TYPE_INTX:
+	case PCITEST_IRQ_TYPE_INTX:
 		dev_err(dev, "Failed to request IRQ %d for Legacy\n",
 			pci_irq_vector(pdev, i));
 		break;
-	case IRQ_TYPE_MSI:
+	case PCITEST_IRQ_TYPE_MSI:
 		dev_err(dev, "Failed to request IRQ %d for MSI %d\n",
 			pci_irq_vector(pdev, i),
 			i + 1);
 		break;
-	case IRQ_TYPE_MSIX:
+	case PCITEST_IRQ_TYPE_MSIX:
 		dev_err(dev, "Failed to request IRQ %d for MSI-X %d\n",
 			pci_irq_vector(pdev, i),
 			i + 1);
@@ -408,7 +403,7 @@ static int pci_endpoint_test_intx_irq(struct pci_endpoint_test *test)
 	u32 val;
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
-				 IRQ_TYPE_INTX);
+				 PCITEST_IRQ_TYPE_INTX);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 0);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
 				 COMMAND_RAISE_INTX_IRQ);
@@ -428,7 +423,8 @@ static int pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
 	int ret;
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
-				 msix ? IRQ_TYPE_MSIX : IRQ_TYPE_MSI);
+				 msix ? PCITEST_IRQ_TYPE_MSIX :
+				 PCITEST_IRQ_TYPE_MSI);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, msi_num);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
 				 msix ? COMMAND_RAISE_MSIX_IRQ :
@@ -504,7 +500,8 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
 	if (use_dma)
 		flags |= FLAG_USE_DMA;
 
-	if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) {
+	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
+	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
 		return -EINVAL;
 	}
@@ -636,7 +633,8 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
 	if (use_dma)
 		flags |= FLAG_USE_DMA;
 
-	if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) {
+	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
+	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
 		return -EINVAL;
 	}
@@ -732,7 +730,8 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
 	if (use_dma)
 		flags |= FLAG_USE_DMA;
 
-	if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) {
+	if (irq_type < PCITEST_IRQ_TYPE_INTX ||
+	    irq_type > PCITEST_IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
 		return -EINVAL;
 	}
@@ -802,7 +801,8 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	if (req_irq_type < IRQ_TYPE_INTX || req_irq_type > IRQ_TYPE_MSIX) {
+	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;
 	}
@@ -926,7 +926,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	test->test_reg_bar = 0;
 	test->alignment = 0;
 	test->pdev = pdev;
-	test->irq_type = IRQ_TYPE_UNDEFINED;
+	test->irq_type = PCITEST_IRQ_TYPE_UNDEFINED;
 
 	data = (struct pci_endpoint_test_data *)ent->driver_data;
 	if (data) {
@@ -1077,23 +1077,23 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
 static const struct pci_endpoint_test_data default_data = {
 	.test_reg_bar = BAR_0,
 	.alignment = SZ_4K,
-	.irq_type = IRQ_TYPE_MSI,
+	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 static const struct pci_endpoint_test_data am654_data = {
 	.test_reg_bar = BAR_2,
 	.alignment = SZ_64K,
-	.irq_type = IRQ_TYPE_MSI,
+	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 static const struct pci_endpoint_test_data j721e_data = {
 	.alignment = 256,
-	.irq_type = IRQ_TYPE_MSI,
+	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 static const struct pci_endpoint_test_data rk3588_data = {
 	.alignment = SZ_64K,
-	.irq_type = IRQ_TYPE_MSI,
+	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 /*
-- 
2.48.1

[PATCH 3/7] selftests: pci_endpoint: Use IRQ_TYPE_* defines from UAPI header

[Niklas Cassel]

In order to improve readability, use the IRQ_TYPE_* defines from the UAPI
header rather than using raw values.

No functional change.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 .../selftests/pci_endpoint/pci_endpoint_test.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 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 d05e107d0698..fdf4bc6aa9d2 100644
--- a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
+++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
@@ -99,11 +99,11 @@ TEST_F(pci_ep_basic, LEGACY_IRQ_TEST)
 {
 	int ret;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0);
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_INTX);
 	ASSERT_EQ(0, ret) TH_LOG("Can't set Legacy IRQ type");
 
 	pci_ep_ioctl(PCITEST_GET_IRQTYPE, 0);
-	ASSERT_EQ(0, ret) TH_LOG("Can't get Legacy IRQ type");
+	ASSERT_EQ(PCITEST_IRQ_TYPE_INTX, ret) TH_LOG("Can't get Legacy IRQ type");
 
 	pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0);
 	EXPECT_FALSE(ret) TH_LOG("Test failed for Legacy IRQ");
@@ -113,11 +113,11 @@ TEST_F(pci_ep_basic, MSI_TEST)
 {
 	int ret, i;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
 	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
 
 	pci_ep_ioctl(PCITEST_GET_IRQTYPE, 0);
-	ASSERT_EQ(1, ret) TH_LOG("Can't get MSI IRQ type");
+	ASSERT_EQ(PCITEST_IRQ_TYPE_MSI, ret) TH_LOG("Can't get MSI IRQ type");
 
 	for (i = 1; i <= 32; i++) {
 		pci_ep_ioctl(PCITEST_MSI, i);
@@ -129,11 +129,11 @@ TEST_F(pci_ep_basic, MSIX_TEST)
 {
 	int ret, i;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 2);
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSIX);
 	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI-X IRQ type");
 
 	pci_ep_ioctl(PCITEST_GET_IRQTYPE, 0);
-	ASSERT_EQ(2, ret) TH_LOG("Can't get MSI-X IRQ type");
+	ASSERT_EQ(PCITEST_IRQ_TYPE_MSIX, ret) TH_LOG("Can't get MSI-X IRQ type");
 
 	for (i = 1; i <= 2048; i++) {
 		pci_ep_ioctl(PCITEST_MSIX, i);
@@ -181,7 +181,7 @@ TEST_F(pci_ep_data_transfer, READ_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
+	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++) {
@@ -200,7 +200,7 @@ TEST_F(pci_ep_data_transfer, WRITE_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
+	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++) {
@@ -219,7 +219,7 @@ TEST_F(pci_ep_data_transfer, COPY_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
+	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++) {
-- 
2.48.1

[PATCH 4/7] PCI: endpoint: Add intx_capable to epc_features

[Niklas Cassel]

In struct pci_epc_features, an EPC driver can already specify if they
support MSI (by setting msi_capable) and MSI-X (by setting msix_capable).

Thus, for consistency, allow an EPC driver to specify if it supports
INTx interrupts as well (by setting intx_capable).

Since this struct is zero initialized, EPC drivers that want to claim
INTx support will need to set intx_capable to true.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 include/linux/pci-epc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 9970ae73c8df..5872652291cc 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -232,6 +232,7 @@ struct pci_epc_features {
 	unsigned int	linkup_notifier : 1;
 	unsigned int	msi_capable : 1;
 	unsigned int	msix_capable : 1;
+	unsigned int	intx_capable : 1;
 	struct	pci_epc_bar_desc bar[PCI_STD_NUM_BARS];
 	size_t	align;
 };
-- 
2.48.1

Re: [PATCH 4/7] PCI: endpoint: Add intx_capable to epc_features

[Bjorn Helgaas]

On Mon, Mar 10, 2025 at 12:10:21PM +0100, Niklas Cassel wrote:
> In struct pci_epc_features, an EPC driver can already specify if they
> support MSI (by setting msi_capable) and MSI-X (by setting msix_capable).
> 
> Thus, for consistency, allow an EPC driver to specify if it supports
> INTx interrupts as well (by setting intx_capable).
> 
> Since this struct is zero initialized, EPC drivers that want to claim
> INTx support will need to set intx_capable to true.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  include/linux/pci-epc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 9970ae73c8df..5872652291cc 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -232,6 +232,7 @@ struct pci_epc_features {
>  	unsigned int	linkup_notifier : 1;
>  	unsigned int	msi_capable : 1;
>  	unsigned int	msix_capable : 1;
> +	unsigned int	intx_capable : 1;

Kernel-doc warning:

  $ find include -name \*pci\* | xargs scripts/kernel-doc -none
  include/linux/pci-epc.h:239: warning: Function parameter or struct member 'intx_capable' not described in 'pci_epc_features'

I'm actually not sure why we merged this, since there's nothing in the
tree that sets intx_capable to anything other than false.  Maybe
there's something coming?

Bjorn

Re: [PATCH 4/7] PCI: endpoint: Add intx_capable to epc_features

[Niklas Cassel]

On 21 March 2025 22:50:34 CET, Bjorn Helgaas <helgaas@kernel.org> wrote:
>On Mon, Mar 10, 2025 at 12:10:21PM +0100, Niklas Cassel wrote:
>> In struct pci_epc_features, an EPC driver can already specify if they
>> support MSI (by setting msi_capable) and MSI-X (by setting msix_capable).
>> 
>> Thus, for consistency, allow an EPC driver to specify if it supports
>> INTx interrupts as well (by setting intx_capable).
>> 
>> Since this struct is zero initialized, EPC drivers that want to claim
>> INTx support will need to set intx_capable to true.
>> 
>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>> ---
>>  include/linux/pci-epc.h | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>> index 9970ae73c8df..5872652291cc 100644
>> --- a/include/linux/pci-epc.h
>> +++ b/include/linux/pci-epc.h
>> @@ -232,6 +232,7 @@ struct pci_epc_features {
>>  	unsigned int	linkup_notifier : 1;
>>  	unsigned int	msi_capable : 1;
>>  	unsigned int	msix_capable : 1;
>> +	unsigned int	intx_capable : 1;
>
>Kernel-doc warning:
>
>  $ find include -name \*pci\* | xargs scripts/kernel-doc -none
>  include/linux/pci-epc.h:239: warning: Function parameter or struct member 'intx_capable' not described in 'pci_epc_features'

I will send a fix.


>
>I'm actually not sure why we merged this, since there's nothing in the
>tree that sets intx_capable to anything other than false.  Maybe
>there's something coming?

See
https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint-test&id=5ce641b3ddb8aec9be4bea72e0cd97d2c0ff54a4


>
>Bjorn

Re: [PATCH 4/7] PCI: endpoint: Add intx_capable to epc_features

[Bjorn Helgaas]

On Fri, Mar 21, 2025 at 10:55:22PM +0100, Niklas Cassel wrote:
> On 21 March 2025 22:50:34 CET, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >On Mon, Mar 10, 2025 at 12:10:21PM +0100, Niklas Cassel wrote:
> >> In struct pci_epc_features, an EPC driver can already specify if they
> >> support MSI (by setting msi_capable) and MSI-X (by setting msix_capable).
> >> 
> >> Thus, for consistency, allow an EPC driver to specify if it supports
> >> INTx interrupts as well (by setting intx_capable).
> >> 
> >> Since this struct is zero initialized, EPC drivers that want to claim
> >> INTx support will need to set intx_capable to true.

> >> @@ -232,6 +232,7 @@ struct pci_epc_features {
> >>  	unsigned int	linkup_notifier : 1;
> >>  	unsigned int	msi_capable : 1;
> >>  	unsigned int	msix_capable : 1;
> >> +	unsigned int	intx_capable : 1;
> >
> >Kernel-doc warning:
> >
> >  $ find include -name \*pci\* | xargs scripts/kernel-doc -none
> >  include/linux/pci-epc.h:239: warning: Function parameter or struct member 'intx_capable' not described in 'pci_epc_features'
> 
> I will send a fix.

Thanks will watch for it.

> >I'm actually not sure why we merged this, since there's nothing in the
> >tree that sets intx_capable to anything other than false.  Maybe
> >there's something coming?
> 
> See
> https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint-test&id=5ce641b3ddb8aec9be4bea72e0cd97d2c0ff54a4

Yeah, I saw that, but there's no actual benefit since there's no
driver that sets "intx_capable = true".

Re: [PATCH 4/7] PCI: endpoint: Add intx_capable to epc_features

[Krzysztof Wilczyński]

Hello,

[...]
> > >>  	unsigned int	linkup_notifier : 1;
> > >>  	unsigned int	msi_capable : 1;
> > >>  	unsigned int	msix_capable : 1;
> > >> +	unsigned int	intx_capable : 1;
> > >
> > >Kernel-doc warning:
> > >
> > >  $ find include -name \*pci\* | xargs scripts/kernel-doc -none
> > >  include/linux/pci-epc.h:239: warning: Function parameter or struct member 'intx_capable' not described in 'pci_epc_features'
> > 
> > I will send a fix.
> 
> Thanks will watch for it.

The missing kernel-doc has been added, see:

  https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint-test&id=4b313c69a38e28b2f002198c3909fb553e9b0176

Hopefully, there should be no more warnings.

Thank you both!

	Krzysztof

[PATCH 5/7] PCI: dw-rockchip: EP mode cannot raise INTx interrupts

[Niklas Cassel]

Neither rk3568 or rk3588 supports INTx interrupts.

Since epc_features is zero initialized, this is strictly not needed.
However, setting intx_capable explicitly to false makes it more clear
that neither rk3568 or rk3588 supports INTx interrupts.

No functional change.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 7bf22146cfd1..c42766146e1e 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -298,6 +298,7 @@ static const struct pci_epc_features rockchip_pcie_epc_features_rk3568 = {
 	.linkup_notifier = true,
 	.msi_capable = true,
 	.msix_capable = true,
+	.intx_capable = false,
 	.align = SZ_64K,
 	.bar[BAR_0] = { .type = BAR_RESIZABLE, },
 	.bar[BAR_1] = { .type = BAR_RESIZABLE, },
@@ -318,6 +319,7 @@ static const struct pci_epc_features rockchip_pcie_epc_features_rk3588 = {
 	.linkup_notifier = true,
 	.msi_capable = true,
 	.msix_capable = true,
+	.intx_capable = false,
 	.align = SZ_64K,
 	.bar[BAR_0] = { .type = BAR_RESIZABLE, },
 	.bar[BAR_1] = { .type = BAR_RESIZABLE, },
-- 
2.48.1

[PATCH 6/7] PCI: endpoint: pci-epf-test: Expose supported IRQ types in CAPS register

[Niklas Cassel]

Expose the supported IRQ types in the CAPS register.

This way, the host side driver (drivers/misc/pci_endpoint_test.c) can know
which IRQ types that the endpoint supports.

The host side driver will make use of this information in a follow-up
commit.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 58ac19fcdd63..50eb4106369f 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -45,6 +45,9 @@
 #define TIMER_RESOLUTION		1
 
 #define CAP_UNALIGNED_ACCESS		BIT(0)
+#define CAP_MSI				BIT(1)
+#define CAP_MSIX			BIT(2)
+#define CAP_INTX			BIT(3)
 
 static struct workqueue_struct *kpcitest_workqueue;
 
@@ -774,6 +777,15 @@ static void pci_epf_test_set_capabilities(struct pci_epf *epf)
 	if (epc->ops->align_addr)
 		caps |= CAP_UNALIGNED_ACCESS;
 
+	if (epf_test->epc_features->msi_capable)
+		caps |= CAP_MSI;
+
+	if (epf_test->epc_features->msix_capable)
+		caps |= CAP_MSIX;
+
+	if (epf_test->epc_features->intx_capable)
+		caps |= CAP_INTX;
+
 	reg->caps = cpu_to_le32(caps);
 }
 
-- 
2.48.1

[PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Niklas Cassel]

For PCITEST_MSI we really want to set PCITEST_SET_IRQTYPE explicitly
to PCITEST_IRQ_TYPE_MSI, since we want to test if MSI works.

For PCITEST_MSIX we really want to set PCITEST_SET_IRQTYPE explicitly
to PCITEST_IRQ_TYPE_MSIX, since we want to test if MSI works.

For PCITEST_LEGACY_IRQ we really want to set PCITEST_SET_IRQTYPE explicitly
to PCITEST_IRQ_TYPE_INTX, since we want to test if INTx works.

However, for PCITEST_WRITE, PCITEST_READ, PCITEST_COPY, we really don't
care which IRQ type that is used, we just want to use a IRQ type that is
supported by the EPC.

The old behavior was to always use MSI for PCITEST_WRITE, PCITEST_READ,
PCITEST_COPY, was to always set IRQ type to MSI before doing the actual
test, however, there are EPC drivers that do not support MSI.

Add a new PCITEST_IRQ_TYPE_AUTO, that will use the CAPS register to see
which IRQ types the endpoint supports, and use one of the supported IRQ
types.

For backwards compatibility, if the endpoint does not expose any supported
IRQ type in the CAPS register, simply fallback to using MSI, as it was
unconditionally done before.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c              | 25 +++++++++++++++----
 include/uapi/linux/pcitest.h                  |  1 +
 .../pci_endpoint/pci_endpoint_test.c          | 12 ++++-----
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 849d730ba14d..d294850a35a1 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;
 };
 
@@ -802,11 +806,23 @@ 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_MSIX) {
+	    req_irq_type > PCITEST_IRQ_TYPE_AUTO) {
 		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;
 
@@ -892,13 +908,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;
 }
 
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index 304bf9be0f9a..d3aa8715a525 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -27,6 +27,7 @@
 #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 fdf4bc6aa9d2..ac26481d29d9 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_MSI);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO 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_MSI);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO 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_MSI);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
-- 
2.48.1

Re: [PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Manivannan Sadhasivam]

On Mon, Mar 10, 2025 at 12:10:24PM +0100, Niklas Cassel wrote:
> For PCITEST_MSI we really want to set PCITEST_SET_IRQTYPE explicitly
> to PCITEST_IRQ_TYPE_MSI, since we want to test if MSI works.
> 
> For PCITEST_MSIX we really want to set PCITEST_SET_IRQTYPE explicitly
> to PCITEST_IRQ_TYPE_MSIX, since we want to test if MSI works.
> 
> For PCITEST_LEGACY_IRQ we really want to set PCITEST_SET_IRQTYPE explicitly
> to PCITEST_IRQ_TYPE_INTX, since we want to test if INTx works.
> 
> However, for PCITEST_WRITE, PCITEST_READ, PCITEST_COPY, we really don't
> care which IRQ type that is used, we just want to use a IRQ type that is
> supported by the EPC.
> 
> The old behavior was to always use MSI for PCITEST_WRITE, PCITEST_READ,
> PCITEST_COPY, was to always set IRQ type to MSI before doing the actual
> test, however, there are EPC drivers that do not support MSI.
> 
> Add a new PCITEST_IRQ_TYPE_AUTO, that will use the CAPS register to see
> which IRQ types the endpoint supports, and use one of the supported IRQ
> types.
> 

If the intention is to let the test figure out the supported IRQ type, why can't
you move the logic to set the supported IRQ to
pci_endpoint_test_{copy/read/write} functions itself?

PCITEST_IRQ_TYPE_AUTO is not really an IRQ type. So adding it doesn't look right
to me.

- Mani

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

Re: [PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Niklas Cassel]

Hello Mani,

On Fri, Mar 14, 2025 at 06:15:48PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 10, 2025 at 12:10:24PM +0100, Niklas Cassel wrote:
> > For PCITEST_MSI we really want to set PCITEST_SET_IRQTYPE explicitly
> > to PCITEST_IRQ_TYPE_MSI, since we want to test if MSI works.
> > 
> > For PCITEST_MSIX we really want to set PCITEST_SET_IRQTYPE explicitly
> > to PCITEST_IRQ_TYPE_MSIX, since we want to test if MSI works.
> > 
> > For PCITEST_LEGACY_IRQ we really want to set PCITEST_SET_IRQTYPE explicitly
> > to PCITEST_IRQ_TYPE_INTX, since we want to test if INTx works.
> > 
> > However, for PCITEST_WRITE, PCITEST_READ, PCITEST_COPY, we really don't
> > care which IRQ type that is used, we just want to use a IRQ type that is
> > supported by the EPC.
> > 
> > The old behavior was to always use MSI for PCITEST_WRITE, PCITEST_READ,
> > PCITEST_COPY, was to always set IRQ type to MSI before doing the actual
> > test, however, there are EPC drivers that do not support MSI.
> > 
> > Add a new PCITEST_IRQ_TYPE_AUTO, that will use the CAPS register to see
> > which IRQ types the endpoint supports, and use one of the supported IRQ
> > types.
> > 
> 
> If the intention is to let the test figure out the supported IRQ type, why can't
> you move the logic to set the supported IRQ to
> pci_endpoint_test_{copy/read/write} functions itself?

If you look at how things were before your commit 392188bb0f6e ("selftests:
pci_endpoint: Migrate to Kselftest framework"):

echo "Read Tests"
echo

pcitest -i 1

pcitest -r -s 1
pcitest -r -s 1024
pcitest -r -s 1025
pcitest -r -s 1024000
pcitest -r -s 1024001
echo

echo "Write Tests"
echo

pcitest -w -s 1
pcitest -w -s 1024
pcitest -w -s 1025
pcitest -w -s 1024000
pcitest -w -s 1024001
echo

echo "Copy Tests"
echo

pcitest -c -s 1
pcitest -c -s 1024
pcitest -c -s 1025
pcitest -c -s 1024000
pcitest -c -s 1024001


All three test cases were using MSI.


However, there was no error handling, so for a platform only supporting
MSI-X, the call to "pcitest -i 1" could fail, and the tests were still
going to use MSI-X (or whatever supported by the platform), and pass.



After your commit 392188bb0f6e ("selftests: pci_endpoint: Migrate to
Kselftest framework"):


TEST_F(pci_ep_data_transfer, READ_TEST)
{
	struct pci_endpoint_test_xfer_param param = {};
	int ret, i;

	if (variant->use_dma)
		param.flags = PCITEST_FLAGS_USE_DMA;

	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
	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);
		EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
					 test_size[i]);
	}
}

TEST_F(pci_ep_data_transfer, WRITE_TEST)
{
	struct pci_endpoint_test_xfer_param param = {};
	int ret, i;

	if (variant->use_dma)
		param.flags = PCITEST_FLAGS_USE_DMA;

	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
	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);
		EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
					 test_size[i]);
	}
}

TEST_F(pci_ep_data_transfer, COPY_TEST)
{
	struct pci_endpoint_test_xfer_param param = {};
	int ret, i;

	if (variant->use_dma)
		param.flags = PCITEST_FLAGS_USE_DMA;

	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
	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);
		EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
					 test_size[i]);
	}
}


Each test case explicitly calls ioctl(PCITEST_SET_IRQTYPE, 1); to use MSI,
and unlike before, will fail the test case if PCITEST_SET_IRQTYPE fails.


Then take Kunihiko commit 9240c27c3fdd ("misc: pci_endpoint_test: Remove
global 'irq_type' and 'no_msi'")

Before your and Kunihiko commits, a platform that did set the kernel module
parameter 'irq_type' would, if 'pcitest -i 1' failed,  use the value set by
that kernel module parameter for the read/write/copy test cases.

There is no guarantee that an EPC supports MSI, it might support only legacy
and INTx, so I was trying to restore use cases that were previously working.



I guess one option would be to remove the
"pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);" calls from the test cases that you
added, and then let the test cases themselves set the proper irq_type in
the BAR register. But, wouldn't that be an API change? READ/WRITE/COPY
test ioctls have always respected the (a successful) PCITEST_SET_IRQTYPE,
now all of a sudden, they shouldn't?


Since your commit 392188bb0f6e: each test case calls PCITEST_SET_IRQTYPE,
and gives an error if the PCITEST_SET_IRQTYPE ioctl() fails.

See Kunihiko commit log:
"... all tests that use interrupts first call ioctl(SET_IRQTYPE)
to set "test->irq_type", then write the value of test->irq_type into
the register pointed by test_reg_bar, and request the interrupt to the
endpoint. The endpoint function driver, pci-epf-test, refers to the
register, and determine which type of interrupt to raise."

READ/WRITE/COPY test cases/ioctls use interrupts.


I guess we could modify the read/write/copy test cases to not call
ioctl(SET_IRQTYPE), and remove the verification that ioctl(SET_IRQTYPE)
succeded, and change the behavior from older kernel releases, and make
READ/WRITE/COPY ioctls from now on ignore the configured irq_type using
ioctl(SET_IRQTYPE).

(If the user is using a selftest binary that has the ioctl(SET_IRQTYPE)
and ioctl(SET_IRQTYPE) verification in these test cases, the IRQ_TYPE
will get changed, so the verification will pass, but the succeeding
ioctl will not use that irq_type.)

But...

Is that really simpler / less confusing that just adding IRQ_TYPE_AUTO,
to maintain the uniformity that all test cases that will trigger IRQs
call ioctl(SET_IRQTYPE) before the actual ioctl that will trigger IRQs?


Kind regards,
Niklas

Re: [PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Manivannan Sadhasivam]

On Fri, Mar 14, 2025 at 06:25:54PM +0100, Niklas Cassel wrote:
> Hello Mani,
> 
> On Fri, Mar 14, 2025 at 06:15:48PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 10, 2025 at 12:10:24PM +0100, Niklas Cassel wrote:
> > > For PCITEST_MSI we really want to set PCITEST_SET_IRQTYPE explicitly
> > > to PCITEST_IRQ_TYPE_MSI, since we want to test if MSI works.
> > > 
> > > For PCITEST_MSIX we really want to set PCITEST_SET_IRQTYPE explicitly
> > > to PCITEST_IRQ_TYPE_MSIX, since we want to test if MSI works.
> > > 
> > > For PCITEST_LEGACY_IRQ we really want to set PCITEST_SET_IRQTYPE explicitly
> > > to PCITEST_IRQ_TYPE_INTX, since we want to test if INTx works.
> > > 
> > > However, for PCITEST_WRITE, PCITEST_READ, PCITEST_COPY, we really don't
> > > care which IRQ type that is used, we just want to use a IRQ type that is
> > > supported by the EPC.
> > > 
> > > The old behavior was to always use MSI for PCITEST_WRITE, PCITEST_READ,
> > > PCITEST_COPY, was to always set IRQ type to MSI before doing the actual
> > > test, however, there are EPC drivers that do not support MSI.
> > > 
> > > Add a new PCITEST_IRQ_TYPE_AUTO, that will use the CAPS register to see
> > > which IRQ types the endpoint supports, and use one of the supported IRQ
> > > types.
> > > 
> > 
> > If the intention is to let the test figure out the supported IRQ type, why can't
> > you move the logic to set the supported IRQ to
> > pci_endpoint_test_{copy/read/write} functions itself?
> 
> If you look at how things were before your commit 392188bb0f6e ("selftests:
> pci_endpoint: Migrate to Kselftest framework"):
> 
> echo "Read Tests"
> echo
> 
> pcitest -i 1
> 
> pcitest -r -s 1
> pcitest -r -s 1024
> pcitest -r -s 1025
> pcitest -r -s 1024000
> pcitest -r -s 1024001
> echo
> 
> echo "Write Tests"
> echo
> 
> pcitest -w -s 1
> pcitest -w -s 1024
> pcitest -w -s 1025
> pcitest -w -s 1024000
> pcitest -w -s 1024001
> echo
> 
> echo "Copy Tests"
> echo
> 
> pcitest -c -s 1
> pcitest -c -s 1024
> pcitest -c -s 1025
> pcitest -c -s 1024000
> pcitest -c -s 1024001
> 
> 
> All three test cases were using MSI.
> 
> 
> However, there was no error handling, so for a platform only supporting
> MSI-X, the call to "pcitest -i 1" could fail, and the tests were still
> going to use MSI-X (or whatever supported by the platform), and pass.
> 
> 
> 
> After your commit 392188bb0f6e ("selftests: pci_endpoint: Migrate to
> Kselftest framework"):
> 
> 
> TEST_F(pci_ep_data_transfer, READ_TEST)
> {
> 	struct pci_endpoint_test_xfer_param param = {};
> 	int ret, i;
> 
> 	if (variant->use_dma)
> 		param.flags = PCITEST_FLAGS_USE_DMA;
> 
> 	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
> 	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);
> 		EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
> 					 test_size[i]);
> 	}
> }
> 
> TEST_F(pci_ep_data_transfer, WRITE_TEST)
> {
> 	struct pci_endpoint_test_xfer_param param = {};
> 	int ret, i;
> 
> 	if (variant->use_dma)
> 		param.flags = PCITEST_FLAGS_USE_DMA;
> 
> 	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
> 	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);
> 		EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
> 					 test_size[i]);
> 	}
> }
> 
> TEST_F(pci_ep_data_transfer, COPY_TEST)
> {
> 	struct pci_endpoint_test_xfer_param param = {};
> 	int ret, i;
> 
> 	if (variant->use_dma)
> 		param.flags = PCITEST_FLAGS_USE_DMA;
> 
> 	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
> 	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);
> 		EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)",
> 					 test_size[i]);
> 	}
> }
> 
> 
> Each test case explicitly calls ioctl(PCITEST_SET_IRQTYPE, 1); to use MSI,
> and unlike before, will fail the test case if PCITEST_SET_IRQTYPE fails.
> 

Thanks for spotting the breakage!

> 
> Then take Kunihiko commit 9240c27c3fdd ("misc: pci_endpoint_test: Remove
> global 'irq_type' and 'no_msi'")
> 
> Before your and Kunihiko commits, a platform that did set the kernel module
> parameter 'irq_type' would, if 'pcitest -i 1' failed,  use the value set by
> that kernel module parameter for the read/write/copy test cases.
> 
> There is no guarantee that an EPC supports MSI, it might support only legacy
> and INTx, so I was trying to restore use cases that were previously working.
> 
> 
> 
> I guess one option would be to remove the
> "pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);" calls from the test cases that you
> added, and then let the test cases themselves set the proper irq_type in
> the BAR register. But, wouldn't that be an API change? READ/WRITE/COPY
> test ioctls have always respected the (a successful) PCITEST_SET_IRQTYPE,
> now all of a sudden, they shouldn't?
> 

This makes no difference IMO. The previous behavior which you explained above,
ignored the result of 'pcitest -i 1'. And it was not user configurable. I think
the original intention was to use MSI for tests if available, else use whatever
the platform supports.

If you want to restore the original behavior, you should remove the ASSERT_EQ()
from READ/WRITE/COPY tests first. Then to ensure that the tests make use of the
supported IRQ type, you can have the logic in the READ/WRITE/COPY tests itself.
If test->irq_type != PCITEST_IRQ_TYPE_UNDEFINED, then just use whatever the
test->irq_type is. Otherwise, use whatever the platform supports.

I don't see a necessity to add a new IRQ_TYPE and then getting it passed to the
host, if the host can figure out the IRQ_TYPE on its own.

- Mani

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

Re: [PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Niklas Cassel]

On Tue, Mar 18, 2025 at 02:26:56PM +0530, Manivannan Sadhasivam wrote:
> > 
> > I guess one option would be to remove the
> > "pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);" calls from the test cases that you
> > added, and then let the test cases themselves set the proper irq_type in
> > the BAR register. But, wouldn't that be an API change? READ/WRITE/COPY
> > test ioctls have always respected the (a successful) PCITEST_SET_IRQTYPE,
> > now all of a sudden, they shouldn't?
> > 
> 
> This makes no difference IMO. The previous behavior which you explained above,
> ignored the result of 'pcitest -i 1'. And it was not user configurable. I think
> the original intention was to use MSI for tests if available, else use whatever
> the platform supports.
> 
> If you want to restore the original behavior, you should remove the ASSERT_EQ()
> from READ/WRITE/COPY tests first. Then to ensure that the tests make use of the
> supported IRQ type, you can have the logic in the READ/WRITE/COPY tests itself.
> If test->irq_type != PCITEST_IRQ_TYPE_UNDEFINED, then just use whatever the
> test->irq_type is. Otherwise, use whatever the platform supports.

I can submit a patch series that modifies PCITEST_{READ,WRITE,COPY} to always
figure out the IRQ type to use by themselves.

But you can't have the cake and eat it too.

Either PCITEST_{READ,WRITE,COPY} always ignores PCITEST_SET_IRQTYPE or
they don't always ignore PCITEST_SET_IRQTYPE.

Only ignoring it "if test->irq_type != PCITEST_IRQ_TYPE_UNDEFINED"
makes no sense IMO.


Kind regards,
Niklas

Re: [PATCH 7/7] misc: pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Niklas Cassel]

On Tue, Mar 18, 2025 at 10:45:18AM +0100, Niklas Cassel wrote:
> On Tue, Mar 18, 2025 at 02:26:56PM +0530, Manivannan Sadhasivam wrote:
> > > 
> > > I guess one option would be to remove the
> > > "pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);" calls from the test cases that you
> > > added, and then let the test cases themselves set the proper irq_type in
> > > the BAR register. But, wouldn't that be an API change? READ/WRITE/COPY
> > > test ioctls have always respected the (a successful) PCITEST_SET_IRQTYPE,
> > > now all of a sudden, they shouldn't?
> > > 
> > 
> > This makes no difference IMO. The previous behavior which you explained above,
> > ignored the result of 'pcitest -i 1'. And it was not user configurable. I think
> > the original intention was to use MSI for tests if available, else use whatever
> > the platform supports.
> > 
> > If you want to restore the original behavior, you should remove the ASSERT_EQ()
> > from READ/WRITE/COPY tests first. Then to ensure that the tests make use of the
> > supported IRQ type, you can have the logic in the READ/WRITE/COPY tests itself.
> > If test->irq_type != PCITEST_IRQ_TYPE_UNDEFINED, then just use whatever the
> > test->irq_type is. Otherwise, use whatever the platform supports.
> 
> I can submit a patch series that modifies PCITEST_{READ,WRITE,COPY} to always
> figure out the IRQ type to use by themselves.
> 
> But you can't have the cake and eat it too.
> 
> Either PCITEST_{READ,WRITE,COPY} always ignores PCITEST_SET_IRQTYPE or
> they don't always ignore PCITEST_SET_IRQTYPE.
> 
> Only ignoring it "if test->irq_type != PCITEST_IRQ_TYPE_UNDEFINED"
> makes no sense IMO.

Please have a look at:
https://lore.kernel.org/linux-pci/20250318103330.1840678-6-cassel@kernel.org/T/#t

I hope that it addresses your concerns.


Kind regards,
Niklas

Re: [PATCH 0/7] pci_endpoint_test: Add support for PCITEST_IRQ_TYPE_AUTO

[Krzysztof Wilczyński]

Hello,

> For the PCITEST_WRITE, PCITEST_READ, and PCITEST_COPY test cases,
> tools/testing/selftests/pci_endpoint/pci_endpoint_test.c unconditionally
> uses MSIs, even for EPC drivers that do not support MSI.
> (E.g. an EPC could support only INTx, or only MSI-X.)
> 
> Thus, improve tools/testing/selftests/pci_endpoint/pci_endpoint_test.c to
> use any supported IRQ type (by introducing a new IRQ type
> PCITEST_IRQ_TYPE_AUTO).
> 
> Note that it is only the PCITEST_WRITE, PCITEST_READ, and PCITEST_COPY test
> cases that will use this new IRQ type. (PCITEST_MSI, PCITEST_MSIX, and
> PCITEST_LEGACY_IRQ actually test a specific IRQ type, so these test cases
> must still use a specific IRQ type.)

Applied to endpoint-test, thank you!

	Krzysztof