[PATCH v3 0/5] Fix some issues related to an interrupt type in pci_endpoint_test

[PATCH v3 0/5] Fix some issues related to an interrupt type in pci_endpoint_test

[Kunihiko Hayashi]

This series solves some issues about global "irq_type" that is used for
indicating the current type for users.

In addition, avoid an unexpected warning that occur due to interrupts
remaining after displaying an error caused by devm_request_irq().

Patch 1-3 include fixes for stable kernels that have global "irq_type".
Patch 4-5 include improvements for the latest.

Changes since v2:
- Rebase to v6.14-rc1
- Update message to clarify, and add result of call trace (patch 1)
- Add Reviewed-by: lines (patch 2)
- Add new patch to remove global "irq_type" variable (patch 4)
- Add new patch to replace "devm" version of IRQ functions (patch 5)

Changes since v1:
- Divide original patch into two
- Add an error message example
- Add "pcitest" display example
- Add a patch to fix an interrupt remaining issue

Kunihiko Hayashi (5):
  misc: pci_endpoint_test: Avoid issue of interrupts remaining after
    request_irq error
  misc: pci_endpoint_test: Fix disyplaying irq_type after request_irq
    error
  misc: pci_endpoint_test: Fix irq_type to convey the correct type
  misc: pci_endpoint_test: Remove global irq_type
  misc: pci_endpoint_test: Do not use managed irq functions

 drivers/misc/pci_endpoint_test.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] misc: pci_endpoint_test: Avoid issue of interrupts remaining after request_irq error

[Kunihiko Hayashi]

After devm_request_irq() fails with error in
pci_endpoint_test_request_irq(), pci_endpoint_test_free_irq_vectors() is
called assuming that all IRQs have been released.

