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

[Mohit Dsor <mohit.dsor@oss.qualcomm.com>]

On Mon, Aug 24, 2026 at 02:21:25PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Incorrect HPD read response length causes HDMI to never be detected.
> - [High] Sysfs attributes are accessible before drvdata is initialized, causing a NULL pointer dereference.
> - [Medium] Incorrect interrupt clear sequence leaves other active interrupts stuck high.
> - [Medium] Global CRC table is initialized concurrently in probe(), creating a data race.
> --
> 
> --- Patch [2]: [PATCH v11 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver ---
> commit 1a2f947d19c88c8a2effc903f8f83ec6e9fd330b
> Author: Sunyun Yang <syyang@lontium.com>
> Subject: drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
> 
> This commit introduces a new DRM bridge driver for the Lontium LT9611C 
> series chips. It provides conversion from MIPI DSI and I2S signals to 
> HDMI output, and includes firmware upgrade capability and sysfs integration.
> 
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > index 0000000000000..148da2bf5648e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > @@ -0,0 +1,1283 @@
> 
> [ ... ]
> 
> > +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_NONE;
> > +
> > +	/*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 sequence leave other active interrupts stuck in the clear state?
> 
> If irq_status contains other active interrupts (for example, bit 1 is set),
> the second write will write a 1 to those bits. Since there is no subsequent
> write of 0 to clear them, this appears to leave them permanently asserted.
Right now our driver only supports hpd interrupt.
> 
> > +
> > +	schedule_work(&lt9611c->work);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void lt9611c_hpd_work(struct work_struct *work)
> > +{
> > +	struct lt9611c *lt9611c = container_of(work, struct lt9611c, work);
> > +	struct device *dev = lt9611c->dev;
> > +	static const u8 hpd_data[] = { 0x00 };
> > +	struct lt9611c_cmd cmd = {
> > +		.hdr = { LT9611C_FUNC_READ, LT9611C_TYPE_HDMI, 0x31, LT9611C_CMD_SEP },
> > +		.data = hpd_data,
> > +		.data_len = 1,
> > +	};
> > +	u8 hpd_status;
> > +	struct lt9611c_rsp rsp = { .data = &hpd_status, .data_len = 1 };
> 
> [Severity: High]
> Will this ever read the actual HPD status payload?
> 
> As demonstrated by lt9611c_get_edid_block(), the LT9611C chip's read protocol
> echoes the subcommand index byte (Y0) as the first byte of the response.
> Because data_len is set to 1, the driver will only read this echo byte (0x00)
> instead of the actual HPD status in the second byte, causing hpd_status to
> always evaluate to false.
> 
> This same pattern also appears in lt9611c_bridge_detect().
Will be fixed in v12.
> 
> [ ... ]
> 
> > +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", lt9611c->fw_version);
> 
> [Severity: High]
> Can this result in a NULL pointer dereference?
> 
> Since the sysfs files are registered via lt9611c_driver.driver.dev_groups,
> the driver core creates these groups before calling the probe() function.
> However, i2c_set_clientdata() is only called at the very end of probe().
> 
> If a user accesses this sysfs file during the initialization window,
> dev_get_drvdata() will return NULL, which is immediately dereferenced here.
Will be fixed in v12.
> 
> > +}
> > +
> > +static DEVICE_ATTR_RW(firmware);
> 
> [ ... ]
> 
> > +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);
> 
> [Severity: Medium]
> Is it safe to unconditionally initialize the global lt9611c_crc8_table here?
> 
> If multiple LT9611C devices probe concurrently on different CPUs, this
> will result in multiple threads writing to the same global array
> simultaneously without synchronization.
Will be fixed in v12.
> 
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +		return dev_err_probe(dev, -ENODEV, "device doesn't support I2C\n");
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260824-lt9611c-v7-v11-0-eb4a79cb597c@oss.qualcomm.com?part=2