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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/bridge: probe race causing NULL pointer dereference via early devm_drm_bridge_add()
- [High] drm/bridge: use-after-free of workqueue on unbind due to reversed devres cleanup ordering
- [Medium] drm/bridge: asynchronous atomic_enable breaks DRM synchronous bridge chain initialization
--

commit 2cf60daaeefff033f413189c179402f2bd50e48c
Author: Yi Zhang <zhanyi@qti.qualcomm.com>

drm/bridge: add support for lontium lt9211c bridge

This commit extends the lontium-lt9211 driver to support the LT9211C
variant by detecting the new chip ID and implementing its specific
initialization sequences. It defers the final bridge configuration to
a work item scheduled from atomic_enable() to wait for an active DSI stream.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt9211.c b/drivers/gpu/drm/bridge/lontium-lt9211.c
> index 23682384daf46..e6c08a7fdc27c 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9211.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9211.c

[ ... ]

> @@ -523,6 +1271,21 @@ static void lt9211_atomic_enable(struct drm_bridge *bridge,
>  	if (ret)
>  		return;
>  
> +	if (ctx->chip_data->chip_type == LT9211C) {
> +		drm_mode_copy(&ctx->lt9211c_mode, mode);
> +		ctx->lt9211c_bpp24 = lvds_format_24bpp;
> +		ctx->lt9211c_jeida = lvds_format_jeida;
> +		ctx->lt9211c_de_high = !!(bus_flags & DRM_BUS_FLAG_DE_HIGH);
> +		ctx->lt9211c_crtc = crtc;
> +
> +		/*
> +		 * Needs a live DSI stream to auto-detect RX resolution/format;
> +		 * defer bridge configuration until vblank inside the work item.
> +		 */
> +		queue_work(ctx->wq, &ctx->lt9211_work);
> +		return;
> +	}

[Severity: Medium]
Does returning asynchronously here violate the DRM bridge API contract?

By offloading the hardware initialization to a workqueue and returning
immediately, the DRM core might proceed to enable downstream panels in the
chain before the LVDS signal is actually active.

Could this result in display initialization failures on platforms where
downstream panels require a stable LVDS stream during their enable sequence?

[ ... ]

> @@ -748,45 +1516,53 @@ static int lt9211_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> -	ctx->regmap = devm_regmap_init_i2c(client, &lt9211_regmap_config);
> +	ctx->chip_data = i2c_get_match_data(client);
> +	if (!ctx->chip_data)
> +		return -ENODEV;
> +
> +	ctx->regmap = devm_regmap_init_i2c(client, ctx->chip_data->regmap_config);
>  	if (IS_ERR(ctx->regmap))
>  		return PTR_ERR(ctx->regmap);
>  
> -	dev_set_drvdata(dev, ctx);
> -	i2c_set_clientdata(client, ctx);
> -
>  	ctx->bridge.of_node = dev->of_node;
> -	drm_bridge_add(&ctx->bridge);
> +	ret = devm_drm_bridge_add(dev, &ctx->bridge);
> +	if (ret)
> +		return ret;

[Severity: High]
Is there a risk of a NULL pointer dereference here if the bridge is exposed
before its workqueue is initialized?

Because devm_drm_bridge_add() globally exposes the bridge to the subsystem, a
concurrent display controller probe could immediately discover it and initiate
an atomic commit.

If lt9211_atomic_enable() is reached before ctx->wq and ctx->lt9211_work are
initialized further down in lt9211_probe(), it will call queue_work() with an
uninitialized NULL workqueue.

>  	ret = lt9211_host_attach(ctx);
>  	if (ret)
> -		drm_bridge_remove(&ctx->bridge);
> -
> -	return ret;
> -}
> +		return ret;
>  
> -static void lt9211_remove(struct i2c_client *client)
> -{
> -	struct lt9211 *ctx = i2c_get_clientdata(client);
> +	if (ctx->chip_data->chip_type == LT9211C) {
> +		ctx->wq = create_workqueue("lt9211_work");
> +		if (!ctx->wq)
> +			return -ENOMEM;
> +		/* Register after DSI attach so devres flushes work before freeing ctx->dsi. */
> +		ret = devm_add_action_or_reset(dev, lt9211_destroy_wq, ctx->wq);
> +		if (ret)
> +			return ret;
> +		INIT_WORK(&ctx->lt9211_work, lt9211_work_func);
> +	}

[Severity: High]
Could this devres ordering lead to a use-after-free during driver unbind?

The devres infrastructure executes cleanup actions in reverse order of
addition (LIFO). Since devm_drm_bridge_add() was called earlier in the probe
sequence, the workqueue cleanup (lt9211_destroy_wq) will execute before the
bridge is unregistered from the DRM subsystem.

If the DRM core invokes lt9211_atomic_enable() or lt9211_atomic_disable()
during this unbind window, the driver will dereference the freed ctx->wq
pointer in queue_work() or cancel_work_sync().

> -	drm_bridge_remove(&ctx->bridge);
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-add-lt9211c-bridge-v9-0-d78b39677a61@oss.qualcomm.com?part=2