Re: [PATCH 27/28] i2c: cgbc: Propagate hardware errors when polling read status

[Thomas Richard (congatec GmbH)]

> In cgbc_i2c_xfer_msg(), read_poll_timeout() uses the same ret variable
> to store the polled return value of cgbc_i2c_get_status() and the
> timeout result of the macro itself.  When cgbc_i2c_get_status() returns
> a negative error code (such as -EIO on an I2C NACK), the loop condition
> ret != CGBC_I2C_STAT_BUSY evaluates to true and read_poll_timeout()
> overwrites ret with 0.  The driver then issues CGBC_I2C_CMD_DATA, which
> completes with CGBC_STATUS_CMD_READY without populating msg->buf,
> leaving uninitialised heap memory in the caller's read buffer.
> 
> Store the polled status in a separate variable, verify that it equals
> CGBC_I2C_STAT_DAT, and check that CGBC_I2C_CMD_DATA returns the expected
> number of bytes.
> 
> Fixes: 6894f640b8f3 ("i2c: Congatec Board Controller i2c bus driver")
> Closes: https://sashiko.dev/#/bug/linux-66cbf4b1-a219-4012-969d-236417bf419a
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
>
> diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
> index 682ba639d887..3226838cca46 100644
> --- a/drivers/i2c/busses/i2c-cgbc.c
> +++ b/drivers/i2c/busses/i2c-cgbc.c
> @@ -198,9 +198,9 @@ static int cgbc_i2c_xfer_msg(struct i2c_adapter *adap)
>  	struct cgbc_device_data *cgbc = i2c->cgbc;
>  	struct i2c_msg *msg = i2c->msg;
>  	u8 cmd[CGBC_I2C_CMD_SIZE];
> -	int ret, max_len, len, i;
> +	int ret, status, max_len, len, i;
>  	unsigned int cmd_len;
> -	u8 cmd_data;
> +	u8 cmd_data, data_len;
>  
>  	struct cgbc_i2c_transfer xfer = {
>  		.bus_id = i2c->bus_id,
> @@ -259,17 +259,32 @@ static int cgbc_i2c_xfer_msg(struct i2c_adapter *adap)
>  		if (ret)
>  			goto err;
>  
> -		ret = read_poll_timeout(cgbc_i2c_get_status, ret,
> -					ret != CGBC_I2C_STAT_BUSY, 0,
> +		ret = read_poll_timeout(cgbc_i2c_get_status, status,
> +					status != CGBC_I2C_STAT_BUSY, 0,
>  					2 * i2c->read_maxtime_us, false, adap);
> -		if (ret < 0)
> +		if (ret)
> +			goto err;
> +
> +		if (status < 0) {
> +			ret = status;
>  			goto err;
> +		}
> +
> +		if (status != CGBC_I2C_STAT_DAT) {
> +			ret = -EIO;
> +			goto err;
> +		}

We could save a goto and make code more readable IMO

if (ret)
	goto err;

if (status != CGBC_I2C_STAT_DAT) {
	ret = (status < 0) ? status : -EIO;
	goto err;
}

>  
>  		cmd_data = CGBC_I2C_CMD_DATA | i2c->bus_id;
>  		ret = cgbc_command(cgbc, &cmd_data, sizeof(cmd_data),
> -				   msg->buf + i2c->pos, len, NULL);
> +				   msg->buf + i2c->pos, len, &data_len);
>  		if (ret)
>  			goto err;
> +
> +		if (data_len != len) {
> +			ret = -EIO;
> +			goto err;
> +		}
>  	}
>  
>  	if (len == (msg->len - i2c->pos)) {

Otherwise looks good to me.

Reviewed-by: Thomas Richard (congatec GmbH) <thomas.richard@xxxxxxxxxxx>
Tested-by: Thomas Richard (congatec GmbH) <thomas.richard@xxxxxxxxxxx>

Best Regards,
Thomas

-- 
Thomas Richard (congatec GmbH) <thomas.richard@xxxxxxxxxxx>