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

On Sun, 9 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()). Both families match
> regardless of whether the redundant message's "if" also has a trailing
> "else", via an "else S" clause with S otherwise unused.
>
> 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 <[email protected]>
> ---

Hello,

Thanks for the update, which I will look into shortly.

But you are not structuring your patches in the right way.  The part above
the first --- is what will go into the git comit.  That looks fine.

But I don't understand what is the big pile of text below the ---.  When
there is a new version, I want to see what has changed as quickly as
possible.  So the changes in v3 should be the first thing that I see below
the ---, not the last thing.

For the following information, I'm not sure it should be here at all.
Either it is important, in which case it should be above the --- so people
in the future can easily benefit from it, or it is not important, in which
case it should be dropped.

julia


> 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;
>
> Matches regardless of whether the "if" also has a trailing "else", via
> an "else S" clause with S otherwise unused, e.g.
> drivers/media/platform/microchip/microchip-isc-clk.c:269-275:
>
>     isc_clk->clk = clk_register(isc->dev, &isc_clk->hw);
> -   if (IS_ERR(isc_clk->clk)) {
> -           dev_err(isc->dev, "%s: clock register fail\n", clk_name);
> +   if (IS_ERR(isc_clk->clk))
>         return PTR_ERR(isc_clk->clk);
> -   } else if (id == ISC_MCK) {
> +   else if (id == ISC_MCK) {
>         of_clk_add_provider(np, of_clk_src_simple_get, isc_clk->clk);
>     }
>
> 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, unchanged after this revision, and verified to have zero false
>   positives.
> - All four modes (report/context/patch/org) verified via "make
>   coccicheck COCCI=<path> MODE=<mode> [M=<path>]" against the
>   drivers/clk baseline and the new else-branch case above.
> - "make coccicheck COCCI=<path> MODE=report" (whole tree, no M=) finds
>   94 hits; the 21 outside drivers/clk are not part of this series.
> ---
> Changes in v3 (Julia):
> - Match an "if" regardless of a trailing "else" (else S, S unused),
>   across context/patch/report/org rules for both families. Found via
>   this to be a real, previously-invisible case in
>   drivers/media/platform/microchip/microchip-isc-clk.c.
> - Fix the org-mode script rules: cocci.print_main() takes (message,
>   position), not just a position; the previous calls omitted the
>   message entirely.
> - Drop the MAINTAINERS addition from v2 per Julia's comment that a
>   specific maintainer isn't needed for this file.
> - Link to v2: https://patch.msgid.link/[email protected]
>
> 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/[email protected]
> ---
>  scripts/coccinelle/api/clk_register.cocci | 165 ++++++++++++++++++++++++++++++
>  1 file changed, 165 insertions(+)
>
> diff --git a/scripts/coccinelle/api/clk_register.cocci b/scripts/coccinelle/api/clk_register.cocci
> new file mode 100644
> index 000000000000..150279827bf4
> --- /dev/null
> +++ b/scripts/coccinelle/api/clk_register.cocci
> @@ -0,0 +1,165 @@
> +// 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)$";
> +statement S;
> +@@
> +
> +clk = \(clk_register\|devm_clk_register\)(...);
> +if ( IS_ERR(clk) )
> +{
> +...
> +*voidfn(...);
> +...
> +}
> +else S
> +
> +@depends on patch@
> +expression clk;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +@@
> +
> +clk = \(clk_register\|devm_clk_register\)(...);
> +-if ( IS_ERR(clk) )
> +-voidfn(...);
> +
> +@depends on patch@
> +expression clk;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +statement S, S_else;
> +@@
> +
> +clk = \(clk_register\|devm_clk_register\)(...);
> +if ( IS_ERR(clk) )
> +(
> +-{
> +-voidfn(...);
> +S
> +-}
> +|
> +{
> +...
> +-voidfn(...);
> +...
> +}
> +)
> +else S_else
> +
> +@r1 depends on org || report@
> +position p1;
> +expression clk;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +statement S;
> +@@
> +
> +clk = \(clk_register\|devm_clk_register\)(...);
> +if ( IS_ERR(clk) )
> +{
> +...
> +voidfn@p1(...);
> +...
> +}
> +else S
> +
> +@depends on context@
> +expression ret;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +statement S;
> +@@
> +
> +ret = \(clk_hw_register\|devm_clk_hw_register\|of_clk_hw_register\)(...);
> +if ( \( ret \| ret < 0 \) )
> +{
> +...
> +*voidfn(...);
> +...
> +}
> +else S
> +
> +@depends on patch@
> +expression ret;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +@@
> +
> +ret = \(clk_hw_register\|devm_clk_hw_register\|of_clk_hw_register\)(...);
> +-if ( \( ret \| ret < 0 \) )
> +-voidfn(...);
> +
> +@depends on patch@
> +expression ret;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +statement S, S_else;
> +@@
> +
> +ret = \(clk_hw_register\|devm_clk_hw_register\|of_clk_hw_register\)(...);
> +if ( \( ret \| ret < 0 \) )
> +(
> +-{
> +-voidfn(...);
> +S
> +-}
> +|
> +{
> +...
> +-voidfn(...);
> +...
> +}
> +)
> +else S_else
> +
> +@r2 depends on org || report@
> +position p2;
> +expression ret;
> +identifier voidfn =~ "^(dev_err|dev_warn|pr_err|pr_warn)$";
> +statement S;
> +@@
> +
> +ret = \(clk_hw_register\|devm_clk_hw_register\|of_clk_hw_register\)(...);
> +if ( \( ret \| ret < 0 \) )
> +{
> +...
> +voidfn@p2(...);
> +...
> +}
> +else S
> +
> +@script:python depends on org@
> +p1 << r1.p1;
> +@@
> +
> +msg = "line %s is redundant because clk_register() already prints an error on failure" % (p1[0].line)
> +cocci.print_main(msg, p1)
> +
> +@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;
> +@@
> +
> +msg = "line %s is redundant because clk_hw_register() already prints an error on failure" % (p2[0].line)
> +cocci.print_main(msg, p2)
> +
> +@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 <[email protected]>
>
>