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

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

[Phil Edworthy]

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

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

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

[Phil Edworthy]

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

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

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

[Stephen Boyd]

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?

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

[Stephen Boyd]

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?

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

[Phil Edworthy]

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

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

[Phil Edworthy]

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

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

[Stephen Boyd]

Quoting Phil Edworthy (2022-05-18 12:05:25)
> On 18 May 2022 19:33 Stephen Boyd wrote:
> > 
> > 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;
>       ...

That is the implementation in drivers/clk/clk.c correct? What about the
other implementations of clk_disable()

	$ git grep "void clk_disable("
	arch/arm/mach-omap1/clock.c:void clk_disable(struct clk *clk)
	arch/m68k/coldfire/clk.c:void clk_disable(struct clk *clk)
	arch/mips/bcm63xx/clk.c:void clk_disable(struct clk *clk)
	arch/mips/lantiq/clk.c:void clk_disable(struct clk *clk)
	drivers/clk/clk.c:void clk_disable(struct clk *clk)
	drivers/sh/clk/core.c:void clk_disable(struct clk *clk)

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

Maybe those drivers never run on these architectures that haven't
converted over to the common clk framework (i.e. drivers/clk/clk.c).

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

[Markus Elfring]

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



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


Regards,
Markus

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

[Phil Edworthy]

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

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

[Markus Elfring]

>> 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
https://github.com/coccinelle/coccinelle/blob/ae337fce1512ff15aabc3ad5b6d2e537f97ab62a/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);
-}
)




By the way:
Would you like to add a SmPL rule for the operation mode “context”?



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


Regards,
Markus

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

[Phil Edworthy]

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

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

[Markus Elfring]

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


* I find that the variable names should be swapped for these two function calls.

* Will any message variants become more helpful here?

Regards,
Markus

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

[Markus Elfring]

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


How do you think about a wording variant for the commit description
like the following?

The pointer which is passed as the parameter “clk” to these functions
is checked already by 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.


I suggest to adjust this information and conclusion.

Do you expect that all driver implementations can depend on such functions
which perform a well-known input parameter validation?


I propose to extend source code analyses and corresponding transformations
accordingly also by the means of the semantic patch language.

The mentioned macro detects null and error pointers.
This detection can obviously be split into two cases.

Various contributors showed special care for the handling of null pointers.

See also:
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/scripts/coccinelle/free/ifnullfree.cocci?h=v5.18-rc7

* https://build.opensuse.org/package/show/home:elfring:semantic_patching:Deletion_of_checks_before_specific_function_calls/Deletion_of_checks_before_specific_function_calls_in_Linux_source_files


Can the software development attention grow for error pointers in similar ways
(besides existing analysis approaches)?


…
> +++ b/scripts/coccinelle/api/clk_disable_unprepare.cocci
…
> +/// Unnecessary check for valid ptr before calling clk_disable_unprepare


Point questionable checks out before calling clk_disable_unprepare().
Some pointer checks do not need to be repeated because called functions perform
input parameter validation before further data processing will happen.



> +// URL: http://coccinelle.lip6.fr/
> +// Comments:


Would you like to add anything for the attribute “Keywords”?

Regards,
Markus