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

Pre-existing issues:
- [High] Pre-existing division by zero vulnerability if `bus-frequency` is set to 0 in the device tree.
--

commit 5605a3233169152a9f01c947946a36d0f4f9add6
Author: Ryan Chen <ryan_chen@aspeedtech.com>

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

This commit changes the driver to use i2c_parse_fw_timings() to read the
standard "clock-frequency" property. It falls 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);

[Severity: High]
This is a pre-existing issue, but if "clock-frequency" is absent and
"bus-frequency" is explicitly set to 0 in the device tree,
bus->bus_frequency will be set to 0 here.

This can happen if 0 is used as a placeholder in the device tree for the
bootloader to fill in, but the bootloader fails to do so.

Does this lead to a division by zero panic later during initialization?
Looking at aspeed_i2c_init_clk() in drivers/i2c/busses/i2c-aspeed.c where
bus->bus_frequency is used:

	divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);

Would it make sense to validate that bus->bus_frequency is non-zero after
reading it?

> +		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;
> +		}
>  	}
>  
>  	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-upstream_i2c-v33-0-6d5338fc56ed@aspeedtech.com?part=2