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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Missing `disable_irq()` in `lt9611c_remove()` creates a race condition leading to Use-After-Free and unpowered hardware access.
- [High] Stack buffer overflow in `lt9611c_hdmi_write_hdmi_infoframe` due to unchecked `len` parameter.
- [High] Permanent module reference leak due to unbalanced `drm_bridge_get()` in `lt9611c_parse_dt()`.
- [High] Self-deadlock in `lt9611c_hdmi_audio_prepare` due to double locking of `ocm_lock` via nested DRM helper callbacks.
- [High] Erratic interrupt clearing sequence can permanently lock up the edge-triggered interrupt line and lose events.
- [High] Ignored return value of `devm_drm_bridge_add()` can lead to successful probe with an unregistered bridge.
- [High] Missing `cancel_work_sync()` in `lt9611c_bridge_suspend()` allows the HPD work to execute while the hardware is powered off.
- [High] Race condition between sysfs file creation and initialization of `clientdata` causes a NULL pointer dereference.
- [Medium] Synchronous firmware loading while holding a hardware mutex (`ocm_lock`).
- [Medium] Ignored flash erase timeout allows firmware upgrade to silently corrupt device flash.
--

--- Patch [2]: [PATCH v7 2/2] drm/bridge: Add Lontium LT9609C(EX/UXD) MIPI DSI to HDMI driver ---
commit b64f10044aaf95610928583023bd2bc668abab4e
Author: Sunyun Yang <syyang@lontium.com>
Subject: drm/bridge: Add Lontium LT9609C(EX/UXD) MIPI DSI to HDMI driver

This commit introduces the driver for the Lontium LT9611C DSI to HDMI bridge. 
It adds support for configuring video timing, reading EDID, handling HPD 
interrupts, and managing firmware upgrades.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> new file mode 100644
> index 0000000000000..e8bb385d15206
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> @@ -0,0 +1,1293 @@

[ ... ]

> +static void lt9611c_block_erase(struct lt9611c *lt9611c)
> +{
> +	struct device *dev = lt9611c->dev;
> +	int i;
> +	unsigned int block_num;
> +	unsigned int flash_status = 0;
> +	u32 flash_addr = 0;
> +
> +	for (block_num = 0; block_num < 2; block_num++) {
> +		flash_addr = (block_num * 0x008000);
> +		lt9611c_erase_op(lt9611c, flash_addr);
> +		msleep(100);
> +		i = 0;
> +		while (1) {
> +			read_flash_reg_status(lt9611c, &flash_status);
> +			if ((flash_status & 0x01) == 0)
> +				break;
> +
> +			if (i > 50)
> +				break;

[Severity: Medium]
Does this ignore the flash erase timeout? If the polling loop in 
lt9611c_block_erase() times out, it breaks out silently. Since this function 
has no return value, lt9611c_firmware_upgrade() will proceed to write the new 
firmware over an unerased block, which could corrupt the device firmware.

> +
> +			i++;
> +			msleep(50);
> +		}
> +	}
> +
> +	dev_dbg(dev, "erase flash done.\n");
> +}

[ ... ]

