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

[Lee Jones]

On Fri, 23 Oct 2026, Thomas Richard (congatec GmbH) wrote:

> > 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;

Ternary operators certainly do not make the code _more_ readable.

Concise, yes.  Readable, no.  The current code is preferable.

> }
> 
> >  
> >  		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>

Thanks for these.  Much appreciated.

-- 
Lee Jones