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/msm: default separate_gpu_kms to auto selection Date: Tue, 31 Mar 2026 17:21:55 +1000 Message-ID: In-Reply-To: <20260330-separate_gpu_kms-v3-1-a3e54b9c9417@oss.qualcomm.com> References: <20260330-separate_gpu_kms-v3-1-a3e54b9c9417@oss.qualcomm.com> <20260330-separate_gpu_kms-v3-1-a3e54b9c9417@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 **Bug: `of_node_put()` missing in `msm_separate_gpu_kms_components()`** This is the most significant issue. `of_find_matching_node()` returns a nod= e with an incremented refcount, but the function never calls `of_node_put()= `: ```c bool msm_separate_gpu_kms_components(void) { struct device_node *np; np =3D of_find_matching_node(NULL, msm_gpu_match); if (!msm_gpu_node_present(np)) return true; if (separate_gpu_kms =3D=3D 1) return true; if (separate_gpu_kms =3D=3D 0) return false; return msm_separate_gpu_kms_auto(); } ``` Every return path after the `of_find_matching_node()` call leaks the node r= eference. This function is called from multiple call sites (`adreno_probe`,= `msm_drm_bind`, `msm_drv_probe`), so the leak compounds. An `of_node_put(n= p)` is needed before every `return`. A common pattern would be: ```c bool msm_separate_gpu_kms_components(void) { struct device_node *np; bool ret; np =3D of_find_matching_node(NULL, msm_gpu_match); if (!msm_gpu_node_present(np)) { of_node_put(np); return true; } of_node_put(np); if (separate_gpu_kms =3D=3D 1) return true; if (separate_gpu_kms =3D=3D 0) return false; return msm_separate_gpu_kms_auto(); } ``` Note that `of_node_put(NULL)` is safe, so the `!msm_gpu_node_present(np)` p= ath can call it unconditionally. After the present-check, `np` is no longer= used, so the put can happen right after. **Multiple calls to `msm_separate_gpu_kms_components()` re-scan device tree= each time** This function is called from at least three sites during probe. Each call d= oes `of_find_matching_node` + `for_each_matching_node` (via `msm_count_gpus= `) + three more `for_each_matching_node` calls (via `msm_count_mdss/mdp4/md= p5`). Consider caching the result after the first evaluation, since the har= dware topology doesn't change at runtime. This is a minor performance conce= rn, not a correctness issue. **Forward declaration of `mdp4_dt_match[]` and `mdp5_dt_match[]`** ```c static const struct of_device_id mdp4_dt_match[]; int msm_count_mdp4(void) ``` These forward-declare `static` arrays from later in the same file. This wor= ks but some compilers may warn about incomplete array types. It's a pre-exi= sting pattern in the MSM driver (already done for `mdss_dt_match` and `msm_= gpu_match`), so it's consistent. **`msm_count_mdp4()` and `msm_count_mdp5()` are not `static` but lack decla= rations in msm_drv.h prior to patch** The patch correctly adds declarations in `msm_drv.h`: ```c int msm_count_mdp4(void); int msm_count_mdp5(void); ``` This is fine. **Minor: `msm_separate_gpu_kms_auto()` returns `false` when `gpus <=3D 0`** ```c if (gpus <=3D 0 || display_subsystems <=3D 0) return false; ``` When no GPU is found (`gpus =3D=3D 0`), this returns `false` (don't separat= e). But `msm_separate_gpu_kms_components()` already handles the no-GPU case= by returning `true` (separate) before reaching auto. So this path in `msm_= separate_gpu_kms_auto()` is effectively dead for the `gpus <=3D 0` case. No= t a bug, but the comment `/* If exactly one GPU and one display subsystem s= ingle card */` could note this nuance. **Behavioral consideration: `separate_gpu_kms=3D0` overrides no-GPU detecti= on** When a user forces `separate_gpu_kms=3D0` but no recognized Adreno GPU is p= resent, `msm_separate_gpu_kms_components()` returns `true` (separate) regar= dless =E2=80=94 because the `!msm_gpu_node_present(np)` check comes before = the `separate_gpu_kms =3D=3D 0` check. This is documented behavior ("ensure= display subsystems are always exposed as separate DRM devices when no reco= gnized Adreno GPU is present, regardless of the separate_gpu_kms setting"),= but it means the module parameter can't actually force combined mode on sy= stems without a recognized GPU. This is probably the right call, but worth = a comment in the code to explain why the parameter is ignored in this case. **Summary**: The of_node_put leak is a real bug that needs fixing. The over= all design and auto-detection logic are clean and well-structured. With the= refcount leak fixed, this looks ready to merge. --- Generated by Claude Code Patch Reviewer