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: <20220826173457.GA3785@Mahakal>
Date:   Fri, 26 Aug 2022 23:04:59 +0530
From:   "<Vishal Badole>" <badolevishal1116@...il.com>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     mturquette@...libre.com, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, chinmoyghosh2001@...il.com,
        vimal.kumar32@...il.com, Mintu Patel <mintupatel89@...il.com>
Subject: Re: [PATCH v3] Common clock: To list active consumers of clocks

On Mon, Aug 22, 2022 at 04:50:12PM -0700, Stephen Boyd wrote:
> 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. 
Agreed, we will create a separate patch for the same.

> 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.
> 
Use case for per user count: If a consumer acquires the clocks without 
calling clk_get() or devm_clk_get() and enables without calling clk_enable() 
or clk_prepare_enable() (by bypassing the common clock framework), then dev_id 
will not be sufficient to tell about how clk is acquired. Here per user enable 
count can be used to tell that how clk is acquired and it is enabled by
that particular device or not in case also where dev_id is NULL.

We referred regualtor framework suggested by you in one of review point
where they are also using enable count.
> >  
> > @@ -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.

Agreed, We will create a fresh patch where we will print dev_id and
consumer id in clock summary.