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

[Guru Das Srinagesh <linux@gurudas.dev>]

On Sun, Aug 09, 2026 at 10:00:22PM +0200, Julia Lawall wrote:
> 
> 
> On Mon, 3 Aug 2026, Guru Das Srinagesh wrote:
> 
> > Enforce commit 12a0fd23e870 ("clk: Print an error when clk registration
> > fails"): clk_register(), clk_hw_register(), and their devm_/of_ variants
> > log their own error on failure, so driver-side error prints after these
> > calls are redundant.
> >
> > Two independent match families, one per return-value convention:
> > pointer return checked via IS_ERR() (clk_register()/devm_clk_register()),
> > and int return checked via a nonzero value (clk_hw_register()/
> > devm_clk_hw_register()/of_clk_hw_register()).
> >
> > In "patch" mode, removing the redundant message also collapses the
> > enclosing braces when only one statement remains, and deletes the whole
> > "if" when the message was already the only (braceless) statement.
> >
> > Assisted-by: Claude:claude-sonnet-5 coccinelle
> > Signed-off-by: Guru Das Srinagesh <linux@gurudas.dev>
> > ---
> > Add a Coccinelle semantic patch enforcing commit 12a0fd23e870 ("clk:
> > Print an error when clk registration fails"): flags, and in "patch"
> > mode removes, driver-side error prints that are now redundant after
> > clk_register()/clk_hw_register() and their devm_/of_ variants.
> >
> > Two independent match families, one per return-value convention.
> >
> > Pointer return, IS_ERR()-checked (clk_register()/devm_clk_register()),
> > e.g. drivers/clk/clk-xgene.c:152-157:
> >
> >     clk = clk_register(dev, &apmclk->hw);
> >     if (IS_ERR(clk)) {
> > -       pr_err("%s: could not register clk %s\n", __func__, name);
> >         kfree(apmclk);
> >         return NULL;
> >     }
> >
> > Int return, nonzero-checked (clk_hw_register()/devm_clk_hw_register()/
> > of_clk_hw_register()), e.g. drivers/clk/meson/meson-clkc-utils.c:49-54:
> >
> >     ret = devm_clk_hw_register(dev, hw);
> > -   if (ret) {
> > -           dev_err(dev, "registering %s clock failed\n",
> > -                   hw->init->name);
> > +   if (ret)
> >         return ret;
> > -   }
> >
> > Already-braceless single-statement case: the whole "if" is deleted
> > instead of just the message, e.g. drivers/clk/ux500/clk-sysctrl.c:171-175:
> >
> >     clk_reg = devm_clk_register(clk->dev, &clk->hw);
> > -   if (IS_ERR(clk_reg))
> > -           dev_err(dev, "clk_sysctrl: clk_register failed\n");
> >
> >     return clk_reg;
> >
> > Testing:
> > - Baseline: coccinelle 1.3.1, the Torvalds tree at v7.2-rc5.
> > - "make coccicheck COCCI=<path> MODE=report M=drivers/clk" produced 73
> >   hits and verified to have zero false positives.
> > - "MODE=patch" verified separately on scratch copies of affected files
> >   to confirm minimal, correct diffs.
> >
> > checkpatch flagged that this new file needs a MAINTAINERS entry, and I'd
> > like to maintain it, so this adds a standalone entry rather than leaving
> > the file uncovered. There's no direct precedent for an individual .cocci
> > file getting its own entry - the only other named .cocci file in
> > MAINTAINERS, scripts/coccinelle/api/string_choices.cocci, was added to
> > the existing GENERIC STRING LIBRARY entry by that subsystem's
> > maintainer, not as a new one. Happy to fold this into COMMON CLK
> > FRAMEWORK or drop it entirely depending on what the maintainers prefer.
> > ---
> > Changes in v2 (Julia):
> > - Use a literal function-name disjunction instead of a regex identifier,
> >   enabling spatch's file pre-filter optimization.
> > - In "patch" mode, drop braces when only one statement remains, and
> >   delete the whole "if" when the message was the only (braceless)
> >   statement.
> > - Drop two never-observed condition variants (IS_ERR(clk) == 1, ret !=
> >   0); keep the one with real precedent (ret < 0).
> > - Link to v1: https://patch.msgid.link/20260802-cocci-clk-register-v1-1-df68afcb1eef@gurudas.dev
> > ---
> >  MAINTAINERS                               |   5 +
> >  scripts/coccinelle/api/clk_register.cocci | 153 ++++++++++++++++++++++++++++++
> >  2 files changed, 158 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 716acfc3d7c1..26788ccbf98c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6363,6 +6363,11 @@ L:	linux-clk@vger.kernel.org
> >  S:	Maintained
> >  F:	include/linux/clk.h
> >
> > +CLK_REGISTER() COCCINELLE CHECK
> > +M:	Guru Das Srinagesh <linux@gurudas.dev>
> > +S:	Maintained
> > +F:	scripts/coccinelle/api/clk_register.cocci
> > +
> 
> I don't think there needs to be a specific maintainer for this file.

Removed.

> 
> >  CLOCKSOURCE, CLOCKEVENT DRIVERS
> >  M:	Daniel Lezcano <daniel.lezcano@kernel.org>
> >  M:	Thomas Gleixner <tglx@kernel.org>
> > diff --git a/scripts/coccinelle/api/clk_register.cocci b/scripts/coccinelle/api/clk_register.cocci
> > new file mode 100644
> > index 000000000000..86a8be92f2a2
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/clk_register.cocci
> > @@ -0,0 +1,153 @@
> > +// 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 voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> > +@@
> > +
> > +clk = \(clk_register\|devm_clk_register\)(...);
> > +if ( IS_ERR(clk) )
> > +{
> > +...
> > +*voidfn(...);
> > +...
> > +}
> 
> This will only match an if with a then branch, no else branch.  There is
> actually one case in the kernel where there is a relevant if with an else
> branch, so you may as well extend this to match that cas too.  If you just
> add else S, where S is a statement metavariable that is not used
> elsewhere, then it will match either the case with an else or the case
> with a then branch only.
> 
> Likewise for the other rules.

Done, thank you for catching the edge case which I'm guessing is
drivers/media/platform/microchip/microchip-isc-clk.c.

> 
> [...]
> 
> > +@script:python depends on report@
> > +p1 << r1.p1;
> > +@@
> > +
> > +msg = "line %s is redundant because clk_register() already prints an error on failure" % (p1[0].line)
> > +coccilib.report.print_report(p1[0], msg)
> > +
> > +@script:python depends on org@
> > +p2 << r2.p2;
> > +@@
> > +
> > +cocci.print_main(p2)
> 
> This code is not correct. print_main seens a string as the first argument
> and a position (which you have) as the second argument.  You can make a
> string in the same was as in the report rules.
> 
> The same is true for the other org mode rule.  If you don't have org mode,
> you can still check that the code works by testing the semantic patch with
> -D org.  It's just a text format.

Done, sorry for the oversight.

> 
> julia
> 
> > +
> > +@script:python depends on report@
> > +p2 << r2.p2;
> > +@@
> > +
> > +msg = "line %s is redundant because clk_hw_register() already prints an error on failure" % (p2[0].line)
> > +coccilib.report.print_report(p2[0], msg)
> >
> > ---
> > base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
> > change-id: 20260802-cocci-clk-register-951d94251af4
> >
> > Best regards,
> > --
> > Guru Das Srinagesh <linux@gurudas.dev>
> >
> >
>