[PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID_IMX8
@ 2025-03-31 18:29 Frank Li
  2025-04-01 10:05 ` Niklas Cassel
  0 siblings, 1 reply; 4+ messages in thread
From: Frank Li @ 2025-03-31 18:29 UTC (permalink / raw)
  To: Manivannan Sadhasivam, Krzysztof Wilczyński,
	Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman,
	Kunihiko Hayashi, open list:PCI ENDPOINT SUBSYSTEM, open list
  Cc: imx

Ensure driver_data is set for PCI_DEVICE_ID_IMX8 to specify the IRQ type,
preventing probe failure.

Fixes the following error:
  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

Fixes: a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and 'no_msi'")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/misc/pci_endpoint_test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d294850a35a12..da96dba7357c6 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -1125,7 +1125,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x81c0),
 	  .driver_data = (kernel_ulong_t)&default_data,
 	},
-	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),},
+	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),
+	 .driver_data = (kernel_ulong_t)&default_data,
+	},
 	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_LS1088A),
 	  .driver_data = (kernel_ulong_t)&default_data,
 	},
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID_IMX8
  2025-03-31 18:29 [PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID_IMX8 Frank Li
@ 2025-04-01 10:05 ` Niklas Cassel
  2025-04-01 10:29   ` Niklas Cassel
  0 siblings, 1 reply; 4+ messages in thread
From: Niklas Cassel @ 2025-04-01 10:05 UTC (permalink / raw)
  To: Frank Li
  Cc: Manivannan Sadhasivam, Krzysztof Wilczyński,
	Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman,
	Kunihiko Hayashi, open list:PCI ENDPOINT SUBSYSTEM, open list,
	imx

On Mon, Mar 31, 2025 at 02:29:10PM -0400, Frank Li wrote:
> Ensure driver_data is set for PCI_DEVICE_ID_IMX8 to specify the IRQ type,
> preventing probe failure.
> 
> Fixes the following error:
>   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
> 
> Fixes: a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and 'no_msi'")
> Cc: stable@vger.kernel.org
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d294850a35a12..da96dba7357c6 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -1125,7 +1125,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x81c0),
>  	  .driver_data = (kernel_ulong_t)&default_data,
>  	},
> -	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),},
> +	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),
> +	 .driver_data = (kernel_ulong_t)&default_data,
> +	},
>  	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_LS1088A),
>  	  .driver_data = (kernel_ulong_t)&default_data,
>  	},
> -- 
> 2.34.1
> 

So the problem appears to be that:
a402006d48a9 ("misc: pci_endpoint_test: Remove global 'irq_type' and 'no_msi'")

did:

@@ -939,15 +930,12 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
        test->pdev = pdev;
        test->irq_type = IRQ_TYPE_UNDEFINED;
 
-       if (no_msi)
-               irq_type = IRQ_TYPE_INTX;
-
        data = (struct pci_endpoint_test_data *)ent->driver_data;
        if (data) {
                test_reg_bar = data->test_reg_bar;
                test->test_reg_bar = test_reg_bar;
                test->alignment = data->alignment;
-               irq_type = data->irq_type;
+               test->irq_type = data->irq_type;
        }
 
        init_completion(&test->irq_raised);
@@ -969,7 +957,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 
        pci_set_master(pdev);
 
-       ret = pci_endpoint_test_alloc_irq_vectors(test, irq_type);
+       ret = pci_endpoint_test_alloc_irq_vectors(test, test->irq_type);


So pci_endpoint_test_probe() calls pci_endpoint_test_alloc_irq_vectors(),
but test->irq_type is only set if driver_data is defined.

You seem to only fix IMX8, but if we go with your solution, I think that
you should also update the other entries that do not specify driver_data:
        { 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),},


But... I suggest that we just remove the pci_endpoint_test_alloc_irq_vectors()
call from pci_endpoint_test_probe().

We know that all tests that requires IRQs will call ioctl(PCITEST_SET_IRQTYPE)
(which will call pci_endpoint_test_set_irq()), before doing the actual ioctl(),
and test->irq_type is by default set to PCITEST_IRQ_TYPE_UNDEFINED
(even if it is overriden with driver_data if it exists), so performing any irq
allocation in probe seems wrong to me.

Thus, I think we should just do:


diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d294850a35a12..c4e5e2c977be2 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,
 };
 
 /*




Or, if we want to keep allocating some kind of IRQ vector in probe(),
just to rule out totally broken platforms, I guess we could also do:

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d294850a35a12..ab2088c7937a7 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,7 +968,7 @@ 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);
+	ret = pci_endpoint_test_alloc_irq_vectors(test, PCITEST_IRQ_TYPE_AUTO);
 	if (ret)
 		goto err_disable_irq;
 
@@ -1092,23 +1090,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,
 };
 
 /*

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID_IMX8
  2025-04-01 10:05 ` Niklas Cassel
@ 2025-04-01 10:29   ` Niklas Cassel
  2025-04-01 13:56     ` Frank Li
  0 siblings, 1 reply; 4+ messages in thread
From: Niklas Cassel @ 2025-04-01 10:29 UTC (permalink / raw)
  To: Frank Li
  Cc: Manivannan Sadhasivam, Krzysztof Wilczyński,
	Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman,
	Kunihiko Hayashi, open list:PCI ENDPOINT SUBSYSTEM, open list,
	imx

On Tue, Apr 01, 2025 at 12:05:37PM +0200, Niklas Cassel wrote:
>
> But... I suggest that we just remove the pci_endpoint_test_alloc_irq_vectors()
> call from pci_endpoint_test_probe().

...

Solution 1.


> 
> Or, if we want to keep allocating some kind of IRQ vector in probe(),
> just to rule out totally broken platforms, I guess we could also do:

...

Solution 2.



Considering that this is a test driver, I actually think Solution 1 is better.
That way, even if the platform 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.


Kind regards,
Niklas

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID_IMX8
  2025-04-01 10:29   ` Niklas Cassel
@ 2025-04-01 13:56     ` Frank Li
  0 siblings, 0 replies; 4+ messages in thread
From: Frank Li @ 2025-04-01 13:56 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Manivannan Sadhasivam, Krzysztof Wilczyński,
	Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman,
	Kunihiko Hayashi, open list:PCI ENDPOINT SUBSYSTEM, open list,
	imx

On Tue, Apr 01, 2025 at 12:29:19PM +0200, Niklas Cassel wrote:
> On Tue, Apr 01, 2025 at 12:05:37PM +0200, Niklas Cassel wrote:
> >
> > But... I suggest that we just remove the pci_endpoint_test_alloc_irq_vectors()
> > call from pci_endpoint_test_probe().
>
> ...
>
> Solution 1.
>
>
> >
> > Or, if we want to keep allocating some kind of IRQ vector in probe(),
> > just to rule out totally broken platforms, I guess we could also do:
>
> ...
>
> Solution 2.
>
>
>
> Considering that this is a test driver, I actually think Solution 1 is better.
> That way, even if the platform 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.

Can you post patch directly, so I can test it.

Frank

>
>
> Kind regards,
> Niklas

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-04-01 13:56 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-31 18:29 [PATCH 1/1] misc: pci_endpoint_test: Set .driver_data for PCI_DEVICE_ID_IMX8 Frank Li
2025-04-01 10:05 ` Niklas Cassel
2025-04-01 10:29   ` Niklas Cassel
2025-04-01 13:56     ` Frank Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox