diff mbox series

[RFC] coccinelle: Add test for unnecessary check before calling clk_disable_unprepare()

Message ID 20220517133556.6934-1-phil.edworthy@renesas.com (mailing list archive)
State RFC, archived
Headers show
Series [RFC] coccinelle: Add test for unnecessary check before calling clk_disable_unprepare() | expand

Commit Message

Phil Edworthy May 17, 2022, 1:35 p.m. UTC 
The clk_disable_unprepare() function only calls clk_disable() and
clk_unprepare(). Both of these functions already check the clk ptr
passed in using the IS_ERR_OR_NULL macro. Many drivers already omit
any checks on the clk ptr, so it is safe to assume this is true for
all call sites.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
Note that this is the first time I've used coccinelle so there may
be some clangers in the script below.
---
 .../api/clk_disable_unprepare.cocci           | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 scripts/coccinelle/api/clk_disable_unprepare.cocci

Comments

Stephen Boyd May 18, 2022, 6:33 p.m. UTC | #1 
Quoting Phil Edworthy (2022-05-17 06:35:56)
> The clk_disable_unprepare() function only calls clk_disable() and
> clk_unprepare(). Both of these functions already check the clk ptr
> passed in using the IS_ERR_OR_NULL macro. Many drivers already omit
> any checks on the clk ptr, so it is safe to assume this is true for
> all call sites.

Skipping the check for NULL makes sense to me because a NULL clk pointer
is still a valid clk. I'm not so sure about IS_ERR() though. Do the
various implementations of clk_disable() and clk_unprepare() check for
IS_ERR() there?
Phil Edworthy May 18, 2022, 7:05 p.m. UTC | #2 
Hi Stephen,

On 18 May 2022 19:33 Stephen Boyd wrote:
> Quoting Phil Edworthy (2022-05-17 06:35:56)
> > The clk_disable_unprepare() function only calls clk_disable() and
> > clk_unprepare(). Both of these functions already check the clk ptr
> > passed in using the IS_ERR_OR_NULL macro. Many drivers already omit
> > any checks on the clk ptr, so it is safe to assume this is true for
> > all call sites.
> 
> Skipping the check for NULL makes sense to me because a NULL clk pointer
> is still a valid clk. I'm not so sure about IS_ERR() though. Do the
> various implementations of clk_disable() and clk_unprepare() check for
> IS_ERR() there?

clk_disable_unprepare() just calls clk_disable() and then clk_unprepare().
This consists of:

