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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Deferring LT9211C bridge configuration to an asynchronous workqueue breaks the DRM display pipeline enablement sequence and introduces race conditions.
- [High] Signed integer overflow in pixel clock calculations leads to undefined behavior.
- [High] Unchecked division by `mode->clock` can cause a kernel panic (divide-by-zero).
- [Medium] Chip ID detection succeeds for either hardware variant but ignores device tree mismatch, leading to incorrect hardware initialization.

Pre-existing issues:
- [High] Unbinding the device via sysfs frees the bridge context while it is still referenced by the DRM pipeline (Use-After-Free).
--

--- Patch [2]: [PATCH v7 2/2] drm/bridge: add support for lontium lt9211c bridge ---
commit 8d2ec0de9ed01158cbba906a0bfbb93f7fab90c7
Author: Yi Zhang <[email protected]>
Subject: drm/bridge: add support for lontium lt9211c bridge

This commit extends the lontium-lt9211 driver to support DSI-to-LVDS
bridge configuration for both LT9211 and LT9211C variants. It detects
chip IDs, provides per-chip configuration, and implements an asynchronous
initialization sequence for the LT9211C.

> diff --git a/drivers/gpu/drm/bridge/lontium-lt9211.c b/drivers/gpu/drm/bridge/lontium-lt9211.c
> --- a/drivers/gpu/drm/bridge/lontium-lt9211.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9211.c
[ ... ]
> @@ -120,14 +209,23 @@ static int lt9211_read_chipid(struct lt9211 *ctx)
>  		return ret;
>  	}
>  
> -	/* Test for known Chip ID. */
> -	if (chipid[0] != REG_CHIPID0_VALUE || chipid[1] != REG_CHIPID1_VALUE) {
> -		dev_err(ctx->dev, "Unknown Chip ID: 0x%02x 0x%02x 0x%02x\n",
> -			chipid[0], chipid[1], chipid[2]);
> -		return -EINVAL;
> +	/* Test for LT9211 Chip ID. */
> +	if (chipid[0] == REG_CHIPID0_VALUE && chipid[1] == REG_CHIPID1_VALUE) {
> +		dev_dbg(ctx->dev, "Detected LT9211 chip\n");
> +		return 0;
>  	}
>  
> -	return 0;
> +	/* Test for LT9211C Chip ID. */
> +	if (chipid[0] == REG_CHIPID0_LT9211C_VALUE &&
> +	    chipid[1] == REG_CHIPID1_LT9211C_VALUE &&
> +	    chipid[2] == REG_CHIPID2_LT9211C_VALUE) {
> +		dev_dbg(ctx->dev, "Detected LT9211C chip\n");
> +		return 0;
> +	}

[Severity: Medium]
Does this chip ID validation check the physically detected chip against the
expected configuration? Since it returns 0 for both variants without checking
ctx->chip_data->chip_type (which comes from the device tree compatible
string), could an incorrectly configured device tree allow the driver to
proceed with the wrong initialization sequence?

[ ... ]
> @@ -454,6 +552,657 @@ static int lt9211_configure_tx(struct lt9211 *ctx, bool jeida,
[ ... ]
> +static int lt9211c_configure_tx(struct lt9211 *ctx, bool jeida, bool bpp24,
> +				bool de, const struct drm_display_mode *mode)
> +{
[ ... ]
> +	if (ctx->lvds_dual_link)
> +		phy_clk = (u32)(mode->clock * 7 / 2);
> +	else
> +		phy_clk = (u32)(mode->clock * 7);

[Severity: High]
Could this pixel clock calculation overflow? Since mode->clock is a signed
32-bit integer, multiplying it by 7 could overflow INT_MAX for high pixel
clocks (like a 4K resolution at ~594,000 kHz) before the result is cast to
u32.

Without a mode_valid or atomic_check callback to restrict the maximum
supported pixel clock, could this cause incorrect hardware initialization or
undefined behavior?

[ ... ]
> +	/*
> +	 * 0x8233: pix_mux sel & pix_div sel
> +	 * To avoid floating point operations, The pixclk_div is enlarged by 10 times
> +	 */
> +	if (mode->clock > 150000) {
> +		val = 0x04;
> +		pixclk_div = 35;
> +	} else {
> +		pixclk_div =
> +			(u8)((phy_clk * sericlk_div * 10) / (mode->clock * 7));

[Severity: High]
Does this division need to ensure mode->clock is non-zero? If userspace
provides a malformed mode with a clock of 0, could this trigger a
divide-by-zero exception and kernel panic? 

Given the driver doesn't implement mode_valid or atomic_check callbacks,
there appears to be no filtering of custom modes before this calculation,
or the similar division below at:
div_set = (u8)(phy_clk * sericlk_div / mode->clock / pre_div)

[ ... ]
> @@ -523,6 +1272,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 after vblank.
> +		 */
> +		queue_delayed_work(ctx->wq, &ctx->lt9211_dw, 0);
> +		return;
> +	}

[Severity: High]
Does deferring the bridge configuration to an asynchronous workqueue break the
DRM atomic enablement sequence? 

By returning immediately from atomic_enable(), downstream bridges like the
panel might be activated before the LT9211C has configured its RX, PLLs, and
TX output.

Additionally, if the CRTC relies on the bridge being active to generate
vblanks, could drm_crtc_wait_one_vblank() in lt9211_delayed_work_func()
cause a deadlock? If the pipeline is disabled while the work is running,
could cancel_delayed_work_sync() block indefinitely or cause a
use-after-free?

[ ... ]
> @@ -748,45 +1517,48 @@ static int lt9211_probe(struct i2c_client *client)
[ ... ]
> -	ctx->bridge.of_node = dev->of_node;
> -	drm_bridge_add(&ctx->bridge);
> -
> -	ret = lt9211_host_attach(ctx);
> +	ctx->bridge.of_node = dev->of_node;
> +	ret = devm_drm_bridge_add(dev, &ctx->bridge);
>  	if (ret)
> -		drm_bridge_remove(&ctx->bridge);
> -
> -	return ret;
> -}
> -
> -static void lt9211_remove(struct i2c_client *client)
> -{
> -	struct lt9211 *ctx = i2c_get_clientdata(client);
> +		return ret;
>  
> -	drm_bridge_remove(&ctx->bridge);
> +	return lt9211_host_attach(ctx);
>  }

[Severity: High]
This is a pre-existing issue, but does tying the bridge context to the device
lifetime (via devm_drm_bridge_alloc and devm_drm_bridge_add) instead of the
DRM object lifetime leave the module vulnerable?

If the device is unbound via sysfs while the display is active, the devres
cleanup frees the context memory. Since the DRM core doesn't synchronously
tear down active displays on bridge removal, would subsequent calls by the DRM
core to functions like lt9211_atomic_disable() on the freed pointer result in
a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2