linux-kernel - Re: [RFC PATCH] clk: core: refine disable unused clocks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1jcyj62gi7.fsf@starbuckisacylon.baylibre.com>
Date: Fri, 08 Nov 2024 10:59:12 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Chuan Liu <chuan.liu@...ogic.com>
Cc: Stephen Boyd <sboyd@...nel.org>,  Neil Armstrong
 <neil.armstrong@...aro.org>,  Kevin Hilman <khilman@...libre.com>,  Martin
 Blumenstingl <martin.blumenstingl@...glemail.com>,
  linux-clk@...r.kernel.org,  linux-kernel@...r.kernel.org,
  linux-amlogic@...ts.infradead.org,  linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH] clk: core: refine disable unused clocks

On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@...ogic.com> wrote:

>>>> -       if (core->flags & CLK_IGNORE_UNUSED)
>>>> +       /*
>>>> +        * If the parent is disabled but the gate is open, we should sanitize
>>>> +        * the situation. This will avoid an unexpected enable of the clock as
>>>> +        * soon as the parent is enabled, without control of CCF.
>>>> +        *
>>>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>>> +        * forcefully enabling a whole part of the subtree.  Just let the
>>>> +        * situation resolve it self on the first enable of the clock
>>>> +        */
>>>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>
> At first, I couldn't grasp the logic behind the 'return' here. Now it's
> clear. This approach is equivalent to completely giving up on
> handling clocks with CLK_OPS_PARENT_ENABLE feature in
> clk_disable_unused_subtree().
>

No. It's handled correctly as long as the tree is in coherent state.

What is not done anymore is fixing up an inconsistent tree, by this I
mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its
own registers but has its parent disabled.

In that particular case, clk_disable_unused_subtree() won't be turning on
everything to properly disable that one clock. That is the root cause of
the problem you reported initially. The clock is disabled anyway.

Every other case are properly handled (at least I think).

>>>>                   goto unlock_out;
>>>>
>>>>           /*
>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>>
>>>>    unlock_out:
>>>>           clk_enable_unlock(flags);
>>>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>>>> -               clk_core_disable_unprepare(core->parent);
>>>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>>    }
>>>>
>>>>    static bool clk_ignore_unused __initdata;
>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>>           clk_prepare_lock();
>>>>
>>>>           hlist_for_each_entry(core, &clk_root_list, child_node)
>>>> -               clk_disable_unused_subtree(core);
>>>> +               clk_disable_unused_subtree(core, true);
>>>>
>>>>           hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>> -               clk_disable_unused_subtree(core);
>>>> +               clk_disable_unused_subtree(core, true);
>>>>
>>>>           hlist_for_each_entry(core, &clk_root_list, child_node)
>>>> -               clk_unprepare_unused_subtree(core);
>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>
>>>>           hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>> -               clk_unprepare_unused_subtree(core);
>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>
>>>>           clk_prepare_unlock();
>>>>
>>>> --
>>>> 2.45.2
>>>>
>> --
>> Jerome

-- 
Jerome