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 <[email protected]> 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)(<9611c->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(<9611c->work); > + > + return IRQ_HANDLED; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
