Re: [PATCH v5 2/4] clk: sunxi-ng: div: add read-only operation support

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

New issues:
- [High] Missing `.set_parent` in `ccu_rodiv_ops` leads to silent software-hardware clock tree desynchronization during reparenting.

Pre-existing issues:
- [High] Unvalidated hardware register read passed to `divider_ro_determine_rate()` can cause a divide-by-zero kernel panic.
--

--- Patch [2]: [PATCH v5 2/4] clk: sunxi-ng: div: add read-only operation support ---
commit d732b1bc291c593bb4873ef5f04edb2855137f30
Author: Jerome Brunet <[email protected]>

clk: sunxi-ng: div: add read-only operation support

This commit adds support for read-only dividers in the sunxi-ng clock
driver. It introduces a new ccu_rodiv_ops structure and updates the rate
determination helper to handle the read-only flag.

> diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
> index 62d680ccb524b..93cb4cb93abe8 100644
> --- a/drivers/clk/sunxi-ng/ccu_div.c
> +++ b/drivers/clk/sunxi-ng/ccu_div.c
> @@ -20,8 +20,22 @@ static int ccu_div_determine_rate_helper(struct ccu_mux_internal *mux,
>  	if (cd->common.features & CCU_FEATURE_FIXED_POSTDIV)
>  		req->rate *= cd->fixed_post_div;
>  
> -	ret = divider_determine_rate(&cd->common.hw, req, cd->div.table,
> -				     cd->div.width, cd->div.flags);
> +	if (cd->div.flags & CLK_DIVIDER_READ_ONLY) {
> +		unsigned long val;
> +		u32 reg;
> +
> +		reg = readl(cd->common.base + cd->common.reg);
> +		val = reg >> cd->div.shift;
> +		val &= (1 << cd->div.width) - 1;
> +
> +		ret = divider_ro_determine_rate(&cd->common.hw, req, cd->div.table,
> +						cd->div.width, cd->div.flags, val);

[Severity: High]
This is a pre-existing issue, but could this unvalidated register read cause 
a divide-by-zero kernel panic?

In ccu_div_determine_rate_helper(), the raw register value is read and
passed as 'val' to divider_ro_determine_rate(). If the register contains
an unexpected value from the bootloader (or 0 when CLK_DIVIDER_ONE_BASED
is set) that _get_div() maps to 0, divider_ro_determine_rate() lacks a 
zero-check before calculating:

    req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);

> +
> +	} else {

[ ... ]

> @@ -143,3 +157,15 @@ const struct clk_ops ccu_div_ops = {
>  	.set_rate	= ccu_div_set_rate,
>  };
>  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
> +
> +const struct clk_ops ccu_rodiv_ops = {
> +	.disable	= ccu_div_disable,
> +	.enable		= ccu_div_enable,
> +	.is_enabled	= ccu_div_is_enabled,
> +
> +	.get_parent	= ccu_div_get_parent,
> +
> +	.determine_rate	= ccu_div_determine_rate,
> +	.recalc_rate	= ccu_div_recalc_rate,
> +};

[Severity: High]
Could the missing .set_parent callback in ccu_rodiv_ops lead to a silent
desynchronization between the software clock tree and the hardware?

The struct provides reparenting capabilities via .get_parent and
.determine_rate, but omits .set_parent. If a rate request causes
ccu_mux_helper_determine_rate() to evaluate and select a new parent, the
Common Clock Framework will apply this new parent in software but skip
updating the hardware since .set_parent is NULL. 

This would leave all downstream clock rate calculations incorrect.

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