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

On Tue, Aug 04, 2026 at 12:59:38PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [High] Missing `disable_irq()` before `cancel_work_sync()` in `lt9611c_remove()` causes a use-after-free when `devm` frees the device context.
> - [High] Bridge is left registered and accessible to DRM core while its power regulators are disabled during module unload.
> - [High] Unconditional dereference of `cdata` returned by `i2c_get_match_data()` without checking for NULL.
> - [Medium] Incorrect interrupt clearing sequence in `lt9611c_irq_thread_handler()` can permanently suppress other pending interrupts.
> - [Medium] The return value of `devm_drm_bridge_add()` is ignored, which can lead to silent probe failures.
> - [Medium] Firmware upgrade via sysfs silently resets the bridge hardware, breaking active DRM display state.
> - [Medium] The IRQ handler incorrectly returns `IRQ_HANDLED` when the interrupt was not generated by this device.
> - [Medium] The return value of `read_poll_timeout` is ignored during flash block erase, which can lead to firmware corruption.
> --
> 
> --- Patch [2]: [PATCH v9 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver ---
> commit 6e88557e672677022a4675c645ef3cbef1ee7ce0
> Author: Sunyun Yang <[email protected]>
> Subject: drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
> 
> This commit introduces a DRM bridge driver for the Lontium LT9611C,
> LT9611EX, and LT9611UXD DSI-to-HDMI chips. It handles configuration over I2C,
> video timing setup, audio infoframe handling, and 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..f5af2856ce00b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> [ ... ]
> > +static void lt9611c_block_erase(struct lt9611c *lt9611c)
> > +{
> > +	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);
> > +		read_poll_timeout(read_flash_reg_status, flash_status,
> > +				  !(flash_status & 0x01),
> > +				  50 * USEC_PER_MSEC, 2500 * USEC_PER_MSEC,
> > +				  false, lt9611c);
> 
> [Severity: Medium]
> Is it safe to ignore the return value of read_poll_timeout() here?
Added the check here.
> 
> If the hardware fails to erase the flash within the timeout, the driver
> will proceed to lt9611c_write_data() anyway. This would cause it to write
> new firmware data over un-erased or partially erased flash pages,
> potentially corrupting the device firmware permanently.
> 
> [ ... ]
> > +static int lt9611c_firmware_upgrade(struct lt9611c *lt9611c)
> > +{
> [ ... ]
> > +	ret = lt9611c_write_crc(lt9611c, fw_crc, FW_SIZE - 1);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to write firmware crc\n");
> > +		goto out_unlock;
> > +	}
> > +
> > +	lt9611c_reset(lt9611c);
> > +	ret = lt9611c_upgrade_result(lt9611c, fw_crc);
> 
> [Severity: Medium]
> Could this reset break the active DRM display state?
> 
> When userspace writes to the firmware sysfs attribute, this function
> resets the bridge chip without notifying the DRM subsystem. This wipes all
> video timings and configurations, meaning an active display pipeline will
> go blank and become desynchronized from the DRM state.
No, it is needed. Added warn lof for this.
> 
> [ ... ]
> > +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;
> > +
> > +	guard(mutex)(&lt9611c->mcu_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: Medium]
> Should this early return use IRQ_NONE instead?
Yes, fixed this in v10.
> 
> Returning IRQ_HANDLED when the interrupt does not belong to this device
> defeats the kernel's spurious interrupt detection mechanism. If the IRQ
> line becomes noisy or stuck, the kernel will falsely assume this driver
> handled it and will not disable the line, potentially causing an interrupt
> storm that freezes the system.
> 
> > +
> > +	/*Clear interrupt: hardware requires two writes with delay*/
> > +	regmap_write(lt9611c->regmap, 0xe0df, irq_status & BIT(0));
> > +	usleep_range(10000, 12000);
> > +	regmap_write(lt9611c->regmap, 0xe0df, irq_status & (~BIT(0)));
> 
> [Severity: Medium]
> Could this interrupt clearing sequence accidentally suppress other pending
> interrupts?
As shared earlier only 1 interrupt is there.
> 
> If multiple interrupts were pending (for example, bit 1 is also set in
> irq_status), the second write outputs a 1 for those other bits because of
> the bitwise inversion logic. If the hardware clears interrupts via a
> pulse mechanism, those other bits are left stuck at 1 and are never written
> back to 0. This would break the clearing mechanism for any interrupt
> other than the primary bit.
> 
> [ ... ]
> > +static int lt9611c_probe(struct i2c_client *client)
> > +{
> > +	struct lt9611c *lt9611c;
> > +	struct device *dev = &client->dev;
> > +	bool fw_updated = false;
> > +	int ret;
> > +
> > +	crc8_populate_msb(lt9611c_crc8_table, LT9611C_CRC_POLYNOMIAL);
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +		return dev_err_probe(dev, -ENODEV, "device doesn't support I2C\n");
> > +
> > +	lt9611c = devm_drm_bridge_alloc(dev, struct lt9611c, bridge, &lt9611c_bridge_funcs);
> > +	if (IS_ERR(lt9611c))
> > +		return dev_err_probe(dev, PTR_ERR(lt9611c), "drm bridge alloc failed.\n");
> > +
> > +	lt9611c->dev = dev;
> > +	lt9611c->client = client;
> > +	const struct lt9611c_chip_data *cdata = i2c_get_match_data(client);
> > +
> > +	lt9611c->chip_type = cdata->chip_type;
> 
> [Severity: High]
> Can cdata be NULL here?
Yes check is added in v10.
> 
> When the device is instantiated via the sysfs bind attribute or new_device
> without a valid match table entry (e.g., matching by driver name fallback),
> i2c_get_match_data() will return NULL. Dereferencing cdata immediately
> would result in a NULL pointer dereference and a kernel panic.
> 
> [ ... ]
> > +	devm_drm_bridge_add(dev, &lt9611c->bridge);
> > +
> > +	enable_irq(client->irq);
> > +
> > +	return 0;
> 
> [Severity: Medium]
> Is there a missing error check for devm_drm_bridge_add()?
Added the check.
> 
> Since devm_drm_bridge_add() can return an error code like -ENOMEM,
> ignoring it allows the driver to probe successfully without registering the
> bridge. This leaves a dysfunctional driver bound to the device, silently
> consuming resources.
> 
> [ ... ]
> > +static void lt9611c_remove(struct i2c_client *client)
> > +{
> > +	struct lt9611c *lt9611c = i2c_get_clientdata(client);
> > +
> > +	cancel_work_sync(&lt9611c->work);
> 
> [Severity: High]
> Could this lead to a use-after-free since the IRQ is not disabled before
> cancelling the work?
Fixed this in v10.
> 
> The IRQ was requested with devm_request_threaded_irq, meaning it remains
> active until devres cleanup, which happens after lt9611c_remove() returns.
> An interrupt firing right after cancel_work_sync() completes will invoke
> the threaded handler, which calls schedule_work(&lt9611c->work). When
> devres frees the lt9611c structure, the newly queued work isn't cancelled,
> causing the workqueue to eventually execute lt9611c_hpd_work on freed memory.
> 
> > +	regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), lt9611c->supplies);
> 
> [Severity: High]
> Does this prematurely power down the chip while the bridge is still active?
Fixed this in v10.
> 
> Because the bridge was registered with devm_drm_bridge_add(), the DRM core
> will only unregister it during devres teardown, which runs after this
> remove function completes. By manually disabling the regulators here, there
> is a window where the bridge is globally visible and can receive DRM calls
> (such as a detect call from userspace polling), but the hardware is powered
> off. This can cause I2C timeouts or bus lockups during module unload.
> 
> > +	of_node_put(lt9611c->dsi1_node);
> > +	of_node_put(lt9611c->dsi0_node);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2