Re: [PATCH v1 3/4] clk: sunxi-ng: sun55i-a523: reparent GPU while pll-gpu changes rate

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `pll_gpu_clk` retains the `CLK_SET_RATE_GATE` flag, which completely prevents the intended dynamic rate changes while the GPU is active.
- [High] Missing `clk_notifier_unregister()` on driver unbind/module unload leads to a Use-After-Free.
- [Medium] The return value of `ccu_mux_notifier_register()` is silently ignored.

Pre-existing issues:
- [High] Unhandled `ABORT_RATE_CHANGE` event in the mux notifier callback permanently parks the GPU on the bypass clock.
- [Medium] Out-of-bounds array read on `gpu_mux_table` via `ccu_mux_helper_set_parent()`.
--

--- Patch [3]: [PATCH v1 3/4] clk: sunxi-ng: sun55i-a523: reparent GPU while pll-gpu changes rate ---
commit faed0fa03ac4f89885fa0cb42878fa743f6eb4a4
Author: Juan Manuel López Carrillo <juanmanuellopezcarrillo@gmail.com>

clk: sunxi-ng: sun55i-a523: reparent GPU while pll-gpu changes rate

This commit registers a mux notifier on pll-gpu to park the GPU mod clock
on a fixed output while the PLL is being reprogrammed. This is intended to
allow glitch-free rate changes for higher speed-bin operating points.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> index 979e53e635228..4341eb66c0b75 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c

[ ... ]

> @@ -1698,6 +1709,10 @@ static int sun55i_a523_ccu_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	/* Reparent the GPU during pll-gpu rate changes */
> +	ccu_mux_notifier_register(pll_gpu_clk.common.hw.clk,
> +				  &sun55i_a523_gpu_nb);
> +

[Severity: High]
Does pll_gpu_clk still have the CLK_SET_RATE_GATE flag set?

Looking at drivers/clk/sunxi-ng/ccu-sun55i-a523.c, pll_gpu_clk is defined
with:

    .hw.init = CLK_HW_INIT_PARENTS_DATA("pll-gpu", osc24M,
                                        &ccu_nkmp_ops,
                                        CLK_SET_RATE_GATE),

The core clock framework enforces this in clk_core_set_rate_nolock():

    if (clk_core_rate_is_protected(core))
        return -EBUSY;

If the GPU driver calls clk_set_rate() while the clock is active, will the
core framework return -EBUSY and prevent the rate change from happening,
rendering this new notifier unused?

[Severity: Medium]
What happens if ccu_mux_notifier_register fails and returns an error?

If there is a low memory condition and clk_notifier_register returns
-ENOMEM, the registration will fail but the probe continues. Will this
leave the GPU clock without the mandatory glitch-free bypass protection
during rate changes?

[Severity: High]
Is it possible this introduces a use-after-free during module unload?

sun55i_a523_ccu_driver does not appear to have a .remove callback to
unregister this notifier.

If the module is unloaded (e.g. via rmmod), the statically allocated
sun55i_a523_gpu_nb is freed. If a clock rate change occurs on pll-gpu
afterwards, or if the global clock notifier list is traversed, could this
crash the system?

[Severity: High]
This is a pre-existing issue, but how does the notifier handle an aborted
rate change?

In drivers/clk/sunxi-ng/ccu_mux.c, ccu_mux_notifier_cb only handles
PRE_RATE_CHANGE and POST_RATE_CHANGE:

    if (event == PRE_RATE_CHANGE) {
        ...
    } else if (event == POST_RATE_CHANGE) {
        ...

If a clock rate change fails downstream after PRE_RATE_CHANGE, the core
emits ABORT_RATE_CHANGE. Since this event is dropped, will the GPU clock
remain permanently stuck at the bypass rate of 600MHz?

[Severity: Medium]
This is a pre-existing issue, but can ccu_mux_helper_set_parent read out of
bounds during POST_RATE_CHANGE?

If the bootloader leaves the hardware mux register with an unmapped value
like 6 or 7, ccu_mux_helper_get_parent returns this raw index. Later,
during the notifier callback, ccu_mux_helper_set_parent blindly uses it:

    if (cm->table)
        index = cm->table[index];

Since gpu_mux_table only has 5 elements:

    static const u8 gpu_mux_table[] = { 0, 2, 3, 4, 5 };

Could this result in an out-of-bounds read and write junk back to the
hardware register?

>  	return 0;
>  }

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