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

On Fri, Jul 17, 2026 at 11:40 PM <[email protected]> wrote:
>
> 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);

Well that would be either an invalid hardware state or unpowered / gated
hardware. Neither is correct and should never happen and blowing up is
the correct response.

In any case we don't have CLK_DIVIDER_ONE_BASED dividers so we are fine.

> > +
> > +     } 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 is an issue. I'm not sure if what Sashiko says actually happens.
But 1. this is "read-only divider", not "read-only mux & divider",
so the .set_parent callback should be provided. And 2. this is using
the ccu_mux_determine_rate_helper, so it's possible a clk_set_rate()
call is going to cause a reparent.

I can add it while applying if there are no other issues.

ChenYu

> This would leave all downstream clock rate calculations incorrect.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
>