On Wed, Sep 02, 2026 at 07:40:19PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s)
> to consider:
> - [High] Double-free of device nodes `dsi0_node` and `dsi1_node` on error
> path due to mixing manual `of_node_put()` with `__free(device_node)`.
> - [High] Regmap page cache becomes desynchronized after hardware resets,
> causing subsequent I2C operations to access the wrong registers.
> - [Medium] Mixing `__free()` scope-based cleanup with `goto`-based error
> handling.
> - [Medium] Bitwise operations in the IRQ handler corrupt the interrupt clear
> register if multiple interrupts are pending.
> - [Low] I2C device ID array is not declared const and uses positional
> initializers.
> --
>
> commit 79126746072fd218bc214fab821198cac4bc10bd
> Author: Sunyun Yang <[email protected]>
>
> drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
>
> This commit adds a new DRM bridge driver for the Lontium LT9611C(EX/UXD)
> MIPI DSI to HDMI bridge chip. It supports configuring the hardware, handling
> interrupts, and triggering firmware upgrades over I2C.
>
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> > new file mode 100644
> > index 0000000000000..ef1e9726fdd3c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c
> [ ... ]
> > +static void lt9611c_reset(struct lt9611c *lt9611c)
> > +{
> > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > + usleep_range(10000, 12000);
> > +
> > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > + msleep(400);
> > +}
>
> [Severity: High]
> Does resetting the hardware here desynchronize the regmap page cache?
>
> The physical reset via GPIO clears the chip's internal page register back
> to 0x00, but regmap's cached page selector isn't explicitly cleared. If the
> last I2C operation was on a different page (like 0xe0), subsequent writes
> might skip the page selector write because regmap thinks it is already set,
> causing those operations to mistakenly target page 0x00 instead.
Not an issue — the regmap config uses no cache type (REGCACHE_NONE by default).
Regmap always performs a real hardware read
of the selector register before deciding whether to update it, so it correctly
detects the page reset to 0x00 after GPIO
reset.
>
> [ ... ]
> > +static irqreturn_t lt9611c_irq_thread_handler(int irq, void *dev_id)
> > +{
> [ ... ]
> > + /* 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 corrupt the clear register if multiple interrupts are pending?
>
> By writing irq_status & (~BIT(0)) back to the hardware clear register, if
> there are other interrupts pending in irq_status (for example, bit 1 is set),
> this will mistakenly write a 1 to those other bits during the final clear
> step for bit 0.
We only support hpd interrupt.
>
> [ ... ]
> > +static int lt9611c_parse_dt(struct device *dev,
> > + struct lt9611c *lt9611c,
> > + struct device_node **dsi0_node,
> > + struct device_node **dsi1_node)
> > +{
> [ ... ]
> > +err_put_dsi1:
> > + of_node_put(*dsi1_node);
> > + of_node_put(*dsi0_node);
> > + return ret;
> > +}
> [ ... ]
> > +static int lt9611c_probe(struct i2c_client *client)
> > +{
> [ ... ]
> > + struct device_node *dsi0_node __free(device_node) = NULL;
> > + struct device_node *dsi1_node __free(device_node) = NULL;
> [ ... ]
> > + ret = lt9611c_parse_dt(dev, lt9611c, &dsi0_node, &dsi1_node);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to parse device tree\n");
>
> [Severity: High]
> Can this cause a double-free of the device nodes?
>
> The pointers dsi0_node and dsi1_node are declared with __free(device_node).
> If lt9611c_parse_dt() fails, it manually calls of_node_put() on both nodes
> but does not set the caller's pointers to NULL. When lt9611c_probe() returns
> an error here, its scope ends and the automatic cleanup will call
> of_node_put()
> again on the same pointers, leading to a reference count underflow.
Will fix it in v13.
>
> [ ... ]
> > + /* Attach primary DSI */
> > + dsi = lt9611c_attach_dsi(lt9611c, dsi0_node);
> > + if (IS_ERR(dsi)) {
> > + ret = PTR_ERR(dsi);
> > + goto err_remove_bridge;
> > + }
>
> [Severity: Medium]
> Does this mix scope-based cleanup with goto-based error handling?
>
> The function declares variables with __free(device_node) at the top but uses
> goto err_remove_bridge and goto err_disable_regulators for cleanup here and
> below. The kernel cleanup subsystem prohibits mixing goto and __free() in the
> same function to avoid confusing ownership semantics and resource leaks.
Will fix it by removing __free(device_node) and using explicit of_node_put in
all exit paths.
>
> [ ... ]
> > +static struct i2c_device_id lt9611c_id[] = {
> > + { "lt9611c", (kernel_ulong_t)<9611c_chip_data[CHIP_LT9611C] },
> > + { "lt9611ex", (kernel_ulong_t)<9611c_chip_data[CHIP_LT9611EX] },
> > + { "lt9611uxd", (kernel_ulong_t)<9611c_chip_data[CHIP_LT9611UXD] },
> > + { /* sentinel */ }
> > +};
>
> [Severity: Low]
> Is this missing a const qualifier and named initializers?
>
> The I2C subsystem dictates that initialized arrays of type struct
> i2c_device_id
> must be declared const and use named initializers instead of positional ones.
Added const qualifier and named initializers .name / .driver_data.
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=2
