Re: [PATCH v32 2/5] i2c: aspeed: Read clock-frequency via i2c_parse_fw_timings()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] I2C adapter is removed after asserting hardware reset, preventing I2C transfers during device unbind.
- [High] Zero value in `bus-frequency` DT property leads to a divide-by-zero exception.
--

commit 92481335123f99873954906861adc75752062695
Author: Ryan Chen <ryan_chen@aspeedtech.com>

i2c: aspeed: Read clock-frequency via i2c_parse_fw_timings()

Use i2c_parse_fw_timings() to read the standard "clock-frequency"
property, and fall back to "bus-frequency" only when the standard
property is absent.

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a26b74c712061..f00bd779146e7 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
[ ... ]
> @@ -1025,12 +1026,18 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>  	}
>  	reset_control_deassert(bus->rst);
>  
> -	ret = of_property_read_u32(pdev->dev.of_node,
> -				   "bus-frequency", &bus->bus_frequency);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev,
> -			"Could not read bus-frequency property\n");
> -		bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
> +	i2c_parse_fw_timings(&pdev->dev, &timings, false);
> +	if (timings.bus_freq_hz) {
> +		bus->bus_frequency = timings.bus_freq_hz;
> +	} else {
> +		ret = of_property_read_u32(pdev->dev.of_node,
> +					   "bus-frequency",
> +					   &bus->bus_frequency);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"Could not read clock-frequency or bus-frequency property\n");
> +			bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but can this code cause a divide-by-zero
exception if the device tree sets "bus-frequency" to 0?

If the property is present and set to 0, of_property_read_u32() succeeds
and leaves bus->bus_frequency as 0. Later, in aspeed_i2c_init_clk(), it
is used as a divisor:

drivers/i2c/busses/i2c-aspeed.c:aspeed_i2c_init_clk() {
    ...
    divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
    ...
}

Does the driver need to validate that the read frequency is strictly
positive to prevent a kernel panic here?


[Severity: High]
This is a pre-existing issue, but does aspeed_i2c_remove_bus() prevent
I2C transfers during device unbind by asserting hardware reset too early?

In drivers/i2c/busses/i2c-aspeed.c:aspeed_i2c_remove_bus(), the code does:

    /* Disable everything. */
    writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
    writel(0, bus->base + ASPEED_I2C_INTR_CTRL_REG);

    spin_unlock_irqrestore(&bus->lock, flags);

    reset_control_assert(bus->rst);

    i2c_del_adapter(&bus->adap);

When i2c_del_adapter() unregisters child I2C devices, their remove
callbacks may attempt I2C transfers to cleanly shut down the peripherals.

Because the hardware controller is already disabled and in reset, will these
transfers silently fail, timeout, or cause external aborts when the CPU
tries to access the registers?

Should i2c_del_adapter() be called before disabling the hardware and
asserting reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-upstream_i2c-v32-0-b66eba921d01@aspeedtech.com?part=2