Re: [PATCH v2 2/2] clk: si544: add support for si549

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional dereference of `client->dev.of_node` during probe fallback path causes a kernel panic on non-DT systems.
- [High] Untrusted or unprogrammed hardware input causes a divide-by-zero kernel panic.
--

commit 8414d21feff1150c560a77278d7dec845cbe570f
Author: Pavel Löbl <pavel@loebl.cz>

clk: si544: add support for si549

Add support for Si549, which only differs from Si544 in internal
oscilator frequency. Si549 also comes in three types A,B and C.
Each having different maximum output frequency.

This patch also fixes maximum frequency of Si544 C. Which should
be 325MHz according to datasheet.

> diff --git a/drivers/clk/clk-si544.c b/drivers/clk/clk-si544.c
> index 09c06ecec1a52..b13aeead9c028 100644
> --- a/drivers/clk/clk-si544.c
> +++ b/drivers/clk/clk-si544.c

[ ... ]

> @@ -145,6 +153,8 @@ static int si544_get_muldiv(struct clk_si544 *data,
>  	settings->delta_m = reg[0] << 8 | reg[1] << 16 | reg[2] << 24;
>  	settings->delta_m >>= 8;
>  
> +	settings->xo_freq = data->chip_info->xo_freq;
> +
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed that si544_get_muldiv() reads the
high-speed divider (hs_div) directly from the I2C device registers without
validation.

Could this lead to a division by zero later?

If the hardware is unprogrammed or an I2C device returns zero for these
registers, settings->hs_div becomes 0. Then, when si544_calc_center_rate() is
called:

si544_calc_center_rate() {
	u32 d = settings->hs_div * BIT(settings->ls_div_bits);
	...
	do_div(vco, d);
}

d would evaluate to 0, triggering a divide-by-zero kernel panic. Is it worth
adding a check for hs_div == 0 after reading the registers?

[ ... ]

> @@ -446,7 +458,7 @@ static int si544_probe(struct i2c_client *client)
>  	init.num_parents = 0;
>  	data->hw.init = &init;
>  	data->i2c_client = client;
> -	data->max_freq = (uintptr_t)i2c_get_match_data(client);
> +	data->chip_info = i2c_get_match_data(client);
>  
>  	if (of_property_read_string(client->dev.of_node, "clock-output-names",
>  			&init.name))

[Severity: High]
This isn't a bug introduced by this patch, but can this fallback cause a NULL
pointer dereference on non-DT systems?

If the driver is instantiated via a mechanism that does not use Device Tree
(such as sysfs new_device), client->dev.of_node will be NULL.

When of_property_read_string() detects the NULL node, it returns an error,
which triggers the fallback logic right below this diff:

		init.name = client->dev.of_node->name;

This unconditionally dereferences the NULL of_node pointer. Should this code
check if client->dev.of_node is valid before attempting to use it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701145101.3932655-1-pavel@loebl.cz?part=2