Re: [PATCH v6 2/2] drm/bridge: add support for lontium lt9211c bridge

[sashiko-bot@kernel.org]

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 <zhanyi@qti.qualcomm.com>

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/20260719-add-lt9211c-bridge-v6-0-e56d22152586@oss.qualcomm.com?part=2