Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

* Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put
@ 2019-05-09  1:47 wen.yang99
  2019-05-09  8:10 ` [Cocci] Coccinelle: " Markus Elfring
  0 siblings, 1 reply; 15+ messages in thread
From: wen.yang99 @ 2019-05-09  1:47 UTC (permalink / raw)
  To: Markus.Elfring; +Cc: wang.yi59, michal.lkml, nicolas.palix, linux-kernel, cocci

[-- Attachment #1.1: Type: text/plain, Size: 4235 bytes --]

Hi Markus,
Thanks for the review.  

> > The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
> > pointer with refcount incremented thus it must be explicitly decremented
> > after the last usage.
> >
> > This SmPL is also looking for places where there is an of_node_put on
> > some path but not on others.
> 
> I suggest to improve this commit description.
> 
> * Possible wording:
> There are functions which increment a reference counter for a device node.
> These functions belong to a programming interface for the management
> of information from device trees.
> The counter must be decremented after the last usage of a device node.
> 
> This SmPL script looks also for places where a of_node_put() call is on
> some paths but not on others.
> 
> * Will the word “patch” be replaced by “code search” in the commit subject
> because the operation modes “report” and “org” are supported here?
> 
> 
> > +@initialize:python@
> > +@@
> 
> Such a SmPL rule would apply to every possible operation mode.
> I have noticed then that the two Python variables from here will be needed
> only in two SmPL rules which depend on the mode “report”.
> 
> * Thus I would prefer to adjust the dependency specification accordingly.
> 
> * Please replace these variables by a separate function like
> the following.
> def display1(p1 ,p2):
> if add_if_not_present(p1[0].line, p2[0].line):
> coccilib.report.print_report(p2[0],
> "prefix"
> + p1[0].line
> + "suffix")
> 
> 
> * Please move another bit of duplicate code to a separate function like
> the following.
> def display2(p1 ,p2):
> cocci.print_main("Choose info 1", p1)
> cocci.print_secs("Choose info 2", p2)
> 
Thanks.
I will update the patch according to your suggestions above.

> > +x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
> 
> If you would like to insist to use such a SmPL disjunction, I would prefer
> an other code formatting here.
> How do you think about to put each function name on a separate line?
> 
> Can such a name list be ever automatically determined from an other
> information source?
> (Are there circumstances to consider under which the application of
> a detailed regular expression would become interesting for a SmPL constraint?)
> 
> Will it be influenced by any sort criteria?
> 
Thanks. 
It's interesting to get the function list automatically.
I'll try to parse the drivers/of/base.c file based on comments like this
"* Returns a node pointer with refcount incremented, use
* of_node_put() on it when done."
to automatically get the name of the function that needs to be checked.

> > +    when != of_node_put(x)
> …
> > +    when != if (x) { ... of_node_put(x) ... }
> 
> I find the second when constraint specification unnecessary because
> the previous one should be sufficient to exclude such a function call.
> 
Thanks.
I added the "when != if (x) { ... of_node_put(x) ... }" statement to avoid
 false positives similar to the following:
./arch/powerpc/platforms/powermac/setup.c:513:1-7: ERROR: missing of_node_put; 
acquired a node pointer with refcount incremented on line 503, 
but without a corresponding object release within this function.

484 static int __init pmac_declare_of_platform_devices(void)
485 {
486         struct device_node *np;
...
503         np = of_find_node_by_type(NULL, "fcu");
504         if (np == NULL) {
505                 /* Some machines have strangely broken device-tree */
506                 np = of_find_node_by_path("/u3@0,f8000000/i2c@f8001000/fan@15e");
507         }
508         if (np) {
509                 of_platform_device_create(np, "temperature", NULL);
510                 of_node_put(np);
511         }
512 
513         return 0;
514 }

We will continue to analyze the code of coccinelle to confirm whether
this false positive is a bug in coccinelle.
But this statement is currently needed here.

--
Regards,
Wen

> 
> Can the specification “when != \( of_node_put \| of_get_next_parent \) (x)”
> be useful?
> 
> 
> > +return x;
> > +|
> > +return of_fwnode_handle(x);
> 
> Can it be nicer to merge this bit of code into another SmPL disjunction?
> 
> +return \( x \| of_fwnode_handle(x) \);
> 
> 
> Regards,
> Markus

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 15+ messages in thread