Re: [PATCH v10 3/6] clk: conf: Support assigned-clock-sscs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Spread Spectrum Configuration (SSC) is applied before clock parents and rates are set, which can result in the SSC configuration being overwritten or becoming mathematically invalid.
- [Medium] Parsing `assigned-clock-sscs` by casting an array of `struct clk_spread_spectrum` to a `u32 *` violates strict typing and is highly brittle.
--

commit dcd88e4f08b6ae89c38dd70d8a94011d30708d37
Author: Peng Fan <peng.fan@nxp.com>

clk: conf: Support assigned-clock-sscs

This commit adds support for parsing Spread Spectrum Configuration (SSC) from
the device tree and configuring clocks accordingly. It introduces a new
function to read the assigned-clock-sscs property and apply the settings.

> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index 303a0bb26e54a..550b8ae375a2c 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -155,6 +155,78 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
[ ... ]
> +static int __set_clk_spread_spectrum(struct device_node *node, bool clk_supplier)
> +{
[ ... ]
> +	sscs = kcalloc(count, elem_size, GFP_KERNEL);
> +	if (!sscs)
> +		return -ENOMEM;
> +
> +	rc = of_property_read_u32_array(node, "assigned-clock-sscs", (u32 *)sscs,
> +					count * 3);

[Severity: Medium]
Is it safe to cast an array of struct clk_spread_spectrum to u32 * here?

If any fields are added to the struct or the compiler adds padding, the OF
helper will read data in 12-byte chunks, which will silently misalign with the
struct array boundaries for all elements after the first. 

Would it be better to read into a dynamically allocated temporary u32 array
and explicitly populate each struct field?

> +	if (rc)
> +		goto free_sscs;
> +
[ ... ]
> @@ -174,6 +246,10 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier)
>  	if (!node)
>  		return 0;
>  
> +	rc = __set_clk_spread_spectrum(node, clk_supplier);
> +	if (rc < 0)
> +		return rc;
> +
>  	rc = __set_clk_parents(node, clk_supplier);

[Severity: High]
Does configuring spread spectrum before setting clock parents and rates cause
issues?

Spread spectrum modulates the base frequency, so its hardware parameters
usually depend on the final base rate and parent clock.  By configuring SSC
first, the settings might be computed based on the old hardware state, or
subsequent rate and parent changes might wipe out the SSC settings. 

Should SSC be configured after parents and rates are set?

>  	if (rc < 0)
>  		return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-clk-v10-v10-0-eb92484eda38@nxp.com?part=3