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

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

[Kunihiko Hayashi]

After devm_request_irq() fails with error,
pci_endpoint_test_free_irq_vectors() is called to free allocated vectors
with pci_free_irq_vectors().

However some requested IRQs are still allocated, 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: 80 at fs/proc/generic.c:717 remove_proc_entry
    +0x190/0x19c

To solve this issue, set the number of remaining IRQs and release the 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 3702dcc89ab7..302955c20979 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -252,6 +252,9 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 		break;
 	}
 
+	test->num_irqs = i;
+	pci_endpoint_test_release_irq(test);
+
 	return false;
 }
 
-- 
2.25.1

[PATCH v2 2/3] 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")
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 302955c20979..a342587fc78a 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -235,7 +235,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 	return true;
 
 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 v2 3/3] 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 a342587fc78a..33058630cd50 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -742,6 +742,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	if (!pci_endpoint_test_request_irq(test))
 		goto err;
 
+	irq_type = test->irq_type;
 	return true;
 
 err:
-- 
2.25.1

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

[Manivannan Sadhasivam]

On Wed, Jan 22, 2025 at 11:24:44AM +0900, Kunihiko Hayashi wrote:
> After devm_request_irq() fails with error,
> pci_endpoint_test_free_irq_vectors() is called to free allocated vectors
> with pci_free_irq_vectors().
> 

You should mention the function name which you are referring to. Here it is,
pci_endpoint_test_request_irq().

> However some requested IRQs are still allocated, so there are still

This is confusing. Are you saying that the previously requested IRQs are not
freed when an error happens during the for loop in
pci_endpoint_test_request_irq()?

> /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: 80 at fs/proc/generic.c:717 remove_proc_entry
>     +0x190/0x19c

When did you encounter this WARN?

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

First of all, using devm_request_irq() is wrong here as
pci_endpoint_test_request_irq() might be called multiple times by the userspace.
So we cannot use managed version of request_irq(). It is infact pointless since
pci_endpoint_test_clear_irq() would free them anyway. Instead we should just use
request_irq() and call pci_endpoint_test_clear_irq() in the error path as like
this patch does.

But this should be a separate patch.

- Mani

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

Re: [PATCH v2 2/3] misc: pci_endpoint_test: Fix disyplaying irq_type after request_irq error

[Manivannan Sadhasivam]

On Wed, Jan 22, 2025 at 11:24:45AM +0900, Kunihiko Hayashi wrote:
> 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")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

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

- Mani

> ---
>  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 302955c20979..a342587fc78a 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -235,7 +235,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
>  	return true;
>  
>  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
> 

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

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

[Manivannan Sadhasivam]

On Wed, Jan 22, 2025 at 11:24:46AM +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".
> 

This is becoming a nuisance. I think we should get rid of the global 'irq_type'
and just stick to the one that is configurable using IOCTL command. Even if the
user has configured the global 'irq_type' it is bound to change with IOCTL
command.

- Mani

> 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 a342587fc78a..33058630cd50 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -742,6 +742,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
>  	if (!pci_endpoint_test_request_irq(test))
>  		goto err;
>  
> +	irq_type = test->irq_type;
>  	return true;
>  
>  err:
> -- 
> 2.25.1
> 

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

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

[Kunihiko Hayashi]

Hi Manivannan,

On 2025/01/28 23:12, Manivannan Sadhasivam wrote:
> On Wed, Jan 22, 2025 at 11:24:44AM +0900, Kunihiko Hayashi wrote:
>> After devm_request_irq() fails with error,
>> pci_endpoint_test_free_irq_vectors() is called to free allocated vectors
>> with pci_free_irq_vectors().
>>
> 
> You should mention the function name which you are referring to. Here it is,
> pci_endpoint_test_request_irq().

I see. I'll make the commit message more clear.

>> However some requested IRQs are still allocated, so there are still
> 
> This is confusing. Are you saying that the previously requested IRQs are not
> freed when an error happens during the for loop in
> pci_endpoint_test_request_irq()?

Yes, after jumping to "fail:" label, it just prints an error message and
returns the function.

The pci_endpoint_test_request_irq() is called from the following functions:
- pci_endpoint_test_probe()
- pci_endpoint_test_set_irq()

Both call pci_endpoint_test_free_irq_vectors() after the error, though,
requested IRQs are not freed anywhere.

>> /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: 80 at fs/proc/generic.c:717 remove_proc_entry
>>      +0x190/0x19c
> 
> When did you encounter this WARN?

Usually request_irq() can successfully get an interrupt.
If request_irq() returned an error, pci_endpoint_test_free_irq_vectors() was
called and the following call-trace was obtained:

