Re: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch resolves an invalid GENMASK evaluation for 0-width P dividers but fails to apply the same check for 0-width M dividers, leaving a register corruption bug intact.
- [High] When an unreachable low clock rate is requested, `ccu_mp_find_best` fails to find a suitable divider and returns `m = 0` and `p = 0`, leading to integer underflows that severely corrupt the CCU hardware register.
--

--- Patch [1]: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers ---
commit e8027d463bc82dfb3ec7ba4b45f1688fa59d1694
Author: Enzo Adriano <[email protected]>

clk: sunxi-ng: ccu_mp: fix clocks without P dividers

This commit modifies the sunxi-ng MP clock driver to conditionally apply the
P divider mask only when the P field is present. It also ensures the update
bit is set if required by the hardware. This prevents invalid GENMASK
evaluations and missing hardware updates.

Closes: https://lore.kernel.org/r/[email protected]

> diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
> index 7cdb0eedc69b5..aa6cb20447f19 100644
> --- a/drivers/clk/sunxi-ng/ccu_mp.c
> +++ b/drivers/clk/sunxi-ng/ccu_mp.c
> @@ -237,12 +237,17 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate,
>  
>  	reg = readl(cmp->common.base + cmp->common.reg);
>  	reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing the
invalid GENMASK for 0-width P dividers, should we apply the same check for
M dividers?

If an MP clock has no M divider (m.width == 0), the GENMASK will underflow
and produce an all-ones mask. The ~GENMASK operation then zeroes out the
entire hardware register, clearing critical fields like the clock gate.

> -	reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +	if (cmp->p.width)
> +		reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +	if (cmp->common.features & CCU_FEATURE_UPDATE_BIT)
> +		reg |= CCU_SUNXI_UPDATE_BIT;
>  	reg |= (m - cmp->m.offset) << cmp->m.shift;

[Severity: High]
This is a pre-existing issue, but can this operation corrupt the hardware
register if an extremely low rate is requested?

If a driver or user requests a rate lower than the minimum achievable rate,
ccu_mp_find_best() will skip all rates:

drivers/clk/sunxi-ng/ccu_mp.c:ccu_mp_find_best() {
    ...
	if (tmp_rate > rate)
		continue;
    ...
}

This causes it to return the initial values for m and p as 0. Back in
ccu_mp_set_rate(), an m of 0 will cause m - cmp->m.offset to underflow
(for example, 0 - 1 = 0xFFFFFFFF). This value is then left-shifted and
OR'd into the register, overwriting adjacent hardware fields with 1s.

> -	if (shift)
> -		reg |= ilog2(p) << cmp->p.shift;
> -	else
> -		reg |= (p - cmp->p.offset) << cmp->p.shift;
> +	if (cmp->p.width) {
> +		if (shift)
> +			reg |= ilog2(p) << cmp->p.shift;
> +		else
> +			reg |= (p - cmp->p.offset) << cmp->p.shift;
> +	}
>  
>  	writel(reg, cmp->common.base + cmp->common.reg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1