Re: [PATCH v6 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:
- [High] Potential division by zero due to unvalidated hardware register value propagating to `divider_ro_determine_rate`.
- [Low] Undocumented API exposure and dead code due to exporting `ccu_mux_helper_unapply_prediv`.
--

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

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

Add support for sunxi-ng read-only dividers. This will be
useful to the a733 oscillator detection logic.

> diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
> index 62d680ccb524b..c385e0160f13b 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]
Can this lead to a division by zero in divider_ro_determine_rate()?

When ccu_div_determine_rate_helper() reads the hardware register and passes
the unvalidated value to divider_ro_determine_rate(), _get_div() could
return 0 if the register value is 0 (with CLK_DIVIDER_ONE_BASED) or if the
value is not found in the table.

Unlike divider_recalc_rate(), divider_ro_determine_rate() does not have an
if (!div) check:

drivers/clk/clk-divider.c:divider_ro_determine_rate() {
    ...
    div = _get_div(table, val, flags, width);
    ...
    req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
    ...
}

Will this cause a panic if the hardware is in an unexpected reset state or
misconfigured?

[ ... ]

> diff --git a/drivers/clk/sunxi-ng/ccu_mux.c b/drivers/clk/sunxi-ng/ccu_mux.c
> index 4503c9780c39f..12bfc99c91721 100644
> --- a/drivers/clk/sunxi-ng/ccu_mux.c
> +++ b/drivers/clk/sunxi-ng/ccu_mux.c
> @@ -67,7 +67,7 @@ unsigned long ccu_mux_helper_apply_prediv(struct ccu_common *common,
>  	return parent_rate / ccu_mux_get_prediv(common, cm, parent_index);
>  }
>  
> -static unsigned long ccu_mux_helper_unapply_prediv(struct ccu_common *common,
> +unsigned long ccu_mux_helper_unapply_prediv(struct ccu_common *common,
>  					    struct ccu_mux_internal *cm,
>  					    int parent_index,
>  					    unsigned long parent_rate)

[Severity: Low]
Is this function intended to be exported?

The commit message only mentions adding support for read-only operation in
sunxi-ng dividers and provides no justification for modifying mux helpers.

Additionally, ccu_mux_helper_unapply_prediv appears to be completely unused
outside of ccu_mux.c in this patch series. Could this change be an
undocumented API export that accidentally leaked from earlier development?

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