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

Kunihiko Hayashi
2025-02-10T01:39:50Z

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