Re: [PATCH v5 2/2] i2c: ls2x: Add clocks property parsing and adjust bus speed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `devm_clk_get_optional_enabled()` dynamically inside `ls2x_i2c_adjust_bus_speed()` causes unbounded devres memory leaks, clock reference/enable leaks, and introduces an AB-BA deadlock during I2C transfers.
- [High] The prescale calculation divides by `div` without verifying it is non-zero, creating a division-by-zero vulnerability.
- [High] The driver silently ignores clock acquisition errors and fails to propagate `-EPROBE_DEFER`, breaking probe deferral.
--

commit feb6cc9d7c19ebc975910d8ee72838e62efa3d45
Author: Hongliang Wang <wanghongliang@loongson.cn>

i2c: ls2x: Add clocks property parsing and adjust bus speed

The i2c-ls2x driver supports dts and acpi parameter passing.
In dts, uses clock framework, by parsing clocks property to
get i2c bus reference clock, and define the div of reference
clock by device data.

> diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
> index b475dd27b7af9..46dafa11b3013 100644
> --- a/drivers/i2c/busses/i2c-ls2x.c
> +++ b/drivers/i2c/busses/i2c-ls2x.c
[ ... ]
> @@ -107,12 +117,30 @@ static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
>  	else
>  		t->bus_freq_hz = LS2X_I2C_FREQ_STD;
>  
> +	if (dev_of_node(dev)) {
> +		clk = devm_clk_get_optional_enabled(dev, NULL);

[Severity: High]
Is it safe to call devm_clk_get_optional_enabled() here?

ls2x_i2c_adjust_bus_speed() is called by ls2x_i2c_init(), which runs during
probe(), system resume(), and error recovery paths in ls2x_i2c_xfer_one().

Calling a devm_ function repeatedly at runtime will cause unbounded devres
memory leaks and clock reference/enable leaks.

Additionally, clk_prepare_enable() acquires the global clock framework's
prepare_lock. Since ls2x_i2c_xfer_one() executes while the I2C core holds
adap->bus_lock, this creates a bus_lock -> prepare_lock dependency. If an
I2C-based clock provider exists on the system, it will try to acquire locks in
the reverse order, which could result in an AB-BA deadlock.

> +		if (!IS_ERR_OR_NULL(clk))

[Severity: High]
Does masking the error here break probe deferral?

If devm_clk_get_optional_enabled() returns -EPROBE_DEFER because the clock
provider hasn't initialized yet, the error is swallowed instead of being
propagated to the probe() function. This could result in the driver binding
successfully without its necessary clock being enabled.

> +			pclk = clk_get_rate(clk);
> +		else
> +			pclk = LS2X_I2C_PCLK_FREQ;
> +
> +		div = priv->div;
> +
> +		val = (pclk * 10) / (div * t->bus_freq_hz) - 1;

[Severity: High]
Can div be zero here, causing a divide-by-zero panic?

div is populated via device_get_match_data(). If the device is dynamically
bound via sysfs or matches a fallback compatible without data,
device_get_match_data() returns NULL, making div 0.

> +	} else {
> +		/* clocks and clock-div are only ACPI properties. */
> +		if (!device_property_read_u32(dev, "clocks", &pclk) &&
> +		    !device_property_read_u32(dev, "clock-div", &div))
> +			val = (pclk * 10) / (div * t->bus_freq_hz) - 1;

[Severity: High]
Can clock-div be zero in the ACPI properties, also causing a divide-by-zero
panic?

If the firmware provides clock-div = 0, this will result in an unhandled
division-by-zero exception.

> +		else
> +			val = LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1;
> +	}
> +
>  	/*
>  	 * According to the chip manual, we can only access the registers as bytes,
>  	 * otherwise the high bits will be truncated.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604015848.18643-1-wanghongliang@loongson.cn?part=2