From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/mediatek: dsi: Store driver data before invoking mipi_dsi_host_register Date: Wed, 11 Feb 2026 17:11:30 +1000 Message-ID: In-Reply-To: <5535ac41-efb8-4bf5-91bb-9b6f63517e8f@collabora.com> References: <5535ac41-efb8-4bf5-91bb-9b6f63517e8f@collabora.com> <5535ac41-efb8-4bf5-91bb-9b6f63517e8f@collabora.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Mailer: Claude Code Patch Reviewer Patch Review **Commit Message Analysis:** The commit message adequately explains the issue and provides a detailed crash trace. However, it is missing critical metadata: 1. **Missing Fixes tag:** This is a bug fix and should include a `Fixes:` tag identifying which commit introduced the regression. Looking at the code, this would help with backporting to stable kernels. 2. **Missing Cc: stable tag:** Given this is a crash fix, it should include `Cc: stable@vger.kernel.org` to ensure it gets backported to stable kernel trees. **Technical Analysis:** ```c dsi->host.ops = &mtk_dsi_ops; dsi->host.dev = dev; + platform_set_drvdata(pdev, dsi); + ret = mipi_dsi_host_register(&dsi->host); ``` **The fix is correct.** The issue stems from this call chain: 1. `mtk_dsi_probe()` calls `mipi_dsi_host_register()` 2. `mipi_dsi_host_register()` → `mipi_dsi_device_register_full()` → `device_add()` 3. `device_add()` → `bus_probe_device()` → driver binding occurs 4. Panel driver's `mipi_dsi_drv_probe()` → `panel_simple_dsi_probe()` 5. Panel calls `mipi_dsi_attach()` → `mtk_dsi_host_attach()` 6. `mtk_dsi_host_attach()` → `component_add()` → `try_to_bring_up_aggregate_device()` 7. `mtk_drm_bind()` → `component_bind_all()` → `mtk_dsi_bind()` 8. **`mtk_dsi_bind()` calls `dev_get_drvdata()` but drvdata was never set → NULL pointer dereference** Moving `platform_set_drvdata()` before `mipi_dsi_host_register()` ensures the driver data is available when the callbacks are triggered. **Code Correctness:** The removed location (after `init_waitqueue_head()`) was too late because all the probe sequencing and component binding can happen synchronously during `mipi_dsi_host_register()`. The new location (immediately before `mipi_dsi_host_register()`) is the earliest safe point where: - The `dsi` structure is fully initialized for basic use - The host ops and device are set - No earlier position would make sense (drvdata shouldn't be set while struct is being populated) **Potential Issues:** Looking at the crash trace more carefully: ``` Call trace: drm_mode_object_add+0x58/0x98 (P) __drm_encoder_init+0x48/0x140 drm_encoder_init+0x6c/0xa0 drm_simple_encoder_init+0x20/0x34 [drm_kms_helper] mtk_dsi_bind+0x34/0x13c [mediatek_drm] ``` The crash happens in `mtk_dsi_bind()` at offset `+0x34`. The crash address `0x0000000000000040` (offset 0x40 from NULL) suggests dereferencing a field in a NULL structure. This aligns with `dev_get_drvdata()` returning NULL and then accessing a field at offset 0x40 in the `mtk_dsi` structure. **Questions/Concerns:** 1. **Error path safety:** If `mipi_dsi_host_register()` fails, the driver returns early. At this point, drvdata is set but the device isn't fully initialized. However, this should be safe because: - The device probe failed, so nothing should be calling into the driver - The error path doesn't explicitly unset drvdata, but the device will be removed anyway 2. **Completeness:** Are there other fields in the `dsi` structure that need to be initialized before `mipi_dsi_host_register()`? Looking at the code: - Lines before the call: Basic host setup, IRQ registration, clocks, PHY - all completed ✓ - Lines after the call: Wait queue, bridge setup - these are safe to do after **Style/Formatting:** The patch follows kernel coding style. The blank line added after `platform_set_drvdata()` is appropriate for visual separation. **Recommendation:** The patch should be accepted with the following amendments: 1. **Add Fixes tag:** Need to identify the commit that introduced this ordering issue (likely when the component framework integration was added or when the probe order changed) 2. **Add Cc: stable tag:** This crash fix should be backported 3. **Consider additional context:** The commit message could briefly mention that this issue manifests when DSI panels are probed synchronously during host registration **Suggested improved commit message:** ``` drm/mediatek: dsi: Store driver data before invoking mipi_dsi_host_register The call to mipi_dsi_host_register triggers a callback to mtk_dsi_bind, which uses dev_get_drvdata to retrieve the mtk_dsi struct, so this structure needs to be stored inside the driver data before invoking it. As drvdata is currently uninitialized it leads to a crash when registering the DSI DRM encoder right after acquiring the mode_config.idr_mutex, blocking all subsequent DRM operations. This manifests when the panel driver's probe is triggered synchronously during mipi_dsi_host_register(), leading to component binding before the DSI host driver has finished its probe sequence. [crash trace as shown...] Fixes: [commit-id] ("commit message of offending patch") Cc: stable@vger.kernel.org Signed-off-by: Luca Leonardo Scorcia Reviewed-by: AngeloGioacchino Del Regno ``` **Verdict:** ✓ Correct fix, should be merged (with metadata improvements) --- Generated by Claude Code Patch Reviewer