[PATCH] coccinelle: put_device: Include of_node_put to avoid false positives

[PATCH] coccinelle: put_device: Include of_node_put to avoid false positives

[Deepak R Varma]

The node reference increased by of_find_device_by_node() can also be
released by using a call to of_node_put(). Hence when this exists, the
script should not trigger a warning, which otherwise will be a false
positive.
Also, improve the warning message to include of_node_put too is missing.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
 scripts/coccinelle/free/put_device.cocci | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/coccinelle/free/put_device.cocci b/scripts/coccinelle/free/put_device.cocci
index f09f1e79bfa6..259195b501aa 100644
--- a/scripts/coccinelle/free/put_device.cocci
+++ b/scripts/coccinelle/free/put_device.cocci
@@ -18,8 +18,10 @@ type T,T1,T2,T3;
 
 id = of_find_device_by_node@p1(x)
 ... when != e = id
+    when != of_node_put(x)
 if (id == NULL || ...) { ... return ...; }
 ... when != put_device(&id->dev)
+    when != of_node_put(x)
     when != platform_device_put(id)
     when != if (id) { ... put_device(&id->dev) ... }
     when != e1 = (T)id
@@ -42,7 +44,7 @@ p2 << search.p2;
 @@
 
 coccilib.report.print_report(p2[0],
-                             "ERROR: missing put_device; call of_find_device_by_node on line "
+                             "ERROR: missing put_device or of_node_put; call of_find_device_by_node on line "
                              + p1[0].line
                              + ", but without a corresponding object release within this function.")
 
-- 
2.34.1

Re: [PATCH] coccinelle: put_device: Include of_node_put to avoid false positives

[Julia Lawall]

> The node reference increased by of_find_device_by_node() can also be
> released by using a call to of_node_put(). Hence when this exists, the
> script should not trigger a warning, which otherwise will be a false
> positive.

Could you explain more about why of_node_put is sufficient?

thanks,
julia

> Also, improve the warning message to include of_node_put too is missing.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> scripts/coccinelle/free/put_device.cocci | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/coccinelle/free/put_device.cocci
> b/scripts/coccinelle/free/put_device.cocci
> index f09f1e79bfa6..259195b501aa 100644
> --- a/scripts/coccinelle/free/put_device.cocci
> +++ b/scripts/coccinelle/free/put_device.cocci
> @@ -18,8 +18,10 @@ type T,T1,T2,T3;
> 
> id = of_find_device_by_node@p1(x)
> ... when != e = id
> +    when != of_node_put(x)
> if (id == NULL || ...) { ... return ...; }
> ... when != put_device(&id->dev)
> +    when != of_node_put(x)
>     when != platform_device_put(id)
>     when != if (id) { ... put_device(&id->dev) ... }
>     when != e1 = (T)id
> @@ -42,7 +44,7 @@ p2 << search.p2;
> @@
> 
> coccilib.report.print_report(p2[0],
> -                             "ERROR: missing put_device; call
> of_find_device_by_node on line "
> +                             "ERROR: missing put_device or of_node_put; call
> of_find_device_by_node on line "
>                              + p1[0].line
>                              + ", but without a corresponding object release within this function.")
> 
> --
> 2.34.1

Re: [PATCH] coccinelle: put_device: Include of_node_put to avoid false positives

[Deepak R Varma]

On Sat, Feb 25, 2023 at 08:17:01PM +0100, Julia Lawall wrote:
> > The node reference increased by of_find_device_by_node() can also be
> > released by using a call to of_node_put(). Hence when this exists, the
> > script should not trigger a warning, which otherwise will be a false
> > positive.
> 
> Could you explain more about why of_node_put is sufficient?

You are right. In fact, I think calling of_node_put() for a prior
of_find_device_by_node() is buggy as a call to of_find_device_by_node()
increments the kref for the device embedding the node and not the kref of the
node. Hence a call to put_device() is required to decrement the device kref.
Calling the of_node_put() attempts to decrement the kref of the node, which I
think is not correct.

May I request any comment on my understanding?

Thank you,
deepak.