[   18.772522] Call trace:
[   18.773743]  remove_proc_entry+0x190/0x19c
[   18.775789]  unregister_irq_proc+0xd0/0x104
[   18.777881]  free_desc+0x4c/0xcc
[   18.779495]  irq_free_descs+0x68/0x8c
[   18.781325]  irq_domain_free_irqs+0x15c/0x1bc
[   18.783502]  msi_domain_free_locked.part.0+0x184/0x1d4
[   18.786069]  msi_domain_free_irqs_all_locked+0x64/0x8c
[   18.788637]  pci_msi_teardown_msi_irqs+0x48/0x54
[   18.790947]  pci_free_msi_irqs+0x18/0x38
[   18.792907]  pci_free_irq_vectors+0x64/0x8c
[   18.794997]  pci_endpoint_test_ioctl+0x7e8/0xf40
[   18.797304]  __arm64_sys_ioctl+0xa4/0xe8
[   18.799265]  invoke_syscall+0x48/0x110
[   18.801139]  el0_svc_common.constprop.0+0x40/0xe8
[   18.803489]  do_el0_svc+0x20/0x2c
[   18.805145]  el0_svc+0x30/0xd0
[   18.806673]  el0t_64_sync_handler+0x13c/0x158
[   18.808850]  el0t_64_sync+0x190/0x194
[   18.810680] ---[ end trace 0000000000000000 ]---

>> To solve this issue, set the number of remaining IRQs and release the IRQs
>> in advance by calling pci_endpoint_test_release_irq().
>>
> 
> First of all, using devm_request_irq() is wrong here as
> pci_endpoint_test_request_irq() might be called multiple times by the userspace.
> So we cannot use managed version of request_irq(). It is infact pointless since
> pci_endpoint_test_clear_irq() would free them anyway. Instead we should just use
> request_irq() and call pci_endpoint_test_clear_irq() in the error path as like
> this patch does.

The pci_endpoint_test_clear_irq() will free all the number of IRQs.
However, in this error case, since only some IRQs are allocated, need to change the
number of IRQs that need to be freed.

And pci_endpoint_test_free_irq_vectors() is called just before return from the function,
call only pci_endpoint_test_release_irq() in the error case of
pci_endpoint_test_request_irq().

For example:
   pci_endpoint_test_set_irq()
     pci_endpoint_test_request_irq()
       request_irq()
       fail:
       >> test->num_irqs = i                // add in this patch
       >> pci_endpoint_test_release_irq()   // add in this patch
     err:
     pci_endpoint_test_free_irq_vectors()

> But this should be a separate patch.

Agreed. I'll add another patch to use non-managed version.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Kunihiko Hayashi]

Hi Manivannan,

On 2025/01/28 23:32, Manivannan Sadhasivam wrote:
> On Wed, Jan 22, 2025 at 11:24:46AM +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".
>>
> 
> This is becoming a nuisance. I think we should get rid of the global 'irq_type'
> and just stick to the one that is configurable using IOCTL command. Even if the
> user has configured the global 'irq_type' it is bound to change with IOCTL
> command.

We can add a new member of 'struct pci_endpoint_test' instead of the global
'irq_type'.

If I remove the global 'irq_type', the following module parameter description
will also be removed.

>> 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)");

I'm concerned about compatibility. Is there any problem with removing this?

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Niklas Cassel]

On Tue, Jan 28, 2025 at 08:02:31PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jan 22, 2025 at 11:24:46AM +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".
> > 
> 
> This is becoming a nuisance. I think we should get rid of the global 'irq_type'
> and just stick to the one that is configurable using IOCTL command. Even if the
> user has configured the global 'irq_type' it is bound to change with IOCTL
> command.

+1


But I also don't like how since we migrated to selftests:
READ_TEST / WRITE_TEST / COPY_TEST unconditionally call
ioctl(PCITEST_SET_IRQTYPE, MSI) before doing their thing.

Will this cause the test case to fail for platforms that only support MSI-X?
(See e.g. dwc/pci-layerscape-ep.c where this could be the case.)


Sure, before, in pcitest.sh, we would do:


pcitest -i 2
        pcitest -x $msix


pcitest -i 1

pcitest -r -s 1
pcitest -r -s 1024
pcitest -r -s 1025
pcitest -r -s 1024000
pcitest -r -s 1024001


Which would probably print an error if:
pcitest -i 1
failed.

but the READ_TEST / WRITE_TEST / COPY_TEST tests themselves
would not fail.


Perhaps we should rethink this, and introduce a new
PCITEST_SET_IRQTYPE, AUTO

I would be fine if
READ_TEST / WRITE_TEST / COPY_TEST
called PCITEST_SET_IRQTYPE, AUTO
before doing their thing.



How I suggest PCITEST_SET_IRQTYPE, AUTO
would work:

Since we now have capabilties merged:
https://lore.kernel.org/linux-pci/20241203063851.695733-4-cassel@kernel.org/

Add epc_features->msi_capable and epc->features->msix_capable
as two new bits in the PCI_ENDPOINT_TEST_CAPS register.

If PCITEST_SET_IRQTYPE, AUTO:
if EP CAP has msi_capable set: set IRQ type MSI
else if EP CAP has msix_capable set: set IRQ type MSI-X
else: set legacy/INTx


