Re: [PATCH v6 11/13] drm/bridge: it6505: don't write an error code back to the reset register

[sashiko-bot@kernel.org]

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 <daniel@makrotopia.org>

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/cover.1784947240.git.daniel@makrotopia.org?part=11