| 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: <20220626182517.GA26001@Mahakal>
Date: Sun, 26 Jun 2022 23:55:21 +0530
From: "<Vishal Badole>" <badolevishal1116@...il.com>
To: Stephen Boyd <sboyd@...nel.org>, mturquette@...libre.com,
inux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: chinmoyghosh2001@...il.com, mintupatel89@...il.com,
vimal.kumar32@...il.com
Subject: Re: [PATCH] Common clock: To list
active consumers of clocks
On Thu, Jun 23, 2022 at 06:05:48PM -0700, Stephen Boyd wrote:
> Quoting <Vishal Badole> (2022-06-22 10:02:20)
> >
> > From f2e9d78bd0f135206fbfbf2e0178a5782b972939 Mon Sep 17 00:00:00 2001
> > From: Vishal Badole <badolevishal1116@...il.com>
> > Date: Tue, 21 Jun 2022 09:55:51 +0530
> > Subject: [PATCH] Common clock: To list active consumers of clocks
>
> That patch is still malformed. Please try again. Also, stop sending it
> as a reply-to the previous patch. Thanks!
>
We have applied and checked the patch on top of the mainline and not
able to see that it is malformed. We will share revised patch using git
send mail.
> >
> > This feature lists the name of clocks and their consumer devices.
> > Using this feature user can easily check which device is using a
> > perticular clock. Consumers without dev_id are listed as no_dev_id.
> >
> > Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@...il.com>
> > Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@...il.com>
> > Co-developed-by: Mintu Patel <mintupatel89@...il.com>
> > Signed-off-by: Mintu Patel <mintupatel89@...il.com>
> > Acked-by: Vimal Kumar <vimal.kumar32@...il.com>
>
> The acked-by tag is largely for maintainer use. Please remove it. See
> Documentation/process/5.Posting.rst for more info.
>
Agreed, We will update this in the revised patch.
> > Signed-off-by: Vishal Badole <badolevishal1116@...il.com>
> > ---
> > drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index ed11918..b191009 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3018,6 +3018,63 @@ static int clk_summary_show(struct seq_file *s, void *data)
> > }
> > DEFINE_SHOW_ATTRIBUTE(clk_summary);
> >
> > +static void clk_consumer_show_one(struct seq_file *s, struct clk_core *core, int level)
> > +{
> > + struct clk *clk_user;
> > + const char *consumer;
> > +
> > + hlist_for_each_entry(clk_user, &core->clks, clks_node) {
> > + if (!clk_user->dev_id)
> > + consumer = "no_dev_id";
> > + else
> > + consumer = clk_user->dev_id;
>
> We can just pass NULL if there isn't a dev_id and get a nice "(NULL)"
> print instead of "no_dev_id".
>
Agreed, we will replace "no_dev_id" with "deviceless" in the revised
patch.
> > +
> > + seq_printf(s, "%*s%-*s %30s %5d %7d ",
> > + level * 3 + 1, "",
> > + 30 - level * 3, clk_user->core->name, consumer,
> > + clk_user->core->enable_count, clk_user->core->prepare_count);
>
> It would be great to not print the core enable count here and instead
> have two levels of enable accounting so we can print the per-user count.
> Basically, one in the 'struct clk_core' and one in the 'struct clk'. If
> that isn't done then this print is going to duplicate the count for
> every 'struct clk' and be meaningless.
>
We will add enable count member to struct clock to represent per user
count and will print that one along with clock and consumer name
> > +
> > + if (clk_user->core->ops->is_enabled)
> > + seq_printf(s, " %8c\n", clk_core_is_enabled(clk_user->core) ? 'Y' : 'N');
> > + else if (!clk_user->core->ops->enable)
> > + seq_printf(s, " %8c\n", 'Y');
> > + else
> > + seq_printf(s, " %8c\n", '?');
>
> I don't think we need any of these prints. They're already covered in
> the summary. And the summary should be able to print the users. See
> regulator_summary_show_subtree() for inspiration. It looks like they
> print "deviceless" for the "no_dev_id" case so maybe just use that
> instead of NULL print.
>
We will remove above prints in the revised patch. We are facing indentation issue whle printing consumer in summary
as given below
enable prepare protect duty hardware per-user
clock count count count rateccuracy phase cycle enable consumer count
clk_mcasp0_fixed 0 0 0 24576000 0 50000 Y
deviceless 0
In this case it will be better to have a separate debugfs entry as
clK_consumer to print clock, consumer and per-user count.
> > + }
> > +}
> > +
> > +static void clk_consumer_show_subtree(struct seq_file *s, struct clk_core *c, int level)
> > +{
> > + struct clk_core *child;
> > +
> > + clk_consumer_show_one(s, c, level);
> > +
> > + hlist_for_each_entry(child, &c->children, child_node)
> > + clk_consumer_show_subtree(s, child, level + 1);
> > +}
> > +
> > +static int clk_consumer_show(struct seq_file *s, void *data)
> > +{
> > + struct clk_core *c;
> > + struct hlist_head **lists = (struct hlist_head **)s->private;
> > +
> > + seq_puts(s, " enable prepare hardware\n");
> > + seq_puts(s, " clock consumer count count enable\n");
> > + seq_puts(s, "-----------------------------------------------------------------------------------------\n");
> > + clk_prepare_lock();
> > +
> > + /*Traversing Linked List to print clock consumer*/
>
> Please run scripts/checkpatch.pl, as this comment needs space after /*
> and before */
>
We will update this in revised patch.
> > +
> > + for (; *lists; lists++)
> > + hlist_for_each_entry(c, *lists, child_node)
> > + clk_consumer_show_subtree(s, c, 0);
> > +
> > + clk_prepare_unlock();
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(clk_consumer);
> > +
> > static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
> > {
> > int phase;
Powered by blists - more mailing lists