Kind regards,
Niklas

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

[Kunihiko Hayashi]

Hi Niklas,

On 2025/01/29 20:58, Niklas Cassel wrote:
> On Tue, Jan 28, 2025 at 08:02:31PM +0530, Manivannan Sadhasivam wrote:
>> On Wed, Jan 22, 2025 at 11:24:46AM +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".
>>>
>>
>> This is becoming a nuisance. I think we should get rid of the global
>> 'irq_type'
>> and just stick to the one that is configurable using IOCTL command. Even
>> if the
>> user has configured the global 'irq_type' it is bound to change with IOCTL
>> command.
> 
> +1

After fixing the issue described in this patch,
we can replace with a new member of 'struct pci_endpoint_test' instead.

> But I also don't like how since we migrated to selftests:
> READ_TEST / WRITE_TEST / COPY_TEST unconditionally call
> ioctl(PCITEST_SET_IRQTYPE, MSI) before doing their thing.

I think that it's better to prepare new patch series.

> Will this cause the test case to fail for platforms that only support MSI-X?
> (See e.g. dwc/pci-layerscape-ep.c where this could be the case.)
> 
> 
> Sure, before, in pcitest.sh, we would do:
> 
> 
> pcitest -i 2
>          pcitest -x $msix
> 
> 
> pcitest -i 1
> 
> pcitest -r -s 1
> pcitest -r -s 1024
> pcitest -r -s 1025
> pcitest -r -s 1024000
> pcitest -r -s 1024001
> 
> 
> Which would probably print an error if:
> pcitest -i 1
> failed.
> 
> but the READ_TEST / WRITE_TEST / COPY_TEST tests themselves
> would not fail.
> 
> 
> Perhaps we should rethink this, and introduce a new
> PCITEST_SET_IRQTYPE, AUTO
> 
> I would be fine if
> READ_TEST / WRITE_TEST / COPY_TEST
> called PCITEST_SET_IRQTYPE, AUTO
> before doing their thing.
> 
> 
> 
> How I suggest PCITEST_SET_IRQTYPE, AUTO
> would work:
> 
> Since we now have capabilties merged:
> https://lore.kernel.org/linux-pci/20241203063851.695733-4-cassel@kernel.org/
> 
> Add epc_features->msi_capable and epc->features->msix_capable
> as two new bits in the PCI_ENDPOINT_TEST_CAPS register.
> 
> If PCITEST_SET_IRQTYPE, AUTO:
> if EP CAP has msi_capable set: set IRQ type MSI
> else if EP CAP has msix_capable set: set IRQ type MSI-X
> else: set legacy/INTx

There is something ambiguous about the behavior for me.

The test->irq_type has a state "UNDEFINED".
After issueing "Clear IRQ", test->irq_type becomes "UNDEFINED" currently,
and all tests with IRQs will fail until new test->irq_type is set.

If SET_IRQTYPE is AUTO, how will test->irq_type be set?

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Niklas Cassel]

On Fri, Jan 31, 2025 at 07:16:54PM +0900, Kunihiko Hayashi wrote:
> Hi Niklas,
> 
> On 2025/01/29 20:58, Niklas Cassel wrote:
> > On Tue, Jan 28, 2025 at 08:02:31PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jan 22, 2025 at 11:24:46AM +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".
> > > > 
> > > 
> > > This is becoming a nuisance. I think we should get rid of the global
> > > 'irq_type'
> > > and just stick to the one that is configurable using IOCTL command. Even
> > > if the
> > > user has configured the global 'irq_type' it is bound to change with IOCTL
> > > command.
> > 
> > +1
> 
> After fixing the issue described in this patch,
> we can replace with a new member of 'struct pci_endpoint_test' instead.

Sorry, but I don't understand what you mean here.
You want this patch to be applied.
Then you want to add a new struct member to struct pci_endpoint_test?
struct pci_endpoint_test already has a struct member named irq_type,
so why do you want to add a new member?

Like Mani suggested, I think it would be nice if we could remove the
global irq_type kernel module parameter, and change so that
PCITEST_GET_IRQTYPE returns test->irq_type.


Note that your series does not apply to pci/next, and needs to be rebased.
(It conflicts with f26d37ee9bda ("misc: pci_endpoint_test: Fix IOCTL return value"))


> 
> > But I also don't like how since we migrated to selftests:
> > READ_TEST / WRITE_TEST / COPY_TEST unconditionally call
> > ioctl(PCITEST_SET_IRQTYPE, MSI) before doing their thing.
> 
> I think that it's better to prepare new patch series.

Here, I was pointing out what I think is a regression with the
migration to selftests. (Which is unrelated to this series.)

I do suggest a way to improve things futher down (introducing a
PCITEST_SET_IRQTYPE, AUTO), but I don't think that you need to be
the one implementing this suggestion, since you did not introduce
this regression.


