Re: [PATCH] drm/bridge: tda998x: Handle short CEC register reads

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dead code in `cec_enamods()` error check allows overwriting the register configuration when `cec_read()` fails.
--

--- Patch [1]: [PATCH] drm/bridge: tda998x: Handle short CEC register reads ---
commit 2bfd5ce7155e5841764b8ec506e16365cd7190b6
Author: Ruoyu Wang <[email protected]>

drm/bridge: tda998x: Handle short CEC register reads

This commit updates the error handling in cec_read() to ensure both messages
of the I2C transfer complete successfully before using the returned byte. If
the transfer is short or fails, it preserves the 0 fallback value to avoid
returning uninitialized stack memory.

> diff --git a/drivers/gpu/drm/bridge/tda998x_drv.c b/drivers/gpu/drm/bridge/tda998x_drv.c
> index 0b966c9844e70..fb414584962b7 100644
> --- a/drivers/gpu/drm/bridge/tda998x_drv.c
> +++ b/drivers/gpu/drm/bridge/tda998x_drv.c
> @@ -438,7 +438,7 @@ cec_read(struct tda998x_priv *priv, u8 addr)
>  	int ret;
>  
>  	ret = i2c_transfer(priv->hdmi->adapter, msg, ARRAY_SIZE(msg));
> -	if (ret < 0) {
> +	if (ret != ARRAY_SIZE(msg)) {
>  		dev_err(&priv->hdmi->dev, "Error %d reading from cec:0x%x\n",
>  			ret, addr);
>  		val = 0;
>  	}
>  
>  	return val;

[Severity: High]
This is a pre-existing issue, but since cec_read() returns a u8, does the
error handling in callers like cec_enamods() work correctly?

For example, in drivers/gpu/drm/bridge/tda998x_drv.c:cec_enamods():

	int val = cec_read(priv, REG_CEC_ENAMODS);

	if (val < 0)
		return;

	if (enable)
		val |= mods;
	else
		val &= ~mods;

	cec_write(priv, REG_CEC_ENAMODS, val);

Because cec_read() returns an unsigned 8-bit integer, val will be
zero-extended and can never be negative. If a read fails due to an I2C error,
it returns 0, which bypasses the val < 0 check.

Could this result in writing back a modified 0 to REG_CEC_ENAMODS and
wiping out other enable bits, potentially causing the CEC module or clocks
to be disabled unexpectedly until a full reset occurs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1