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

[Vishnu Saini <vishnu.saini@oss.qualcomm.com>]

On Sat, Aug 22, 2026 at 05:15:11PM +0000, sashiko-bot@kernel.org wrote:
> 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 <zhanyi@qti.qualcomm.com>
> 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?
will fix this in next revision.
 
> [ ... ]
> > @@ -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?
False positive -- the driver does implement mode_valid: lt9211_mode_valid()
(lt9211.c:1342, wired into lt9211_funcs for both LT9211 and LT9211C) clamps
mode->clock to the 25000-176000 kHz LVDS range before any mode reaches
atomic_enable(). 176000 * 7 = 1,232,000, well under INT_MAX.
No overflow is reachable through the DRM mode-selection path.
 
> [ ... ]
> > +	/*
> > +	 * 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)
False positive -- lt9211_mode_valid() rejects any mode with
clock < 25000 (kHz) before it can reach this code, so mode->clock is
never 0 here. No mode bypasses mode_valid() on this path (DRM core calls
bridge mode_valid for every candidate mode during connector probe).


> [ ... ]
> > @@ -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?
This is a known, intentional tradeoff and was discussed in the
v6->v7 review, bridge fails to read RX width/height/format because the
stream isn't live yet at that point in the pipeline.
The delayed work + drm_crtc_wait_one_vblank() defers just far enough to guarantee a live stream.
On the specific concerns:
   - drm_crtc_wait_one_vblank() has its own internal timeout and will not
     block indefinitely if vblank IRQs stop arriving.
   - cancel_delayed_work_sync() in lt9211_atomic_disable() is therefore
     also bounded -- it can wait at most one vblank-wait timeout plus the
     remaining init sequence, not forever.
 
> [ ... ]
> > @@ -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?

This is pre-existing issue.
The upstream-provided fix for exactly this class of bug is
drm_bridge_unplug()/drm_bridge_enter()/drm_bridge_exit()
Its only current adopter is drivers/gpu/drm/bridge/ti-sn65dsi83.c (d2e8d1bc840b).
Applying it here would mean reintroducing an explicit .remove() callback calling
drm_bridge_unplug(), which is a partial, deliberate reversal of this
series' own "drm/bridge: lt9211: use devm_drm_bridge_add and drop
lt9211_remove" change from v7 -- devm_drm_bridge_add()'s automatic
drm_bridge_remove() on unbind has no "unplug" signal equivalent.

 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260822-add-lt9211c-bridge-v7-0-7dcf567d57c4@oss.qualcomm.com?part=2