| Message ID | 20250402085659.4033434-2-cassel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | misc: pci_endpoint_test: Defer IRQ allocation until ioctl(PCITEST_SET_IRQTYPE) | expand |
On Wed, Apr 02, 2025 at 10:57:00AM +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. > > 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 always > call ioctl(PCITEST_SET_IRQTYPE) Maybe my pcitest is too old. "pcitest -r" have not call ioctl(PCITEST_SET_IRQTYPE). I need run "pcitest -i 1" firstly. It'd better remove pcitest information because pcitest already was removed from git tree. and now pcitest always show NOT OKAY. > 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.) But you remove "irq_type" at driver_data, does it means PCITEST_IRQ_TYPE_AUTO will not be supported? Frank > > Fixes: a402006d48a9c ("misc: pci_endpoint_test: Remove global 'irq_type' and 'no_msi'") > 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 >
On 2 April 2025 22:01:16 CEST, Frank Li <Frank.li@nxp.com> wrote: >On Wed, Apr 02, 2025 at 10:57:00AM +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. >> >> 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 always >> call ioctl(PCITEST_SET_IRQTYPE) > >Maybe my pcitest is too old. "pcitest -r" have not call ioctl(PCITEST_SET_IRQTYPE). >I need run "pcitest -i 1" firstly. It'd better remove pcitest information >because pcitest already was removed from git tree. and now pcitest always >show NOT OKAY. If you are on an old version, the return value from the ioctls have been inverted by Mani: https://github.com/torvalds/linux/commit/f26d37ee9bda938e968d0e11ba1f8f1588b2a135 But you should use the pci endpoint selftest, or the pcitest.sh shell script. Both the pci endpoint selftest and the pcitest.sh shell script always do ioctl(PCITEST_SET_IRQTYPE) before doing a read/write/copy test. Like you said, pcitest and the matching pcitest.sh shell script have been removed, so I suggest using the selftest. > >> 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.) > >But you remove "irq_type" at driver_data, does it means PCITEST_IRQ_TYPE_AUTO >will not be supported? It is supported by the selftest. It is not supported by pcitest since it has been removed from the tree. driver_data was just specifying the default IRQ type, but that IRQ type was always overriden using ioctl(PCITEST_SET_IRQTYPE) before a read/write/copy/test by the pcitest.sh shell script and the selftest, so the IRQ type in driver_data was quite pointless. Kind regards, Niklas
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, }; /*
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 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'") Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/misc/pci_endpoint_test.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-)