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/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver Date: Thu, 04 Jun 2026 14:19:35 +1000 Message-ID: In-Reply-To: <20260601095041.3042950-1-a-kumar2@ti.com> References: <20260601095041.3042950-1-a-kumar2@ti.com> <20260601095041.3042950-1-a-kumar2@ti.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 **Critical: Missing notifier removal in remove path** The notifier is registered in probe via `dev_pm_genpd_add_notifier()`, but = `cdns_mhdp_remove()` never calls `dev_pm_genpd_remove_notifier()`. This wil= l leave a dangling notifier pointing at freed memory after the driver is re= moved. **Critical: Clock management conflicts with `devm_clk_get_enabled()`** The probe function at line 2275 of the current tree uses `devm_clk_get_enab= led()`, which means the clock is managed by devres and will be automaticall= y disabled/unprepared at device removal. But `cdns_mhdp_suspend()` calls `c= lk_disable_unprepare(mhdp->clk)` and `cdns_mhdp_resume()` calls `clk_prepar= e_enable(mhdp->clk)`. This creates a conflict: on driver removal after a re= sume, devres will try to disable an already-managed clock, and the enable/d= isable count will be unbalanced. The driver should either switch to `devm_c= lk_get()` (not the `_enabled` variant) and manage clock enable/disable expl= icitly, or find another approach for suspend/resume clock handling. **Bug: `hw_state` not set to `MHDP_HW_STOPPED` on resume error paths** In `cdns_mhdp_resume()`, if `cdns_mhdp_load_firmware()` or `wait_event_time= out` or `cdns_mhdp_set_firmware_active()` fails, the function returns an er= ror but `mhdp->hw_state` is never updated. The suspend function sets `hw_st= ate =3D MHDP_HW_STOPPED`, and it stays that way on resume failure, but the = comment block in the header file (line 317) says: ``` MHDP_HW_READY <-> MHDP_HW_STOPPED ``` If resume partially fails (e.g. firmware loaded but timeout waiting for rea= dy), the state machine is left in an inconsistent position. At minimum, on = resume failure, `hw_state` should be explicitly set to `MHDP_HW_STOPPED` so= a subsequent suspend/resume cycle doesn't malfunction. **Bug: `wait_event_timeout` return value mishandled in suspend** ```c ret =3D wait_event_timeout(mhdp->fw_load_wq, mhdp->hw_state =3D=3D MHDP_HW_READY, timeout); spin_lock(&mhdp->start_lock); if (mhdp->hw_state !=3D MHDP_HW_READY) { ``` `wait_event_timeout` returns 0 on timeout and >0 on success. Here `ret` is = set to the timeout return value (positive on success), but then later `ret`= is overwritten by `cdns_mhdp_set_firmware_active()`. However, if the wait = succeeds but `hw_state =3D=3D MHDP_HW_READY`, then the code proceeds and `r= et` is still the positive jiffies remaining value. Although it gets overwri= tten by `cdns_mhdp_set_firmware_active()` which returns 0 on success, it wo= uld be cleaner to reset `ret =3D 0` after the wait succeeds, as the existin= g `int ret =3D 0` initialization is dead. Same issue exists in `cdns_mhdp_r= esume()`: ```c ret =3D wait_event_timeout(mhdp->fw_load_wq, mhdp->hw_state =3D=3D MHDP_HW_READY, msecs_to_jiffies(1000)); if (ret =3D=3D 0) { ... ret =3D -ETIMEDOUT; goto phy_off; } ``` If `wait_event_timeout` returns a positive value (success), `ret` is now so= me positive jiffies count but the success path does `mhdp->powered_off =3D = false; return 0;` =E2=80=94 so this particular case is fine. But it is frag= ile and inconsistent. In the suspend case, if the success path after `cdns_= mhdp_set_firmware_active` had any error checks using `ret`, the stale posit= ive value could leak through. **Issue: `cancel_work_sync(&mhdp->hpd_work)` in suspend but not `modeset_re= try_work`** The suspend function cancels `hpd_work` but not `modeset_retry_work`. The r= emove function cancels both (`cancel_work_sync(&mhdp->modeset_retry_work)` = and `flush_work(&mhdp->hpd_work)`). If a modeset retry is pending during su= spend, it could fire after the firmware is deactivated and the PHY is power= ed off, causing failures. **Issue: No bridge state management during suspend/resume** The suspend/resume doesn't interact with the DRM bridge state at all. If th= e display is active when suspend happens, resume will reload/activate firmw= are but won't restore the display mode, link training, or any active stream= state. While this might be handled at a higher level by the DRM framework'= s atomic resume, it's worth considering whether `cdns_mhdp_bridge_hpd_enabl= e()` should be called in resume (as `cdns_mhdp_fw_activate()` does at line = 701) to re-enable HPD interrupt handling. **Minor: `DEFINE_SIMPLE_DEV_PM_OPS` placement** The `DEFINE_SIMPLE_DEV_PM_OPS` macro is placed between the suspend/resume f= unctions and `cdns_mhdp_probe()`. This is fine functionally, but convention= ally PM ops definitions are placed closer to the driver struct. **Minor: Missing `NOTIFY_DONE` return in notifier** ```c static int mhdp_pd_notifier_cb(struct notifier_block *nb, unsigned long action, void *data) { struct cdns_mhdp_device *mhdp =3D container_of(nb, struct cdns_mhdp_device= , pd_nb); if (action =3D=3D GENPD_NOTIFY_OFF) mhdp->powered_off =3D true; return 0; } ``` Returning `0` is equivalent to `NOTIFY_DONE`, so it works, but using the sy= mbolic constant `NOTIFY_DONE` is conventional and clearer. **Minor: Race on `powered_off` flag** `mhdp->powered_off` is a plain `bool` accessed from the genpd notifier call= back (which may run in arbitrary context) and from the suspend/resume paths= . There is no locking around it. While it's likely benign since the writes = are not concurrent in practice (the notifier fires during the power-off seq= uence, and resume reads it after), it would be more robust to use `start_lo= ck` or at minimum make it atomic. --- Generated by Claude Code Patch Reviewer