Re: [PATCH v1 2/3] LoongArch: dts: i2c: Add clocks and clock-frequency properties to 2K1000

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `ls2x-i2c` driver ignores the `clocks` property from the device tree, uses a hardcoded 50 MHz input frequency, and fails to enable the APB clock.
- [High] The driver enables hardware interrupts during probe but fails to disable them if probe fails or when the module is unloaded, risking a shared interrupt storm.
--

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

LoongArch: dts: i2c: Add clocks and clock-frequency properties to 2K1000

This commit adds the clock-frequency and clocks properties to the i2c
nodes for the Loongson 2K1000 device tree.

> diff --git a/arch/loongarch/boot/dts/loongson-2k1000.dtsi b/arch/loongarch/boot/dts/loongson-2k1000.dtsi
> index ab6a55937e9eb..e6f0fe093ab3c 100644
> --- a/arch/loongarch/boot/dts/loongson-2k1000.dtsi
> +++ b/arch/loongarch/boot/dts/loongson-2k1000.dtsi
> @@ -311,6 +311,8 @@ uart0: serial@1fe20000 {
>  		i2c2: i2c@1fe21000 {
>  			compatible = "loongson,ls2k-i2c";
>  			reg = <0x0 0x1fe21000 0x0 0x8>;
> +			clock-frequency = <100000>;
> +			clocks = <&clk LOONGSON2_APB_CLK>;

[Severity: High]
This is a pre-existing issue, but does the ls2x-i2c driver actually use
these clock properties?

Looking at drivers/i2c/busses/i2c-ls2x.c, the driver appears to hardcode
the input clock frequency to 50 MHz:

drivers/i2c/busses/i2c-ls2x.c:
#define LS2X_I2C_PCLK_FREQ  (50 * HZ_PER_MHZ)

It uses this hardcoded value in ls2x_i2c_adjust_bus_speed() to calculate
the prescaler. Because the APB clock can scale, wouldn't ignoring the
clock frequency from the DT lead to out-of-spec I2C bus speeds?

Additionally, the driver never seems to call clk_prepare_enable().
If no other drivers request the shared APB clock, could the clock
framework disable it via clk_disable_unused, hanging the system on
register access?

>  			interrupt-parent = <&liointc0>;
>  			interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;

[Severity: High]
This is a pre-existing issue, but does the ls2x-i2c driver correctly
manage these interrupts during probe failures or module removal?

In drivers/i2c/busses/i2c-ls2x.c:ls2x_i2c_probe(), hardware interrupts
are enabled via ls2x_i2c_init() before the fallible devm_request_irq()
and devm_i2c_add_adapter() operations:

drivers/i2c/busses/i2c-ls2x.c:ls2x_i2c_probe() {
    ...
    ls2x_i2c_init(priv);

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

If probe fails later, or if the driver is unbound (there is no .remove
callback in ls2x_i2c_driver), the devm framework will free the IRQ handler
but leave the hardware interrupts enabled at the controller level.

Because the interrupt is requested with IRQF_SHARED, won't any subsequent
interrupt asserted by the I2C controller go unhandled and cause an
interrupt storm?

>  			status = "disabled";
>  		};

[ ... ]

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