[PATCH 0/4] Congatec Board Controller: Add storage devices support

[PATCH 0/4] Congatec Board Controller: Add storage devices support

[Thomas Richard (congatec GmbH)]

The Congatec Board Controller has an internal I2C bus with several storage
devices (EEPROM and RAM). The bus and devices are virtual but act as real
hardware. This series adds support for both the bus and devices.

List of devices:
- Secure Data EEPROM  (64 bytes, RO): static and dynamic board info
- BIOS EEPROM         (32 bytes, RW): reserved for BIOS applications
- BC EEPROM           (32 bytes, RO): Board Controller operational params
- User EEPROM         (32 bytes, RW): user applications
- BIOS RAM            (32 bytes, RW): reserved for BIOS applications
- BC RAM               (8 bytes, RO): Board Controller operational params
- User RAM            (16 bytes, RW): user applications

The i2c-cgbc driver instantiates these devices. A dedicated driver for
these devices was not necessary since the at24 driver can handle them.
The 24c01 entry (the most generic one) is used, and parameters (size,
read-only flag...) are customized via the software_node properties.

I also took this opportunity to fix an issue reported by Sashiko in the MFD
driver (PATCH 1).

Signed-off-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
---
Thomas Richard (congatec GmbH) (4):
      mfd: cgbc: Fix use of negative error code as valid session handle
      i2c: cgbc: Add virtual I2C bus support
      mfd: cgbc: Add virtual I2C bus support
      i2c: cgbc: Add virtual storage devices on the virtual I2C bus

 drivers/i2c/busses/i2c-cgbc.c | 174 ++++++++++++++++++++++++++++++++++++++++--
 drivers/mfd/cgbc-core.c       |   7 +-
 2 files changed, 174 insertions(+), 7 deletions(-)
---
base-commit: 82442496954504a901faf74105a5ab592d782288
change-id: 20260717-cgbc-i2c-storage-devices-support-b3ee7cea3320

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

[PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle

[Thomas Richard (congatec GmbH)]

The cgbc_session_command() return value was directly cast to u8 and used
as session handle without error checking. Casting a negative error to u8
produces a valid-looking session handle. So check if return value is
positive before to cast and use it.

Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260713-cgbc-core-fix-cgbc-remove-v1-1-79274ad62b3a%40bootlin.com?part=1
Fixes: 6f1067cfbee7 ("mfd: Add Congatec Board Controller driver")
Signed-off-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
---
 drivers/mfd/cgbc-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
index 2becaf797646..4a409234e66c 100644
--- a/drivers/mfd/cgbc-core.c
+++ b/drivers/mfd/cgbc-core.c
@@ -103,7 +103,11 @@ static int cgbc_session_request(struct cgbc_device_data *cgbc)
 	if (ret)
 		return dev_err_probe(cgbc->dev, ret, "device not found or not ready\n");
 
-	cgbc->session = cgbc_session_command(cgbc, CGBC_SESSION_CMD_REQUEST);
+	ret = cgbc_session_command(cgbc, CGBC_SESSION_CMD_REQUEST);
+	if (ret < 0)
+		return dev_err_probe(cgbc->dev, ret, "session handle request timed out\n");
+
+	cgbc->session = ret;
 
 	/* The Board Controller sent us a wrong session handle, we cannot communicate with it */
 	if (cgbc->session < CGBC_SESSION_VALID_MIN || cgbc->session > CGBC_SESSION_VALID_MAX)

-- 
2.53.0

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

[Thomas Richard (congatec GmbH)]

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.

Signed-off-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
---
 drivers/i2c/busses/i2c-cgbc.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
index 25a74fa51aa0..c3f1c83b105a 100644
--- a/drivers/i2c/busses/i2c-cgbc.c
+++ b/drivers/i2c/busses/i2c-cgbc.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 
 #define CGBC_I2C_PRIMARY_BUS_ID	0
+#define CGBC_I2C_VIRTUAL_BUS_ID	3
 #define CGBC_I2C_PM_BUS_ID	4
 
 #define CGBC_I2C_CMD_START	0x40
@@ -147,11 +148,14 @@ static int cgbc_i2c_set_frequency(struct i2c_adapter *adap,
 	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
 	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
 
-	ret = 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 != CGBC_I2C_VIRTUAL_BUS_ID) {
+		ret = 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);
+	}
 
 	cmd[1] = 0x00;
 
