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. [ ... ] > +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. [ ... ] > +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. [ ... ] > + /* 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. [ ... ] > +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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
