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: <1j4j4c3l2n.fsf@starbuckisacylon.baylibre.com>
Date: Tue, 12 Nov 2024 09:36:32 +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 19:49, Chuan Liu <chuan.liu@...ogic.com> wrote:

> On 11/8/2024 5:59 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> 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).
>
> name              en_sts            flags
> clk_a                1          CLK_IGNORE_UNUSED
>     clk_b            0                0
>         clk_c        1         CLK_OPS_PARENT_ENABLE
>
> Based on the above case:
> 1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling
> 'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before
> disabling 'clk_b'). How can to ensure that during the period of
> disabling 'clk_c', 'clk_b' remains enabled?

That's perfect example of incoherent state.
How can 'clk_c' be enabled if its parent is disable. That makes no
sense, so there is no point enabling a whole subtree for this IMO.

>
> 2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be
> disabled later. However, here it goes to a 'goto' statement and then
> return 'false', ultimately resulting in 'clk_c' not being disabled?

We've discussed that 2 times already. This discussion is going in
circles now.

>
>>>>>>                    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

-- 
Jerome