> +static irqreturn_t lt9611c_irq_thread_handler(int irq, void *dev_id)
> +{
> +	struct lt9611c *lt9611c = dev_id;
> +	struct device *dev = lt9611c->dev;
> +	int ret;
> +	unsigned int irq_status;
> +	u8 cmd[5] = {0x52, 0x48, 0x31, 0x3a, 0x00};
> +	u8 data[5];
> +
> +	guard(mutex)(&lt9611c->ocm_lock);
> +
> +	ret = regmap_read(lt9611c->regmap, 0xe084, &irq_status);
> +	if (ret) {
> +		dev_err(dev, "failed to read irq status: %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (!(irq_status & BIT(0)))
> +		return IRQ_HANDLED;

[Severity: High]
Could this early return permanently lock up the edge-triggered interrupt line?
If another status bit triggered the interrupt, this bails out without writing 
to the clear register (0xe0df). Because the interrupt line is configured as 
IRQF_TRIGGER_FALLING, leaving it asserted prevents any future falling edges 
and permanently locks up all future hardware interrupts.

> +
> +	msleep(100);
> +
> +	ret = lt9611c_read_write_flow(lt9611c, cmd, ARRAY_SIZE(cmd), data, ARRAY_SIZE(data));

[ ... ]

> +static int lt9611c_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
> +					     const u8 *buffer, size_t len)
> +{
> +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> +	u8 cmd[5 + LT9611C_INFOFRAME_MAX_SIZE];
> +	u8 data[5];
> +	int ret;
> +
> +	cmd[0] = 0x57;
> +	cmd[1] = 0x48;
> +	cmd[2] = 0x35;
> +	cmd[3] = 0x3a;
> +	cmd[4] = 0x04;/*write vsif*/
> +	memcpy(cmd + 5, buffer, len);

[Severity: High]
Is it possible to overflow the cmd stack buffer here? The cmd array is 
allocated with a fixed size of 37 bytes (5 + LT9611C_INFOFRAME_MAX_SIZE). 
Because the len parameter is passed directly to memcpy without validation, 
passing an infoframe larger than 32 bytes would corrupt the stack.

> +
> +	guard(mutex)(&lt9611c->ocm_lock);
> +
> +	ret = lt9611c_read_write_flow(lt9611c, cmd, 5 + len,
> +				      data, ARRAY_SIZE(data));

[ ... ]

> +static int lt9611c_hdmi_write_audio_infoframe(struct drm_bridge *bridge,
> +					      const u8 *buffer, size_t len)
> +{
> +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> +	u8 *cmd;
> +	u8 data[5];
> +	int ret;
> +
> +	guard(mutex)(&lt9611c->ocm_lock);

[ ... ]

> +static int lt9611c_hdmi_audio_prepare(struct drm_bridge *bridge,
> +				      struct drm_connector *connector,
> +				      struct hdmi_codec_daifmt *fmt,
> +				      struct hdmi_codec_params *hparms)
> +{
> +	struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> +	u8 audio_cmd[6] = {0x57, 0x48, 0x36, 0x3a};
> +	u8 data[5];
> +	int ret;
> +
> +	if (hparms->sample_width == 32)
> +		return -EINVAL;
> +
> +	switch (fmt->fmt) {
> +	case HDMI_I2S:
> +		audio_cmd[4] = 0x01;
> +		break;
> +	case HDMI_SPDIF:
> +		audio_cmd[4] = 0x02;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	audio_cmd[5] = hparms->channels;
> +	guard(mutex)(&lt9611c->ocm_lock);
> +
> +	ret = lt9611c_read_write_flow(lt9611c, audio_cmd, sizeof(audio_cmd),
> +				      data, sizeof(data));
> +	if (ret < 0) {
> +		dev_err(lt9611c->dev, "set audio info failed!\n");
> +		return ret;
> +	}
> +
> +	return drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
> +									&hparms->cea);

[Severity: High]
Will this cause a self-deadlock? The ocm_lock is acquired here in 
lt9611c_hdmi_audio_prepare(). We then call 
drm_atomic_helper_connector_hdmi_update_audio_infoframe(), which internally 
invokes the connector's write_infoframe hook mapped to 
lt9611c_hdmi_write_audio_infoframe(). That inner function attempts to acquire 
ocm_lock again. Since Linux mutexes are not recursive, this guarantees a 
deadlock.

> +}

[ ... ]

> +static int lt9611c_parse_dt(struct device *dev,
> +			    struct lt9611c *lt9611c)
> +{
> +	int ret;
> +
> +	lt9611c->dsi0_node = of_graph_get_remote_node(dev->of_node, 0, -1);
> +	if (!lt9611c->dsi0_node)
> +		return dev_err_probe(dev, -ENODEV, "failed to get remote node for primary dsi\n");
> +
> +	lt9611c->dsi1_node = of_graph_get_remote_node(dev->of_node, 1, -1);
> +
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, -1, NULL, &lt9611c->bridge.next_bridge);
> +	if (ret) {
> +		of_node_put(lt9611c->dsi1_node);
> +		of_node_put(lt9611c->dsi0_node);
> +		return ret;
> +	}
> +	drm_bridge_get(lt9611c->bridge.next_bridge);

[Severity: High]
Does this code leak a module reference to the next bridge? The reference 
acquired by drm_bridge_get() here is never released. The driver's 
lt9611c_remove() function does not call drm_bridge_put() to balance this, 
meaning every unbind/rebind cycle leaks a reference.

> +	return 0;
> +}

[ ... ]

> +static ssize_t lt9611c_firmware_store(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t len)
> +{
> +	struct lt9611c *lt9611c = dev_get_drvdata(dev);
> +	int ret;
> +
> +	lt9611c_lock(lt9611c);
> +
> +	ret = lt9611c_firmware_upgrade(lt9611c);

[Severity: Medium]
Is it safe to hold ocm_lock during a synchronous firmware load? 
lt9611c_firmware_upgrade() calls request_firmware(), which can block for up to 
60 seconds on VFS and user-mode helpers. While blocked, the IRQ threaded 
handler and atomic commit callbacks will be starved as they contend for the 
same mutex.

> +	if (ret < 0)
> +		dev_err(dev, "upgrade failure\n");
> +
> +	lt9611c_unlock(lt9611c);
> +
> +	return ret < 0 ? ret : len;
> +}
> +
> +static ssize_t lt9611c_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", lt9611c->fw_version);

[Severity: High]
Can dev_get_drvdata(dev) return NULL here? Sysfs attributes are added via the 
dev_groups member of the i2c_driver struct, meaning the driver core creates 
them before the probe function finishes. In lt9611c_probe(), 
i2c_set_clientdata() is called very late. If userspace reads this attribute 
concurrently before probe completes, this will dereference a NULL pointer.

> +}

[ ... ]

> +static int lt9611c_probe(struct i2c_client *client)
> +{

[ ... ]

> +	lt9611c->bridge.hdmi_audio_max_i2s_playback_channels = 8;
> +	lt9611c->bridge.hdmi_audio_dai_port = 2;
> +
> +	devm_drm_bridge_add(dev, &lt9611c->bridge);

[Severity: High]
Shouldn't we check the return value of devm_drm_bridge_add()? If devres 
allocation fails, the bridge won't be added to the DRM bridge chain, but probe 
will continue and return success. This leaves the subsystem in an inconsistent 
state.

> +
> +	/* Attach primary DSI */
> +	lt9611c->dsi0 = lt9611c_attach_dsi(lt9611c, lt9611c->dsi0_node);
> +	if (IS_ERR(lt9611c->dsi0)) {

[ ... ]

> +static void lt9611c_remove(struct i2c_client *client)
> +{
> +	struct lt9611c *lt9611c = i2c_get_clientdata(client);
> +
> +	cancel_work_sync(&lt9611c->work);
> +	regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), lt9611c->supplies);
> +	of_node_put(lt9611c->dsi1_node);
> +	of_node_put(lt9611c->dsi0_node);
> +}

[Severity: High]
Does this leave a race condition leading to a use-after-free? The IRQ was 
registered via devm_request_threaded_irq(). Because there is no explicit 
disable_irq() call here, the interrupt line remains active until devres 
cleanup runs after remove() returns. If an interrupt fires after 
cancel_work_sync(), the threaded handler will run, re-queue the work item, and 
attempt I2C access while the device is unpowered.

> +
> +static int lt9611c_bridge_suspend(struct device *dev)
> +{
> +	struct lt9611c *lt9611c = dev_get_drvdata(dev);
> +	int ret;
> +
> +	dev_dbg(lt9611c->dev, "suspend\n");
> +	disable_irq(lt9611c->client->irq);
> +
> +	gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), lt9611c->supplies);

[Severity: High]
Should this flush the workqueue before disabling regulators? If an interrupt 
fired and scheduled lt9611c->work right before disable_irq() was called, the 
work item stays in the queue. When it executes, it will trigger an I2C read in 
lt9611c_bridge_detect() while the device regulators are disabled, leading to 
timeouts or a kernel panic.

> +	if (ret)
> +		dev_err(lt9611c->dev, "regulator bulk disable failed.\n");
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-lt9611c-v7-v7-0-7553a14735fc@oss.qualcomm.com?part=2