Claude review: Re: [PATCH] drm/mediatek: dsi: Store driver data before invoking mipi_dsi_host_register

[Claude Code Review Bot <claude-review@example.com>]

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 <l.scorcia@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
```

**Verdict:** ✓ Correct fix, should be merged (with metadata improvements)

---
Generated by Claude Code Patch Reviewer