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

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

[Lee Jones]

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@vger.kernel.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/i2c/busses/i2c-cgbc.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
index 25a74fa51aa0..9909c25f0b18 100644
--- a/drivers/i2c/busses/i2c-cgbc.c
+++ b/drivers/i2c/busses/i2c-cgbc.c
@@ -205,9 +205,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 = algo_data->bus_id,
@@ -266,17 +266,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 * algo_data->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;
+		}
+
 		cmd_data = CGBC_I2C_CMD_DATA | algo_data->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)) {
-- 
2.56.0.218.g1ec904b173-goog

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@vger.kernel.org
> Signed-off-by: Lee Jones <lee@kernel.org>
>
> 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@bootlin.com>
Tested-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>

Best Regards,
Thomas

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

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@vger.kernel.org
> > Signed-off-by: Lee Jones <lee@kernel.org>
> >
> > 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@bootlin.com>
> Tested-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>

Thanks for these.  Much appreciated.

-- 
Lee Jones