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. - [High] Race condition between lt9611uxc_hpd_work and the IRQ handler can corrupt the edid_read state, leading to permanent EDID read timeouts. - [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. - [Medium] Polling the EDID data buffer directly for a header match races with the MCU's internal memory writes, risking torn reads. - [Low] Unconditional memcmp of 8 bytes may read past the end of buf if a caller provides a short buffer. --
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. [ ... ] > @@ -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. [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? > + 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? > + } 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? 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? > + > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260624-lt9611usc_edid34_misc_next-v2-1-24ce3f577...@oss.qualcomm.com?part=1