void clk_disable(struct clk *clk)
{
	if (IS_ERR_OR_NULL(clk))
		return;
      ...

and:
void clk_unprepare(struct clk *clk)
{
	if (IS_ERR_OR_NULL(clk))
		return;
      ...

i.e. there is no point in checking the clk ptr before calling
clk_disable_unprepare() like this:

if (!clk)
     clk_disable_unprepare(clk);

or any of these variations:
if (clk != NULL)
if (!IS_ERR(clk))
if (!IS_ERR_OR_NULL(clk))

I've had review comments before saying that the check is unnecessary, so
thought it would be good to catch it with coccinelle.

Thanks
Phil
Phil Edworthy May 20, 2022, 1:57 p.m. UTC | #3 
Hi Markus,

Thanks for the review!

On 19 May 2022 20:52 Markus Elfring wrote:
> > +@depends on patch@
> > +expression clk;
> > +@@
> > +
> > +- if ( \( !clk \| clk != NULL \| !IS_ERR(clk)  \|
> > +!IS_ERR_OR_NULL(clk) \) ) { clk_disable_unprepare(clk); }
> 
> 
> I guess that you would like to mark the curly brackets also for deletion.
> 
> How do you think about to handle simple and compound statements with your
> source code transformation approach?

Since we only want to transform code that just calls clk_disable_unprepare
and nothing else, we tend not to have much code like that, but point taken!
Sorry, but my coccinelle skills are only a few days old so I'll need to
work out how to do that.

> > +@script:python depends on report@
> > +p1 << r.p1;
> > +p2 << r.p2;
> > +@@
> > +
> > +msg = "ERROR: clk_disable_unprepare already checks for valid ptr so
> > +check on line %s is not needed" % (p1[0].line)
> 
> I imagine that it can be a bit clearer to split the message into two
> sentences.
> 
> Can a line break help here?
Sorry, I don’t follow you. Perhaps you could make a suggestion for what
you want?

Thanks
Phil
Phil Edworthy May 20, 2022, 4:01 p.m. UTC | #4 
Hi Markus

On 20 May 2022 16:56 Markus Elfring wrote:
> >> I guess that you would like to mark the curly brackets also for
> deletion.
> >>
> >> How do you think about to handle simple and compound statements with
> your
> >> source code transformation approach?
> > Since we only want to transform code that just calls
> clk_disable_unprepare
> > and nothing else, we tend not to have much code like that, but point
> taken!
> > Sorry, but my coccinelle skills are only a few days old so I'll need to
> > work out how to do that.
> 
> 
> I suggest to become more familiar with applications of disjunctions also
> together with the semantic patch language.
> https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/82e87d4b0a9c978b001511270c652a93e72fe64d/docs/manual/cocci_syntax.tex#L1039
> 
> Example of SmPL code:
> (
> -if ( \( !clk \| clk != NULL \| !IS_ERR(clk)  \| !IS_ERR_OR_NULL(clk) \) )
>  clk_disable_unprepare(clk);
> |
> -if ( \( !clk \| clk != NULL \| !IS_ERR(clk)  \| !IS_ERR_OR_NULL(clk) \) )
> -{
>  clk_disable_unprepare(clk);
> -}
> )
Thanks, I'll have a read.

> By the way:
> Would you like to add a SmPL rule for the operation mode "context"?
Right.


> >>> +@script:python depends on report@
> >>> +p1 << r.p1;
> >>> +p2 << r.p2;
> >>> +@@
> >>> +
> >>> +msg = "ERROR: clk_disable_unprepare already checks for valid ptr so
> >>> +check on line %s is not needed" % (p1[0].line)
> >> I imagine that it can be a bit clearer to split the message into two
> >> sentences.
> >>
> >> Can a line break help here?
> > Sorry, I don't follow you. Perhaps you could make a suggestion for what
> > you want?
> 
> 
> The support might be unclear for multi-line messages.
> 
> How do you think about the following code variant?
> 
> 
> coccilib.report.print_report(p2[0],
>                              "ERROR: clk_disable_unprepare() already
> checks for valid pointer. Thus the repeated check is not needed on line
> %s." % (p1[0].line))
Looks ok to me

Thanks
Phil
diff mbox series

Patch

diff --git a/scripts/coccinelle/api/clk_disable_unprepare.cocci b/scripts/coccinelle/api/clk_disable_unprepare.cocci
new file mode 100644
index 000000000000..17f00d3653c9
--- /dev/null
+++ b/scripts/coccinelle/api/clk_disable_unprepare.cocci
@@ -0,0 +1,48 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/// Unnecessary check for valid ptr before calling clk_disable_unprepare
+///
+// Confidence: Moderate
+// Copyright: (C) 2022 Phil Edworthy
+// URL: http://coccinelle.lip6.fr/
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@depends on patch@
+expression clk;
+@@
+
+- if ( \( !clk \| clk != NULL \| !IS_ERR(clk)  \| !IS_ERR_OR_NULL(clk) \) )
+{
+clk_disable_unprepare(clk);
+}
+
+@r depends on !patch exists@
+expression clk;
+position p1,p2;
+@@
+
+if@p1 ( \( !clk \| clk != NULL \| !IS_ERR(clk)  \| !IS_ERR_OR_NULL(clk) \) )
+{
+clk_disable_unprepare@p2(clk);
+}
+
+@script:python depends on org@
+p1 << r.p1;
+p2 << r.p2;
+@@
+
+cocci.print_main("clk_disable_unprepare call",p1)
+cocci.print_secs("Unnecessary check tests",p2)
+
+@script:python depends on report@
+p1 << r.p1;
+p2 << r.p2;
+@@
+
+msg = "ERROR: clk_disable_unprepare already checks for valid ptr so check on line %s is not needed" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)