Re: [PATCH 2/4] i2c: cgbc: Add virtual I2C bus support

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

Pre-existing issues:
- [High] Uninitialized stack variable `data` used as a denominator, leading=
 to a potential division by zero panic.
--

--- Patch [2]: [PATCH 2/4] i2c: cgbc: Add virtual I2C bus support ---
commit 68db5c3b940f7f50540127b0266b86e6b3d69b22
Author: Thomas Richard <[email protected]>

i2c: cgbc: Add virtual I2C bus support

Internally the Board Controller has a virtual I2C bus with some virtual
storage devices on it. Add support for it by defining a i2c_adapter entry.
This virtual bus has a fixed frequency that cannot be changed.

> diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
> index 25a74fa51aa00..c3f1c83b105a8 100644
> --- a/drivers/i2c/busses/i2c-cgbc.c
> +++ b/drivers/i2c/busses/i2c-cgbc.c

[ ... ]

> @@ -147,11 +148,14 @@ static int cgbc_i2c_set_frequency(struct i2c_adapte=
r *adap,
>  	cmd[0] =3D CGBC_I2C_CMD_SPEED | algo_data->bus_id;
>  	cmd[1] =3D cgbc_i2c_freq_to_reg(bus_frequency);
> =20
> -	ret =3D cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
> -	if (ret)
> -		return dev_err_probe(i2c->dev, ret,
> -				     "Failed to initialize I2C bus %s",
> -				     adap->name);
> +	/* Virtual bus has a fixed frequency */
> +	if (algo_data->bus_id !=3D CGBC_I2C_VIRTUAL_BUS_ID) {
> +		ret =3D cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
> +		if (ret)
> +			return dev_err_probe(i2c->dev, ret,
> +					     "Failed to initialize I2C bus %s",
> +					     adap->name);
> +	}
> =20
>  	cmd[1] =3D 0x00;

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
cgbc_i2c_set_frequency() handles the data variable.

Could this lead to a division by zero panic? The data variable is allocated=
 on
the stack uninitialized:

drivers/i2c/busses/i2c-cgbc.c:cgbc_i2c_set_frequency() {
    ...
    u8 cmd[2], data;
    ...
}

When passed to cgbc_command(), if the board controller firmware returns a
successful status but provides a zero byte count, cgbc_command() skips read=
ing
into data and returns 0:

drivers/mfd/cgbc-core.c:cgbc_command() {
    ...
    case CGBC_STATUS_DATA_READY:
        if (istatus > data_size)
            istatus =3D data_size;
        for (i =3D 0; i < istatus; i++) {
            _data[i] =3D ioread8(cgbc->io_cmd + CGBC_CMD_DATA + ((i + 1) % =
4));
    ...
}

If istatus is 0, the loop is skipped and data remains untouched.

When execution continues in cgbc_i2c_set_frequency(), this uninitialized
value is converted to a frequency:

drivers/i2c/busses/i2c-cgbc.c:cgbc_i2c_set_frequency() {
    ...
    ret =3D cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
    if (ret)
        return dev_err_probe(...);

    bus_frequency =3D cgbc_i2c_reg_to_freq(data);
    ...
    algo_data->read_maxtime_us =3D (BITS_PER_BYTE + 1) * CGBC_I2C_READ_MAX_=
LEN
        * USEC_PER_SEC / bus_frequency;
    ...
}

If the uninitialized stack memory evaluates to 0, or if the hardware
explicitly returns a frequency of 0, would this result in a division by zero
during the read_maxtime_us calculation?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-cgbc-i2c-s=
[email protected]?part=3D2