From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panthor: Add a new guard for our custom resume_and_get() PM helper Date: Sat, 16 May 2026 11:40:46 +1000 Message-ID: In-Reply-To: <20260513-panthor-guard-refactor-v1-6-f2d8c15a97ce@collabora.com> References: <20260513-panthor-guard-refactor-v1-0-f2d8c15a97ce@collabora.com> <20260513-panthor-guard-refactor-v1-6-f2d8c15a97ce@collabora.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 This patch has the most concerning changes: **API signature change:** ```c -static inline int panthor_device_resume_and_get(struct panthor_device *ptd= ev) +static inline int panthor_device_resume_and_get(struct device *dev) ``` The function now takes `struct device *` instead of `struct panthor_device = *`, to match the signature expected by the `DEFINE_GUARD_COND` mechanism (w= hich passes through the type from the base guard `pm_runtime_active_auto`, = which operates on `struct device *`). This is the right call for making the= guard work, but it changes the API for all callers. The patch does update = the callsites accordingly. **Guard definition:** ```c DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled_or_suspend, panthor_device_resume_and_get(_T), _RET =3D=3D 0) ``` This overloads the existing `pm_runtime_active_auto` guard with a new `_try= _enabled_or_suspend` variant. The new variant calls `panthor_device_resume_= and_get()` instead of the standard `pm_runtime_resume_and_get()`. On exit, = it calls `pm_runtime_put_autosuspend()` (inherited from the base guard). **Concern: `pm_runtime_mark_last_busy()` is dropped:** The original `tick_work()` called: ```c pm_runtime_mark_last_busy(ptdev->base.dev); pm_runtime_put_autosuspend(ptdev->base.dev); ``` The guard only calls `pm_runtime_put_autosuspend()` on cleanup. The `pm_run= time_mark_last_busy()` call is **removed and not replaced**. This means the= autosuspend timer won't be reset when the tick completes, potentially caus= ing earlier-than-expected device suspension. The same issue applies to `que= ue_run_job()`. **This is a behavioral change that could cause power managem= ent regressions** =E2=80=94 the autosuspend delay effectively starts from t= he last `mark_last_busy` call, and without it, the timeout may expire soone= r than intended. The commit message says "Overload the already existing pm_runtime_active_au= to_try_enabled guard" but it's actually creating a new variant (`_try_enabl= ed_or_suspend`), not overloading an existing one. The name is also panthor-= specific (referencing the "or suspend" recovery behavior), but it's defined= via the generic pm_runtime guard infrastructure. **This is a panthor-speci= fic function injected into a generic guard namespace**, which may not sit w= ell with PM maintainers. Consider defining a separate `panthor_pm` guard in= stead. **Verbosity of the ACQUIRE pattern:** ```c ACQUIRE(pm_runtime_active_auto_try_enabled_or_suspend, pm_guard)(ptdev->bas= e.dev); ret =3D ACQUIRE_ERR(pm_runtime_active_auto_try_enabled_or_suspend, &pm_guar= d); ``` This is significantly less readable than the original `ret =3D panthor_devi= ce_resume_and_get(ptdev);`. The guard name is 46 characters long. While fun= ctionally correct, this is a readability regression at the callsites. A sho= rter alias or a panthor-specific guard would help. **Typo in commit message:** "force the state to suspened" should be "suspen= ded". --- Generated by Claude Code Patch Reviewer