Re: [PATCH v4 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 6 potential issue(s) to consider:

New issues:
- [High] Calling `devm_clk_get_optional_enabled()` dynamically inside the hardware initialization function causes unbounded devres memory leaks, unbalanced clock enable counts, and potential memory reclaim deadlocks.
- [High] The driver silently ignores `-EPROBE_DEFER` from `devm_clk_get_optional_enabled()`, failing to defer probe and instead initializing with an incorrect fallback frequency.
- [High] Unvalidated `div` values from device match data or ACPI properties can cause a division by zero kernel panic during prescaler calculation.
- [Medium] Repurposing the standard "clocks" property as a `u32` integer for ACPI violates standard device property bindings.
- [Medium] The I2C clock prescaler calculation uses 32-bit arithmetic, which can overflow if the reference clock is too large.

Pre-existing issues:
- [High] Hardware interrupts are enabled before the IRQ handler is registered, risking unhandled interrupt storms.
--

commit 09494456e295e0b7fa53296c66ae490bc54b296b
Author: wanghongliang <wanghongliang@loongson.cn>

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

This commit adds clock property parsing for the i2c-ls2x driver to
adjust the bus speed based on the reference clock and divider values
obtained from device tree or ACPI.

> diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
> index b475dd27b7af9..6d332e59452d2 100644
> --- a/drivers/i2c/busses/i2c-ls2x.c
> +++ b/drivers/i2c/busses/i2c-ls2x.c
[ ... ]
> @@ -107,12 +117,29 @@ 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]
Does calling devm_clk_get_optional_enabled() during hardware initialization
risk memory leaks and unbalanced clock counts?

ls2x_i2c_adjust_bus_speed() is called from ls2x_i2c_init(), which executes
during probe, resume, and when an I2C transfer times out in
ls2x_i2c_xfer_one().

Calling a devres allocation function on every timeout or resume cycle
allocates new memory and increments the clock enable count each time without
a matching disable, leading to unbounded leaks. Additionally, performing
devres allocations in the transfer path can cause memory reclaim deadlocks.

> +		if (clk && !IS_ERR(clk))
> +			pclk = clk_get_rate(clk);
> +		else
> +			pclk = LS2X_I2C_PCLK_FREQ;

[Severity: High]
If devm_clk_get_optional_enabled() returns -EPROBE_DEFER, will this code
fail to defer the probe?

By catching the error here and silently falling back to LS2X_I2C_PCLK_FREQ,
the hardware will initialize assuming a 50 MHz clock instead of waiting for
the clock provider to be ready. This could leave the clock unenabled by the
framework and lead to incorrect bus speeds.

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

[Severity: High]
Can 'div' be zero here, leading to a division by zero panic?

If the device is bound manually via sysfs without a matching compatible
string, device_get_match_data() will return NULL, leaving priv->div as 0.

[Severity: Medium]
Will the calculation (pclk * 10) overflow 32-bit arithmetic if the reference
clock rate exceeds approximately 429.5 MHz?

Casting to 64-bit for the multiplication would prevent wraparound.

> +	} else {
> +		if (!device_property_read_u32(dev, "clocks", &pclk) &&

[Severity: Medium]
Does reading "clocks" as a raw 32-bit integer violate standard device
bindings?

In generic device properties and Device Tree, the "clocks" property is
strictly standardized to contain an array of references (phandles) to clock
providers. Overloading this standardized property to store an integer can
cause generic parsing code to fail. Should a standard property like
"clock-frequency" be used instead?

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

[Severity: High]
Could a zero value in the "clock-div" ACPI property cause a division by zero
panic here?

> +		else
> +			val = LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1;
> +	}
> +
[ ... ]
> @@ -295,6 +322,8 @@ static int ls2x_i2c_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->div = (unsigned int)(unsigned long)device_get_match_data(dev);
> +

[Severity: High]
This is a pre-existing issue, but does initializing the hardware before
requesting the shared IRQ risk an unhandled interrupt storm?

ls2x_i2c_probe() currently performs:

	ls2x_i2c_init(priv);

	ret = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, ...);

ls2x_i2c_init() writes CTR_READY_MASK (which includes LS2X_CTR_IEN, bit 6)
to the hardware control register, enabling the device to assert interrupts.

If the device asserts an interrupt immediately on the shared IRQ line, it
will fire before devm_request_irq() completes. Other drivers sharing the IRQ
will return IRQ_NONE, leading to an unhandled interrupt storm that could
cause the kernel to permanently disable the shared IRQ line.

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