> 
> > Will this cause the test case to fail for platforms that only support MSI-X?
> > (See e.g. dwc/pci-layerscape-ep.c where this could be the case.)
> > 
> > 
> > Sure, before, in pcitest.sh, we would do:
> > 
> > 
> > pcitest -i 2
> >          pcitest -x $msix
> > 
> > 
> > pcitest -i 1
> > 
> > pcitest -r -s 1
> > pcitest -r -s 1024
> > pcitest -r -s 1025
> > pcitest -r -s 1024000
> > pcitest -r -s 1024001
> > 
> > 
> > Which would probably print an error if:
> > pcitest -i 1
> > failed.
> > 
> > but the READ_TEST / WRITE_TEST / COPY_TEST tests themselves
> > would not fail.
> > 
> > 
> > Perhaps we should rethink this, and introduce a new
> > PCITEST_SET_IRQTYPE, AUTO
> > 
> > I would be fine if
> > READ_TEST / WRITE_TEST / COPY_TEST
> > called PCITEST_SET_IRQTYPE, AUTO
> > before doing their thing.
> > 
> > 
> > 
> > How I suggest PCITEST_SET_IRQTYPE, AUTO
> > would work:
> > 
> > Since we now have capabilties merged:
> > https://lore.kernel.org/linux-pci/20241203063851.695733-4-cassel@kernel.org/
> > 
> > Add epc_features->msi_capable and epc->features->msix_capable
> > as two new bits in the PCI_ENDPOINT_TEST_CAPS register.
> > 
> > If PCITEST_SET_IRQTYPE, AUTO:
> > if EP CAP has msi_capable set: set IRQ type MSI
> > else if EP CAP has msix_capable set: set IRQ type MSI-X
> > else: set legacy/INTx
> 
> There is something ambiguous about the behavior for me.
> 
> The test->irq_type has a state "UNDEFINED".
> After issueing "Clear IRQ", test->irq_type becomes "UNDEFINED" currently,
> and all tests with IRQs will fail until new test->irq_type is set.

That is fine, no?

If a user calls PCITEST_CLEAR_IRQ without doing a PCITEST_SET_IRQTYPE
afterwards, what else can the tests relying on IRQs to other than fail?

FWIW, tools/testing/selftests/pci_endpoint/pci_endpoint_test.c never does
an ioctl(PCITEST_CLEAR_IRQ).

> 
> If SET_IRQTYPE is AUTO, how will test->irq_type be set?

I was thinking something like this:

pci_endpoint_test_set_irq()
{
	u32 caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);

	...

	if (req_irq_type == IRQ_TYPE_AUTO) {
		if (caps & MSI_CAPABLE)
			test->irq_type = IRQ_TYPE_MSI;
		else if (caps & MSIX_CAPABLE)
			test->irq_type = IRQ_TYPE_MSIX;
		else
			test->irq_type = IRQ_TYPE_INTX;

	}

	...
}


Kind regards,
Niklas

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

[Niklas Cassel]

On Fri, Jan 31, 2025 at 01:10:54PM +0100, Niklas Cassel wrote:
> > 
> > If SET_IRQTYPE is AUTO, how will test->irq_type be set?
> 
> I was thinking something like this:
> 
> pci_endpoint_test_set_irq()
> {
> 	u32 caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> 
> 	...
> 
> 	if (req_irq_type == IRQ_TYPE_AUTO) {
> 		if (caps & MSI_CAPABLE)
> 			test->irq_type = IRQ_TYPE_MSI;
> 		else if (caps & MSIX_CAPABLE)
> 			test->irq_type = IRQ_TYPE_MSIX;
> 		else
> 			test->irq_type = IRQ_TYPE_INTX;
> 
> 	}
> 
> 	...
> }


Or even simpler (since it requires less changes to
pci_endpoint_test_set_irq()):

	if (req_irq_type == IRQ_TYPE_AUTO) {
		if (caps & MSI_CAPABLE)
			req_irq_type = IRQ_TYPE_MSI;
		else if (caps & MSIX_CAPABLE)
			req_irq_type = IRQ_TYPE_MSIX;
		else
			req_irq_type = IRQ_TYPE_INTX;

	}

	...

	/* Sets test->irq_type = req_irq_type; on success */
	pci_endpoint_test_alloc_irq_vectors();



Kind regards,
Niklas

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

