Re: [PATCH v2 2/3] clk: rockchip: Add internal support for 24-bit fractional dividers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: rockchip: double-counted high_nwidth in clk_fd_v2_set_rate() mask
--

commit ff752d6d0f06073d53779cab9bd1294682b4f0b0
Author: Elaine Zhang <zhangqing@rock-chips.com>

clk: rockchip: Add internal support for 24-bit fractional dividers

This commit adds internal support and a clock branch type for Rockchip
24-bit fractional dividers whose numerator and denominator are split across
a 16-bit low register and an 8-bit high register.

> diff --git a/drivers/clk/rockchip/clk-fractional-divider-v2.c b/drivers/clk/rockchip/clk-fractional-divider-v2.c
> new file mode 100644
> index 0000000000000..5ab2c43180661
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-fractional-divider-v2.c

[ ... ]

> +static int clk_fd_v2_set_rate(struct clk_hw *hw, unsigned long rate,
> +			      unsigned long parent_rate)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	unsigned long flags = 0;
> +	unsigned long m, n, m_low, n_low, m_high, n_high;
> +	u32 val, val_high;
> +
> +	rational_best_approximation(rate, parent_rate,
> +				    GENMASK(fd->mwidth - 1, 0),
> +				    GENMASK(fd->nwidth - 1, 0),
> +				    &m, &n);
> +
> +	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
> +		m--;
> +		n--;
> +	}
> +	/*
> +	 * When compensation the fractional divider,
> +	 * the [1:0] bits of the numerator register are omitted,
> +	 * which will lead to a large deviation in the result.
> +	 * Therefore, it is required that the numerator must
> +	 * be greater than 4.
> +	 *
> +	 * Note that there are some exceptions here:
> +	 * If there is an even frac div, we need to keep the original
> +	 * numerator(<4) and denominator. Otherwise, it may cause the
> +	 * issue that the duty ratio is not 50%.
> +	 */
> +	if (m < 4 && m != 0) {
> +		if (n % 2 == 0)
> +			val = 1;
> +		else
> +			val = DIV_ROUND_UP(4, m);
> +
> +		n *= val;
> +		m *= val;
> +		if (n > GENMASK(fd->nwidth + fd->high_nwidth - 1, 0)) {
> +			pr_debug("%s n(%ld) is overflow, use mask value\n",
> +				 __func__, n);
> +			n = GENMASK(fd->nwidth + fd->high_nwidth - 1, 0);
> +		}
> +	}

[Severity: High]
Does this overflow check in clk_fd_v2_set_rate() double-count
fd->high_nwidth and allow n to overflow the 24-bit denominator register?

In clk_hw_register_fractional_divider_v2(), fd->nwidth is already
initialized to the combined width of the low and high registers:

	fd->nwidth = nwidth + high_nwidth;
	...
	fd->high_nwidth = high_nwidth;

For a 24-bit divider (16-bit low + 8-bit high), fd->nwidth is 24 and
fd->high_nwidth is 8, so GENMASK(fd->nwidth + fd->high_nwidth - 1, 0)
evaluates to 32 bits (0xffffffff) instead of 24 bits (0xffffff).

When rational_best_approximation() returns m in {1, 2, 3} and an odd n
such that n * val exceeds 0xffffff (for example, m = 1 and n = 0x400001,
giving m = 4 and n = 0x1000004), the overflow check does not trigger.
Subsequent register extraction in clk_fd_v2_set_rate():

	n_low = n & fd->nmask;
	...
	n_high = (n & GENMASK(fd->nwidth - 1, (fd->nwidth - fd->high_nwidth))) >>
		 (fd->nwidth - fd->high_nwidth);

only extracts bits [23:0], silently truncating the upper bits of n while
keeping the scaled m.

Also, if the mask is changed to GENMASK(fd->nwidth - 1, 0), wouldn't
clamping only n to the maximum mask after multiplying both m and n by val
distort the m / n ratio instead of keeping the unscaled m and n when
n * val overflows?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-add-board-ok3572-c-clk-v2-0-90b98836be6e@qq.com?part=2