| Message ID | 20250210075812.3900646-5-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Fix some issues related to an interrupt type in pci_endpoint_test | expand |
On Mon, Feb 10, 2025 at 04:58:11PM +0900, Kunihiko Hayashi wrote: > The global variable "irq_type" preserves the current value of > ioctl(GET_IRQTYPE), however, it's enough to use test->irq_type. > Remove the variable, and replace with test->irq_type. I think the commit message is missing the biggest point. ioctl(SET_IRQTYPE) sets test->irq_type. PCITEST_WRITE/PCITEST_READ/PCITEST_COPY all use test->irq_type when writing the PCI_ENDPOINT_TEST_IRQ_TYPE register in test_reg_bar. The endpoint function driver (pci-epf-test) will look at PCI_ENDPOINT_TEST_IRQ_TYPE register in test_reg_bar when determining which type of IRQ it should raise. This means that the kernel module parameter is basically useless, since it is unused if anyone has called ioctl(SET_IRQTYPE). Both the old pcitest.sh and the new pci_endpoint_test kselftest call ioctl(SET_IRQTYPE), so in practice the irq_type kernel module parameter is dead code. > > The ioctl(GET_IRQTYPE) returns an error if test->irq_type has > IRQ_TYPE_UNDEFINED. > > Suggested-by: Niklas Cassel <cassel@kernel.org> > Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > --- > drivers/misc/pci_endpoint_test.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index 6a0972e7674f..8d98cd18634d 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -100,10 +100,6 @@ static bool no_msi; > module_param(no_msi, bool, 0444); > MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); Considering that you are removing the irq_type kernel module parameter, it doesn't make sense to keep the no_msi kernel module parameter IMO. The exact same argument for why we are removing irq_type, can be made for no_msi. Kind regards, Niklas
Hi Niklas, On 2025/02/11 1:30, Niklas Cassel wrote: > On Mon, Feb 10, 2025 at 04:58:11PM +0900, Kunihiko Hayashi wrote: >> The global variable "irq_type" preserves the current value of >> ioctl(GET_IRQTYPE), however, it's enough to use test->irq_type. >> Remove the variable, and replace with test->irq_type. > > I think the commit message is missing the biggest point. > > ioctl(SET_IRQTYPE) sets test->irq_type. > PCITEST_WRITE/PCITEST_READ/PCITEST_COPY all use test->irq_type when > writing the PCI_ENDPOINT_TEST_IRQ_TYPE register in test_reg_bar. > > The endpoint function driver (pci-epf-test) will look at > PCI_ENDPOINT_TEST_IRQ_TYPE register in test_reg_bar when determining > which type of IRQ it should raise. > > This means that the kernel module parameter is basically useless, > since it is unused if anyone has called ioctl(SET_IRQTYPE). > > Both the old pcitest.sh and the new pci_endpoint_test kselftest call > ioctl(SET_IRQTYPE), so in practice the irq_type kernel module parameter > is dead code. Thank you for pointing out. Surely, global "irq_type" is only used for return value of ioctl(GET_IRQTYPE). It isn't used in the test case, and the test is completed with test->irq_type and the register. I'll add this point to the explanation. >> >> The ioctl(GET_IRQTYPE) returns an error if test->irq_type has >> IRQ_TYPE_UNDEFINED. >> >> Suggested-by: Niklas Cassel <cassel@kernel.org> >> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >> --- >> drivers/misc/pci_endpoint_test.c | 13 ++++--------- >> 1 file changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/misc/pci_endpoint_test.c > b/drivers/misc/pci_endpoint_test.c >> index 6a0972e7674f..8d98cd18634d 100644 >> --- a/drivers/misc/pci_endpoint_test.c >> +++ b/drivers/misc/pci_endpoint_test.c >> @@ -100,10 +100,6 @@ static bool no_msi; >> module_param(no_msi, bool, 0444); >> MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); > > Considering that you are removing the irq_type kernel module parameter, > it doesn't make sense to keep the no_msi kernel module parameter IMO. > > The exact same argument for why we are removing irq_type, can be made for > no_msi. Agreed. Even if chip doesn't have MSI, test->irq_type starts with IRQ_TYPE_UNDEFINED and will be changed with ioctl(SET_IRQTYPE). "no_msi" can also be removed. Thank you, --- Best Regards Kunihiko Hayashi
On Mon, Feb 10, 2025 at 05:30:42PM +0100, Niklas Cassel wrote: > On Mon, Feb 10, 2025 at 04:58:11PM +0900, Kunihiko Hayashi wrote: > > The global variable "irq_type" preserves the current value of > > ioctl(GET_IRQTYPE), however, it's enough to use test->irq_type. > > Remove the variable, and replace with test->irq_type. > > I think the commit message is missing the biggest point. > > ioctl(SET_IRQTYPE) sets test->irq_type. > PCITEST_WRITE/PCITEST_READ/PCITEST_COPY all use test->irq_type when > writing the PCI_ENDPOINT_TEST_IRQ_TYPE register in test_reg_bar. > > The endpoint function driver (pci-epf-test) will look at > PCI_ENDPOINT_TEST_IRQ_TYPE register in test_reg_bar when determining > which type of IRQ it should raise. > > This means that the kernel module parameter is basically useless, > since it is unused if anyone has called ioctl(SET_IRQTYPE). > > Both the old pcitest.sh and the new pci_endpoint_test kselftest call > ioctl(SET_IRQTYPE), so in practice the irq_type kernel module parameter > is dead code. > +1 > > > > > The ioctl(GET_IRQTYPE) returns an error if test->irq_type has > > IRQ_TYPE_UNDEFINED. > > > > Suggested-by: Niklas Cassel <cassel@kernel.org> > > Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > --- > > drivers/misc/pci_endpoint_test.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > index 6a0972e7674f..8d98cd18634d 100644 > > --- a/drivers/misc/pci_endpoint_test.c > > +++ b/drivers/misc/pci_endpoint_test.c > > @@ -100,10 +100,6 @@ static bool no_msi; > > module_param(no_msi, bool, 0444); > > MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); > > Considering that you are removing the irq_type kernel module parameter, > it doesn't make sense to keep the no_msi kernel module parameter IMO. > > The exact same argument for why we are removing irq_type, can be made for > no_msi. > Right. - Mani
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 6a0972e7674f..8d98cd18634d 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -100,10 +100,6 @@ static bool no_msi; module_param(no_msi, bool, 0444); MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); -static int irq_type = IRQ_TYPE_MSI; -module_param(irq_type, int, 0444); -MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)"); - enum pci_barno { BAR_0, BAR_1, @@ -829,7 +825,6 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test, return ret; } - irq_type = test->irq_type; return 0; } @@ -878,7 +873,7 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, ret = pci_endpoint_test_set_irq(test, arg); break; case PCITEST_GET_IRQTYPE: - ret = irq_type; + ret = test->irq_type; break; case PCITEST_CLEAR_IRQ: ret = pci_endpoint_test_clear_irq(test); @@ -936,14 +931,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, test->irq_type = IRQ_TYPE_UNDEFINED; if (no_msi) - irq_type = IRQ_TYPE_INTX; + test->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); @@ -965,7 +960,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); if (ret) goto err_disable_irq;
The global variable "irq_type" preserves the current value of ioctl(GET_IRQTYPE), however, it's enough to use test->irq_type. Remove the variable, and replace with test->irq_type. The ioctl(GET_IRQTYPE) returns an error if test->irq_type has IRQ_TYPE_UNDEFINED. Suggested-by: Niklas Cassel <cassel@kernel.org> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- drivers/misc/pci_endpoint_test.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)