Re: [PATCH v10 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 2 potential issue(s) to consider:
- [High] Synchronous request_firmware() in probe() blocks kernel boot process.
- [Medium] Unintended interrupt clearing and potential permanent disablement due to flawed bitwise logic in IRQ handler.
--

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

This commit adds the driver for the Lontium LT9611C, LT9611EX, and
LT9611UXD MIPI DSI to HDMI bridge chips. It converts single or dual MIPI
DSI and I2S signals to HDMI 1.4 or 2.0 output.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> index 0000000000000..e6887f2c89a6c 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c

[ ... ]

> +static int lt9611c_firmware_upgrade(struct lt9611c *lt9611c)
> +{
> +	struct device *dev = lt9611c->dev;
> +	const struct firmware *fw;
> +	u8 *buffer;
> +	size_t total_size = FW_SIZE - 1;
> +	u8 fw_crc;
> +	int ret;
> +
> +	/* load firmware — must happen outside the mcu_lock */
> +	ret = request_firmware(&fw, FW_FILE, dev);

[Severity: High]
Does this synchronous firmware load risk stalling the kernel boot process?

If the chip's firmware version reads as 0 during lt9611c_probe(), it
directly invokes lt9611c_firmware_upgrade(), triggering this synchronous
request_firmware() call.

If the firmware is not built-in or the root filesystem is not mounted
yet, this could block the I2C bus probe queue and delay boot for up to
60 seconds (the default loader timeout) or even cause a deadlock.

Could request_firmware_nowait() be used instead when triggered from
the probe path, or could the firmware update be deferred to a worker?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to load '%s'\n", FW_FILE);

[ ... ]

> +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]
Will this second write unintentionally assert the clear register for
other pending interrupts?

The handler attempts to clear BIT(0) by writing irq_status & BIT(0)
(which sets bit 0), waiting, and then writing irq_status & (~BIT(0)).
If irq_status contains any other active bits (for example, BIT(1)),
the second write will push a 1 to those bits in the clear register.

Since there is no subsequent write to reset those bits to 0, does
this leave the clear signal for those other interrupts permanently
asserted?

> +
> +	schedule_work(&lt9611c->work);
> +
> +	return IRQ_HANDLED;
> +}

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