Re: [PATCH v6 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 5 potential issue(s) to consider:
- [Critical] Calling `devm_clk_get_optional_enabled()` dynamically causes unbounded memory leaks and unbalanced clock enable counts.
- [High] Calculation of the I2C prescale value lacks bounds checking, leading to a kernel panic if `div` evaluates to 0.
- [Medium] The driver improperly repurposes the standard `clocks` property to read a raw 32-bit integer frequency in ACPI setups.
- [Medium] The strict check for `dev_of_node(dev)` breaks compatibility with ACPI PRP0001 matching, causing valid match data to be discarded.
- [Low] The numerator calculation `pclk * 10` can overflow a 32-bit integer, resulting in a miscalculated I2C bus speed.
--

commit 3bc9e29253442765caa0c735b574051e32efaa27
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. It adds
clock framework parsing for device tree configurations, and reads
clock and clock-div properties directly for ACPI setups.

> 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)) {

[Severity: Medium]
Does this check break compatibility with ACPI PRP0001 configurations?

During probe(), the driver correctly reads priv->div from the compatible
string using device_get_match_data(), which supports both native Device Tree
and ACPI PRP0001. However, since dev_of_node() evaluates to NULL for ACPI
nodes, PRP0001 setups will be incorrectly routed to the native ACPI fallback
block.

This will discard the valid matched priv->div and attempt to read non-existent
native ACPI properties. Could this check for the presence of priv->div
instead?

> +		clk = devm_clk_get_optional_enabled(dev, NULL);

[Severity: Critical]
Does calling devm_clk_get_optional_enabled() here cause a devres memory leak
and unbalanced clock enable counts?

ls2x_i2c_adjust_bus_speed() is called by ls2x_i2c_init(), which is called not
only during probe, but also during normal operation and error recovery:

ls2x_i2c_resume() -> ls2x_i2c_init()
ls2x_i2c_xfer_one() -> ls2x_i2c_init()

Since devm_clk_get_optional_enabled() dynamically allocates a managed resource
and increments the clock reference and enable counts, calling it repeatedly
without releasing previous allocations will eventually exhaust system memory
and overflow the clock reference tracking.

Can the clock be acquired exactly once in probe() instead?

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

[Severity: Low]
Can the numerator calculation pclk * 10 overflow a 32-bit integer?

Since pclk is a 32-bit unsigned integer, if the reference clock exceeds
approximately 429.49 MHz, the multiplication will wrap around and yield an
artificially small numerator.

While typical reference clocks might be lower, would it be safer to use 64-bit
precision like div_u64((u64)pclk * 10, ...) here?


[Severity: High]
Does this division lack bounds checking for a zero divisor?

If div evaluates to 0, this will result in a division by zero kernel panic.

This regression can happen if the device is manually bound via sysfs
driver_override without an ID match, causing device_get_match_data() to
return NULL. It can also happen in the ACPI branch below if a misconfigured
ACPI table sets clock-div to 0.

Should div be explicitly validated as greater than 0 before performing the
division?

> +	} else {
> +		/* clocks and clock-div are only ACPI properties. */
> +		if (!device_property_read_u32(dev, "clocks", &pclk) &&

[Severity: Medium]
Does this reuse the standard clocks property incorrectly for ACPI setups?

The clocks property is strictly defined in Device Tree and unified ACPI _DSD
environments as an array of phandles to clock providers. Reusing this exact
name to read a raw 32-bit integer frequency creates a binding conflict and
could break generic property parsers.

Could a standard, non-conflicting property name like clock-frequency or
ref-clock-frequency be used instead?

> +		    !device_property_read_u32(dev, "clock-div", &div))
> +			val = (pclk * 10) / (div * t->bus_freq_hz) - 1;
> +		else
> +			val = LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1;
> +	}
> +

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