On Sat, Aug 22, 2026 at 05:15:11PM +0000, [email protected] 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 <[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?
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/[email protected]?part=2