[Niklas Cassel]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On Fri, Jan 31, 2025 at 01:20:38PM +0100, Niklas Cassel wrote:
> On Fri, Jan 31, 2025 at 01:10:54PM +0100, Niklas Cassel wrote:
> > > 
> > > If SET_IRQTYPE is AUTO, how will test->irq_type be set?
> > 
> > I was thinking something like this:
> > 
> > pci_endpoint_test_set_irq()
> > {
> > 	u32 caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> > 
> > 	...
> > 
> > 	if (req_irq_type == IRQ_TYPE_AUTO) {
> > 		if (caps & MSI_CAPABLE)
> > 			test->irq_type = IRQ_TYPE_MSI;
> > 		else if (caps & MSIX_CAPABLE)
> > 			test->irq_type = IRQ_TYPE_MSIX;
> > 		else
> > 			test->irq_type = IRQ_TYPE_INTX;
> > 
> > 	}
> > 
> > 	...
> > }
> 
> 
> Or even simpler (since it requires less changes to
> pci_endpoint_test_set_irq()):
> 
> 	if (req_irq_type == IRQ_TYPE_AUTO) {
> 		if (caps & MSI_CAPABLE)
> 			req_irq_type = IRQ_TYPE_MSI;
> 		else if (caps & MSIX_CAPABLE)
> 			req_irq_type = IRQ_TYPE_MSIX;
> 		else
> 			req_irq_type = IRQ_TYPE_INTX;
> 
> 	}
> 
> 	...
> 
> 	/* Sets test->irq_type = req_irq_type; on success */
> 	pci_endpoint_test_alloc_irq_vectors();


See attached patch.

Mani, removing the global irq_type would go on top of this.


Kind regards,
Niklas

[-- Attachment #2: auto_irq.patch --]
[-- Type: text/plain, Size: 5622 bytes --]

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index d5ac71a49386..5e42930124e2 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -32,6 +32,7 @@
 #define IRQ_TYPE_INTX				0
 #define IRQ_TYPE_MSI				1
 #define IRQ_TYPE_MSIX				2
+#define IRQ_TYPE_AUTO				3
 
 #define PCI_ENDPOINT_TEST_MAGIC			0x0
 
@@ -71,6 +72,8 @@
 
 #define PCI_ENDPOINT_TEST_CAPS			0x30
 #define CAP_UNALIGNED_ACCESS			BIT(0)
+#define CAP_MSI					BIT(1)
+#define CAP_MSIX				BIT(2)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
@@ -126,6 +129,7 @@ struct pci_endpoint_test {
 	struct miscdevice miscdev;
 	enum pci_barno test_reg_bar;
 	size_t alignment;
+	u32 ep_caps;
 	const char *name;
 };
 
@@ -805,11 +809,20 @@ static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	if (req_irq_type < IRQ_TYPE_INTX || req_irq_type > IRQ_TYPE_MSIX) {
+	if (req_irq_type < IRQ_TYPE_INTX || req_irq_type > IRQ_TYPE_AUTO) {
 		dev_err(dev, "Invalid IRQ type option\n");
 		return -EINVAL;
 	}
 
+	if (req_irq_type == IRQ_TYPE_AUTO) {
+		if (test->ep_caps & CAP_MSI)
+			req_irq_type = IRQ_TYPE_MSI;
+		else if (test->ep_caps & CAP_MSIX)
+			req_irq_type = IRQ_TYPE_MSIX;
+		else
+			req_irq_type = IRQ_TYPE_INTX;
+	}
+
 	if (test->irq_type == req_irq_type)
 		return 0;
 
@@ -895,13 +908,12 @@ static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
 {
 	struct pci_dev *pdev = test->pdev;
 	struct device *dev = &pdev->dev;
-	u32 caps;
 
-	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
-	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps);
+	test->ep_caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
+	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", test->ep_caps);
 
 	/* CAP_UNALIGNED_ACCESS is set if the EP can do unaligned access */
-	if (caps & CAP_UNALIGNED_ACCESS)
+	if (test->ep_caps & CAP_UNALIGNED_ACCESS)
 		test->alignment = 0;
 }
 
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index b94e205ae10b..8917f7c6c741 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -45,6 +45,8 @@
 #define TIMER_RESOLUTION		1
 
 #define CAP_UNALIGNED_ACCESS		BIT(0)
+#define CAP_MSI				BIT(1)
+#define CAP_MSIX			BIT(2)
 
 static struct workqueue_struct *kpcitest_workqueue;
 
@@ -753,6 +755,12 @@ static void pci_epf_test_set_capabilities(struct pci_epf *epf)
 	if (epc->ops->align_addr)
 		caps |= CAP_UNALIGNED_ACCESS;
 
+	if (epf_test->epc_features->msi_capable)
+		caps |= CAP_MSI;
+
+	if (epf_test->epc_features->msix_capable)
+		caps |= CAP_MSIX;
+
 	reg->caps = cpu_to_le32(caps);
 }
 
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index acd261f49866..1edbd4357470 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -23,6 +23,11 @@
 #define PCITEST_BARS		_IO('P', 0xa)
 #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
 
+#define PCITEST_IRQ_TYPE_INTX	0
+#define PCITEST_IRQ_TYPE_MSI	1
+#define PCITEST_IRQ_TYPE_MSIX	2
+#define PCITEST_IRQ_TYPE_AUTO	3
+
 #define PCITEST_FLAGS_USE_DMA	0x00000001
 
 struct pci_endpoint_test_xfer_param {
diff --git a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
index c267b822c108..c820a67e6437 100644
--- a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
+++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
@@ -97,7 +97,7 @@ TEST_F(pci_ep_basic, LEGACY_IRQ_TEST)
 {
 	int ret;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0);
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_INTX);
 	ASSERT_EQ(0, ret) TH_LOG("Can't set Legacy IRQ type");
 
 	pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0);
