| 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
| ||
|
Message-ID: <1jttci2k8k.fsf@starbuckisacylon.baylibre.com>
Date: Fri, 08 Nov 2024 09:38:35 +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 15:59, Chuan Liu <chuan.liu@...ogic.com> wrote:
> hi Jerome:
>
> Tranks for your REF. I looked at your patch and there are some parts
> that I don't quite understand: The original intention of
> CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable
> _during _gate/ungate, set rate and re-parent" when setting a clock. After
> setting the clock, it can still be disabled. However, from what I see in
> your patch, the handling logic seems more like "parents need _always _ gate
> during clock gate period"?
As explained in the description, the problem with CLK_IGNORE_UNUSED and
CLK_OPS_PARENT_ENABLE is that you'll get cycle of enable/disable, which
will disable any parent clock that may have a been enabled and expected
to be ignored.
IOW, the CCF changes the state of the tree while inspecting it.
This change solves that.
>
> On 10/4/2024 9:39 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> As it as been pointed out numerous times, flagging a clock with
>> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
>> on. The clock will get disabled if any enable/disable cycle happens on it
>> or its parent clocks.
>>
>> Because enable/disable cycles will disable unused clocks,
>> clk_disable_unused() should not trigger such cycle. Doing so disregard
>> the flag if set for any parent clocks. This is problematic with
>> CLK_OPS_PARENT_ENABLE handling.
>>
>> To solve this, and a couple other issues, pass the parent status to the
>> child while walking the subtree, and return whether child ignored disable,
>> or not.
>>
>> * Knowing the parent status allows to safely disable clocks with
>> CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
>> that, while the clock is not gated it is effectively disabled. Turning on
>> the parents to sanitize the sitation would bring back our initial
>> problem, so just let it sanitize itself when the clock gets used.
>>
>> * If a clock is not actively used (enabled_count == 0), does not have
>> CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
>> child ignored the disable, it should ignore the disable too. Doing so
>> avoids disabling what is feading the children. Let the flag trickle down
>> the tree. This has the added benefit to transfer the information to the
>> unprepare path, so we don't unprepare the parent of a clock that ignored
>> a disable.
>>
>> * An enabled clock must be prepared in CCF but we can't rely solely on
>> counts at clk_disable_unused() stage. Make sure an enabled clock is
>> considered prepared too, even if does not implement the related callback.
>> Also make sure only disabled clocks get unprepared.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
>> ---
>>
>> This is sent as an RFC to continue the discussion started by Chuan.
>> It is not meant to be applied as it is.
>>
>>
>> drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 60 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index d02451f951cf..41c4504a41f1 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
>> }
>> }
>>
>> - /*
>> - * This could be called with the enable lock held, or from atomic
>> - * context. If the parent isn't enabled already, we can't do
>> - * anything here. We can also assume this clock isn't enabled.
>> - */
>> - if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
>
> This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
> normal logic, if the parent is disabled, its children will also be
> forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
> is configured.😳
It's removed.
>
>> - if (!clk_core_is_enabled(core->parent)) {
>> - ret = false;
>> - goto done;
>> - }
>> -
>> ret = core->ops->is_enabled(core->hw);
>> done:
>> if (core->rpm_enabled)
>> @@ -1454,22 +1443,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>> clk_core_unprepare_lock(core);
>> }
>>
>> -static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>> +static bool __init clk_unprepare_unused_subtree(struct clk_core *core,
>> + bool parent_prepared)
>> {
>> struct clk_core *child;
>> + bool prepared;
>>
>> lockdep_assert_held(&prepare_lock);
>>
>> + /*
>> + * Relying on count is not possible at this stage, so consider
>> + * prepared an enabled clock, in case only .is_enabled() is
>> + * implemented
>> + */
>> + if (parent_prepared)
>> + prepared = (clk_core_is_prepared(core) ||
>> + clk_core_is_enabled(core));
>> + else
>> + prepared = false;
>> +
>> hlist_for_each_entry(child, &core->children, child_node)
>> - clk_unprepare_unused_subtree(child);
>> + if (clk_unprepare_unused_subtree(child, prepared) &&
>> + prepared && !core->prepare_count)
>> + core->flags |= CLK_IGNORE_UNUSED;
>>
>> - if (core->prepare_count)
>> - return;
>> + if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
>> + goto out;
>>
>> - if (core->flags & CLK_IGNORE_UNUSED)
>> - return;
>> + if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
>> + goto out;
>>
>> - if (clk_core_is_prepared(core)) {
>> + /* Do not unprepare an enabled clock */
>> + if (clk_core_is_prepared(core) &&
>> + !clk_core_is_enabled(core)) {
>> trace_clk_unprepare(core);
>> if (core->ops->unprepare_unused)
>> core->ops->unprepare_unused(core->hw);
>> @@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>> core->ops->unprepare(core->hw);
>> trace_clk_unprepare_complete(core);
>> }
>> +
>> +out:
>> + return (core->flags & CLK_IGNORE_UNUSED) && prepared;
>> }
>>
>> -static void __init clk_disable_unused_subtree(struct clk_core *core)
>> +static bool __init clk_disable_unused_subtree(struct clk_core *core,
>> + bool parent_enabled)
>> {
>> struct clk_core *child;
>> unsigned long flags;
>> + bool enabled;
>>
>> lockdep_assert_held(&prepare_lock);
>>
>> - hlist_for_each_entry(child, &core->children, child_node)
>> - clk_disable_unused_subtree(child);
>> + flags = clk_enable_lock();
>>
>> - if (core->flags & CLK_OPS_PARENT_ENABLE)
>> - clk_core_prepare_enable(core->parent);
>> + /* Check if the clock is enabled from root to this clock */
>> + if (parent_enabled)
>> + enabled = clk_core_is_enabled(core);
>> + else
>> + enabled = false;
>>
>> - flags = clk_enable_lock();
>> + hlist_for_each_entry(child, &core->children, child_node)
>> + /*
>> + * If any child ignored disable, this clock should too,
>> + * unless there is, valid reason for the clock to be enabled
>> + */
>> + if (clk_disable_unused_subtree(child, enabled) &&
>> + enabled && !core->enable_count)
>> + core->flags |= CLK_IGNORE_UNUSED;
>>
>> - if (core->enable_count)
>> + if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
>> goto unlock_out;
>>
>> - 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))
>> 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
Powered by blists - more mailing lists