On Wed, Oct 07, 2026 at 08:32:56PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s)
> to consider:
> - [Critical] drm: build failure due to non-existent .atomic_create_state hook
> in drm_bridge_funcs
> - [Critical] drm: type mismatch and memory corruption in atomic_enable hook
> signature
> - [High] drm: missing mandatory initialization for HDMI bridge operations
> - [High] sysfs: NULL pointer dereference in firmware attributes due to
> registration race
> - [Medium] drm/bridge: missing atomic_disable leaves LT9611C hardware powered
> on
> --
>
> commit 0482492a2bb6e8ef5381a82384662e6858358155
> Author: Sunyun Yang <[email protected]>
>
> drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
>
> This commit adds support for the Lontium LT9611C series MIPI DSI to HDMI
> bridge chip. It supports configuring single or dual port DSI inputs and
> provides HDMI output functionality along with audio and sysfs firmware
> update support.
>
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > new file mode 100644
> > index 0000000000000..6082c89b388bf
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> [ ... ]
> > +static void lt9611c_bridge_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_atomic_commit *state)
> > +{
> > + struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > + struct drm_connector *connector;
> > + struct drm_connector_state *conn_state;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_display_mode *mode;
> > + int ret;
> > +
> > + connector = drm_atomic_get_new_connector_for_encoder(state,
> > bridge->encoder);
>
> [Severity: Critical]
> Does the signature of this atomic_enable hook match what the DRM core expects?
>
> The atomic_enable hook in drm_bridge_funcs expects the second argument to be
> a struct drm_bridge_state pointer for the old bridge state. Treating this
> argument as the top-level struct drm_atomic_commit pointer and passing it to
> drm_atomic_get_new_connector_for_encoder() casts the bridge state to a commit
> state, which can cause out-of-bounds memory access when dereferencing the
> state arrays.
The atomic_enable hook in this kernel tree (include/drm/drm_bridge.h:339) takes
a struct drm_atomic_commit *, while drm_atomic_get_new_connector_for_encoder()
expects a const struct drm_atomic_commit *; therefore, the usage is correct,
type-safe, and does not require any cast.
>
> [ ... ]
> > +static const struct drm_bridge_funcs lt9611c_bridge_funcs = {
> > + .attach = lt9611c_bridge_attach,
> > + .detect = lt9611c_bridge_detect,
> > + .edid_read = lt9611c_bridge_edid_read,
> > + .atomic_enable = lt9611c_bridge_atomic_enable,
> > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>
> [Severity: Medium]
> Is an atomic_disable hook needed here to properly power down the bridge?
>
> The driver implements atomic_enable to set up video timings and dual DSI
> ports, but lacks a corresponding atomic_disable. During display blanking
> or system DPMS off, the hardware might remain active, wasting power or
> potentially locking up when the upstream DSI host stops its clock.
>
> > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > + .atomic_create_state = drm_atomic_helper_bridge_create_state,
>
The LT9611C bridge does not require an atomic_disable hook for correct
operation. The bridge MCU firmware manages its own state and automatically
enters a low-power idle state when the DSI input clock is removed, i.e. when
the upstream DSI host stops transmitting. Explicit power-down via an MCU
command is therefore unnecessary and would require an additional MCU command
sequence that could potentially race with DSI host shutdown.
System-level power management is handled by lt9611c_bridge_suspend(), which
disables the regulators and IRQ, while display blanking/DPMS is naturally
handled by the upstream DSI host stopping its output.
If a future use case requires explicit bridge shutdown between mode
transitions, an atomic_disable hook can be added at that time.
> [Severity: Critical]
> Does this compile against the current drm_bridge_funcs definition?
>
> The struct drm_bridge_funcs does not contain an atomic_create_state member,
> and the assigned helper drm_atomic_helper_bridge_create_state does not exist.
> The correct hook for resetting bridge state is atomic_reset, pointing to
> drm_atomic_helper_bridge_reset. Without a valid reset hook, atomic state
> allocation for the bridge will fail.
>
> [ ... ]
atomic_create_state is a valid member of struct drm_bridge_funcs in this kernel
(see include/drm/drm_bridge.h:507), and drm_atomic_helper_bridge_create_state
is the corresponding helper (see
drivers/gpu/drm/drm_atomic_state_helper.c:909). This pattern is already used by
other drivers in the same tree, such as chipone-icn6211.c. The atomic_reset
hook you mentioned belongs to the older API; this driver correctly uses the
newer atomic_create_state interface.
> > +static ssize_t firmware_store(struct device *dev, struct device_attribute
> > *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct lt9611c *lt9611c = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + dev_warn(dev, "starting firmware upgrade — display will be
> > disrupted\n");
> > +
> > + ret = lt9611c_firmware_upgrade(lt9611c);
>
> [Severity: High]
> Can lt9611c be NULL here if the sysfs file is accessed before probe completes?
>
> The sysfs group is registered via lt9611c_driver.driver.dev_groups, which
> means the files are created by the driver core before probe runs. If userspace
> accesses this before i2c_set_clientdata() is called, dev_get_drvdata(dev)
> will return NULL, causing an immediate crash when it is passed to
> lt9611c_firmware_upgrade().
>
Will fix it
> [ ... ]
> > +static ssize_t firmware_show(struct device *dev, struct device_attribute
> > *attr, char *buf)
> > +{
> > + struct lt9611c *lt9611c = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "0x%04x\n", READ_ONCE(lt9611c->fw_version));
> > +}
>
> [Severity: High]
> Similar to firmware_store, this will dereference a NULL lt9611c pointer if
> the sysfs attribute is read before the client data is set in probe, or if
> probe fails.
>
Will fix it
> [ ... ]
> > + lt9611c->bridge.of_node = client->dev.of_node;
> > + lt9611c->bridge.ops = DRM_BRIDGE_OP_DETECT |
> > + DRM_BRIDGE_OP_EDID |
> > + DRM_BRIDGE_OP_HPD |
> > + DRM_BRIDGE_OP_HDMI |
> > + DRM_BRIDGE_OP_HDMI_AUDIO;
> > + lt9611c->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>
> [Severity: High]
> Since this driver sets the DRM_BRIDGE_OP_HDMI flag, does it also need to
> initialize supported_hdmi_ver, supported_formats, and max_bpc?
>
> When DRM_BRIDGE_OP_HDMI is set, leaving these uninitialized (zero) causes
> drm_bridge_add() to log a warning for an unknown HDMI version. The HDMI state
> helpers will also reject display modes because no valid color formats or
> depths are advertised, which prevents display modesetting.
The driver works correctly without these fields for the following reasons:
supported_hdmi_ver - leaving this as HDMI_VERSION_UNKNOWN (0) only results in a
DRM_WARN from drm_bridge_add() (drivers/gpu/drm/drm_bridge.c:464) and does not
affect functionality. The only consequence is that ycbcr_420_allowed is
evaluated as false, which is appropriate for this device since the LT9611C
family does not support YCbCr 4:2:0 input.
supported_formats - leaving this field unset (0) also results in
ycbcr_420_allowed = false. Display mode filtering is based on EDID-reported
sink capabilities and does not directly depend on supported_formats. RGB modes
are therefore unaffected.
max_bpc - leaving this at 0 effectively disables deep-color support, which is
acceptable because the LT9611C firmware does not currently expose any mechanism
for deep-color negotiation.
These fields can be populated in a follow-up once the hardware capabilities
have been fully characterized, for example, whether the MCU firmware supports
YCbCr output formats or deep-color modes. Until then, leaving them unset is
preferable to advertising capabilities that may not actually be supported.
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=2
