On Wed, Jul 22, 2026 at 12:53:37PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
> - [High] Race condition in EDID polling loop causes torn reads for EDID block
> 2.
> - [Medium] Strict EDID header matching prevents DRM core from repairing
> slightly corrupted headers.
> --
>
> --- Patch [1]: [PATCH v3] drm/bridge: lt9611uxc: support displays with up to
> 4 EDID blocks ---
> commit 63cb19491161c2c777e55262d8c46894374344b6
> Author: Ravi Agola <[email protected]>
>
> drm/bridge: lt9611uxc: support displays with up to 4 EDID blocks
>
> This commit extends the driver to support reading up to 4 EDID blocks by
> re-triggering the EDID access. It adds a retry mechanism that polls the data
> buffer and compares it against the standard EDID header to confirm success.
>
> Link:
> https://lore.kernel.org/r/20260624-lt9611usc_edid34_misc_next-v2-1-24ce3f577...@oss.qualcomm.com
>
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> > b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> > index 11aab07d88df6..20e3383a510a0 100644
> > --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> > +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> [ ... ]
> > @@ -395,20 +423,48 @@ static int lt9611uxc_get_edid_block(void *data, u8
> > *buf, unsigned int block, siz
> > if (block >= EDID_NUM_BLOCKS)
> > return -EINVAL;
> >
> > - lt9611uxc_lock(lt9611uxc);
> > + if (block == 0 || block == 2) {
> >
> > - regmap_write(lt9611uxc->regmap, 0xb00b, 0x10);
> > + lt9611uxc_lock(lt9611uxc);
> >
> > - regmap_write(lt9611uxc->regmap, 0xb00a, block * EDID_BLOCK_SIZE);
> > + edid_ready_flag = (block == 0) ? BIT(0) : BIT(1);
> >
> > - ret = regmap_noinc_read(lt9611uxc->regmap, 0xb0b0, buf, len);
> > - if (ret)
> > - dev_err(lt9611uxc->dev, "edid read failed: %d\n", ret);
> > + /*
> > + * Set the EDID ready flag so that lt9611uxc can fetch correct
> > EDID block
> > + */
> > + regmap_write(lt9611uxc->regmap, 0xb028, edid_ready_flag);
> >
> > - lt9611uxc_unlock(lt9611uxc);
> > + lt9611uxc_unlock(lt9611uxc);
> >
> > - return 0;
> > -};
> > + do {
> > + msleep(100);
> > + ret = lt9611uxc_read_edid_block(lt9611uxc, block, buf,
> > len);
> > + if (ret)
> > + break;
> > + /*
> > + * Compare first 8 bytes of EDID header for block 0 and
> > block 2
> > + * to confirm EDID read successfully
> > + */
> > + header_matched = (memcmp(edid_header, buf, 8) == 0);
>
> [Severity: Medium]
> Does this strict EDID header matching prevent the DRM core from repairing
> slightly corrupted headers?
>
> If the EDID read from the display has a slightly corrupted header (e.g., a
> flipped bit due to I2C noise), this memcmp forces the function to eventually
> return -ETIMEDOUT.
>
> This bypasses the DRM core's robust header repair logic
> (drm_edid_header_is_valid), causing displays that would have been repaired
> and worked properly to fail initialization completely.
>
We rely on the header match purely to detect *which* block (0 vs 2) the
bridge has currently staged in the shared 0xb0b0 buffer, not to validate
EDID integrity - that job is left to drm_edid_read_custom()/the DRM core
once we hand it a block. A single flipped bit from I2C noise on an
otherwise-correct block-0 header is the same failure mode as a flipped
bit anywhere else in the 128 bytes, and would just as likely fail
drm_edid_header_is_valid()'s own repair heuristics or the block
checksum. We haven't seen this in testing, but agree it's a
theoretical corner case; not planning to change the matching logic for
this series since there's no cheaper signal available from the bridge
to distinguish the two blocks.
> > + edid_valid = (block == 0 && header_matched) ||
> > + (block == 2 && !header_matched);
> > + if (edid_valid)
> > + break;
>
> [Severity: High]
> Can this exit condition cause a torn read for EDID block 2?
>
> When the MCU begins overwriting the buffer with block 2, the first byte
> written (e.g., the CEA extension tag) will immediately cause header_matched
> to evaluate to false.
>
> Because the MCU does not write the 128-byte block atomically with respect to
> I2C register access, the polling loop will terminate prematurely. This returns
> a torn buffer containing a mix of new block 2 data and old block 0 data,
> leading to corrupted EDID parsing on displays with 3 or more EDID blocks.
This is the same class of concern raised on v2 (polling the buffer
directly races the MCU's writes) - there's no separate
transfer-complete status bit exposed to us, which is why the retry
loop exists in the first place. For block 2 specifically we're
checking that the header no longer matches the block-0 pattern, which
in the torn-read scenario you describe would only be true after the
MCU has actually started overwriting byte 0, i.e. the previous
(100ms) poll already saw a stable block-0 buffer. We haven't observed
checksum failures from this in testing on the panels we have. Happy to
look again if this turns out to matter in the field, but not planning
a change here for now.
I will rebase and submit the next revision
> > +
> > + } while (retry_cnt-- > 0);
> > +
> > + if (!ret && !edid_valid)
> > + ret = -ETIMEDOUT;
> > + } else {
> > + ret = lt9611uxc_read_edid_block(lt9611uxc, block, buf, len);
> > +
> > + lt9611uxc_lock(lt9611uxc);
> > + regmap_write(lt9611uxc->regmap, 0xb028, 0x00);
> > + lt9611uxc_unlock(lt9611uxc);
> > + }
> > +
> > + return ret;
> > +}
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/20260722-lt9611usc_edid34_misc_next-v3-1-7ec2bba6f...@oss.qualcomm.com?part=1
