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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing NULL check for `i2c_get_match_data()` leads to a NULL pointer dereference.
- [Low] The `si544_id` array uses positional initializers instead of named initializers, violating the I2C subsystem rules.

Pre-existing issues:
- [High] Unconditional dereference of `client->dev.of_node` causes a kernel panic on non-DT systems.
- [High] Division by zero occurs in `si544_calc_center_rate()` if the hardware returns 0 for the `HS_DIV` register.
--

--- Patch [2]: [PATCH v3 2/2] clk: si544: add support for si549 ---
commit 12920363f4810dc248ea69506597ff1f3d421e35
Author: Pavel Löbl <pavel@loebl.cz>

clk: si544: add support for si549

This patch adds support for the Si549 programmable clock generator, which
differs from the Si544 primarily in internal oscillator frequency. It
introduces three types (A, B, and C) with different maximum output
frequencies and fixes the maximum frequency for the Si544 C variant.

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

[ ... ]

> @@ -258,15 +270,16 @@ static unsigned long si544_calc_center_rate(
>  		const struct clk_si544_muldiv *settings)
>  {
>  	u32 d = settings->hs_div * BIT(settings->ls_div_bits);
> +	u32 fxo = settings->xo_freq;
>  	u64 vco;
>  
>  	/* Calculate VCO from the fractional part */
> -	vco = (u64)settings->fb_div_frac * FXO;
> -	vco += (FXO / 2);
> +	vco = (u64)settings->fb_div_frac * fxo;
> +	vco += (fxo / 2);
>  	vco >>= 32;
>  
>  	/* Add the integer part of the VCO frequency */
> -	vco += (u64)settings->fb_div_int * FXO;
> +	vco += (u64)settings->fb_div_int * fxo;
>  
>  	/* Apply divider to obtain the generated frequency */
>  	do_div(vco, d);

[Severity: High]
This is a pre-existing issue, but can this result in a division by zero if
the hardware returns 0 for the HS_DIV register?

During driver probe, si544_get_muldiv() reads the HS_DIV register from the
I2C device. If the hardware is uninitialized or malfunctioning, settings->hs_div
evaluates to 0:

si544_get_muldiv() {
    ...
    settings->hs_div = (reg[1] & 0x07) << 8 | reg[0];
    ...
}

This would cause the divisor d to be 0 here in si544_calc_center_rate(),
leading to a division by zero when executing do_div(vco, d).

[ ... ]

> @@ -446,7 +459,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);

[Severity: High]
Is it possible to hit a NULL pointer dereference here if the driver is bound
manually via sysfs?

If the driver is bound without a matching id_table entry (for example, via
driver_override), i2c_get_match_data() can return NULL. This NULL pointer is
then unconditionally assigned to data->chip_info.

Later in si544_get_muldiv(), the unvalidated pointer is dereferenced:

si544_get_muldiv() {
    ...
    settings->xo_freq = data->chip_info->xo_freq;
    ...
}

Could a NULL check be added for data->chip_info before continuing the probe?

>  
>  	if (of_property_read_string(client->dev.of_node, "clock-output-names",
>  			&init.name))
>  		init.name = client->dev.of_node->name;

[Severity: High]
This isn't a bug introduced by this patch, but could this fallback path
trigger a NULL pointer dereference on non-Device Tree systems?

On platforms like ACPI or board files where client->dev.of_node is NULL,
of_property_read_string() safely handles the NULL pointer and returns -ENOSYS.
This causes the if condition to evaluate to true.

The fallback path then unconditionally dereferences client->dev.of_node to
access its name, which will cause a kernel panic if it is NULL.

[ ... ]

> @@ -478,18 +491,54 @@ static int si544_probe(struct i2c_client *client)
>  	return 0;
>  }

[ ... ]

>  static const struct i2c_device_id si544_id[] = {
> -	{ "si544a", 1500000000 },
> -	{ "si544b", 800000000 },
> -	{ "si544c", 350000000 },
> +	{ "si544a", (kernel_ulong_t)&clk_si544a_info },
> +	{ "si544b", (kernel_ulong_t)&clk_si544b_info },
> +	{ "si544c", (kernel_ulong_t)&clk_si544c_info },
> +	{ "si549a", (kernel_ulong_t)&clk_si549a_info },
> +	{ "si549b", (kernel_ulong_t)&clk_si549b_info },
> +	{ "si549c", (kernel_ulong_t)&clk_si549c_info },
>  	{ }
>  };

[Severity: Low]
Are these array initializations compliant with the I2C subsystem requirements?

The I2C subsystem guidelines state that initialized arrays of type
struct i2c_device_id must use named initializers, but these new entries use
positional initializers.

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