Re: [PATCH v9 2/2] clk: add TI CDCE6214 clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] clk: cdce6214: uninitialized memory used when regmap_read() fails
- [Medium] clk: cdce6214: integer overflow in pll_determine_rate() min check
- [Medium] clk: cdce6214: missing upper bound check allows silent truncation of PLL ndiv
- [Medium] clk: cdce6214: power leak in cdce6214_clk_pll_prepare() on lock timeout
- [Medium] clk: cdce6214: missing EEPROM boot delay after reset de-assertion
--

commit 3f176632480b02a9e90a4fa95363d1fa484107d5
Author: Sascha Hauer <s.hauer@pengutronix.de>
Subject: clk: add TI CDCE6214 clock driver

This commit introduces a new common clock framework driver for the TI 
CDCE6214 clock synthesizer. It registers multiple clock components including
the PLL, outputs, and dividers.

> diff --git a/drivers/clk/clk-cdce6214.c b/drivers/clk/clk-cdce6214.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/clk-cdce6214.c
> @@ -0,0 +1,1594 @@
[ ... ]
> +static int cdce6214_clk_out_is_prepared(struct clk_hw *hw)
> +{
> +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> +	struct cdce6214 *priv = clock->priv;
> +	unsigned int mask = cdce6214_clk_out_mask(clock->index);
> +	unsigned int val;
> +
> +	if (!mask)
> +		return -EINVAL;
> +
> +	regmap_read(priv->regmap, R4, &val);
> +
> +	return !(val & mask);

[Severity: High]
Is it possible for I2C transfers to fail here? If regmap_read() returns an 
error, val will remain uninitialized, leading to undefined behavior when the 
mask is applied. Should the return value be checked?

[ ... ]
> +static unsigned long cdce6214_clk_pll_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate)
> +{
> +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> +	struct cdce6214 *priv = clock->priv;
> +	unsigned long ndiv, num, den;
> +	unsigned int val;
> +
> +	regmap_read(priv->regmap, R30, &val);
> +	ndiv = FIELD_GET(R30_PLL_NDIV, val);

[Severity: High]
Similar to above, if this read fails, could we end up calculating the rate 
with garbage data from the stack and propagating a corrupted frequency to 
the clock framework?

[ ... ]
> +static int cdce6214_clk_pll_determine_rate(struct clk_hw *hw,
> +					   struct clk_rate_request *req)
> +{
> +	if (!req->best_parent_rate)
> +		return -EINVAL;
> +
> +	req->rate = clamp(req->rate, CDCE6214_VCO_MIN, CDCE6214_VCO_MAX);
> +
> +	if (req->rate < req->best_parent_rate * CDCE6214_PLL_NDIV_MIN)

[Severity: Medium]
Does this multiplication risk overflowing on 32-bit systems? 

If the parent rate is high (e.g., > 178.9 MHz), could the multiplication
req->best_parent_rate * CDCE6214_PLL_NDIV_MIN wrap around the 32-bit unsigned 
long, bypassing the lower bound check?

> +		return -EINVAL;
> +
> +	req->min_rate = CDCE6214_VCO_MIN;
> +	req->max_rate = CDCE6214_VCO_MAX;

[Severity: Medium]
Is an upper bound check also needed here to ensure the computed ndiv fits 
into the 15-bit hardware maximum? If the parent clock is very low, it seems 
a valid VCO rate could still result in an ndiv that is too large.

[ ... ]
> +static int cdce6214_clk_pll_prepare(struct clk_hw *hw)
> +{
> +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> +	struct cdce6214 *priv = clock->priv;
> +
> +	regmap_clear_bits(priv->regmap, R5, R5_PLL_POWER_BITS);
> +
> +	regmap_set_bits(priv->regmap, R0, RO_RECAL);
> +
> +	return cdce6214_wait_pll_lock(priv);

[Severity: Medium]
If the PLL lock times out, does this exit path leave the LDOs and VCO 
permanently active? Would it make sense to roll back the R5_PLL_POWER_BITS 
changes on error to prevent wasting power?

[ ... ]
> +static int cdce6214_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +				     unsigned long parent_rate)
> +{
> +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> +	struct cdce6214 *priv = clock->priv;
> +	unsigned long ndiv, num, den;
> +	int ret;
> +
> +	ret = pll_calc_values(parent_rate, rate, &ndiv, &num, &den);
> +	if (ret < 0)
> +		return ret;
> +
> +	regmap_update_bits(priv->regmap, R34, R34_PLL_DEN_23_16,
> +			   FIELD_PREP(R34_PLL_DEN_23_16, den >> 16));
> +	regmap_update_bits(priv->regmap, R33, R33_PLL_DEN_15_0,
> +			   FIELD_PREP(R33_PLL_DEN_15_0, den & 0xffff));
> +	regmap_update_bits(priv->regmap, R32, R32_PLL_NUM_23_16,
> +			   FIELD_PREP(R32_PLL_NUM_23_16, num >> 16));
> +	regmap_update_bits(priv->regmap, R31, R31_PLL_NUM_15_0,
> +			   FIELD_PREP(R31_PLL_NUM_15_0, num & 0xffff));
> +	regmap_update_bits(priv->regmap, R30, R30_PLL_NDIV,
> +			   FIELD_PREP(R30_PLL_NDIV, ndiv));

[Severity: Medium]
If an out-of-bounds ndiv reaches this point, will it be silently truncated 
by FIELD_PREP and written to the register, breaking the frequency 
configuration?

[ ... ]
> +static int cdce6214_pinconf_get_xo_cload(struct cdce6214 *priv, unsigned int pin)
> +{
> +	unsigned int val;
> +
> +	if (pin != SECREF)
> +		return -EOPNOTSUPP;
> +
> +	regmap_read(priv->regmap, R24, &val);
> +
> +	val = FIELD_GET(R24_IP_XO_CLOAD, val);
> +
> +	if (val >= ARRAY_SIZE(ip_xo_cload))
> +		return -EINVAL;
> +
> +	return ip_xo_cload[val];

[Severity: High]
Is this another place where a failed I2C read could cause us to evaluate 
uninitialized stack memory? It seems this could lead to returning random 
elements.

[ ... ]
> +static int cdce6214_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct cdce6214 *priv;
> +	struct pinctrl_dev *pctl;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	priv->dev = dev;
> +	i2c_set_clientdata(client, priv);
> +	dev_set_drvdata(dev, priv);
> +
> +	priv->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->reset_gpio)) {
> +		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
> +				     "failed to get reset gpio\n");
> +	}
> +
> +	priv->regmap = devm_regmap_init_i2c(client, &cdce6214_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "failed to init regmap\n");
> +
> +	ret = cdce6214_configure(priv);

[Severity: Medium]
Does the CDCE6214 require a brief boot delay after the reset de-assertion 
to load its EEPROM before I2C communication begins? It looks like we call 
cdce6214_configure() immediately, which might cause probe failures if the 
hardware isn't ready.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-clk-cdce6214-v9-0-f2ea74fd38a2@pengutronix.de?part=2