@@ -338,6 +342,7 @@ static const struct i2c_algorithm cgbc_i2c_algorithm = {
 static struct i2c_algo_cgbc_data cgbc_i2c_algo_data[] = {
 	{ .bus_id = CGBC_I2C_PRIMARY_BUS_ID },
 	{ .bus_id = CGBC_I2C_PM_BUS_ID },
+	{ .bus_id = CGBC_I2C_VIRTUAL_BUS_ID },
 };
 
 static const struct i2c_adapter cgbc_i2c_adapter[] = {
@@ -357,6 +362,14 @@ static const struct i2c_adapter cgbc_i2c_adapter[] = {
 		.algo_data	= &cgbc_i2c_algo_data[1],
 		.nr		= -1,
 	},
+	{
+		.owner          = THIS_MODULE,
+		.name           = "Congatec Virtual I2C adapter",
+		.class          = I2C_CLASS_DEPRECATED,
+		.algo           = &cgbc_i2c_algorithm,
+		.algo_data      = &cgbc_i2c_algo_data[2],
+		.nr             = -1,
+	},
 };
 
 static int cgbc_i2c_probe(struct platform_device *pdev)

-- 
2.53.0

[PATCH 3/4] mfd: cgbc: Add virtual I2C bus support

[Thomas Richard (congatec GmbH)]

Internally the Board Controller has a virtual I2C bus with some virtual
storage devices (EEPROM and RAM). So add a new I2C cell for this virtual
bus.

Signed-off-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
---
 drivers/mfd/cgbc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
index 4a409234e66c..81bd0a381ca1 100644
--- a/drivers/mfd/cgbc-core.c
+++ b/drivers/mfd/cgbc-core.c
@@ -240,6 +240,7 @@ static struct mfd_cell cgbc_devs[] = {
 	{ .name = "cgbc-gpio"	},
 	{ .name = "cgbc-i2c", .id = 1 },
 	{ .name = "cgbc-i2c", .id = 2 },
+	{ .name = "cgbc-i2c", .id = 3 },
 	{ .name = "cgbc-hwmon"	},
 	{ .name = "cgbc-backlight" },
 };

-- 
2.53.0

[PATCH 4/4] i2c: cgbc: Add virtual storage devices on the virtual I2C bus

[Thomas Richard (congatec GmbH)]

Add support for virtual storage devices exposed by the Board Controller
on its virtual I2C bus. There are two device types: EEPROM for persistent
storage and RAM for non-persistent storage:
- Secure Data EEPROM  (64 bytes, RO): static and dynamic board info
- BIOS EEPROM         (32 bytes, RW): reserved for BIOS applications
- BC EEPROM           (32 bytes, RO): Board Controller operational params
- User EEPROM         (32 bytes, RW): user applications
- BIOS RAM            (32 bytes, RW): reserved for BIOS applications
- BC RAM               (8 bytes, RO): Board Controller operational params
- User RAM            (16 bytes, RW): user applications

Use the at24 driver with the 24c01 entry (the most generic one) for all
virtual storage devices, customizing parameters via software_node
properties (size, pagesize, read-only flag, label).

Signed-off-by: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
---
 drivers/i2c/busses/i2c-cgbc.c | 151 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 150 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
index c3f1c83b105a..418de90e11bb 100644
--- a/drivers/i2c/busses/i2c-cgbc.c
+++ b/drivers/i2c/busses/i2c-cgbc.c
@@ -372,6 +372,149 @@ static const struct i2c_adapter cgbc_i2c_adapter[] = {
 	},
 };
 
