Re: [PATCH v10 2/6] clk: Introduce clk_hw_set_spread_spectrum

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk_hw_set_spread_spectrum() modifies clock hardware state without checking if the clock rate is exclusively protected.
- [Medium] clk_hw_set_spread_spectrum() silently returns success (0) when the clock provider does not implement the set_spread_spectrum callback.
--

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

clk: Introduce clk_hw_set_spread_spectrum

This commit adds clk_hw_set_spread_spectrum() to configure the spread spectrum
feature of a clock. It also introduces the set_spread_spectrum callback for
clock drivers to provide hardware-specific implementations.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 048adfa86a5d0..8c78621cde253 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c

[ ... ]

> @@ -2774,6 +2774,33 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>  
> +int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectrum *ss_conf)
> +{
> +	struct clk_core *core;
> +	int ret;
> +
> +	if (!hw)
> +		return 0;
> +
> +	core = hw->core;
> +
> +	clk_prepare_lock();
> +
> +	ret = clk_pm_runtime_get(core);
> +	if (ret)
> +		goto fail;
> +

[Severity: High]
Should this check if the clock rate is exclusively protected before modifying
the hardware state?

Other clock adjustment functions like clk_set_phase() or clk_set_rate() check
clk_core_rate_is_protected(core) and return -EBUSY to prevent interference
with consumers that require a strict, stable clock. Since enabling spread
spectrum dynamically modulates the frequency, it seems like it could disrupt
consumers relying on that protection.

> +	if (core->ops->set_spread_spectrum)
> +		ret = core->ops->set_spread_spectrum(hw, ss_conf);
> +

[Severity: Medium]
If the clock provider does not implement the set_spread_spectrum callback,
will this silently return 0 (success) because ret was set to 0 by a
successful clk_pm_runtime_get(core)?

Returning success when the operation is unsupported might mislead callers into
believing the spread spectrum modulation is active. Does it make sense to
return an error code like -EOPNOTSUPP instead?

> +	clk_pm_runtime_put(core);
> +
> +fail:
> +	clk_prepare_unlock();
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_set_spread_spectrum);

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