Re: [PATCH] coccinelle: Detect clk_register() anti-pattern

Hi Julia,

Thank you so much for the review.

On Sun, Aug 02, 2026 at 11:01:08AM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 2 Aug 2026, Guru Das Srinagesh wrote:

--->8---

> > diff --git a/scripts/coccinelle/api/clk_register.cocci b/scripts/coccinelle/api/clk_register.cocci
> > new file mode 100644
> > index 000000000000..fe5bcd4b00d4
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/clk_register.cocci
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/// Remove error messages after clk registration failures, because
> > +/// clk_register(), clk_hw_register(), and their variants already log
> > +/// an error when they fail. See commit 12a0fd23e870 ("clk: Print an
> > +/// error when clk registration fails").
> > +//
> > +// Confidence: Medium
> > +// Options: --include-headers
> > +
> > +virtual patch
> > +virtual context
> > +virtual org
> > +virtual report
> > +
> > +@depends on context@
> > +expression clk;
> > +identifier reg =~ "^(devm_)?clk_register$";
> > +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> > +@@
> > +
> > +clk = reg(...);
> 
> From a performance point of view, it would be desirable to put instead
> \(devm_clk_register\|clk_register\).  This will trigger some
> optimizations that won't be triggered by a regular expression.

Done.

> 
> 
> > +if ( \( IS_ERR(clk) \| IS_ERR(clk) == 1 \) )
> > +{
> > +...
> > +*voidfn(...);
> > +...
> > +}
> > +
> > +@depends on patch@
> > +expression clk;
> > +identifier reg =~ "^(devm_)?clk_register$";
> > +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> > +@@
> > +
> > +clk = reg(...);
> > +if ( \( IS_ERR(clk) \| IS_ERR(clk) == 1 \) )
> > +{
> > +...
> > +-voidfn(...);
> > +...
> > +}
> > +
> > +@r1 depends on org || report@
> > +position p1;
> > +expression clk;
> > +identifier reg =~ "^(devm_)?clk_register$";
> > +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> > +@@
> > +
> > +clk = reg(...);
> > +if ( \( IS_ERR(clk) \| IS_ERR(clk) == 1 \) )
> > +{
> > +...
> > +voidfn@p1(...);
> > +...
> > +}
> > +
> > +@depends on context@
> > +expression ret;
> > +identifier reg =~ "^(devm_)?(of_)?clk_hw_register$";
> > +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> > +@@
> > +
> > +ret = reg(...);
> > +if ( \( ret \| ret != 0 \| ret < 0 \) )
> > +{
> > +...
> > +*voidfn(...);
> > +...
> > +}
> > +
> > +@depends on patch@
> > +expression ret;
> > +identifier reg =~ "^(devm_)?(of_)?clk_hw_register$";
> > +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> > +@@
> > +
> > +ret = reg(...);
> > +if ( \( ret \| ret != 0 \| ret < 0 \) )
> > +{
> > +...
> > +-voidfn(...);
> > +...
> > +}
> 
> I think yourpatch rule should have an extra case for where there are
> currently only two statements in the if branch.  In that case the {}
> should be removed.
> 
> julia

Done.

> 
> > +
> > +@r2 depends on org || report@
> > +position p2;
> > +expression ret;

--->8---

Thank you.

Guru Das.