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

[sashiko-bot@kernel.org]

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.

[ ... ]

> +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().

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