Re: [PATCH v4 2/2] clk: en7523: add support for .set_rate for en7581

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, Nov 06, 2026 at 11:45:13AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] clk: en7523: division by zero in divider_get_val() when rate
>   is 0
> - [Low] clk: en7523: misuse of dev_err_probe() in runtime
>   en7581_set_rate()
> --
> 
> commit 21c9a3ba438d14108d5599a5785e71997f1e9fbe
> Author: Christian Marangi <ansuelsmth@gmail.com>
> 
> clk: en7523: add support for .set_rate for en7581
> 
> This commit adds clock divisor tables and implements .determine_rate and
> .set_rate callbacks for the Airoha EN7581 SoC clocks.
> 
> > diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> > index 6640e814d36a4..db7447be31c33 100644
> > --- a/drivers/clk/clk-en7523.c
> > +++ b/drivers/clk/clk-en7523.c
> 
> [ ... ]
> 
> > @@ -775,11 +864,148 @@ static unsigned long en7581_recalc_rate(struct clk_hw *hw,
> >  		return 0;
> >  	}
> >  
> > -	return rate / en7523_get_div(desc, val);
> > +	if (desc->div_bits) {
> > +		val >>= desc->div_shift;
> > +		val &= (1 << desc->div_bits) - 1;
> > +
> > +		rate = divider_recalc_rate(hw, rate, val,
> > +					   desc->div_table,
> > +					   desc->div_flags,
> > +					   desc->div_bits);
> > +	}
> > +
> > +	return rate;
> > +}
> > +
> > +/*
> > + * Simulate rate with every divisor supported
> > + * 1. Search the divisor for the requested rate
> > + * 2. Simulate the final rate with the new divisor
> > + */
> > +static unsigned long en7581_get_div_for_rate(struct clk_hw *hw,
> > +					     unsigned long rate,
> > +					     unsigned long parent_rate,
> > +					     const struct en_clk_desc *desc,
> > +					     int *div_val)
> > +{
> > +	*div_val = divider_get_val(rate, parent_rate, desc->div_table,
> > +				   desc->div_bits, desc->div_flags);
> > +	if (*div_val < 0)
> > +		return 0;
> > +
> > +	return divider_recalc_rate(hw, parent_rate, *div_val,
> > +				   desc->div_table, desc->div_flags,
> > +				   desc->div_bits);
> > +}
> > +
> > +static int en7581_get_vals_for_rate(struct clk_hw *hw,
> > +				    const struct en_clk_desc *desc,
> > +				    unsigned long rate,
> > +				    u32 *base_val, u32 *div_val)
> > +{
> > +	struct device *dev = clk_hw_get_dev(hw);
> > +	unsigned long parent_rate, tmp_rate;
> > +	int tmp_base_val = 0;
> > +	int tmp_div_val = 0;
> > +
> > +	if (!desc->base_bits && !desc->div_bits)
> > +		return -EINVAL;
> 
> [Severity: Medium]
> Can en7581_get_vals_for_rate() pass rate == 0 into
> en7581_get_div_for_rate() -> divider_get_val()?
> 
> Calling clk_round_rate(clk, 0) or clk_set_rate(clk, 0) forwards
> req->rate == 0 to en7581_determine_rate(), and divider_get_val() divides
> by rate without checking for zero:
> 
> drivers/clk/clk-divider.c:divider_get_val() {
> 	unsigned int div, value;
> 
> 	div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
>     ...
> }
> 
> Rejecting !rate at the start of en7581_get_vals_for_rate() would avoid a
> division by zero.
>

Protective coding and I'm surprised divider_get_val doesn't handle such
case... Anyway I will add this extra check whatever...
 
> [ ... ]
> 
> > +static int en7581_set_rate(struct clk_hw *hw, unsigned long rate,
> > +			   unsigned long parent_rate)
> > +{
> > +	struct en_clk *en_clk = container_of(hw, struct en_clk, hw);
> > +	const struct en_clk_desc *desc = en_clk->desc;
> > +	struct device *dev = clk_hw_get_dev(hw);
> > +	struct regmap *map = en_clk->map;
> > +	u32 base_val, div_val;
> > +	u32 reg, val, mask;
> > +	int err;
> > +
> > +	err = en7581_get_vals_for_rate(hw, en_clk->desc, rate,
> > +				       &base_val, &div_val);
> > +	if (err)
> > +		return err;
> > +
> > +	if (desc->div_bits) {
> > +		reg = desc->div_reg ? desc->div_reg : desc->base_reg;
> > +
> > +		mask = (BIT(desc->div_bits) - 1) << desc->div_shift;
> > +		val = div_val << desc->div_shift;
> > +
> > +		err = regmap_update_bits(map, reg, mask, val);
> > +		if (err)
> > +			return dev_err_probe(dev, err, "Failed to update div reg for clock %s\n",
> > +					     desc->name);
> > +	}
> > +
> > +	if (desc->base_bits) {
> > +		mask = (BIT(desc->base_bits) - 1) << desc->base_shift;
> > +		val = base_val << desc->base_shift;
> > +
> > +		err = regmap_update_bits(map, desc->base_reg, mask, val);
> > +		if (err)
> > +			return dev_err_probe(dev, err, "Failed to update reg for clock %s\n",
> > +					     desc->name);
> > +	}
> 
> [Severity: Low]
> Should en7581_set_rate() use dev_err() instead of dev_err_probe() here?
> en7581_set_rate() is a runtime clk_ops callback rather than a probe
> function, and the sibling helpers en7581_recalc_rate() and
> en7581_get_vals_for_rate() already use dev_err().
> 

This is funny... dev_err_probe is not only to handle PROBE_DEFER or probe
context... it does also gives more formatted error code and convert the
error code to the plain text error giving more info. I'm keeping this.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261106113227.182324-1-ansuelsmth@gmail.com?part=2

-- 
	Ansuel