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. [ ... ] > +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, [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. [ ... ] > +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(). [ ... ] > +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. [ ... ] > + 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. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
