Re: [PATCH] clk: disable unused clocks registered after boot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: unused clocks can bypass disable due to TOCTOU race in registration
--

commit 757dd3574808273e528cadee01291d010712df6b
Author: Alexandre Belloni <alexandre.belloni@bootlin.com>

clk: disable unused clocks registered after boot

This commit introduces a delayed work task to periodically scan for and disable
unused clocks registered by dynamically loaded modules after boot. It uses a
flag, clk_disable_unused_done, to determine whether the initial late_initcall
scan has completed.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fef87167a60b5..f455d2482a5a5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
[ ... ]
> @@ -1621,10 +1619,31 @@ static int __init clk_disable_unused(void)
>  	clk_prepare_unlock();
>  
>  	clk_pm_runtime_put_all();
> +}
> +
> +static void clk_disable_unused_workfn(struct work_struct *work)
> +{
> +	clk_disable_unused();
> +}
> +
> +static DECLARE_DELAYED_WORK(clk_disable_unused_work, clk_disable_unused_workfn);
> +
> +static int __init clk_disable_unused_init(void)
> +{
> +	if (clk_ignore_unused) {
> +		pr_warn("clk: Not disabling unused clocks\n");
> +		return 0;
> +	}
> +
> +	pr_info("clk: Disabling unused clocks\n");
> +
> +	clk_disable_unused();
> +
> +	WRITE_ONCE(clk_disable_unused_done, true);

[Severity: Medium]
Does setting clk_disable_unused_done after clk_disable_unused() returns and
drops prepare_lock open up a time-of-check to time-of-use race?

If a driver registering a clock (such as via an async probe thread) adds the
clock right after the boot scan completes, could it slip through before this
flag is updated?

>  
>  	return 0;
>  }
> -late_initcall_sync(clk_disable_unused);
> +late_initcall_sync(clk_disable_unused_init);
[ ... ]
> @@ -4462,8 +4481,13 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  	clk_core_link_consumer(core, hw->clk);
>  
>  	ret = __clk_core_init(core);
> -	if (!ret)
> +	if (!ret) {
> +		if (READ_ONCE(clk_disable_unused_done))
> +			mod_delayed_work(system_power_efficient_wq,
> +					 &clk_disable_unused_work,
> +					 CLK_DISABLE_UNUSED_DELAY);
>  		return hw->clk;
> +	}

[Severity: Medium]
Can a concurrent registration complete __clk_core_init() after the boot
scan finishes, but read clk_disable_unused_done as false?

In this sequence, __clk_core_init() takes the prepare_lock after
clk_disable_unused() has already dropped it. The clock is added to the
internal lists, but the boot scan has already passed it by.

Then, __clk_register() reads clk_disable_unused_done. If
clk_disable_unused_init() hasn't updated the flag yet, it will read false
and skip scheduling the delayed work.

Would this leave the newly registered unused clock permanently enabled since
it missed both the boot scan and the delayed work?

>  
>  	clk_prepare_lock();
>  	clk_core_unlink_consumer(hw->clk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917144011.3014860-1-alexandre.belloni@bootlin.com?part=1