Re: [PATCH v30 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:

New issues:
- [High] The `struct i2c_timings timings` variable is declared on the stack without initialization. If the `clock-frequency` device tree property is missing, `timings.bus_freq_hz` remains uninitialized, leading to undefined behavior and incorrectly bypassing the fallback logic.

Pre-existing issues:
- [High] The driver disables the I2C controller and asserts hardware reset before unregistering the I2C adapter, leading to hanging I2C transfers during teardown.
--

commit 295034b0d28e53ac8b8ea47c906ff258ad2c83f6
Author: Ryan Chen <ryan_chen@aspeedtech.com>
Subject: i2c: aspeed: Read clock-frequency via i2c_parse_fw_timings()

This commit updates the Aspeed I2C driver to read the standard
clock-frequency property using i2c_parse_fw_timings(). It falls back to
the legacy bus-frequency property only if the standard property is absent.

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a26b74c712061..137889e57ddc6 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -1000,6 +1000,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>  	const struct of_device_id *match;
>  	struct aspeed_i2c_bus *bus;
>  	struct clk *parent_clk;
> +	struct i2c_timings timings;

[Severity: High]
Does the timings structure need to be zero-initialized here?

>  	int irq, ret;
>  
>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);

[ ... ]

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

[Severity: High]
Since use_defaults is passed as false to i2c_parse_fw_timings(), if the
clock-frequency device tree property is missing, i2c_parse_timing() will
not modify timings.bus_freq_hz.

Could this leave timings.bus_freq_hz with uninitialized stack memory,
which might evaluate to true and bypass the fallback to the bus-frequency
property?

> +		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;
> +		}
>  	}

[ ... ]

>  static void aspeed_i2c_remove_bus(struct platform_device *pdev)
>  {
>  	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&bus->lock, flags);
>  
>  	/* 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);

[Severity: High]
This is a pre-existing issue, but does the I2C controller get disabled
before child devices are unregistered?

In aspeed_i2c_remove_bus(), the driver disables the I2C controller and
asserts the hardware reset before calling i2c_del_adapter().

Since i2c_del_adapter() coordinates the removal of registered I2C child
devices, their remove() callbacks might issue final I2C transfers to
gracefully shut down. Because the controller is already disabled and held
in reset, could these transfers hang and expire the timeout, leading to
teardown delays and inconsistent hardware state?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-upstream_i2c-v30-0-5d4f9adc3530@aspeedtech.com?part=2