linux-kernel - Re: [PATCH 05/13] clk: keep track of the trigger of an ongoing clk_set

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: <gyx5a6sacm6xens4jmxqynehloumsxyft35u6nd445qsv5345l@553vkj27ywef>
Date:   Tue, 19 Sep 2023 09:06:05 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Benjamin Bara <bbara93@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Abel Vesa <abelvesa@...nel.org>, Peng Fan <peng.fan@....com>,
        Frank Oltmanns <frank@...manns.dev>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        Benjamin Bara <benjamin.bara@...data.com>
Subject: Re: [PATCH 05/13] clk: keep track of the trigger of an ongoing
 clk_set_rate

On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@...data.com>
> 
> When we keep track of the rate change trigger, we can easily check if an
> affected clock is affiliated with the trigger. Additionally, the trigger
> is added to the notify data, so that drivers can implement workarounds
> that might be necessary if a shared parent changes.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> ---
>  drivers/clk/clk.c   | 12 ++++++++++++
>  include/linux/clk.h |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 4954d31899ce..8f4f92547768 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -33,6 +33,9 @@ static struct task_struct *enable_owner;
>  static int prepare_refcnt;
>  static int enable_refcnt;
>  
> +/* responsible for ongoing rate change, protected by prepare_lock */
> +static struct clk *rate_trigger_clk;
> +
>  static HLIST_HEAD(clk_root_list);
>  static HLIST_HEAD(clk_orphan_list);
>  static LIST_HEAD(clk_notifier_list);
> @@ -1742,6 +1745,7 @@ static int __clk_notify(struct clk_core *core, unsigned long msg,
>  
>  	cnd.old_rate = old_rate;
>  	cnd.new_rate = new_rate;
> +	cnd.trigger = rate_trigger_clk ? : core->parent->hw->clk;
>  
>  	list_for_each_entry(cn, &clk_notifier_list, node) {
>  		if (cn->clk->core == core) {
> @@ -2513,6 +2517,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>  	/* prevent racing with updates to the clock topology */
>  	clk_prepare_lock();
>  
> +	rate_trigger_clk = clk;
> +

So I don't think that interacts very well with the clk_hw_set_rate
function you introduced. It looks like you only consider the initial
clock here so you wouldn't update rate_trigger_clk on a clk_hw_set_rate
call, but that creates some inconsistencies:

  - If we call clk_hw_set_rate outside of the set_rate path (but in
    .init for example), then we end up with a notifier without a trigger
    clock set.

  - More generally, depending on the path we're currently in, a call to
    clk_hw_set_rate will notify a clock in different ways which is a bit
    weird to me. The trigger clock can also be any clock, parent or
    child, at any level, which definitely complicates things at the
    driver level.

The rate propagation is top-down, so could be get away with just setting
the parent clock that triggered the notification?

Either way, we need unit tests for that too.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)