linux-kernel - Re: [PATCH v3] Common clock: To list active consumers of 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: <20220822235014.86203C433D6@smtp.kernel.org>
Date:   Mon, 22 Aug 2022 16:50:12 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Vishal Badole <badolevishal1116@...il.com>
Cc:     mturquette@...libre.com, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, chinmoyghosh2001@...il.com,
        vimal.kumar32@...il.com,
        Vishal Badole <badolevishal1116@...il.com>,
        Mintu Patel <mintupatel89@...il.com>
Subject: Re: [PATCH v3] Common clock: To list active consumers of clocks

Quoting Vishal Badole (2022-08-02 11:09:47)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f00d4c1..c96079f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -102,6 +102,7 @@ struct clk {
>         unsigned long min_rate;
>         unsigned long max_rate;
>         unsigned int exclusive_count;
> +       unsigned int enable_count;
>         struct hlist_node clks_node;
>  };
>  
> @@ -1008,6 +1009,10 @@ void clk_disable(struct clk *clk)
>                 return;
>  
>         clk_core_disable_lock(clk->core);
> +
> +       if (clk->enable_count > 0)
> +               clk->enable_count--;
> +
>  }
>  EXPORT_SYMBOL_GPL(clk_disable);
>  
> @@ -1169,10 +1174,16 @@ EXPORT_SYMBOL_GPL(clk_restore_context);
>   */
>  int clk_enable(struct clk *clk)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>  
> -       return clk_core_enable_lock(clk->core);
> +       ret = clk_core_enable_lock(clk->core);
> +       if (!ret)
> +               clk->enable_count++;
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_enable);

We'll want the above three hunks to be a different patch so we can
discuss the merits of tracking per user enable counts. Do you have a
usecase for this or is it "just for fun"? By adding a count we have more
code, and we waste more memory to track this stat. I really would rather
not bloat just because, so please elaborate on your use case.

>  
> @@ -2953,28 +2964,41 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
>                                  int level)
>  {
>         int phase;
> +       struct clk *clk_user;
> +       int multi_node = 0;
>  
> -       seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu ",
> +       seq_printf(s, "%*s%-*s %-7d %-8d %-8d %-11lu %-10lu ",
>                    level * 3 + 1, "",
> -                  30 - level * 3, c->name,
> +                  35 - level * 3, c->name,
>                    c->enable_count, c->prepare_count, c->protect_count,
>                    clk_core_get_rate_recalc(c),
>                    clk_core_get_accuracy_recalc(c));
>  
>         phase = clk_core_get_phase(c);
>         if (phase >= 0)
> -               seq_printf(s, "%5d", phase);
> +               seq_printf(s, "%-5d", phase);
>         else
>                 seq_puts(s, "-----");
>  
> -       seq_printf(s, " %6d", clk_core_get_scaled_duty_cycle(c, 100000));
> +       seq_printf(s, " %-6d", clk_core_get_scaled_duty_cycle(c, 100000));
>  
>         if (c->ops->is_enabled)
> -               seq_printf(s, " %9c\n", clk_core_is_enabled(c) ? 'Y' : 'N');
> +               seq_printf(s, " %5c ", clk_core_is_enabled(c) ? 'Y' : 'N');
>         else if (!c->ops->enable)
> -               seq_printf(s, " %9c\n", 'Y');
> +               seq_printf(s, " %5c ", 'Y');
>         else
> -               seq_printf(s, " %9c\n", '?');
> +               seq_printf(s, " %5c ", '?');
> +
> +       hlist_for_each_entry(clk_user, &c->clks, clks_node) {
> +               seq_printf(s, "%*s%-*s  %-4d\n",
> +                          level * 3 + 2 + 105 * multi_node, "",
> +                          30,
> +                          clk_user->dev_id ? clk_user->dev_id : "deviceless",
> +                          clk_user->enable_count);
> +
> +               multi_node = 1;

This part that prints the dev_id might be useful and can be the first
patch in the series. In that same patch, please print the con_id so we
know which clk it is for the device. We should also improve of_clk_get()
so that the index is visible to the 'struct clk::con_id' somehow. Maybe
we can convert the integer index into a string and assign that to con_id
in that case as well.