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

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 <[email protected]>
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.

> +
> +	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().

[ ... ]

> +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.

> +}
> +
> +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.

> +
> +	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/[email protected]?part=2