Re: [PATCH v17 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver

[sashiko-bot@kernel.org]

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 <syyang@lontium.com>

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/20261008-lt9611c-v7-v17-0-1e8f5f2b2f19@oss.qualcomm.com?part=2