* [PATCH] coccinelle: Detect clk_register() anti-pattern
@ 2026-08-02 8:53 Guru Das Srinagesh
2026-08-02 9:01 ` Julia Lawall
0 siblings, 1 reply; 3+ messages in thread
From: Guru Das Srinagesh @ 2026-08-02 8:53 UTC (permalink / raw)
To: Julia Lawall, Nicolas Palix, Michael Turquette, Stephen Boyd
Cc: linux-kernel, cocci, Brian Masney, linux-clk, Guru Das Srinagesh
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()).
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);
return ret;
}
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.
---
MAINTAINERS | 5 ++
scripts/coccinelle/api/clk_register.cocci | 125 ++++++++++++++++++++++++++++++
2 files changed, 130 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
+
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..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(...);
+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(...);
+...
+}
+
+@r2 depends on org || report@
+position p2;
+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@p2(...);
+...
+}
+
+@script:python depends on org@
+p1 << r1.p1;
+@@
+
+cocci.print_main(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;
+@@
+
+cocci.print_main(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 <linux@gurudas.dev>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] coccinelle: Detect clk_register() anti-pattern
2026-08-02 8:53 [PATCH] coccinelle: Detect clk_register() anti-pattern Guru Das Srinagesh
@ 2026-08-02 9:01 ` Julia Lawall
2026-08-04 6:14 ` Guru Das Srinagesh
0 siblings, 1 reply; 3+ messages in thread
From: Julia Lawall @ 2026-08-02 9:01 UTC (permalink / raw)
To: Guru Das Srinagesh
Cc: Nicolas Palix, Michael Turquette, Stephen Boyd, linux-kernel,
cocci, Brian Masney, linux-clk
On Sun, 2 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()).
>
> 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);
> return ret;
> }
>
> 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
I don't have an opinion about this. If you want to be responsible for it
that's fine with me.
> 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.
> ---
> MAINTAINERS | 5 ++
> scripts/coccinelle/api/clk_register.cocci | 125 ++++++++++++++++++++++++++++++
> 2 files changed, 130 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
> +
> 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..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.
> +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
> +
> +@r2 depends on org || report@
> +position p2;
> +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@p2(...);
> +...
> +}
> +
> +@script:python depends on org@
> +p1 << r1.p1;
> +@@
> +
> +cocci.print_main(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;
> +@@
> +
> +cocci.print_main(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 <linux@gurudas.dev>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] coccinelle: Detect clk_register() anti-pattern
2026-08-02 9:01 ` Julia Lawall
@ 2026-08-04 6:14 ` Guru Das Srinagesh
0 siblings, 0 replies; 3+ messages in thread
From: Guru Das Srinagesh @ 2026-08-04 6:14 UTC (permalink / raw)
To: Julia Lawall
Cc: Nicolas Palix, Michael Turquette, Stephen Boyd, linux-kernel,
cocci, Brian Masney, linux-clk
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-04 6:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-02 8:53 [PATCH] coccinelle: Detect clk_register() anti-pattern Guru Das Srinagesh
2026-08-02 9:01 ` Julia Lawall
2026-08-04 6:14 ` Guru Das Srinagesh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox