Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing error check for it6505_read() inside it6505_aux_i2c_wait() causes I2C read failures to be silently truncated and falsely treated as successful AUX operations, populating caller buffers with error codes masquerading as valid data. - [High] Blind restoration of the entire REG_RESET_CTRL register in it6505_audio_input() clobbers concurrent modifications made by other threads or workqueues. -- --- Patch [11]: [PATCH v6 11/13] drm/bridge: it6505: don't write an error code back to the reset register --- commit 6cdd627967e5d418a885e4cc00b3b1ef771660fc Author: Daniel Golle <[email protected]> drm/bridge: it6505: don't write an error code back to the reset register This commit addresses an issue in it6505_audio_input() where a negative error code from it6505_read() could be written back to the reset register. It adds a check to return early if reading REG_RESET_CTRL fails, and also ensures the audio input frequency readback is strictly positive. > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 1678ec5b3ba70..aeaa7b5ecf5aa 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -1516,12 +1516,15 @@ static bool it6505_audio_input(struct it6505 *it6505) > int reg05, regbe; > > reg05 = it6505_read(it6505, REG_RESET_CTRL); > + if (reg05 < 0) > + return false; > + > it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, 0x00); > usleep_range(3000, 4000); > regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ); > it6505_write(it6505, REG_RESET_CTRL, reg05); [Severity: High] This is a pre-existing issue, but does this blind write back of the cached reg05 value clobber concurrent modifications? If this is called from the threaded IRQ handler or link_works, it can run concurrently with the hdcp_work delayed workqueue. If hdcp_work clears HDCP_RESET during the 3000-4000us sleep window, that concurrent change will be permanently overwritten and lost when the stale reg05 value is restored here. > - return regbe != 0xFF; > + return regbe > 0 && regbe != 0xFF; > } > > static void it6505_setup_audio_channel_status(struct it6505 *it6505) While reviewing this fix for missing error checks, I noticed another similar pre-existing issue in the same file: [Severity: High] This isn't a bug introduced by this patch, but does the lack of an error check for it6505_read() inside it6505_aux_i2c_wait() cause I2C read failures to be treated as successful operations? drivers/gpu/drm/bridge/ite-it6505.c:it6505_aux_i2c_wait() { ... if (it6505_read(it6505, REG_AUX_USER_CTRL) & AUX_EVENT) break; ... *reply = it6505_read(it6505, REG_AUX_USER_REPLY) >> 4; ... } If it6505_read(it6505, REG_AUX_USER_CTRL) fails, it returns a negative errno like -EIO (-5 = 0xFFFFFFFB). The bitwise check & AUX_EVENT (0x10) will incorrectly evaluate to true, breaking the wait loop. Then, if the read of REG_AUX_USER_REPLY also fails, the negative return is right-shifted and truncated to 0xFF, which avoids error assignment and returns success. This eventually leads to it6505_aux_i2c_readb() reading garbage data into the output buffer: drivers/gpu/drm/bridge/ite-it6505.c:it6505_aux_i2c_readb() { ... buf[i] = it6505_read(it6505, REG_AUX_USER_RXB(0 + i)); ... } Could this silently propagate hardware error codes to the DRM layer as valid data? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