+static const struct property_entry cgbc_secure_data_eeprom_props[] = {
+	PROPERTY_ENTRY_U32("size", 64),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_BOOL("read-only"),
+	PROPERTY_ENTRY_STRING("label", "cgbc-secure-data-eeprom"),
+	{ }
+};
+
+static const struct software_node cgbc_secure_data_eeprom_node = {
+	.properties = cgbc_secure_data_eeprom_props,
+};
+
+static const struct property_entry cgbc_bc_eeprom_props[] = {
+	PROPERTY_ENTRY_U32("size", 32),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_BOOL("read-only"),
+	PROPERTY_ENTRY_STRING("label", "cgbc-bc-eeprom"),
+	{ }
+};
+
+static const struct software_node cgbc_bc_eeprom_node = {
+	.properties = cgbc_bc_eeprom_props,
+};
+
+static const struct property_entry cgbc_user_eeprom_props[] = {
+	PROPERTY_ENTRY_U32("size", 32),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_STRING("label", "cgbc-user-eeprom"),
+	{ }
+};
+
+static const struct software_node cgbc_user_eeprom_node = {
+	.properties = cgbc_user_eeprom_props,
+};
+
+static const struct property_entry cgbc_bios_eeprom_props[] = {
+	PROPERTY_ENTRY_U32("size", 32),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_STRING("label", "cgbc-bios-eprom"),
+	{ }
+};
+
+static const struct software_node cgbc_bios_eeprom_node = {
+	.properties = cgbc_bios_eeprom_props,
+};
+
+static const struct property_entry cgbc_bc_ram_props[] = {
+	PROPERTY_ENTRY_U32("size", 8),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_BOOL("read-only"),
+	PROPERTY_ENTRY_STRING("label", "cgbc-bc-ram"),
+	{ }
+};
+
+static const struct software_node cgbc_bc_ram_node = {
+	.properties = cgbc_bc_ram_props,
+};
+
+static const struct property_entry cgbc_user_ram_props[] = {
+	PROPERTY_ENTRY_U32("size", 16),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_STRING("label", "cgbc-user-ram"),
+	{ }
+};
+
+static const struct software_node cgbc_user_ram_node = {
+	.properties = cgbc_user_ram_props,
+};
+
+static const struct property_entry cgbc_bios_ram_props[] = {
+	PROPERTY_ENTRY_U32("size", 32),
+	PROPERTY_ENTRY_U32("pagesize", 1),
+	PROPERTY_ENTRY_STRING("label", "cgbc-bios-ram"),
+	{ }
+};
+
+static const struct software_node cgbc_bios_ram_node = {
+	.properties = cgbc_bios_ram_props,
+};
+
+static const struct i2c_board_info cgbc_i2c_board_info[] = {
+	{
+		.type = "24c01",
+		.addr = 0x40,
+		.dev_name = "cgbc-secure-data-eeprom",
+		.swnode = &cgbc_secure_data_eeprom_node,
+	},
+	{
+		.type = "24c01",
+		.addr = 0x48,
+		.dev_name = "cgbc-bc-eeprom",
+		.swnode = &cgbc_bc_eeprom_node,
+	},
+	{
+		.type = "24c01",
+		.addr = 0x50,
+		.dev_name = "cgbc-user-eeprom",
+		.swnode = &cgbc_user_eeprom_node,
+	},
+	{
+		.type = "24c01",
+		.addr = 0x58,
+		.dev_name = "cgbc-bios-eeprom",
+		.swnode = &cgbc_bios_eeprom_node,
+	},
+	{
+		.type = "24c01",
+		.addr = 0x60,
+		.dev_name = "cgbc-bc-ram",
+		.swnode = &cgbc_bc_ram_node,
+	},
+	{
+		.type = "24c01",
+		.addr = 0x68,
+		.dev_name = "cgbc-user-ram",
+		.swnode = &cgbc_user_ram_node,
+	},
+	{
+		.type = "24c01",
+		.addr = 0x70,
+		.dev_name = "cgbc-bios-ram",
+		.swnode = &cgbc_bios_ram_node,
+	}
+};
+
+static void cgbc_i2c_instantiate_devices(struct i2c_adapter *adap)
+{
+	struct i2c_algo_cgbc_data *algo_data = adap->algo_data;
+	struct i2c_client *client;
+	int i;
+
+	/* We only instantiate devices on Virtual bus */
+	if (algo_data->bus_id != CGBC_I2C_VIRTUAL_BUS_ID)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(cgbc_i2c_board_info); i++) {
+		client  = i2c_new_client_device(adap, &cgbc_i2c_board_info[i]);
+		if (IS_ERR(client))
+			dev_err(&adap->dev, "Failed to register %s\n",
+				cgbc_i2c_board_info[i].dev_name);
+	}
+}
+
 static int cgbc_i2c_probe(struct platform_device *pdev)
 {
 	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
@@ -393,7 +536,13 @@ static int cgbc_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return i2c_add_numbered_adapter(&i2c->adap);
+	ret = i2c_add_numbered_adapter(&i2c->adap);
+	if (ret)
+		return ret;
+
+	cgbc_i2c_instantiate_devices(&i2c->adap);
+
+	return ret;
 }
 
 static void cgbc_i2c_remove(struct platform_device *pdev)

