[PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[Niklas Cassel]

Commit a402006d48a9 ("misc: pci_endpoint_test: Remove global 'irq_type'
and 'no_msi'") changed so that the default IRQ vector requested by
pci_endpoint_test_probe() was no longer the module param 'irq_type',
but instead test->irq_type. test->irq_type is by default
IRQ_TYPE_UNDEFINED (until someone calls ioctl(PCITEST_SET_IRQTYPE)).

However, the commit also changed so that after initializing test->irq_type
to IRQ_TYPE_UNDEFINED, it also overrides it with driver_data->irq_type, if
the PCI device and vendor ID provides driver_data.

This causes a regression for PCI device and vendor IDs that do not provide
driver_data, and the driver now fails to probe on such platforms.

Considering that the pci endpoint selftests and the old pcitest.sh always
call ioctl(PCITEST_SET_IRQTYPE) before performing any test that requires
IRQs, simply remove the allocation of IRQs in pci_endpoint_test_probe(),
and defer it until ioctl(PCITEST_SET_IRQTYPE) has been called.

A positive side effect of this is that even if the endpoint controller has
issues with IRQs, the user can do still do all the tests/ioctls() that do
not require working IRQs, e.g. PCITEST_BAR and PCITEST_BARS.

This also means that we can remove the now unused irq_type from
driver_data. The irq_type will always be the one configured by the user
using ioctl(PCITEST_SET_IRQTYPE). (A user that does not know, or care
which irq_type that is used, can use PCITEST_IRQ_TYPE_AUTO. This has
superseded the need for a default irq_type in driver_data.)

Fixes: a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and 'no_msi'")
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-and-tested-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d294850a35a1..c4e5e2c977be 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -122,7 +122,6 @@ struct pci_endpoint_test {
 struct pci_endpoint_test_data {
 	enum pci_barno test_reg_bar;
 	size_t alignment;
-	int irq_type;
 };
 
 static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
@@ -948,7 +947,6 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		test_reg_bar = data->test_reg_bar;
 		test->test_reg_bar = test_reg_bar;
 		test->alignment = data->alignment;
-		test->irq_type = data->irq_type;
 	}
 
 	init_completion(&test->irq_raised);
@@ -970,10 +968,6 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	ret = pci_endpoint_test_alloc_irq_vectors(test, test->irq_type);
-	if (ret)
-		goto err_disable_irq;
-
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
 		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
 			base = pci_ioremap_bar(pdev, bar);
@@ -1009,10 +1003,6 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		goto err_ida_remove;
 	}
 
-	ret = pci_endpoint_test_request_irq(test);
-	if (ret)
-		goto err_kfree_test_name;
-
 	pci_endpoint_test_get_capabilities(test);
 
 	misc_device = &test->miscdev;
@@ -1020,7 +1010,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	misc_device->name = kstrdup(name, GFP_KERNEL);
 	if (!misc_device->name) {
 		ret = -ENOMEM;
-		goto err_release_irq;
+		goto err_kfree_test_name;
 	}
 	misc_device->parent = &pdev->dev;
 	misc_device->fops = &pci_endpoint_test_fops;
@@ -1036,9 +1026,6 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 err_kfree_name:
 	kfree(misc_device->name);
 
-err_release_irq:
-	pci_endpoint_test_release_irq(test);
-
 err_kfree_test_name:
 	kfree(test->name);
 
@@ -1051,8 +1038,6 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 			pci_iounmap(pdev, test->bar[bar]);
 	}
 
-err_disable_irq:
-	pci_endpoint_test_free_irq_vectors(test);
 	pci_release_regions(pdev);
 
 err_disable_pdev:
@@ -1092,23 +1077,19 @@ 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 = PCITEST_IRQ_TYPE_MSI,
 };
 
 static const struct pci_endpoint_test_data am654_data = {
 	.test_reg_bar = BAR_2,
 	.alignment = SZ_64K,
-	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 static const struct pci_endpoint_test_data j721e_data = {
 	.alignment = 256,
-	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 static const struct pci_endpoint_test_data rk3588_data = {
 	.alignment = SZ_64K,
-	.irq_type = PCITEST_IRQ_TYPE_MSI,
 };
 
 /*
-- 
2.49.0

Re: [PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[Manivannan Sadhasivam]

On Wed, 16 Apr 2025 16:28:26 +0200, Niklas Cassel wrote:
> Commit a402006d48a9 ("misc: pci_endpoint_test: Remove global 'irq_type'
> and 'no_msi'") changed so that the default IRQ vector requested by
> pci_endpoint_test_probe() was no longer the module param 'irq_type',
> but instead test->irq_type. test->irq_type is by default
> IRQ_TYPE_UNDEFINED (until someone calls ioctl(PCITEST_SET_IRQTYPE)).
> 
> However, the commit also changed so that after initializing test->irq_type
> to IRQ_TYPE_UNDEFINED, it also overrides it with driver_data->irq_type, if
> the PCI device and vendor ID provides driver_data.
> 
> [...]

Applied, thanks!

[1/1] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)
      commit: 9d564bf7ab67ec326ec047d2d95087d8d888f9b1

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Re: [PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[Bjorn Helgaas]

On Sun, Apr 20, 2025 at 09:31:46AM +0530, Manivannan Sadhasivam wrote:
> On Wed, 16 Apr 2025 16:28:26 +0200, Niklas Cassel wrote:
> > Commit a402006d48a9 ("misc: pci_endpoint_test: Remove global 'irq_type'
> > and 'no_msi'") changed so that the default IRQ vector requested by
> > pci_endpoint_test_probe() was no longer the module param 'irq_type',
> > but instead test->irq_type. test->irq_type is by default
> > IRQ_TYPE_UNDEFINED (until someone calls ioctl(PCITEST_SET_IRQTYPE)).
> > 
> > However, the commit also changed so that after initializing test->irq_type
> > to IRQ_TYPE_UNDEFINED, it also overrides it with driver_data->irq_type, if
> > the PCI device and vendor ID provides driver_data.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)
>       commit: 9d564bf7ab67ec326ec047d2d95087d8d888f9b1

a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and
'no_msi'") appeared in v6.15-rc1, and apparently caused a regression
that some driver fails to probe on such platforms.

Since a402006d48a9c caused a regression and hasn't appeared in a
release yet, this sounds like a candidate for v6.15 via pci/for-linus?

I can move it if you agree.  I *would* like to have a little more
detail about the regression, e.g., the affected driver, so I can
justify merging this after the merge window.

Bjorn

Re: [PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[Niklas Cassel]

On Tue, Apr 22, 2025 at 12:53:19PM -0500, Bjorn Helgaas wrote:
> On Sun, Apr 20, 2025 at 09:31:46AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, 16 Apr 2025 16:28:26 +0200, Niklas Cassel wrote:
> > > Commit a402006d48a9 ("misc: pci_endpoint_test: Remove global 'irq_type'
> > > and 'no_msi'") changed so that the default IRQ vector requested by
> > > pci_endpoint_test_probe() was no longer the module param 'irq_type',
> > > but instead test->irq_type. test->irq_type is by default
> > > IRQ_TYPE_UNDEFINED (until someone calls ioctl(PCITEST_SET_IRQTYPE)).
> > > 
> > > However, the commit also changed so that after initializing test->irq_type
> > > to IRQ_TYPE_UNDEFINED, it also overrides it with driver_data->irq_type, if
> > > the PCI device and vendor ID provides driver_data.
> > > 
> > > [...]
> > 
> > Applied, thanks!
> > 
> > [1/1] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)
> >       commit: 9d564bf7ab67ec326ec047d2d95087d8d888f9b1
> 
> a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and
> 'no_msi'") appeared in v6.15-rc1, and apparently caused a regression
> that some driver fails to probe on such platforms.
> 
> Since a402006d48a9c caused a regression and hasn't appeared in a
> release yet, this sounds like a candidate for v6.15 via pci/for-linus?

I agree:
https://lore.kernel.org/linux-pci/Z_0mUhHzGqNrMBGg@ryzen/


> 
> I can move it if you agree.  I *would* like to have a little more
> detail about the regression, e.g., the affected driver, so I can
> justify merging this after the merge window.

All drivers without any driver_data defined, i.e.:
        { PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),},
        { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774A1),},
        { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774B1),},
        { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774C0),},
        { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774E1),},


For those platforms, without this patch, you will get:
  pci-endpoint-test 0001:01:00.0: Invalid IRQ type selected
  pci-endpoint-test 0001:01:00.0: probe with driver pci-endpoint-test failed with error -22


Kind regards,
Niklas

Re: [PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[Bjorn Helgaas]

On Wed, Apr 23, 2025 at 02:46:03PM +0200, Niklas Cassel wrote:
> On Tue, Apr 22, 2025 at 12:53:19PM -0500, Bjorn Helgaas wrote:
> > On Sun, Apr 20, 2025 at 09:31:46AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, 16 Apr 2025 16:28:26 +0200, Niklas Cassel wrote:
> > > > Commit a402006d48a9 ("misc: pci_endpoint_test: Remove global 'irq_type'
> > > > and 'no_msi'") changed so that the default IRQ vector requested by
> > > > pci_endpoint_test_probe() was no longer the module param 'irq_type',
> > > > but instead test->irq_type. test->irq_type is by default
> > > > IRQ_TYPE_UNDEFINED (until someone calls ioctl(PCITEST_SET_IRQTYPE)).
> > > > 
> > > > However, the commit also changed so that after initializing test->irq_type
> > > > to IRQ_TYPE_UNDEFINED, it also overrides it with driver_data->irq_type, if
> > > > the PCI device and vendor ID provides driver_data.
> > > > 
> > > > [...]
> > > 
> > > Applied, thanks!
> > > 
> > > [1/1] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)
> > >       commit: 9d564bf7ab67ec326ec047d2d95087d8d888f9b1
> > 
> > a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and
> > 'no_msi'") appeared in v6.15-rc1, and apparently caused a regression
> > that some driver fails to probe on such platforms.
> > 
> > Since a402006d48a9c caused a regression and hasn't appeared in a
> > release yet, this sounds like a candidate for v6.15 via pci/for-linus?
> 
> I agree:
> https://lore.kernel.org/linux-pci/Z_0mUhHzGqNrMBGg@ryzen/
> 
> > I can move it if you agree.  I *would* like to have a little more
> > detail about the regression, e.g., the affected driver, so I can
> > justify merging this after the merge window.
> 
> All drivers without any driver_data defined, i.e.:
>         { PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),},
>         { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774A1),},
>         { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774B1),},
>         { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774C0),},
>         { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A774E1),},
> 
> 
> For those platforms, without this patch, you will get:
>   pci-endpoint-test 0001:01:00.0: Invalid IRQ type selected
>   pci-endpoint-test 0001:01:00.0: probe with driver pci-endpoint-test failed with error -22

So I guess this means it's "only" the host side
pci_endpoint_test_driver that won't probe, and the problem only
happens on the host controllers listed above or controllers with
programmable Vendor/Device ID that are set to match one of these?

I cherry picked this patch to pci/for-linus for v6.15, thanks!

Mani, this was the only patch on pci/misc-endpoint, so I deleted the
branch.

Bjorn

Re: [PATCH RESEND] misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE)

[Niklas Cassel]

On 24 April 2025 00:19:20 CEST, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> 
>> For those platforms, without this patch, you will get:
>>   pci-endpoint-test 0001:01:00.0: Invalid IRQ type selected
>>   pci-endpoint-test 0001:01:00.0: probe with driver pci-endpoint-test failed with error -22
>
>So I guess this means it's "only" the host side
>pci_endpoint_test_driver that won't probe, and the problem only
>happens on the host controllers listed above or controllers with
>programmable Vendor/Device ID that are set to match one of these?

Correct!


Kind regards,
Niklas