Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Moving `devm_mipi_dsi_device_register_full()` and `devm_mipi_dsi_attach()` to the DRM bridge `.attach` callback causes duplicate registrations, resource leaks, and permanent failures if probe defers or unbinds occur. - [High] Moving `mipi_dsi_attach()` to the DRM bridge `.attach` callback creates a circular dependency with DSI hosts (like samsung-dsim) that rely on device attach to register their own DRM bridge. - [Medium] Using `dev_err()` instead of `dev_err_probe()` for host attachment failures causes log spam on expected probe deferrals. Pre-existing issues: - [High] If the MIPI DSI host unbinds before the sn65dsi83 bridge, the DSI device is freed, leading to a use-after-free when the bridge's devres action fires. - [Medium] If the device tree is missing the endpoint, the driver incorrectly returns `-EPROBE_DEFER` instead of a hard error, leading to an infinite deferral loop. -- --- Patch [1]: [PATCH] drm/bridge: sn65dsi83: perform host_attach on attach instead of probe --- commit 246eecbf86024f7bc9178b065620b4bc9cfe26df Author: Paul Geurts <[email protected]> drm/bridge: sn65dsi83: perform host_attach on attach instead of probe This patch moves the sn65dsi83 DSI host attachment from the probe function to the DRM bridge attach callback. This is intended to break an infinite probe deferral loop between the i.MX8M Mini MIPI DSI host and the bridge. > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index e6dbe51d0dba9..637a29ab284f8 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -288,6 +288,8 @@ static const int lvds_vod_swing_clock_table[2][4][2] = { > [ ... ] > @@ -298,6 +300,13 @@ static int sn65dsi83_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > { > struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge); > + int ret = 0; > + > + ret = sn65dsi83_host_attach(ctx); [Severity: High] Does moving the devm-managed registrations into the bridge attach callback cause resource leaks on failures? The drm_bridge_attach() callback can be called multiple times if the DRM pipeline bind process defers due to downstream components. Because the devm_mipi_dsi_device_register_full() and devm_mipi_dsi_attach() allocations inside sn65dsi83_host_attach() are tied to the I2C device which hasn't unbound, a failure later in the attach process doesn't clean them up. The next attach attempt might try to register a new DSI device with the same name, resulting in an -EEXIST failure and permanently breaking the display pipeline. [Severity: High] Could delaying the DSI host attach until the DRM bridge attach callback create a circular dependency where the KMS pipeline can never be built? If a DSI host (like samsung-dsim) expects to register its own DRM bridge inside its mipi_dsi_host_ops.attach callback, that will now only happen when sn65dsi83_attach() is called. However, sn65dsi83_attach() is only called during the DRM pipeline bind phase. This bind phase will fail and return -EPROBE_DEFER because it cannot find the host's bridge (which hasn't been registered yet). This forms a deadlock where the pipeline defers and never calls sn65dsi83_attach(). [Severity: High] This is a pre-existing issue, but does sn65dsi83_host_attach() create a use-after-free risk if the DSI host unbinds before the bridge? Inside sn65dsi83_host_attach(), the child DSI device is registered using devm_mipi_dsi_device_register_full(dev, host, &info). The device is added as a child of the MIPI DSI host. If the DSI host driver unbinds, it unregisters all children, freeing the DSI device. However, the devm action is tied to the sn65dsi83 I2C bridge device. If the bridge later unbinds, its devres cleanup will call mipi_dsi_device_unregister() on the already-freed pointer. [Severity: Medium] This is a pre-existing issue, but does sn65dsi83_host_attach() mask device tree configuration errors? If the device tree is missing the endpoint, of_graph_get_endpoint_by_regs() returns NULL. This cascades to the host being NULL, and the function returns -EPROBE_DEFER. Should this return a hard error like -EINVAL instead of deferring indefinitely when the endpoint is entirely missing? > + if (ret) { > + dev_err(ctx->dev, "Failed to attach DSI host %d\n", ret); [Severity: Medium] Will this spam the kernel log with errors during expected probe deferrals? If the DSI host is not yet available, sn65dsi83_host_attach() will return -EPROBE_DEFER. Since dev_err() is used here instead of dev_err_probe() or an explicit check, it will log an error on every deferred bind attempt. > + return ret; > + } > > return drm_bridge_attach(encoder, ctx->panel_bridge, > [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