-- 
2.53.0

Re: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardcoded `.id` values are used in the `mfd_cell` array `cgbc_devs` instead of relying on `PLATFORM_DEVID_AUTO`.
- [High] Uninitialized kernel stack memory can be leaked to userspace via sysfs if the board controller firmware returns fewer than 3 bytes of data.
- [Medium] The `mfd_cell` array `cgbc_devs` is missing the `const` qualifier.
--

--- Patch [1]: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle ---
commit d37d718d69c4fd9448146a18eac17c7d57dcefc2
Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>

mfd: cgbc: Fix use of negative error code as valid session handle

The cgbc_session_command() return value was directly cast to u8 and used
as session handle without error checking. Casting a negative error to u8
produces a valid-looking session handle. So check if return value is
positive before to cast and use it.

Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260713-cgbc-core-fix-cgbc-remove-v1-1-79274ad62b3a%40bootlin.com?part=1
Fixes: 6f1067cfbee7 ("mfd: Add Congatec Board Controller driver")

> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> index 2becaf797646d..4a409234e66c8 100644
> --- a/drivers/mfd/cgbc-core.c
> +++ b/drivers/mfd/cgbc-core.c

[ ... ]

>  static struct mfd_cell cgbc_devs[] = {
>  	{ .name = "cgbc-wdt"	},

[Severity: Medium]
This is a pre-existing issue, but should cgbc_devs be declared with the
const qualifier?

The MFD subsystem guidelines require mfd_cell arrays to be static const
so they are placed in .rodata rather than mutable memory.

[ ... ]

>  	{ .name = "cgbc-i2c", .id = 1 },
>  	{ .name = "cgbc-i2c", .id = 2 },

[Severity: High]
This is a pre-existing issue, but could this cause device naming collisions
in sysfs?

The cells use hardcoded .id values instead of allowing the MFD core to handle
auto-indexing. The subsystem guidelines recommend using PLATFORM_DEVID_AUTO
to prevent driver load failures.

[ ... ]

>  static int cgbc_get_version(struct cgbc_device_data *cgbc)
>  {
>  	...
> 	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
> 	if (ret)
> 		return ret;
> 
> 	cgbc->version.feature = data[0];
> 	cgbc->version.major = data[1];
> 	cgbc->version.minor = data[2];

[Severity: High]
This is a pre-existing issue, but can this leak uninitialized stack memory
to userspace?

If the board controller hardware returns fewer than 3 bytes during the
CGBC_CMD_GET_FW_REV command, cgbc_command() might still successfully verify
the truncated response and return 0.

The code then unconditionally reads data[0], data[1], and data[2], storing
potentially uninitialized stack bytes into cgbc->version, which is later
exposed via the cgbc_version sysfs attribute.

[ ... ]

>  static int cgbc_init_device(struct cgbc_device_data *cgbc)
>  {
>  	...
> 	ret = mfd_add_devices(cgbc->dev, -1, cgbc_devs, ARRAY_SIZE(cgbc_devs),
> 			      NULL, 0, NULL);

[Severity: High]
This is a pre-existing issue, but should this pass PLATFORM_DEVID_AUTO
instead of -1 (PLATFORM_DEVID_NONE)?

Using PLATFORM_DEVID_NONE combined with hardcoded .id values in the mfd_cell
array violates MFD guidelines.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-cgbc-i2c-storage-devices-support-v1-0-fed38510671c@bootlin.com?part=1

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

[sashiko-bot]

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 <thomas.richard@bootlin.com>

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_adapter *adap,
>  	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
>  	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
>  
> -	ret = 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 != CGBC_I2C_VIRTUAL_BUS_ID) {
> +		ret = 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);
> +	}
>  
>  	cmd[1] = 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 reading
into data and returns 0:

drivers/mfd/cgbc-core.c:cgbc_command() {
    ...
    case CGBC_STATUS_DATA_READY:
        if (istatus > data_size)
            istatus = data_size;
        for (i = 0; i < istatus; i++) {
            _data[i] = 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 = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
    if (ret)
        return dev_err_probe(...);

    bus_frequency = cgbc_i2c_reg_to_freq(data);
    ...
    algo_data->read_maxtime_us = (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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-cgbc-i2c-storage-devices-support-v1-0-fed38510671c@bootlin.com?part=2

Re: [PATCH 3/4] mfd: cgbc: Add virtual I2C bus support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Hard-coded cell IDs are used for the newly added `cgbc-i2c` subdevice instead of `PLATFORM_DEVID_AUTO`.
--

--- Patch [3]: [PATCH 3/4] mfd: cgbc: Add virtual I2C bus support ---
commit 4f0038d5dc42bfd45540675cdbf89f68ab3673d5
Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>

mfd: cgbc: Add virtual I2C bus support

This commit adds a new I2C cell to support the internal virtual I2C bus 
provided by the Board Controller.

> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> index 4a409234e66c8..81bd0a381ca13 100644
> --- a/drivers/mfd/cgbc-core.c
> +++ b/drivers/mfd/cgbc-core.c
> @@ -240,6 +240,7 @@ static struct mfd_cell cgbc_devs[] = {
>  	{ .name = "cgbc-gpio"	},
>  	{ .name = "cgbc-i2c", .id = 1 },
>  	{ .name = "cgbc-i2c", .id = 2 },
> +	{ .name = "cgbc-i2c", .id = 3 },

[Severity: Low]
Should this use PLATFORM_DEVID_AUTO instead of hard-coding the cell ID?

>  	{ .name = "cgbc-hwmon"	},
>  	{ .name = "cgbc-backlight" },
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-cgbc-i2c-storage-devices-support-v1-0-fed38510671c@bootlin.com?part=3

Re: [PATCH 4/4] i2c: cgbc: Add virtual storage devices on the virtual I2C bus

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Typo in the software node label property for the BIOS EEPROM ('eprom' instead of 'eeprom').
--

--- Patch [4]: [PATCH 4/4] i2c: cgbc: Add virtual storage devices on the virtual I2C bus ---
commit 98b86180cbc7d34ae655eafe5dcbc85e15d4bc49
Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>

i2c: cgbc: Add virtual storage devices on the virtual I2C bus

This commit adds support for virtual storage devices exposed by the Board
Controller on its virtual I2C bus. It uses the generic 24c01 at24 driver
for all virtual storage devices, customizing their parameters via
software_node properties.

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

[ ... ]

> @@ -372,6 +372,149 @@ static const struct i2c_adapter cgbc_i2c_adapter[] = {

[ ... ]

> +static const struct property_entry cgbc_bios_eeprom_props[] = {
> +	PROPERTY_ENTRY_U32("size", 32),
> +	PROPERTY_ENTRY_U32("pagesize", 1),
> +	PROPERTY_ENTRY_STRING("label", "cgbc-bios-eprom"),

[Severity: Medium]
Does this label contain a typo? The other devices use the spelling "eeprom",
but this one uses "eprom". 

Since this label is exposed to userspace via sysfs (as an nvmem device),
could this naming inconsistency break udev rules or user scripts relying on
a standard "eeprom" naming scheme for these storage devices?

> +	{ }
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-cgbc-i2c-storage-devices-support-v1-0-fed38510671c@bootlin.com?part=4

Re: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle

[Thomas Richard]

On 8/4/26 5:29 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Hardcoded `.id` values are used in the `mfd_cell` array `cgbc_devs` instead of relying on `PLATFORM_DEVID_AUTO`.
> - [High] Uninitialized kernel stack memory can be leaked to userspace via sysfs if the board controller firmware returns fewer than 3 bytes of data.
> - [Medium] The `mfd_cell` array `cgbc_devs` is missing the `const` qualifier.
> --
> 
> --- Patch [1]: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle ---
> commit d37d718d69c4fd9448146a18eac17c7d57dcefc2
> Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
> 
> mfd: cgbc: Fix use of negative error code as valid session handle
> 
> The cgbc_session_command() return value was directly cast to u8 and used
> as session handle without error checking. Casting a negative error to u8
> produces a valid-looking session handle. So check if return value is
> positive before to cast and use it.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260713-cgbc-core-fix-cgbc-remove-v1-1-79274ad62b3a%40bootlin.com?part=1
> Fixes: 6f1067cfbee7 ("mfd: Add Congatec Board Controller driver")
> 
>> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
>> index 2becaf797646d..4a409234e66c8 100644
>> --- a/drivers/mfd/cgbc-core.c
>> +++ b/drivers/mfd/cgbc-core.c
> 
> [ ... ]
> 
>>  static struct mfd_cell cgbc_devs[] = {
>>  	{ .name = "cgbc-wdt"	},
> 
> [Severity: Medium]
> This is a pre-existing issue, but should cgbc_devs be declared with the
> const qualifier?
> 
> The MFD subsystem guidelines require mfd_cell arrays to be static const
> so they are placed in .rodata rather than mutable memory.

yes indeed. Not a big deal, I can add a patch to fix this in v2.

> 
> [ ... ]
> 
>>  	{ .name = "cgbc-i2c", .id = 1 },
>>  	{ .name = "cgbc-i2c", .id = 2 },
> 
> [Severity: High]
> This is a pre-existing issue, but could this cause device naming collisions
> in sysfs?
> 
> The cells use hardcoded .id values instead of allowing the MFD core to handle
> auto-indexing. The subsystem guidelines recommend using PLATFORM_DEVID_AUTO
> to prevent driver load failures.

PLATFORM_DEVID_NONE (-1) is used and id starts from 1.
The first I2C bus receives a platform_id of 0 (cgbc-i2c.0, cgbc-i2c.1),
so no collision. While this design is maybe not ideal, but it works for
now. Also the plan is to add support for additional boards and I2C
busses in the future. And since some boards won't support all I2C
busses, so this scheme will probably be reworked.

> 
> [ ... ]
> 
>>  static int cgbc_get_version(struct cgbc_device_data *cgbc)
>>  {
>>  	...
>> 	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
>> 	if (ret)
>> 		return ret;
>>
>> 	cgbc->version.feature = data[0];
>> 	cgbc->version.major = data[1];
>> 	cgbc->version.minor = data[2];
> 
> [Severity: High]
> This is a pre-existing issue, but can this leak uninitialized stack memory
> to userspace?
> 
> If the board controller hardware returns fewer than 3 bytes during the
> CGBC_CMD_GET_FW_REV command, cgbc_command() might still successfully verify
> the truncated response and return 0.
> 
> The code then unconditionally reads data[0], data[1], and data[2], storing
> potentially uninitialized stack bytes into cgbc->version, which is later
> exposed via the cgbc_version sysfs attribute.

No. If CGBC_CMD_GET_FW_REV command succeeds (and we check cgbc_command's
return code), it returns exactly 4 bytes. So there is no way to get
uninitialized values in data buffer.

Also to prevent buffer overflow, cgbc_command verifies that the buffer
is large enough to hold all received data.

> 
> [ ... ]
> 
>>  static int cgbc_init_device(struct cgbc_device_data *cgbc)
>>  {
>>  	...
>> 	ret = mfd_add_devices(cgbc->dev, -1, cgbc_devs, ARRAY_SIZE(cgbc_devs),
>> 			      NULL, 0, NULL);
> 
> [Severity: High]
> This is a pre-existing issue, but should this pass PLATFORM_DEVID_AUTO
> instead of -1 (PLATFORM_DEVID_NONE)?
> 
> Using PLATFORM_DEVID_NONE combined with hardcoded .id values in the mfd_cell
> array violates MFD guidelines.

Ditto (see above)

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Thomas Richard]

On 8/4/26 5:43 PM, sashiko-bot@kernel.org wrote:
> 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 <thomas.richard@bootlin.com>
> 
> 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_adapter *adap,
>>  	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
>>  	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
>>  
>> -	ret = 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 != CGBC_I2C_VIRTUAL_BUS_ID) {
>> +		ret = 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);
>> +	}
>>  
>>  	cmd[1] = 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 reading
> into data and returns 0:
> 
> drivers/mfd/cgbc-core.c:cgbc_command() {
>     ...
>     case CGBC_STATUS_DATA_READY:
>         if (istatus > data_size)
>             istatus = data_size;
>         for (i = 0; i < istatus; i++) {
>             _data[i] = 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 = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
>     if (ret)
>         return dev_err_probe(...);
> 
>     bus_frequency = cgbc_i2c_reg_to_freq(data);
>     ...
>     algo_data->read_maxtime_us = (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?

The answer is still no. If CGBC_I2C_CMD_SPEED succeeds (and we check
cgbc_command's return code), it returns exactly 1 byte. We provided a
data buffer with size 1 to cgbc_command, so there is no way for
uninitialized stack memory to be read if the command succeeded.

To prevent buffer overflow, cgbc_command verifies that the buffer is
large enough to hold all received data.

Best Regards,
Thomas

Re: [PATCH 4/4] i2c: cgbc: Add virtual storage devices on the virtual I2C bus

[Thomas Richard]

On 8/4/26 5:58 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Typo in the software node label property for the BIOS EEPROM ('eprom' instead of 'eeprom').
> --
> 
> --- Patch [4]: [PATCH 4/4] i2c: cgbc: Add virtual storage devices on the virtual I2C bus ---
> commit 98b86180cbc7d34ae655eafe5dcbc85e15d4bc49
> Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
> 
> i2c: cgbc: Add virtual storage devices on the virtual I2C bus
> 
> This commit adds support for virtual storage devices exposed by the Board
> Controller on its virtual I2C bus. It uses the generic 24c01 at24 driver
> for all virtual storage devices, customizing their parameters via
> software_node properties.
> 
>> diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
>> index c3f1c83b105a8..418de90e11bb8 100644
>> --- a/drivers/i2c/busses/i2c-cgbc.c
>> +++ b/drivers/i2c/busses/i2c-cgbc.c
> 
> [ ... ]
> 
>> @@ -372,6 +372,149 @@ static const struct i2c_adapter cgbc_i2c_adapter[] = {
> 
> [ ... ]
> 
>> +static const struct property_entry cgbc_bios_eeprom_props[] = {
>> +	PROPERTY_ENTRY_U32("size", 32),
>> +	PROPERTY_ENTRY_U32("pagesize", 1),
>> +	PROPERTY_ENTRY_STRING("label", "cgbc-bios-eprom"),
> 
> [Severity: Medium]
> Does this label contain a typo? The other devices use the spelling "eeprom",
> but this one uses "eprom". 
> 
> Since this label is exposed to userspace via sysfs (as an nvmem device),
> could this naming inconsistency break udev rules or user scripts relying on
> a standard "eeprom" naming scheme for these storage devices?
Yes indeed. I missed it because I used i2c device to sysfs entry to test
(/sys/bus/i2c/devices/i2c-cgbc-bios-eeprom/eeprom). And this typo
impacts only nvmem device.

Will fix it in v2.

Best Regards,
Thomas

Re: [PATCH 3/4] mfd: cgbc: Add virtual I2C bus support

[Lee Jones]

On Tue, 04 Aug 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Hard-coded cell IDs are used for the newly added `cgbc-i2c` subdevice instead of `PLATFORM_DEVID_AUTO`.
> --
> 
> --- Patch [3]: [PATCH 3/4] mfd: cgbc: Add virtual I2C bus support ---
> commit 4f0038d5dc42bfd45540675cdbf89f68ab3673d5
> Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
> 
> mfd: cgbc: Add virtual I2C bus support
> 
> This commit adds a new I2C cell to support the internal virtual I2C bus 
> provided by the Board Controller.
> 
> > diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> > index 4a409234e66c8..81bd0a381ca13 100644
> > --- a/drivers/mfd/cgbc-core.c
> > +++ b/drivers/mfd/cgbc-core.c
> > @@ -240,6 +240,7 @@ static struct mfd_cell cgbc_devs[] = {
> >  	{ .name = "cgbc-gpio"	},
> >  	{ .name = "cgbc-i2c", .id = 1 },
> >  	{ .name = "cgbc-i2c", .id = 2 },
> > +	{ .name = "cgbc-i2c", .id = 3 },
> 
> [Severity: Low]
> Should this use PLATFORM_DEVID_AUTO instead of hard-coding the cell ID?

I had this question too.

-- 
Lee Jones

Re: [PATCH 3/4] mfd: cgbc: Add virtual I2C bus support

[Thomas Richard]

On 8/12/26 1:20 PM, Lee Jones wrote:
> On Tue, 04 Aug 2026, sashiko-bot@kernel.org wrote:
> 
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Low] Hard-coded cell IDs are used for the newly added `cgbc-i2c` subdevice instead of `PLATFORM_DEVID_AUTO`.
>> --
>>
>> --- Patch [3]: [PATCH 3/4] mfd: cgbc: Add virtual I2C bus support ---
>> commit 4f0038d5dc42bfd45540675cdbf89f68ab3673d5
>> Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>
>>
>> mfd: cgbc: Add virtual I2C bus support
>>
>> This commit adds a new I2C cell to support the internal virtual I2C bus 
>> provided by the Board Controller.
>>
>>> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
>>> index 4a409234e66c8..81bd0a381ca13 100644
>>> --- a/drivers/mfd/cgbc-core.c
>>> +++ b/drivers/mfd/cgbc-core.c
>>> @@ -240,6 +240,7 @@ static struct mfd_cell cgbc_devs[] = {
>>>  	{ .name = "cgbc-gpio"	},
>>>  	{ .name = "cgbc-i2c", .id = 1 },
>>>  	{ .name = "cgbc-i2c", .id = 2 },
>>> +	{ .name = "cgbc-i2c", .id = 3 },
>>
>> [Severity: Low]
>> Should this use PLATFORM_DEVID_AUTO instead of hard-coding the cell ID?
> 

I use mfd_add_devices() with PLATFORM_DEVID_NONE.
So I2C busses will have platform_id from 0 to 2 [1].
And I2C adapter is selected using the platform device id [2].

I'm not 100% convinced by this implementation. I planned to add new I2C
busses and new boards support, and each board selects some busses. I
guess it will be the right time to rework it. If you have any idea to
implement this in a better way, please let me know.

[1]
https://elixir.bootlin.com/linux/v7.2-rc7/source/drivers/mfd/mfd-core.c#L164-L167
[2]
https://elixir.bootlin.com/linux/v7.2-rc7/source/drivers/i2c/busses/i2c-cgbc.c#L374


Best Regards,
Thomas