On Wed, Jun 24, 2026 at 08:53:33AM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 5 potential issue(s) > to consider: > - [High] Clearing the 0xb028 EDID bank register to 0x00 after reading blocks > 1 or 3 prevents DRM core retries from succeeding. No, this is expected behaviour for bridge to read correct edid for block0, 1 and block2, 3. after reading block1 bridge will fetch edid for block2, 3 similarly after reading block3 bridge will fetch edid for block0, 1.
> - [High] Race condition between lt9611uxc_hpd_work and the IRQ handler can > corrupt the edid_read state, leading to permanent EDID read timeouts. yes, will add edid_read state change inside mutex lock in v3. > - [Medium] lt9611uxc_get_edid_block() silently returns success (0) when the > EDID polling loop times out, feeding incorrect data to the DRM core and > amplifying retry delays. Correct, Will return -ETIMEDOUT (or similar) when retry_cnt is exhausted without a match in v3. > - [Medium] Polling the EDID data buffer directly for a header match races > with the MCU's internal memory writes, risking torn reads. This is a fair concern in principle, but there's no separate transfer complete status bit exposed to us and is the reason for the retry loop in the first place. > - [Low] Unconditional memcmp of 8 bytes may read past the end of buf if a > caller provides a short buffer. Not currently reachable drm_edid.c always calls with len == EDID_LENGTH. > > commit f8ef2cd8a0639401cecc3757e315eeb45035f222 > Author: Ravi Agola <[email protected]> > > drm/bridge: lt9611uxc: support displays with up to 4 EDID blocks > > The LT9611UXC bridge can fetch only 2 EDID blocks at a time, which > previously limited EDID reading to 2 blocks and prevented support > for displays exposing more than 2 EDID blocks. Add driver support to fetch > up to 4 EDID blocks by re-triggering EDID access. > > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > > b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > > --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > > +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > > [ ... ] > > > @@ -170,6 +170,9 @@ static void lt9611uxc_hpd_work(struct work_struct *work) > > connected = lt9611uxc->hdmi_connected; > > mutex_unlock(<9611uxc->ocm_lock); > > > > + if (!connected) > > + lt9611uxc->edid_read = false; > > + > > drm_bridge_hpd_notify(<9611uxc->bridge, > > connected ? > > [Severity: High] > Could this create a race condition with the IRQ handler? > > If an HPD connect interrupt fires right after ocm_lock is dropped, the IRQ > handler could set both hdmi_connected and edid_read to true and queue a new > work. > > When this thread resumes, it will evaluate the stale connected variable > (false) and overwrite edid_read back to false. > > When the new work thread runs, it sees connected == true and skips updating > edid_read, potentially causing lt9611uxc_wait_for_edid() to permanently > time out. Yes, `edid_read = false` in lt9611uxc_hpd_work() is written after ocm_lock is dropped, so it's not actually protected by the lock that the IRQ handler uses when setting edid_read = true. Will move that assignment inside the ocm_lock section in v3. > [ ... ] > > > @@ -395,20 +423,44 @@ 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: Low] > Is it possible for len to be less than 8 here? If a caller provides a short > buffer, this memcmp could read past the end of buf. Not currently reachable drm_edid.c always calls with len == EDID_LENGTH. > [Severity: Medium] > Does polling the I2C SRAM buffer directly for a header match risk a torn > read? > > Since the MCU writes to this SRAM sequentially from the slow DDC bus, the CPU > might see the first 8 bytes match and immediately break the loop, while the > MCU is still writing the remaining 120 bytes. > > Could this cause a mix of old and new data to be read, leading to EDID > checksum failures? No, this is a fair concern in principle, but there's no separate transfer complete status bit exposed to us and is the reason for the retry loop in the first place. we haven't seen any failures during testing. > > + if ((block == 0 && header_matched) || > > + (block == 2 && !header_matched)) > > + break; > > + > > + } while (retry_cnt-- > 0); > > [Severity: Medium] > If this retry loop times out (retry_cnt drops below 0), will it incorrectly > return success? > > Since lt9611uxc_read_edid_block() sets ret to 0 upon completing the I2C > read (even if it's the wrong block), ret remains 0. > > This could cause the DRM core to process garbage data, fail validation, and > repeatedly retry, multiplying the delay. Should this return an error code like > -ETIMEDOUT when exhausted? Yes. `ret` retains the value from the last (technically successful) I2C read even if header_matched never turns out right, so an exhausted retry loop reports success with wrong content. Will return -ETIMEDOUT (or similar) when retry_cnt is exhausted without a match in v3. > > + } else { > > + ret = lt9611uxc_read_edid_block(lt9611uxc, block, buf, len); > > + > > + lt9611uxc_lock(lt9611uxc); > > + regmap_write(lt9611uxc->regmap, 0xb028, 0x00); > > + lt9611uxc_unlock(lt9611uxc); > > + } > > [Severity: High] > Will clearing the 0xb028 register to 0x00 break DRM core retries for blocks 1 > and 3? No, 0xb028 register is cleard as per bridge requirement for block1 and block3, as after block1 it will fetch edid for block 2 and 3 and after block 3 bridge will fetch edid for block 0 and block1. reties are required for block0 and block2 to make sure edid is fetch correctly. > If the DRM core encounters a checksum error and retries the read, it will > enter this else branch again. > > But since the register was already cleared to 0x00, won't the retry > consistently read invalid data and fail? > This is fine, it will return ret accordingly. register 0xb028 is set as per bridge requirement. > > + > > + return ret; > > +} > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/20260624-lt9611usc_edid34_misc_next-v2-1-24ce3f577...@oss.qualcomm.com?part=1
