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/vc4: fix NULL dereference in vc4_hvs_unbind Date: Tue, 05 May 2026 08:48:38 +1000 Message-ID: In-Reply-To: <20260502121251.39206-3-thorsten.blum@linux.dev> References: <20260502121251.39206-3-thorsten.blum@linux.dev> <20260502121251.39206-3-thorsten.blum@linux.dev> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness:** The fix correctly addresses the crash. The root cause is i= n `vc4_drv.c:416` where `dev_set_drvdata(dev, NULL)` is called in `vc4_drm_= unbind()` before the devm-managed `vc4_component_unbind_all()` fires at `vc= 4_drv.c:275`, which subsequently calls each component's unbind (including `= vc4_hvs_unbind`). By the time `vc4_hvs_unbind` runs, `dev_get_drvdata(maste= r)` returns NULL. **Code change:** ```c - struct vc4_dev *vc4 =3D to_vc4_dev(drm); - struct vc4_hvs *hvs =3D vc4->hvs; + struct vc4_dev *vc4; + struct vc4_hvs *hvs; struct drm_mm_node *node, *next; + if (!drm) + return; + + vc4 =3D to_vc4_dev(drm); + hvs =3D vc4->hvs; ``` This is clean and correct =E2=80=94 the early return prevents the NULL dere= ference path entirely. **Minor observations:** 1. **Consistent with existing code?** The other unbind functions in this dr= iver (`vc4_v3d_unbind` in `vc4_v3d.c:494`) do **not** have this same NULL g= uard but are susceptible to the same issue. If this NULL drvdata scenario c= an be triggered for `vc4_hvs_unbind`, it can likely be triggered for `vc4_v= 3d_unbind` as well (which does `struct vc4_dev *vc4 =3D to_vc4_dev(drm)` wi= thout checking). Consider adding the same guard there, or at least noting t= hat those functions may need fixing too. `vc4_crtc_unbind` and `vc4_txp_unb= ind` don't dereference `master` drvdata, so they are safe. 2. **Root cause vs. symptom:** This is a band-aid. The real issue is the or= dering in `vc4_drm_unbind()` =E2=80=94 it clears drvdata before the devm-ma= naged `component_unbind_all` action fires. Moving `dev_set_drvdata(dev, NUL= L)` into a devm action registered *before* the `component_unbind_all` actio= n (so it runs *after*) would fix this at the source. However, for a stable-= targeted fix, the defensive NULL check is the safer and more appropriate ap= proach. 3. **Fixes tag and Cc stable:** Both look correct =E2=80=94 the fix referen= ces `c8b75bca92cb` which introduced the original `vc4_hvs_unbind`, and the = stable tag is appropriate for a NULL deref fix. 4. **Unused variable after early return:** `hvs` is assigned at line 1764 b= ut only used at lines 1778-1779 (`clk_disable_unprepare(hvs->disp_clk)` and= `clk_disable_unprepare(hvs->core_clk)`). Meanwhile lines 1766-1776 all use= `vc4->hvs` directly instead of `hvs`. This is a pre-existing inconsistency= (not introduced by this patch), but the patch does perpetuate it by keepin= g the `hvs` local variable. **Overall:** Good fix, appropriate for stable. The `vc4_v3d_unbind` gap is = worth flagging to the author as a follow-up. --- Generated by Claude Code Patch Reviewer