<[email protected]> 于2026年7月14日周二 19:16写道：
>
> 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.
>
Valid point. Will fix by protecting the initial removed check with ocm_lock:
mutex_lock(<7911exc->upgrade_lock);
scoped_guard(mutex, <7911exc->ocm_lock) {
if (lt7911exc->removed)
goto out_unlock_upgrade;
}
> > + 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().
>
This is a known limitation of the DRM bridge framework, not specific
to this driver. drm_bridge_remove() does not clear bridge.dev, and
there is no mechanism to bind the bridge lifecycle to the drm_device
lifecycle. All bridge drivers that call drm_kms_helper_hotplug_event()
share this risk. A proper fix requires framework-level support for
bridge-to-drm_device lifetime tracking. Additionally, this scenario
requires root sysfs access to trigger firmware upgrade while
simultaneously unloading the DRM driver, which is an administrator
operation outside normal usage.
> > +
> > + 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.
>
This is a false positive. The -EBUSY guard at the top of
lt7911exc_dsi_host_attach() prevents re-attach when output_bridge is
already set. After the first successful attach, any panel rebind
attempt is rejected before reaching devm_drm_of_get_bridge() or
drm_bridge_add(), so no devres leak or list corruption can occur. This
follows the same pattern used in cdns-dsi-core.c (cdns_dsi_attach,
line 1052).
> > + 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.
>
This override is a hardware constraint. The DSI output stream
(register 0xe0b0) must be enabled after the panel is powered on. The
override is safe because transfer() is a stub — panel DCS commands are
silently discarded and panel initialization is handled entirely by the
chip's internal firmware. The panel driver's prepare_prev_first
preference is therefore irrelevant for this chip. The comment above
the assignment documents this rationale.
> > +
> > + 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.
>
This is a false positive. The -EBUSY guard at the top of
lt7911exc_dsi_host_attach() prevents re-attach when output_bridge is
already set. After the first successful attach, any panel rebind
attempt is rejected before reaching devm_drm_of_get_bridge() or
drm_bridge_add(), so no devres leak or list corruption can occur. This
follows the same pattern used in cdns-dsi-core.c (cdns_dsi_attach,
line 1052).
> > +
> > + 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.
>
This is intentional and consistent with cdns-dsi-core.c, where
cdns_dsi_detach() (line 1082-1091) does not clear output->dev either.
Clearing output_bridge would allow re-attach, which would call
devm_drm_of_get_bridge() again and leak the previous devm allocation.
The DRM bridge framework does not currently support dynamic
reconfiguration of the bridge chain. Panel rebind is not supported;
reloading the bridge driver is required, matching the upstream
convention.
> > +
> > + return 0;
> > +}
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=2
