From: Zijun Hu <quic_zijuhu@quicinc.com>

Simplify devm_free_irq() implementation by dedicated API devres_release()
which have below advantages than current devres_destroy() + free_irq().

It is simpler if devm_free_irq() is undoing what any devm_request_irq()
variant did, otherwise, it can avoid wrong and undesired free_irq().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
linux-next tree has similar fixes as shown below:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0ee4dcafda9576910559f0471a3d6891daf9ab92
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=9bd133f05b1dca5ca4399a76d04d0f6f4d454e44
---
 kernel/irq/devres.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
index b3e98668f4dd..eb16a58e0322 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -141,9 +141,8 @@ void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id)
 {
 	struct irq_devres match_data = { irq, dev_id };
 
-	WARN_ON(devres_destroy(dev, devm_irq_release, devm_irq_match,
+	WARN_ON(devres_release(dev, devm_irq_release, devm_irq_match,
 			       &match_data));
-	free_irq(irq, dev_id);
 }
 EXPORT_SYMBOL(devm_free_irq);
 

---
base-commit: 9bd133f05b1dca5ca4399a76d04d0f6f4d454e44
change-id: 20241017-devres_kernel_fix-4b29be51bded

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

On Thu, Oct 17 2024 at 23:16, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> Simplify devm_free_irq() implementation by dedicated API devres_release()
> which have below advantages than current devres_destroy() + free_irq().
>
> It is simpler if devm_free_irq() is undoing what any devm_request_irq()
> variant did, otherwise, it can avoid wrong and undesired free_irq().

This is confusing at best. What's the wrong an undesired free_irq()?

Thanks,

        tglx

On 2024/10/18 04:33, Thomas Gleixner wrote:
> On Thu, Oct 17 2024 at 23:16, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Simplify devm_free_irq() implementation by dedicated API devres_release()
>> which have below advantages than current devres_destroy() + free_irq().
>>
>> It is simpler if devm_free_irq() is undoing what any devm_request_irq()
>> variant did, otherwise, it can avoid wrong and undesired free_irq().
> 
> This is confusing at best. What's the wrong an undesired free_irq()?
> 

thanks for code review.

for current devm_free_irq(..., irq_A, ...):
it is undesired if irq_A is requested by request_irq().
it is wrong and dangerous if irq_A was never requested.

sorry to have confused commit message.
any suggestion to correct it ? (^^)

> Thanks,
> 
>         tglx

On Fri, Oct 18 2024 at 06:36, Zijun Hu wrote:
> On 2024/10/18 04:33, Thomas Gleixner wrote:
>>> It is simpler if devm_free_irq() is undoing what any devm_request_irq()
>>> variant did, otherwise, it can avoid wrong and undesired free_irq().
>> 
>> This is confusing at best. What's the wrong an undesired free_irq()?
>> 
> for current devm_free_irq(..., irq_A, ...):
> it is undesired if irq_A is requested by request_irq().
> it is wrong and dangerous if irq_A was never requested.

There is nothing dangerous about it if it was never requested.
free_irq() won't find a irq action which matches devid and do nothing
than emitting a warning.

But that's not relevant either because there is no matching devres entry
when the interrupt was not requested via devres_request_irq(), so
free_irq() will not be reached because devres_destroy() will return
-ENOENT.

So all this change does is changing the logic from:

devres_free_irq()
    if (devres_destroy())
        return;
    free_irq();
    
to
    devres_release()

where devres_release() does the same thing as the above, i.e. it looks
up the devres for a match and if found, it removes and frees the devres
pointer and invokes the release function which in turn invokes
free_irq().

So in terms of code logic this is exactly the same and does neither
avoid or prevent anything.

All it does is sparing a single line of code.

Thanks,

        tglx

On 10/18/2024 4:57 PM, Thomas Gleixner wrote:
> On Fri, Oct 18 2024 at 06:36, Zijun Hu wrote:
>> On 2024/10/18 04:33, Thomas Gleixner wrote:
>>>> It is simpler if devm_free_irq() is undoing what any devm_request_irq()
>>>> variant did, otherwise, it can avoid wrong and undesired free_irq().
>>>
>>> This is confusing at best. What's the wrong an undesired free_irq()?
>>>
>> for current devm_free_irq(..., irq_A, ...):
>> it is undesired if irq_A is requested by request_irq().
>> it is wrong and dangerous if irq_A was never requested.
> 
> There is nothing dangerous about it if it was never requested.
> free_irq() won't find a irq action which matches devid and do nothing
> than emitting a warning.
> 

you are right. it is not dangerous but a unnecessary call of free_irq().

> But that's not relevant either because there is no matching devres entry
> when the interrupt was not requested via devres_request_irq(), so
> free_irq() will not be reached because devres_destroy() will return
> -ENOENT.
> 
no, WARN_ON(devres_destroy()) doesn't return, the next free_irq() will
be still reached.

> So all this change does is changing the logic from:
> 
> devres_free_irq()
>     if (devres_destroy())
>         return;
>     free_irq();
>     
> to
>     devres_release()
> 

if irq to free was ever requested by devm_request_irq()
then both logic is exactly same.

otherwise, actually change devres_free_irq()'s logic from

if (irq is not requested by devm_request_irq() {
	warn;
}
free_irq()

To

if (irq is not requested by devm_request_irq() {
	warn;
	return;
}


> where devres_release() does the same thing as the above, i.e. it looks
> up the devres for a match and if found, it removes and frees the devres
> pointer and invokes the release function which in turn invokes
> free_irq().
> 
> So in terms of code logic this is exactly the same and does neither
> avoid or prevent anything.
> 
> All it does is sparing a single line of code.
> 
> Thanks,
> 
>         tglx
> 
> 
> 
>

On Fri, Oct 18 2024 at 17:28, quic zijuhu wrote:
> On 10/18/2024 4:57 PM, Thomas Gleixner wrote:
> if irq to free was ever requested by devm_request_irq()
> then both logic is exactly same.
>
> otherwise, actually change devres_free_irq()'s logic from
>
> if (irq is not requested by devm_request_irq() {
> 	warn;
> }
> free_irq()
>
> To
>
> if (irq is not requested by devm_request_irq() {
> 	warn;
> 	return;
> }
>

Ah, you are right. I thought there is a return there.

So you want to explain it maybe this way:

   If devres_destroy() does not find a matching devres entry, then
   devm_free_irq() emits a warning and tries to free the interrupt.

   That's wrong as devm_free_irq() should only undo what
   devm_request_irq() set up.

   Replace devres_destroy() with a call to devres_release() which only
   invokes the release function (free_irq()) in case that a matching
   devres entry was found.

Or something like that.

Thanks,

        tglx

On 2024/10/18 18:58, Thomas Gleixner wrote:
> On Fri, Oct 18 2024 at 17:28, quic zijuhu wrote:
>> On 10/18/2024 4:57 PM, Thomas Gleixner wrote:
>> if irq to free was ever requested by devm_request_irq()
>> then both logic is exactly same.

[snip]

>> if (irq is not requested by devm_request_irq() {
>> 	warn;
>> 	return;
>> }
>>
> 
> Ah, you are right. I thought there is a return there.
> 
> So you want to explain it maybe this way:
> 
>    If devres_destroy() does not find a matching devres entry, then
>    devm_free_irq() emits a warning and tries to free the interrupt.
> 
>    That's wrong as devm_free_irq() should only undo what
>    devm_request_irq() set up.
> 
>    Replace devres_destroy() with a call to devres_release() which only
>    invokes the release function (free_irq()) in case that a matching
>    devres entry was found.
> 

thank you. good proposal, let me update commit base on it and send v2.

> Or something like that.
> 
> Thanks,
> 
>         tglx