Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Missing cancellation of the delayed work in the disable path leads to I2C writes to an unpowered/reset device. - [High] The delayed work timer is not cancelled during driver removal, resulting in a critical use-after-free. - [High] The dynamically allocated workqueue is leaked if probe fails during host attach. - [High] LVDS format configurations (bpp24, jeida, de) are left uninitialized when queuing the delayed work, resulting in incorrect hardware configuration. - [High] Deferring hardware enablement to a delayed workqueue violates DRM atomic subsystem guarantees. - [Low] Signed error codes from `regmap_write()` are accumulated using bitwise OR, mangling the error values. --
--- Patch [2]: [PATCH v6 2/2] drm/bridge: add support for lontium lt9211c bridge --- commit 1ae4415156f1af5c0293e73eda918915f0e313c8 Author: Yi Zhang <[email protected]> drm/bridge: add support for lontium lt9211c bridge Adds support for the LT9211C variant to the existing lt9211 driver. It detects the chip by ID, adds a new regmap for LT9211C, and implements DSI-to-LVDS init sequences. A delayed workqueue is used to configure LT9211C after the DSI clock is running. > diff --git a/drivers/gpu/drm/bridge/lontium-lt9211.c > b/drivers/gpu/drm/bridge/lontium-lt9211.c > index f39d83a5ae377..36c004df34fcd 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt9211.c > +++ b/drivers/gpu/drm/bridge/lontium-lt9211.c > @@ -454,6 +526,651 @@ static int lt9211_configure_tx(struct lt9211 *ctx, bool > jeida, > [ ... ] > +static int lt9211c_configure_plls(struct lt9211 *ctx, > + const struct drm_display_mode *mode) > +{ > [ ... ] > + if (mode->clock < 44000) { > + ret = regmap_write(ctx->regmap, 0xd00c, 0x60); > + ret |= regmap_write(ctx->regmap, 0xd01b, 0x00); > + ret |= regmap_write(ctx->regmap, 0xd01c, 0x60); > + } else { > + ret = regmap_write(ctx->regmap, 0xd00c, 0x40); > + ret |= regmap_write(ctx->regmap, 0xd01b, 0x00); > + ret |= regmap_write(ctx->regmap, 0xd01c, 0x40); > + } [Severity: Low] Does using bitwise OR here mangle the signed error codes returned by regmap_write()? Accumulating negative error values via bitwise OR creates a different negative value, which destroys the semantic meaning of the original error code. > [ ... ] > @@ -523,6 +1240,15 @@ static void lt9211_atomic_enable(struct drm_bridge > *bridge, > if (ret) > return; > > + if (ctx->chip_type == LT9211C && ctx->wq) { > + drm_mode_copy(&ctx->mode, mode); > + /* LT9211C must enable after mipi clock enable */ > + queue_delayed_work(ctx->wq, &ctx->lt9211_dw, > + msecs_to_jiffies(100)); > + dev_dbg(ctx->dev, "LT9211C enabled.\n"); > + return; > + } [Severity: High] Does this code fail to save the LVDS format configurations to the context struct? The variables jeida, bpp24, and de are extracted from the bridge state earlier in this function, but they are never saved to ctx->jeida, ctx->bpp24, or ctx->de before queuing the work. When lt9211c_configure_tx() runs later in the workqueue, won't it read uninitialized (zero) values for these fields? [Severity: High] By deferring the hardware initialization to a delayed workqueue, does this introduce a regression violating the DRM atomic subsystem's expectation that the hardware is fully configured when atomic_enable completes? If the goal is to wait for the upstream DSI host to enable its clock first, would it be better to set the pre_enable_prev_first flag on the bridge rather than bypassing the synchronous atomic commit design? [Severity: High] What happens if the display pipeline is rapidly disabled before this 100ms timer expires? Since this patch introduces a delayed work item, if the driver's disable path powers off the chip and asserts reset: lt9211_atomic_disable() gpiod_set_value(ctx->reset_gpio, 0); usleep_range(10000, 11000); ret = regulator_disable(ctx->vccio); without calling cancel_delayed_work_sync(), the delayed work could execute on a powered-down device, leading to I2C timeouts and bus lockups. > [ ... ] > @@ -747,10 +1489,25 @@ static int lt9211_probe(struct i2c_client *client) > [ ... ] > + if (ctx->chip_type == LT9211C) { > + ctx->wq = create_workqueue("lt9211_work"); > + if (!ctx->wq) > + return -ENOMEM; > + INIT_DELAYED_WORK(&ctx->lt9211_dw, lt9211_delayed_work_func); > + } > + > dev_set_drvdata(dev, ctx); > i2c_set_clientdata(client, ctx); > > @@ -768,17 +1525,22 @@ static void lt9211_remove(struct i2c_client *client) > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > ret = lt9211_host_attach(ctx); > if (ret) > drm_bridge_remove(&ctx->bridge); > > return ret; > } [Severity: High] If lt9211_host_attach() returns an error like -EPROBE_DEFER, does this error path leak the dynamically allocated workqueue? It looks like destroy_workqueue(ctx->wq) is missing before returning, which could lead to resource exhaustion if probe is deferred multiple times. > static void lt9211_remove(struct i2c_client *client) > { > struct lt9211 *ctx = i2c_get_clientdata(client); > > + if (ctx->wq) > + destroy_workqueue(ctx->wq); > + > drm_bridge_remove(&ctx->bridge); > } [Severity: High] Can this result in a regression where a use-after-free occurs if the delayed work timer is still pending during driver removal? destroy_workqueue() flushes active work, but it does not cancel pending timers for delayed works. Should we call cancel_delayed_work_sync(&ctx->lt9211_dw) before destroying the workqueue to prevent the timer from firing after the context is freed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
