cocci.inria.fr archive mirror
 help / color / mirror / Atom feed
From: Markus Elfring <Markus.Elfring@web.de>
To: Wen Yang <wen.yang99@zte.com.cn>, cocci@systeme.lip6.fr
Cc: Yi Wang <wang.yi59@zte.com.cn>,
	Michal Marek <michal.lkml@markovi.net>,
	Nicolas Palix <nicolas.palix@imag.fr>,
	linux-kernel@vger.kernel.org
Subject: Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put
Date: Tue, 7 May 2019 17:27:15 +0200	[thread overview]
Message-ID: <3a3ad66c-833a-b35d-7d75-32189ca67436@web.de> (raw)
In-Reply-To: <1557216744-25339-1-git-send-email-wen.yang99@zte.com.cn>

> 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)


> +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?


> +    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.


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
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

  reply	other threads:[~2019-05-07 15:28 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-07  8:12 [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put Wen Yang
2019-05-07 15:27 ` Markus Elfring [this message]
2019-05-09  1:47   ` wen.yang99
2019-05-09  8:10     ` [Cocci] Coccinelle: " Markus Elfring
  -- strict thread matches above, loose matches on Subject: below --
2019-03-15  2:24 [Cocci] [PATCH] coccinelle: " Wen Yang
2019-03-15  7:29 ` Julia Lawall
2019-03-15 16:24 ` Markus Elfring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3a3ad66c-833a-b35d-7d75-32189ca67436@web.de \
    --to=markus.elfring@web.de \
    --cc=cocci@systeme.lip6.fr \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michal.lkml@markovi.net \
    --cc=nicolas.palix@imag.fr \
    --cc=wang.yi59@zte.com.cn \
    --cc=wen.yang99@zte.com.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).