@@ -108,7 +108,7 @@ TEST_F(pci_ep_basic, MSI_TEST)
 {
 	int ret, i;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSI);
 	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
 
 	for (i = 1; i <= 32; i++) {
@@ -121,7 +121,7 @@ TEST_F(pci_ep_basic, MSIX_TEST)
 {
 	int ret, i;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 2);
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_MSIX);
 	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI-X IRQ type");
 
 	for (i = 1; i <= 2048; i++) {
@@ -170,8 +170,8 @@ TEST_F(pci_ep_data_transfer, READ_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
@@ -189,8 +189,8 @@ TEST_F(pci_ep_data_transfer, WRITE_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];
@@ -208,8 +208,8 @@ TEST_F(pci_ep_data_transfer, COPY_TEST)
 	if (variant->use_dma)
 		param.flags = PCITEST_FLAGS_USE_DMA;
 
-	pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1);
-	ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type");
+	pci_ep_ioctl(PCITEST_SET_IRQTYPE, PCITEST_IRQ_TYPE_AUTO);
+	ASSERT_EQ(0, ret) TH_LOG("Can't set AUTO IRQ type");
 
 	for (i = 0; i < ARRAY_SIZE(test_size); i++) {
 		param.size = test_size[i];

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

[Kunihiko Hayashi]

Hi Niklas,

On 2025/01/31 21:10, Niklas Cassel wrote:
> On Fri, Jan 31, 2025 at 07:16:54PM +0900, Kunihiko Hayashi wrote:
>> Hi Niklas,
>>
>> On 2025/01/29 20:58, Niklas Cassel wrote:
>>> On Tue, Jan 28, 2025 at 08:02:31PM +0530, Manivannan Sadhasivam wrote:
>>>> On Wed, Jan 22, 2025 at 11:24:46AM +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".
>>>>>
>>>>
>>>> This is becoming a nuisance. I think we should get rid of the global
>>>> 'irq_type'
>>>> and just stick to the one that is configurable using IOCTL command.
> Even
>>>> if the
>>>> user has configured the global 'irq_type' it is bound to change with
> IOCTL
>>>> command.
>>>
>>> +1
>>
>> After fixing the issue described in this patch,
>> we can replace with a new member of 'struct pci_endpoint_test' instead.
> 
> Sorry, but I don't understand what you mean here.
> You want this patch to be applied.
> Then you want to add a new struct member to struct pci_endpoint_test?
> struct pci_endpoint_test already has a struct member named irq_type,
> so why do you want to add a new member?

Sorry for confusion.

The internal state (test->irq_type) has the state IRQ_TYPE_UNDEFINED and
the global irq_type doesn't. Then I've thought that ioctl(GET_IRQTYPE)
should not return UNDEFINED state.

However, ioctl(GET_IRQTYPE) can return with an error if the state is
UNDEFINED. This is new behavior, but not inconsistent.
So the additional irq_type is no longer necessary.

> Like Mani suggested, I think it would be nice if we could remove the
> global irq_type kernel module parameter, and change so that
> PCITEST_GET_IRQTYPE returns test->irq_type.

I see.
I'll add new patch to remove the global irq_type and replace with
test->irq_type.

> Note that your series does not apply to pci/next, and needs to be rebased.
> (It conflicts with f26d37ee9bda ("misc: pci_endpoint_test: Fix IOCTL
> return value"))

Yes, I've confirmed the conflict and I'll rebase it next.

>>
>>> But I also don't like how since we migrated to selftests:
>>> READ_TEST / WRITE_TEST / COPY_TEST unconditionally call
>>> ioctl(PCITEST_SET_IRQTYPE, MSI) before doing their thing.
>>
>> I think that it's better to prepare new patch series.
> 
> Here, I was pointing out what I think is a regression with the
> migration to selftests. (Which is unrelated to this series.)
> 
> I do suggest a way to improve things futher down (introducing a
> PCITEST_SET_IRQTYPE, AUTO), but I don't think that you need to be
> the one implementing this suggestion, since you did not introduce
> this regression.

Okay, I expect another topic after we remove the global variables.

>>
>>> Will this cause the test case to fail for platforms that only support
> MSI-X?
>>> (See e.g. dwc/pci-layerscape-ep.c where this could be the case.)
>>>
>>>
>>> Sure, before, in pcitest.sh, we would do:
>>>
>>>
>>> pcitest -i 2
>>>           pcitest -x $msix
>>>
>>>
>>> pcitest -i 1
>>>
>>> pcitest -r -s 1
>>> pcitest -r -s 1024
>>> pcitest -r -s 1025
>>> pcitest -r -s 1024000
>>> pcitest -r -s 1024001
>>>
>>>
>>> Which would probably print an error if:
>>> pcitest -i 1
>>> failed.
>>>
>>> but the READ_TEST / WRITE_TEST / COPY_TEST tests themselves
>>> would not fail.
>>>
>>>
>>> Perhaps we should rethink this, and introduce a new
>>> PCITEST_SET_IRQTYPE, AUTO
>>>
>>> I would be fine if
>>> READ_TEST / WRITE_TEST / COPY_TEST
>>> called PCITEST_SET_IRQTYPE, AUTO
>>> before doing their thing.
>>>
>>>
>>>
>>> How I suggest PCITEST_SET_IRQTYPE, AUTO
>>> would work:
>>>
>>> Since we now have capabilties merged:
>>>
> https://lore.kernel.org/linux-pci/20241203063851.695733-4-cassel@kernel.or
> g/
>>>
>>> Add epc_features->msi_capable and epc->features->msix_capable
>>> as two new bits in the PCI_ENDPOINT_TEST_CAPS register.
>>>
>>> If PCITEST_SET_IRQTYPE, AUTO:
>>> if EP CAP has msi_capable set: set IRQ type MSI
>>> else if EP CAP has msix_capable set: set IRQ type MSI-X
>>> else: set legacy/INTx
>>
>> There is something ambiguous about the behavior for me.
>>
>> The test->irq_type has a state "UNDEFINED".
>> After issueing "Clear IRQ", test->irq_type becomes "UNDEFINED"
> currently,
>> and all tests with IRQs will fail until new test->irq_type is set.
> 
> That is fine, no?
> 
> If a user calls PCITEST_CLEAR_IRQ without doing a PCITEST_SET_IRQTYPE
> afterwards, what else can the tests relying on IRQs to other than fail?
> 
> FWIW, tools/testing/selftests/pci_endpoint/pci_endpoint_test.c never does
> an ioctl(PCITEST_CLEAR_IRQ).

As mentioned above, PCITEST_GET_IRQTYPE can fail with an error, and
I understand that this will not affect the test.

Thank you,

>>
>> If SET_IRQTYPE is AUTO, how will test->irq_type be set?
> 
> I was thinking something like this:
> 
> pci_endpoint_test_set_irq()
> {
> 	u32 caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> 
> 	...
> 
> 	if (req_irq_type == IRQ_TYPE_AUTO) {
> 		if (caps & MSI_CAPABLE)
> 			test->irq_type = IRQ_TYPE_MSI;
> 		else if (caps & MSIX_CAPABLE)
> 			test->irq_type = IRQ_TYPE_MSIX;
> 		else
> 			test->irq_type = IRQ_TYPE_INTX;
> 
> 	}
> 
> 	...
> }
> 
> 
> Kind regards,
> Niklas

---
Best Regards
Kunihiko Hayashi

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

[Manivannan Sadhasivam]

On Wed, Jan 29, 2025 at 04:54:46PM +0900, Kunihiko Hayashi wrote:
> Hi Manivannan,
> 
> On 2025/01/28 23:12, Manivannan Sadhasivam wrote:
> > On Wed, Jan 22, 2025 at 11:24:44AM +0900, Kunihiko Hayashi wrote:
> > > After devm_request_irq() fails with error,
> > > pci_endpoint_test_free_irq_vectors() is called to free allocated vectors
> > > with pci_free_irq_vectors().
> > > 
> > 
> > You should mention the function name which you are referring to. Here it is,
> > pci_endpoint_test_request_irq().
> 
> I see. I'll make the commit message more clear.
> 
> > > However some requested IRQs are still allocated, so there are still
> > 
> > This is confusing. Are you saying that the previously requested IRQs are not
> > freed when an error happens during the for loop in
> > pci_endpoint_test_request_irq()?
> 
> Yes, after jumping to "fail:" label, it just prints an error message and
> returns the function.
> 
> The pci_endpoint_test_request_irq() is called from the following functions:
> - pci_endpoint_test_probe()
> - pci_endpoint_test_set_irq()
> 
> Both call pci_endpoint_test_free_irq_vectors() after the error, though,
> requested IRQs are not freed anywhere.
> 

You should not use the word 'allocated' since that has a different meaning
and the source of confusion.

> > > /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: 80 at fs/proc/generic.c:717 remove_proc_entry
> > >      +0x190/0x19c
> > 
> > When did you encounter this WARN?
> 
> Usually request_irq() can successfully get an interrupt.
> If request_irq() returned an error, pci_endpoint_test_free_irq_vectors() was
> called and the following call-trace was obtained:
> 
> [   18.772522] Call trace:
> [   18.773743]  remove_proc_entry+0x190/0x19c
> [   18.775789]  unregister_irq_proc+0xd0/0x104
> [   18.777881]  free_desc+0x4c/0xcc
> [   18.779495]  irq_free_descs+0x68/0x8c
> [   18.781325]  irq_domain_free_irqs+0x15c/0x1bc
> [   18.783502]  msi_domain_free_locked.part.0+0x184/0x1d4
> [   18.786069]  msi_domain_free_irqs_all_locked+0x64/0x8c
> [   18.788637]  pci_msi_teardown_msi_irqs+0x48/0x54
> [   18.790947]  pci_free_msi_irqs+0x18/0x38
> [   18.792907]  pci_free_irq_vectors+0x64/0x8c
> [   18.794997]  pci_endpoint_test_ioctl+0x7e8/0xf40
> [   18.797304]  __arm64_sys_ioctl+0xa4/0xe8
> [   18.799265]  invoke_syscall+0x48/0x110
> [   18.801139]  el0_svc_common.constprop.0+0x40/0xe8
> [   18.803489]  do_el0_svc+0x20/0x2c
> [   18.805145]  el0_svc+0x30/0xd0
> [   18.806673]  el0t_64_sync_handler+0x13c/0x158
> [   18.808850]  el0t_64_sync+0x190/0x194
> [   18.810680] ---[ end trace 0000000000000000 ]---
> 

Please add this info to the patch description.

- Mani

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

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

[Kunihiko Hayashi]

Hi Manivannan,

On 2025/02/08 3:02, Manivannan Sadhasivam wrote:
> On Wed, Jan 29, 2025 at 04:54:46PM +0900, Kunihiko Hayashi wrote:
>> Hi Manivannan,
>>
>> On 2025/01/28 23:12, Manivannan Sadhasivam wrote:
>>> On Wed, Jan 22, 2025 at 11:24:44AM +0900, Kunihiko Hayashi wrote:
>>>> After devm_request_irq() fails with error,
>>>> pci_endpoint_test_free_irq_vectors() is called to free allocated
>>>> vectors
>>>> with pci_free_irq_vectors().
>>>>
>>>
>>> You should mention the function name which you are referring to. Here it
>>> is,
>>> pci_endpoint_test_request_irq().
>>
>> I see. I'll make the commit message more clear.
>>
>>>> However some requested IRQs are still allocated, so there are still
>>>
>>> This is confusing. Are you saying that the previously requested IRQs are
>>> not
>>> freed when an error happens during the for loop in
>>> pci_endpoint_test_request_irq()?
>>
>> Yes, after jumping to "fail:" label, it just prints an error message and
>> returns the function.
>>
>> The pci_endpoint_test_request_irq() is called from the following
>> functions:
>> - pci_endpoint_test_probe()
>> - pci_endpoint_test_set_irq()
>>
>> Both call pci_endpoint_test_free_irq_vectors() after the error, though,
>> requested IRQs are not freed anywhere.
>>
> 
> You should not use the word 'allocated' since that has a different meaning
> and the source of confusion.

Surely, 'allocated' means the vectors allocated with pci_irq_vectors(), not
the interrupts requested with request_irq().

I'll rewrite with a right description.

>>>> /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: 80 at fs/proc/generic.c:717
>>>> remove_proc_entry
>>>>       +0x190/0x19c
>>>
>>> When did you encounter this WARN?
>>
>> Usually request_irq() can successfully get an interrupt.
>> If request_irq() returned an error, pci_endpoint_test_free_irq_vectors()
>> was
>> called and the following call-trace was obtained:
>>
>> [   18.772522] Call trace:
>> [   18.773743]  remove_proc_entry+0x190/0x19c
>> [   18.775789]  unregister_irq_proc+0xd0/0x104
>> [   18.777881]  free_desc+0x4c/0xcc
>> [   18.779495]  irq_free_descs+0x68/0x8c
>> [   18.781325]  irq_domain_free_irqs+0x15c/0x1bc
>> [   18.783502]  msi_domain_free_locked.part.0+0x184/0x1d4
>> [   18.786069]  msi_domain_free_irqs_all_locked+0x64/0x8c
>> [   18.788637]  pci_msi_teardown_msi_irqs+0x48/0x54
>> [   18.790947]  pci_free_msi_irqs+0x18/0x38
>> [   18.792907]  pci_free_irq_vectors+0x64/0x8c
>> [   18.794997]  pci_endpoint_test_ioctl+0x7e8/0xf40
>> [   18.797304]  __arm64_sys_ioctl+0xa4/0xe8
>> [   18.799265]  invoke_syscall+0x48/0x110
>> [   18.801139]  el0_svc_common.constprop.0+0x40/0xe8
>> [   18.803489]  do_el0_svc+0x20/0x2c
>> [   18.805145]  el0_svc+0x30/0xd0
>> [   18.806673]  el0t_64_sync_handler+0x13c/0x158
>> [   18.808850]  el0t_64_sync+0x190/0x194
>> [   18.810680] ---[ end trace 0000000000000000 ]---
>>
> 
> Please add this info to the patch description.

Okay, I'll add the call trace into the patch.

Thank you,

---
Best Regards
Kunihiko Hayashi