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

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

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