However some requested IRQs remain unreleased, so there are still
/proc/irq/* entries remaining and we encounters WARN() with the following
message:

    remove_proc_entry: removing non-empty directory 'irq/30', leaking at
    least 'pci-endpoint-test.0'
    WARNING: CPU: 0 PID: 202 at fs/proc/generic.c:719 remove_proc_entry
    +0x190/0x19c

And show the call trace that led to this issue:

    [   12.050005] Call trace:
    [   12.051226]  remove_proc_entry+0x190/0x19c (P)
    [   12.053448]  unregister_irq_proc+0xd0/0x104
    [   12.055541]  free_desc+0x4c/0xd0
    [   12.057155]  irq_free_descs+0x68/0x90
    [   12.058984]  irq_domain_free_irqs+0x15c/0x1bc
    [   12.061161]  msi_domain_free_locked.part.0+0x184/0x1d4
    [   12.063728]  msi_domain_free_irqs_all_locked+0x64/0x8c
    [   12.066296]  pci_msi_teardown_msi_irqs+0x48/0x54
    [   12.068604]  pci_free_msi_irqs+0x18/0x38
    [   12.070564]  pci_free_irq_vectors+0x64/0x8c
    [   12.072654]  pci_endpoint_test_ioctl+0x870/0x1068
    [   12.075006]  __arm64_sys_ioctl+0xb0/0xe8
    [   12.076967]  invoke_syscall+0x48/0x110
    [   12.078841]  el0_svc_common.constprop.0+0x40/0xe8
    [   12.081192]  do_el0_svc+0x20/0x2c
    [   12.082848]  el0_svc+0x30/0xd0
    [   12.084376]  el0t_64_sync_handler+0x144/0x168
    [   12.086553]  el0t_64_sync+0x198/0x19c
    [   12.088383] ---[ end trace 0000000000000000 ]---

To solve this issue, set the number of remaining IRQs to test->num_irqs
and release IRQs in advance by calling pci_endpoint_test_release_irq().

Cc: stable@vger.kernel.org
Fixes: e03327122e2c ("pci_endpoint_test: Add 2 ioctl commands")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/misc/pci_endpoint_test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d5ac71a49386..bbcccd425700 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -259,6 +259,9 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 		break;
 	}
 
+	test->num_irqs = i;
+	pci_endpoint_test_release_irq(test);
+
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v3 1/5] misc: pci_endpoint_test: Avoid issue of interrupts remaining after request_irq error

[Manivannan Sadhasivam]

On Mon, Feb 10, 2025 at 04:58:08PM +0900, Kunihiko Hayashi wrote:
> After devm_request_irq() fails with error in
> pci_endpoint_test_request_irq(), pci_endpoint_test_free_irq_vectors() is
> called assuming that all IRQs have been released.
> 
> However some requested IRQs remain unreleased, so there are still
> /proc/irq/* entries remaining and we encounters WARN() with the following

s/we encounters/this results in WARN()

> message:
> 
>     remove_proc_entry: removing non-empty directory 'irq/30', leaking at
>     least 'pci-endpoint-test.0'
>     WARNING: CPU: 0 PID: 202 at fs/proc/generic.c:719 remove_proc_entry
>     +0x190/0x19c
> 
> And show the call trace that led to this issue:

You can remove this backtrace.

> 
>     [   12.050005] Call trace:
>     [   12.051226]  remove_proc_entry+0x190/0x19c (P)
>     [   12.053448]  unregister_irq_proc+0xd0/0x104
>     [   12.055541]  free_desc+0x4c/0xd0
>     [   12.057155]  irq_free_descs+0x68/0x90
>     [   12.058984]  irq_domain_free_irqs+0x15c/0x1bc
>     [   12.061161]  msi_domain_free_locked.part.0+0x184/0x1d4
>     [   12.063728]  msi_domain_free_irqs_all_locked+0x64/0x8c
>     [   12.066296]  pci_msi_teardown_msi_irqs+0x48/0x54
>     [   12.068604]  pci_free_msi_irqs+0x18/0x38
>     [   12.070564]  pci_free_irq_vectors+0x64/0x8c
>     [   12.072654]  pci_endpoint_test_ioctl+0x870/0x1068
>     [   12.075006]  __arm64_sys_ioctl+0xb0/0xe8
>     [   12.076967]  invoke_syscall+0x48/0x110
>     [   12.078841]  el0_svc_common.constprop.0+0x40/0xe8
>     [   12.081192]  do_el0_svc+0x20/0x2c
>     [   12.082848]  el0_svc+0x30/0xd0
>     [   12.084376]  el0t_64_sync_handler+0x144/0x168
>     [   12.086553]  el0t_64_sync+0x198/0x19c
>     [   12.088383] ---[ end trace 0000000000000000 ]---
> 
> To solve this issue, set the number of remaining IRQs to test->num_irqs
> and release IRQs in advance by calling pci_endpoint_test_release_irq().
> 
> Cc: stable@vger.kernel.org
> Fixes: e03327122e2c ("pci_endpoint_test: Add 2 ioctl commands")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/misc/pci_endpoint_test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..bbcccd425700 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -259,6 +259,9 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
>  		break;
>  	}
>  
> +	test->num_irqs = i;
> +	pci_endpoint_test_release_irq(test);
> +
>  	return ret;
>  }
>  
> -- 
> 2.25.1
> 

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

Re: [PATCH v3 1/5] misc: pci_endpoint_test: Avoid issue of interrupts remaining after request_irq error

[Kunihiko Hayashi]

Hi Manivannan,

On 2025/02/15 2:21, Manivannan Sadhasivam wrote:
> On Mon, Feb 10, 2025 at 04:58:08PM +0900, Kunihiko Hayashi wrote:
>> After devm_request_irq() fails with error in
>> pci_endpoint_test_request_irq(), pci_endpoint_test_free_irq_vectors() is
>> called assuming that all IRQs have been released.
>>
>> However some requested IRQs remain unreleased, so there are still
>> /proc/irq/* entries remaining and we encounters WARN() with the following
> 
> s/we encounters/this results in WARN()

I see. I'll fix it.

>> message:
>>
>>      remove_proc_entry: removing non-empty directory 'irq/30', leaking at
>>      least 'pci-endpoint-test.0'
>>      WARNING: CPU: 0 PID: 202 at fs/proc/generic.c:719 remove_proc_entry
>>      +0x190/0x19c
>>
>> And show the call trace that led to this issue:
> 
> You can remove this backtrace.

I'll add more information instead.

> 
>>
>>      [   12.050005] Call trace:
>>      [   12.051226]  remove_proc_entry+0x190/0x19c (P)
>>      [   12.053448]  unregister_irq_proc+0xd0/0x104
>>      [   12.055541]  free_desc+0x4c/0xd0
>>      [   12.057155]  irq_free_descs+0x68/0x90
>>      [   12.058984]  irq_domain_free_irqs+0x15c/0x1bc
>>      [   12.061161]  msi_domain_free_locked.part.0+0x184/0x1d4
>>      [   12.063728]  msi_domain_free_irqs_all_locked+0x64/0x8c
>>      [   12.066296]  pci_msi_teardown_msi_irqs+0x48/0x54
>>      [   12.068604]  pci_free_msi_irqs+0x18/0x38
>>      [   12.070564]  pci_free_irq_vectors+0x64/0x8c
>>      [   12.072654]  pci_endpoint_test_ioctl+0x870/0x1068
>>      [   12.075006]  __arm64_sys_ioctl+0xb0/0xe8
>>      [   12.076967]  invoke_syscall+0x48/0x110
>>      [   12.078841]  el0_svc_common.constprop.0+0x40/0xe8
>>      [   12.081192]  do_el0_svc+0x20/0x2c
>>      [   12.082848]  el0_svc+0x30/0xd0
>>      [   12.084376]  el0t_64_sync_handler+0x144/0x168
>>      [   12.086553]  el0t_64_sync+0x198/0x19c
>>      [   12.088383] ---[ end trace 0000000000000000 ]---
>>
>> To solve this issue, set the number of remaining IRQs to test->num_irqs
>> and release IRQs in advance by calling pci_endpoint_test_release_irq().
>>
>> Cc: stable@vger.kernel.org
>> Fixes: e03327122e2c ("pci_endpoint_test: Add 2 ioctl commands")
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thank you,

---
Best Regards
Kunihiko Hayashi

[PATCH v3 2/5] misc: pci_endpoint_test: Fix disyplaying irq_type after request_irq error

[Kunihiko Hayashi]

There are two variables that indicate the interrupt type to be used
in the next test execution, global "irq_type" and test->irq_type.

The former is referenced from pci_endpoint_test_get_irq() to preserve
the current type for ioctl(PCITEST_GET_IRQTYPE).

In pci_endpoint_test_request_irq(), since this global variable is
referenced when an error occurs, the unintended error message is
displayed.

For example, the following message shows "MSI 3" even if the current
irq type becomes "MSI-X".

    # pcitest -i 2
    pci-endpoint-test 0000:01:00.0: Failed to request IRQ 30 for MSI 3
    SET IRQ TYPE TO MSI-X:          NOT OKAY

Fix this issue by using test->irq_type instead of global "irq_type".

Cc: stable@vger.kernel.org
Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype")
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/misc/pci_endpoint_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index bbcccd425700..f13fa32ef91a 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -242,7 +242,7 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 	return 0;
 
 fail:
-	switch (irq_type) {
+	switch (test->irq_type) {
 	case IRQ_TYPE_INTX:
 		dev_err(dev, "Failed to request IRQ %d for Legacy\n",
 			pci_irq_vector(pdev, i));
-- 
2.25.1

[PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Kunihiko Hayashi]

There are two variables that indicate the interrupt type to be used
in the next test execution, "irq_type" as global and test->irq_type.

The global is referenced from pci_endpoint_test_get_irq() to preserve
the current type for ioctl(PCITEST_GET_IRQTYPE).

The type set in this function isn't reflected in the global "irq_type",
so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
As a result, the wrong type will be displayed in "pcitest" as follows:

    # pcitest -i 0
    SET IRQ TYPE TO LEGACY:         OKAY
    # pcitest -I
    GET IRQ TYPE:           MSI

Fix this issue by propagating the current type to the global "irq_type".

Cc: stable@vger.kernel.org
Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/misc/pci_endpoint_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index f13fa32ef91a..6a0972e7674f 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -829,6 +829,7 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 		return ret;
 	}
 
+	irq_type = test->irq_type;
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Niklas Cassel]

On Mon, Feb 10, 2025 at 04:58:10PM +0900, Kunihiko Hayashi wrote:
> There are two variables that indicate the interrupt type to be used
> in the next test execution, "irq_type" as global and test->irq_type.
> 
> The global is referenced from pci_endpoint_test_get_irq() to preserve
> the current type for ioctl(PCITEST_GET_IRQTYPE).
> 
> The type set in this function isn't reflected in the global "irq_type",
> so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
> As a result, the wrong type will be displayed in "pcitest" as follows:
> 
>     # pcitest -i 0
>     SET IRQ TYPE TO LEGACY:         OKAY
>     # pcitest -I
>     GET IRQ TYPE:           MSI
> 
> Fix this issue by propagating the current type to the global "irq_type".
> 
> Cc: stable@vger.kernel.org
> Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index f13fa32ef91a..6a0972e7674f 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -829,6 +829,7 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
>  		return ret;
>  	}
>  
> +	irq_type = test->irq_type;

It feels a bit silly to add this line, when you remove this exact line in
the next patch. Perhaps just drop this patch?


Kind regards,
Niklas

Re: [PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Kunihiko Hayashi]

Hi Niklas,

On 2025/02/11 1:01, Niklas Cassel wrote:
> On Mon, Feb 10, 2025 at 04:58:10PM +0900, Kunihiko Hayashi wrote:
>> There are two variables that indicate the interrupt type to be used
>> in the next test execution, "irq_type" as global and test->irq_type.
>>
>> The global is referenced from pci_endpoint_test_get_irq() to preserve
>> the current type for ioctl(PCITEST_GET_IRQTYPE).
>>
>> The type set in this function isn't reflected in the global "irq_type",
>> so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
>> As a result, the wrong type will be displayed in "pcitest" as follows:
>>
>>      # pcitest -i 0
>>      SET IRQ TYPE TO LEGACY:         OKAY
>>      # pcitest -I
>>      GET IRQ TYPE:           MSI
>>
>> Fix this issue by propagating the current type to the global "irq_type".
>>
>> Cc: stable@vger.kernel.org
>> Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module
> parameter to determine irqtype")
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>   drivers/misc/pci_endpoint_test.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/misc/pci_endpoint_test.c
> b/drivers/misc/pci_endpoint_test.c
>> index f13fa32ef91a..6a0972e7674f 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -829,6 +829,7 @@ static int pci_endpoint_test_set_irq(struct
> pci_endpoint_test *test,
>>   		return ret;
>>   	}
>>   
>> +	irq_type = test->irq_type;
> 
> It feels a bit silly to add this line, when you remove this exact line in
> the next patch. Perhaps just drop this patch?

This fix will be removed in patch 4/5, so it seems no means.
However, there is an issue in the stable version, as mentioned in the
commit message, so I fix it here.

I'll treat it separately if necessary.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Niklas Cassel]

On Thu, Feb 13, 2025 at 07:21:45PM +0900, Kunihiko Hayashi wrote:
> Hi Niklas,
> 
> On 2025/02/11 1:01, Niklas Cassel wrote:
> > On Mon, Feb 10, 2025 at 04:58:10PM +0900, Kunihiko Hayashi wrote:
> > > There are two variables that indicate the interrupt type to be used
> > > in the next test execution, "irq_type" as global and test->irq_type.
> > > 
> > > The global is referenced from pci_endpoint_test_get_irq() to preserve
> > > the current type for ioctl(PCITEST_GET_IRQTYPE).
> > > 
> > > The type set in this function isn't reflected in the global "irq_type",
> > > so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
> > > As a result, the wrong type will be displayed in "pcitest" as follows:
> > > 
> > >      # pcitest -i 0
> > >      SET IRQ TYPE TO LEGACY:         OKAY
> > >      # pcitest -I
> > >      GET IRQ TYPE:           MSI
> > > 
> > > Fix this issue by propagating the current type to the global "irq_type".
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module
> > parameter to determine irqtype")
> > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > ---
> > >   drivers/misc/pci_endpoint_test.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/misc/pci_endpoint_test.c
> > b/drivers/misc/pci_endpoint_test.c
> > > index f13fa32ef91a..6a0972e7674f 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -829,6 +829,7 @@ static int pci_endpoint_test_set_irq(struct
> > pci_endpoint_test *test,
> > >   		return ret;
> > >   	}
> > > +	irq_type = test->irq_type;
> > 
> > It feels a bit silly to add this line, when you remove this exact line in
> > the next patch. Perhaps just drop this patch?
> 
> This fix will be removed in patch 4/5, so it seems no means.
> However, there is an issue in the stable version, as mentioned in the
> commit message, so I fix it here.

Ok. Having a small fix first that can be backported, which is then followed
by a bigger cleanup, makes sense in this case.

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Manivannan Sadhasivam]

On Mon, Feb 10, 2025 at 04:58:10PM +0900, Kunihiko Hayashi wrote:
> There are two variables that indicate the interrupt type to be used
> in the next test execution, "irq_type" as global and test->irq_type.
> 
> The global is referenced from pci_endpoint_test_get_irq() to preserve
> the current type for ioctl(PCITEST_GET_IRQTYPE).
> 
> The type set in this function isn't reflected in the global "irq_type",
> so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
> As a result, the wrong type will be displayed in "pcitest" as follows:
> 
>     # pcitest -i 0
>     SET IRQ TYPE TO LEGACY:         OKAY
>     # pcitest -I
>     GET IRQ TYPE:           MSI
> 

Could you please post the failure with kselftest that got merged into v6.14-rc1?

> Fix this issue by propagating the current type to the global "irq_type".
> 
> Cc: stable@vger.kernel.org
> Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index f13fa32ef91a..6a0972e7674f 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -829,6 +829,7 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
>  		return ret;
>  	}
>  
> +	irq_type = test->irq_type;
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 

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

Re: [PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Kunihiko Hayashi]

Hi Manivannan,

On 2025/02/15 2:25, Manivannan Sadhasivam wrote:
> On Mon, Feb 10, 2025 at 04:58:10PM +0900, Kunihiko Hayashi wrote:
>> There are two variables that indicate the interrupt type to be used
>> in the next test execution, "irq_type" as global and test->irq_type.
>>
>> The global is referenced from pci_endpoint_test_get_irq() to preserve
>> the current type for ioctl(PCITEST_GET_IRQTYPE).
>>
>> The type set in this function isn't reflected in the global "irq_type",
>> so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
>> As a result, the wrong type will be displayed in "pcitest" as follows:
>>
>>      # pcitest -i 0
>>      SET IRQ TYPE TO LEGACY:         OKAY
>>      # pcitest -I
>>      GET IRQ TYPE:           MSI
>>
> 
> Could you please post the failure with kselftest that got merged into
> v6.14-rc1?

The kselftest doesn't call GET_IRQTYPE, so current kselftest doesn't fail.

If necessary, I can add GET_IRQTYPE test after SET_IRQTYPE of each
interrupt test prior to this patch.

         pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0);
         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");

However, pci_ep_ioctl() returns zero if OK, the return value should be
changed to the actual return value.

  #define pci_ep_ioctl(cmd, arg)                 \
  ({                                             \
         ret = ioctl(self->fd, cmd, arg);        \
-       ret = ret < 0 ? -errno : 0;             \
+       ret = ret < 0 ? -errno : ret;           \
  })

Before applying the patch, this test fails.

#  RUN           pci_ep_basic.LEGACY_IRQ_TEST ...
# pci_endpoint_test.c:104:LEGACY_IRQ_TEST:Expected 0 (0) == ret (1)
# pci_endpoint_test.c:104:LEGACY_IRQ_TEST:Can't get Legacy IRQ type
# LEGACY_IRQ_TEST: Test terminated by assertion
#          FAIL  pci_ep_basic.LEGACY_IRQ_TEST

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v3 3/5] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Manivannan Sadhasivam]

On Mon, Feb 17, 2025 at 08:26:44PM +0900, Kunihiko Hayashi wrote:
> Hi Manivannan,
> 
> On 2025/02/15 2:25, Manivannan Sadhasivam wrote:
> > On Mon, Feb 10, 2025 at 04:58:10PM +0900, Kunihiko Hayashi wrote:
> > > There are two variables that indicate the interrupt type to be used
> > > in the next test execution, "irq_type" as global and test->irq_type.
> > > 
> > > The global is referenced from pci_endpoint_test_get_irq() to preserve
> > > the current type for ioctl(PCITEST_GET_IRQTYPE).
> > > 
> > > The type set in this function isn't reflected in the global "irq_type",
> > > so ioctl(PCITEST_GET_IRQTYPE) returns the previous type.
> > > As a result, the wrong type will be displayed in "pcitest" as follows:
> > > 
> > >      # pcitest -i 0
> > >      SET IRQ TYPE TO LEGACY:         OKAY
> > >      # pcitest -I
> > >      GET IRQ TYPE:           MSI
> > > 
> > 
> > Could you please post the failure with kselftest that got merged into
> > v6.14-rc1?
> 
> The kselftest doesn't call GET_IRQTYPE, so current kselftest doesn't fail.
> 
> If necessary, I can add GET_IRQTYPE test after SET_IRQTYPE of each
> interrupt test prior to this patch.
> 
>         pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0);
>         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");
> 

Sure.

> However, pci_ep_ioctl() returns zero if OK, the return value should be
> changed to the actual return value.
> 
>  #define pci_ep_ioctl(cmd, arg)                 \
>  ({                                             \
>         ret = ioctl(self->fd, cmd, arg);        \
> -       ret = ret < 0 ? -errno : 0;             \
> +       ret = ret < 0 ? -errno : ret;           \
>  })
> 

Ok.

> Before applying the patch, this test fails.
> 
> #  RUN           pci_ep_basic.LEGACY_IRQ_TEST ...
> # pci_endpoint_test.c:104:LEGACY_IRQ_TEST:Expected 0 (0) == ret (1)
> # pci_endpoint_test.c:104:LEGACY_IRQ_TEST:Can't get Legacy IRQ type
> # LEGACY_IRQ_TEST: Test terminated by assertion
> #          FAIL  pci_ep_basic.LEGACY_IRQ_TEST
> 

Looks good to me.

- Mani

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

[PATCH v3 4/5] misc: pci_endpoint_test: Remove global irq_type

[Kunihiko Hayashi]

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(-)

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;
 
-- 
2.25.1

Re: [PATCH v3 4/5] misc: pci_endpoint_test: Remove global irq_type

[Niklas Cassel]

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

Re: [PATCH v3 4/5] misc: pci_endpoint_test: Remove global irq_type

[Kunihiko Hayashi]

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

Re: [PATCH v3 4/5] misc: pci_endpoint_test: Remove global irq_type

[Manivannan Sadhasivam]

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

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

[PATCH v3 5/5] misc: pci_endpoint_test: Do not use managed irq functions

[Kunihiko Hayashi]

The pci_endpoint_test_request_irq() and pci_endpoint_test_release_irq()
are called repeatedly by the users through pci_endpoint_test_set_irq().
So using the managed version of IRQ functions within these functions
has no effect.

Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/misc/pci_endpoint_test.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 8d98cd18634d..9465d2ab259a 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -212,10 +212,9 @@ static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test)
 {
 	int i;
 	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
 
 	for (i = 0; i < test->num_irqs; i++)
-		devm_free_irq(dev, pci_irq_vector(pdev, i), test);
+		free_irq(pci_irq_vector(pdev, i), test);
 
 	test->num_irqs = 0;
 }
@@ -228,9 +227,9 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 	struct device *dev = &pdev->dev;
 
 	for (i = 0; i < test->num_irqs; i++) {
-		ret = devm_request_irq(dev, pci_irq_vector(pdev, i),
-				       pci_endpoint_test_irqhandler,
-				       IRQF_SHARED, test->name, test);
+		ret = request_irq(pci_irq_vector(pdev, i),
+				  pci_endpoint_test_irqhandler, IRQF_SHARED,
+				  test->name, test);
 		if (ret)
 			goto fail;
 	}
-- 
2.25.1

Re: [PATCH v3 5/5] misc: pci_endpoint_test: Do not use managed irq functions

[Manivannan Sadhasivam]

On Mon, Feb 10, 2025 at 04:58:12PM +0900, Kunihiko Hayashi wrote:
> The pci_endpoint_test_request_irq() and pci_endpoint_test_release_irq()
> are called repeatedly by the users through pci_endpoint_test_set_irq().
> So using the managed version of IRQ functions within these functions
> has no effect.
> 
> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/misc/pci_endpoint_test.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 8d98cd18634d..9465d2ab259a 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -212,10 +212,9 @@ static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test)
>  {
>  	int i;
>  	struct pci_dev *pdev = test->pdev;
> -	struct device *dev = &pdev->dev;
>  
>  	for (i = 0; i < test->num_irqs; i++)
> -		devm_free_irq(dev, pci_irq_vector(pdev, i), test);
> +		free_irq(pci_irq_vector(pdev, i), test);
>  
>  	test->num_irqs = 0;
>  }
> @@ -228,9 +227,9 @@ static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
>  	struct device *dev = &pdev->dev;
>  
>  	for (i = 0; i < test->num_irqs; i++) {
> -		ret = devm_request_irq(dev, pci_irq_vector(pdev, i),
> -				       pci_endpoint_test_irqhandler,
> -				       IRQF_SHARED, test->name, test);
> +		ret = request_irq(pci_irq_vector(pdev, i),
> +				  pci_endpoint_test_irqhandler, IRQF_SHARED,
> +				  test->name, test);
>  		if (ret)
>  			goto fail;
>  	}
> -- 
> 2.25.1
> 

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