Re: [PATCH v2 2/2] i2c: Add driver for gpio based busses with shared SCL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The child fwnode reference is dropped at the end of the scoped loop, leaving a dangling pointer in the adapter device.
- [High] An integer overflow in the clock-frequency calculation can cause a division by zero panic.
- [Low] Usage of non-existent API `devm_mutex_init()` will cause a build failure.
--

commit 26b139c7eeb6ff63f03c34df65162da9a405e505
Author: Markus Stockhausen <markus.stockhausen@gmx.de>

i2c: Add driver for gpio based busses with shared SCL

This commit introduces a new I2C driver for hardware designs that share a
single SCL line across multiple busses. It synchronizes data transfers
across up to 32 busses using a central mutex.

> diff --git a/drivers/i2c/busses/i2c-gpio-shared.c b/drivers/i2c/busses/i2c-gpio-shared.c
> new file mode 100644
> index 0000000000000..4c2e4011fcbf1
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-gpio-shared.c
[ ... ]
> +static int gpio_shared_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ctx = devm_kzalloc(dev, struct_size(ctx, bus, bus_count), GFP_KERNEL);
> +	if (!ctx)
> +		return dev_err_probe(dev, -ENOMEM, "memory allocation failed\n");
> +
> +	ret = devm_mutex_init(dev, &ctx->lock);

Is this devm_mutex_init() intended to be a different function? It does not
appear to exist in the kernel and will likely cause a build failure.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "mutex initialization failed\n");
> +
> +	platform_set_drvdata(pdev, ctx);
> +
> +	ctx->scl = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> +	if (IS_ERR(ctx->scl))
> +		return dev_err_probe(dev, PTR_ERR(ctx->scl), "shared SCL GPIO not found\n");
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		struct i2c_algo_bit_data *bit_data;
[ ... ]
> +		u32 bus_num, clock_freq;
[ ... ]
> +		/*
> +		 * clock-frequency specifies the I2C bus frequency. Convert to the half-period
> +		 * delay in microseconds that i2c-algo-bit expects. Default to 5 us (~100 kHz)
> +		 * if not specified. This is usually lower than the configured frequency,
> +		 * especially near the 400 kHz limit.
> +		 */
> +		if (!fwnode_property_read_u32(child, "clock-frequency", &clock_freq) &&
> +		    clock_freq > 0)
> +			bit_data->udelay = max(1u, DIV_ROUND_UP(1000000, 2 * clock_freq));

If clock-frequency is provided as 0x80000000 in the device tree, could the
32-bit unsigned multiplication 2 * clock_freq wrap around to 0? That would
cause DIV_ROUND_UP() to perform a division by zero panic.

> +		else
> +			bit_data->udelay = 5;
[ ... ]
> +		adap->dev.parent = dev;
> +		adap->owner = THIS_MODULE;
> +		adap->algo_data = bit_data;
> +		adap->bus_recovery_info = ri;
> +		device_set_node(&adap->dev, child);

Since device_for_each_child_node_scoped() automatically drops the fwnode
reference at the end of each iteration, and device_set_node() assigns the
pointer without incrementing the reference count, will this leave
adap->dev.fwnode pointing to unreferenced memory when the loop advances?

> +		snprintf(adap->name, sizeof(adap->name), "i2c-gpio-shared:%u", bus_num);
> +
> +		ret = devm_add_action_or_reset(dev, gpio_shared_del_adapter, adap);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511162528.84508-1-markus.stockhausen@gmx.de?part=2