From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: clk: Add __clk_disable_unprepare_counts_only() helper
Date: Thu, 28 May 2026 12:34:29 +1000
Message-ID: <review-patch1-20260527094811.116977-2-johannes.goede@oss.qualcomm.com>
In-Reply-To: <20260527094811.116977-2-johannes.goede@oss.qualcomm.com>
References: <20260527094811.116977-1-johannes.goede@oss.qualcomm.com> <20260527094811.116977-2-johannes.goede@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**pm_runtime leak/hazard in unprepare path:**
The patch gates `core->ops->unprepare` with the new `call_unprepare_op` fla=
g, but `clk_pm_runtime_put(core)` at the end of `clk_core_unprepare()` is s=
till called unconditionally:

```c
	if (call_unprepare_op && core->ops->unprepare)
		core->ops->unprepare(core->hw);

	trace_clk_unprepare_complete(core);
	clk_core_unprepare(core->parent, call_unprepare_op);
	clk_pm_runtime_put(core);   // <-- still called in counts-only mode
```

When `clk_core_prepare()` was called by simpledrm, it called `clk_pm_runtim=
e_get()` (line 1110 of clk.c in drm-next). The matching `clk_pm_runtime_put=
()` calls `pm_runtime_put_sync()`, which can trigger the clock controller d=
evice's `runtime_suspend` callback. If simpledrm held the only prepare refe=
rence on clocks from a given controller, this could power down the controll=
er's power domain =E2=80=94 exactly the kind of hardware disturbance the se=
ries is trying to avoid.

This should either be gated the same way as the ops callback, or the ration=
ale for why it's safe should be documented.

**Misleading tracepoints:**
`trace_clk_unprepare` / `trace_clk_unprepare_complete` and `trace_clk_disab=
le` / `trace_clk_disable_complete` fire regardless of the `call_*_op` flag.=
 When no actual disable/unprepare happens, these trace events are misleadin=
g and could confuse ftrace-based debugging. Consider gating them with the s=
ame boolean, or adding distinct trace events for counts-only operations.

**WARN for CLK_IS_CRITICAL:**
In the counts-only path, the WARN for decrementing a critical clock's last =
reference still fires:

```c
	if (WARN(core->enable_count =3D=3D 1 && core->flags & CLK_IS_CRITICAL,
	    "Disabling critical %s\n", core->name))
		return;
```

While CLK_IS_CRITICAL clocks shouldn't appear in simple-framebuffer DT node=
s, the semantic is arguably wrong: we're not "disabling" the clock, we're r=
eleasing a count. This is a minor concern but could be confusing if it trig=
gers.

**API naming and export:**
The function is `EXPORT_SYMBOL_GPL` and the `__` prefix clearly signals "in=
ternal/unusual." The kerneldoc is good. Consider whether a `clk_release_cou=
nts()` or `clk_detach_counts()` name would be clearer, since the function d=
oesn't disable or unprepare anything.

**Overall correctness of the bool-parameter approach:**
Adding `bool call_unprepare_op` / `bool call_disable_op` to internal functi=
ons is a reasonable pattern (the cover letter correctly notes that duplicat=
ing the logic is worse). The changes to all existing callers to pass `true`=
 are mechanically correct =E2=80=94 I verified each call site:
- `clk_core_unprepare_lock()` =E2=80=94 passes `true` =E2=9C=93
- `clk_core_prepare()` error path =E2=80=94 passes `true` =E2=9C=93 =20
- `clk_change_rate()` =E2=80=94 passes `true` =E2=9C=93
- `__clk_core_init()` =E2=80=94 passes `true` =E2=9C=93
- `clk_core_disable_lock()` =E2=80=94 passes `true` =E2=9C=93
- `clk_core_enable()` error path =E2=80=94 passes `true` =E2=9C=93

---
Generated by Claude Code Patch Reviewer