Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Dynamic devm allocation and bridge registration in DSI host attach/detach causes devres leaks and list corruption. - [High] Asynchronous workqueue dereferences dangling `bridge.dev` if the DRM display controller unbinds. - [Medium] Failure to clear `output_bridge` in `dsi_host_detach` prevents panel re-attachment. - [Low] Mutating `pre_enable_prev_first` of a downstream bridge object violates subsystem API layering. - [Low] Unsynchronized reads of `lt7911exc->removed` in the asynchronous workqueue. --
--- Patch [2]: [PATCH v16 2/2] drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge --- commit f0896e2dbabe50d3cbe793868df9c6d4afd6db5f Author: Sunyun Yang <[email protected]> drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge This commit adds support for the Lontium LT7911EXC bridge chip, which converts eDP input to MIPI DSI output. It implements firmware upgrades, GPIO reset handling, and DSI host interface attachment. > diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c > b/drivers/gpu/drm/bridge/lontium-lt7911exc.c > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c [ ... ] > @@ -344,14 +344,14 @@ static void lt7911exc_firmware_upgrade_work(struct > work_struct *work) > +{ > + struct lt7911exc *lt7911exc = container_of(work, struct lt7911exc, > work); > + struct device *dev = lt7911exc->dev; > + const struct firmware *fw; > + u8 *buffer; > + size_t total_size = FW_SIZE - 4; > + u32 crc32, version; > + int ret; > + > + mutex_lock(<7911exc->upgrade_lock); > + if (lt7911exc->removed) [Severity: Low] Should this read of the removed flag be protected by the ocm_lock? It appears this can be evaluated concurrently with driver removal since the lock isn't held here. > + goto out_unlock_upgrade; > + [ ... ] > +out_unlock_upgrade: > + scoped_guard(mutex, <7911exc->ocm_lock) { > + if (!lt7911exc->removed) > + lt7911exc->upgrade = false; > + } > + > + /* Notify DRM to re-trigger modeset after firmware upgrade */ > + if (!lt7911exc->removed && lt7911exc->bridge.dev) > + drm_kms_helper_hotplug_event(lt7911exc->bridge.dev); [Severity: High] Could this dereference a freed drm_device if the upstream DRM display controller unbinds before the firmware upgrade completes? The I2C device has a lifecycle independent of the DRM display controller. If the upstream DRM driver is unloaded, the drm_device is destroyed, but the I2C device remains, leaving bridge.dev populated but dangling. If a sysfs write triggers this asynchronous workqueue in that window, it looks like it will pass a freed pointer to drm_kms_helper_hotplug_event(). > + > + mutex_unlock(<7911exc->upgrade_lock); > +} [ ... ] > +static int lt7911exc_dsi_host_attach(struct mipi_dsi_host *host, struct > mipi_dsi_device *dev) > +{ > + struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host); > + struct drm_bridge *bridge; > + > + /* currently do not support connecting several DSI devices to the same > host */ > + if (lt7911exc->output_bridge) > + return -EBUSY; > + > + if (dev->lanes > 4) { > + dev_err(lt7911exc->dev, "unsupported number of data > lanes(%u)\n", dev->lanes); > + return -EINVAL; > + } > + > + bridge = devm_drm_of_get_bridge(lt7911exc->dev, host->dev->of_node, 1, > 0); [Severity: High] Is it safe to dynamically allocate devm memory during panel attach? If the downstream panel driver is repeatedly unbound and rebound by a local administrator, this will repeatedly allocate devres memory without freeing it, causing a memory leak. > + if (IS_ERR(bridge)) { > + dev_err(lt7911exc->dev, "failed to add DSI device\n"); > + return PTR_ERR(bridge); > + } > + > + lt7911exc->output_bridge = bridge; > + > + /* > + * Force panel-first enable order: the DSI output stream (0xe0b0) > + * must not be enabled before the panel is powered on. > + * This override is safe because LT7911EXC's transfer() is a stub — > + * panel DCS commands are silently discarded and panel init is done > + * by the chip's internal firmware. The panel driver's > + * prepare_prev_first preference is therefore irrelevant here. > + */ > + lt7911exc->output_bridge->pre_enable_prev_first = false; [Severity: Low] Is it acceptable to modify the state of a downstream bridge object that is owned and managed by another driver? Doing so bypasses the subsystem API layering. > + > + drm_bridge_add(<7911exc->bridge); [Severity: High] Does this corrupt the global bridge list if the panel driver is rebound? When the downstream panel unbinds, lt7911exc_dsi_host_detach() calls drm_bridge_remove(), which moves the bridge to the bridge_lingering_list and destroys its mutexes. If the panel binds again, this drm_bridge_add() will be called on the same bridge structure, invoking list_add_tail() on a node that is already tracked in the lingering list. > + > + return 0; > +} > + > +static int lt7911exc_dsi_host_detach(struct mipi_dsi_host *host, struct > mipi_dsi_device *dev) > +{ > + struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host); > + > + drm_bridge_remove(<7911exc->bridge); [Severity: Medium] Does this prevent the panel from re-attaching later? The detach function does not set lt7911exc->output_bridge back to NULL. If the downstream panel driver tries to attach again, lt7911exc_dsi_host_attach() will check if output_bridge is set and return -EBUSY, leaving the display pipeline permanently broken until the bridge driver is reloaded. > + > + return 0; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
