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_______________________________________________
Cocci mailing list
[email protected]
https://systeme.lip6.fr/mailman/listinfo/cocci
