Re: [PATCH v2 1/3] clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `ccu_maskdiv_set_rate` recalculates a different mask factor (`M`) than what `ccu_maskdiv_determine_rate` initially chose, silently programming the hardware to a lower frequency than negotiated.
- [High] `ccu_maskdiv_determine_rate` ignores the `CLK_SET_RATE_NO_REPARENT` flag, potentially causing unintended clock reparenting.
--

--- Patch [1]: [PATCH v2 1/3] clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type ---
commit cf0833bab086c6beae6c2b221f361043441cc718
Author: Juan Manuel López Carrillo <juanmanuellopezcarrillo@gmail.com>

clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type

This commit adds a cycle-masking divider for the A523/T527 GPU clock.
It masks M pulses out of every 2^width parent cycles instead of using a
linear divider. It also adds custom determine_rate and set_rate functions
to support this behavior.

> diff --git a/drivers/clk/sunxi-ng/ccu_maskdiv.c b/drivers/clk/sunxi-ng/ccu_maskdiv.c
> new file mode 100644
> index 0000000000000..4ad49d51405b3
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_maskdiv.c

[ ... ]

> +static int ccu_maskdiv_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
> +	unsigned long best_rate = 0, best_parent_rate = 0;
> +	struct clk_hw *best_parent = NULL;
> +	unsigned int best_m = UINT_MAX;
> +	unsigned int i;
> +
> +	for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> +		struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);

[Severity: High]
Does this iteration need to respect the CLK_SET_RATE_NO_REPARENT flag?

Unlike other sunxi-ng multiplexers that use ccu_mux_helper_determine_rate(),
this loops unconditionally across all available parents. If a driver specifies
CLK_SET_RATE_NO_REPARENT, would this silently ignore it and potentially cause
unintended clock reparenting?

> +		unsigned long parent_rate, new_rate;
> +		unsigned int m;

[ ... ]

> +static int ccu_maskdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
> +	unsigned int m;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	m = ccu_maskdiv_find_m(parent_rate, rate, cmd->width);

[Severity: High]
Does ccu_maskdiv_set_rate() end up programming a different mask factor (M)
than what ccu_maskdiv_determine_rate() initially chose?

Because ccu_maskdiv_calc_rate() truncates the remainder (e.g. 1000 * 13 / 16
= 812), feeding the result back into ccu_maskdiv_find_m() evaluates this
truncated output again. For example, 812 * 16 / 1000 = 12, instead of the
original 13.

Could this inappropriately increase M and silently program the hardware to
a lower frequency than what was negotiated?

> +
> +	spin_lock_irqsave(cmd->common.lock, flags);

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