> 
> thanks,
> julia
> 
> > Also, improve the warning message to include of_node_put too is missing.
> > 
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> > scripts/coccinelle/free/put_device.cocci | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/coccinelle/free/put_device.cocci
> > b/scripts/coccinelle/free/put_device.cocci
> > index f09f1e79bfa6..259195b501aa 100644
> > --- a/scripts/coccinelle/free/put_device.cocci
> > +++ b/scripts/coccinelle/free/put_device.cocci
> > @@ -18,8 +18,10 @@ type T,T1,T2,T3;
> > 
> > id = of_find_device_by_node@p1(x)
> > ... when != e = id
> > +    when != of_node_put(x)
> > if (id == NULL || ...) { ... return ...; }
> > ... when != put_device(&id->dev)
> > +    when != of_node_put(x)
> >     when != platform_device_put(id)
> >     when != if (id) { ... put_device(&id->dev) ... }
> >     when != e1 = (T)id
> > @@ -42,7 +44,7 @@ p2 << search.p2;
> > @@
> > 
> > coccilib.report.print_report(p2[0],
> > -                             "ERROR: missing put_device; call
> > of_find_device_by_node on line "
> > +                             "ERROR: missing put_device or of_node_put; call
> > of_find_device_by_node on line "
> >                              + p1[0].line
> >                              + ", but without a corresponding object release within this function.")
> > 
> > --
> > 2.34.1

Re: [PATCH] coccinelle: put_device: Include of_node_put to avoid false positives

[Julia Lawall]

On Mon, 27 Feb 2023, Deepak R Varma wrote:

> On Sat, Feb 25, 2023 at 08:17:01PM +0100, Julia Lawall wrote:
> > > The node reference increased by of_find_device_by_node() can also be
> > > released by using a call to of_node_put(). Hence when this exists, the
> > > script should not trigger a warning, which otherwise will be a false
> > > positive.
> >
> > Could you explain more about why of_node_put is sufficient?
>
> You are right. In fact, I think calling of_node_put() for a prior
> of_find_device_by_node() is buggy as a call to of_find_device_by_node()
> increments the kref for the device embedding the node and not the kref of the
> node. Hence a call to put_device() is required to decrement the device kref.
> Calling the of_node_put() attempts to decrement the kref of the node, which I
> think is not correct.
>
> May I request any comment on my understanding?

I think that decrementing a kref and reaching 0 would trigger some cleanup
action.  I don't know what that cleanup action is in this case.

Did someone tell you that of_node_put was good enough?  Perhaps that
person could provide more explanation.

In looking quickly though the code, the focus seemed to be on the device.
The of node is just used for comparison to check that we have the right
one.  But I don't know if cleaning up the of node could somehow trigger
cleaning up the device as well.

julia

>
> Thank you,
> deepak.
>
> >
> > thanks,
> > julia
> >
> > > Also, improve the warning message to include of_node_put too is missing.
> > >
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > > scripts/coccinelle/free/put_device.cocci | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/coccinelle/free/put_device.cocci
> > > b/scripts/coccinelle/free/put_device.cocci
> > > index f09f1e79bfa6..259195b501aa 100644
> > > --- a/scripts/coccinelle/free/put_device.cocci
> > > +++ b/scripts/coccinelle/free/put_device.cocci
> > > @@ -18,8 +18,10 @@ type T,T1,T2,T3;
> > >
> > > id = of_find_device_by_node@p1(x)
> > > ... when != e = id
> > > +    when != of_node_put(x)
> > > if (id == NULL || ...) { ... return ...; }
> > > ... when != put_device(&id->dev)
> > > +    when != of_node_put(x)
> > >     when != platform_device_put(id)
> > >     when != if (id) { ... put_device(&id->dev) ... }
> > >     when != e1 = (T)id
> > > @@ -42,7 +44,7 @@ p2 << search.p2;
> > > @@
> > >
> > > coccilib.report.print_report(p2[0],
> > > -                             "ERROR: missing put_device; call
> > > of_find_device_by_node on line "
> > > +                             "ERROR: missing put_device or of_node_put; call
> > > of_find_device_by_node on line "
> > >                              + p1[0].line
> > >                              + ", but without a corresponding object release within this function.")
> > >
> > > --
> > > 2.34.1
>
>
>