[PATCH] clocksource: Add debugfs support

[PATCH] clocksource: Add debugfs support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add a top-level "clocksource" directory to debugfs. For each clocksource
registered with the system, a subdirectory will be added with attributes
that can be queried to obtain information about the clocksource.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/linux/clocksource.h |  3 ++
 kernel/time/clocksource.c   | 60 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 86d143db6523..89424da76244 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -118,6 +118,9 @@ struct clocksource {
 	u64			wd_last;
 #endif
 	struct module		*owner;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs;
+#endif
 };
 
 /*
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 7cb09c4cf21c..51266f53df83 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -911,6 +911,63 @@ void __clocksource_update_freq_scale(struct clocksource *cs, u32 scale, u32 freq
 }
 EXPORT_SYMBOL_GPL(__clocksource_update_freq_scale);
 
+#ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
+
+static struct dentry *debugfs_root;
+
+static int clocksource_debugfs_counter_show(struct seq_file *s, void *data)
+{
+	struct clocksource *cs = s->private;
+
+	seq_printf(s, "%llu\n", cs->read(cs));
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(clocksource_debugfs_counter);
+
+static void clocksource_debugfs_add(struct clocksource *cs)
+{
+	if (!debugfs_root)
+		return;
+
+	cs->debugfs = debugfs_create_dir(cs->name, debugfs_root);
+
+	debugfs_create_file("counter", 0444, cs->debugfs, cs,
+			    &clocksource_debugfs_counter_fops);
+}
+
+static void clocksource_debugfs_remove(struct clocksource *cs)
+{
+	debugfs_remove_recursive(cs->debugfs);
+}
+
+static int __init init_clocksource_debugfs(void)
+{
+	struct clocksource *cs;
+
+	debugfs_root = debugfs_create_dir("clocksource", NULL);
+
+	mutex_lock(&clocksource_mutex);
+
+	list_for_each_entry(cs, &clocksource_list, list)
+		clocksource_debugfs_add(cs);
+
+	mutex_unlock(&clocksource_mutex);
+
+	return 0;
+}
+late_initcall(init_clocksource_debugfs);
+#else
+static inline void clocksource_debugfs_add(struct clocksource *cs)
+{
+}
+
+static inline void clocksource_debugfs_remove(struct clocksource *cs)
+{
+}
+#endif
+
 /**
  * __clocksource_register_scale - Used to install new clocksources
  * @cs:		clocksource to be registered
@@ -951,6 +1008,7 @@ int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32 freq)
 	clocksource_select();
 	clocksource_select_watchdog(false);
 	__clocksource_suspend_select(cs);
+	clocksource_debugfs_add(cs);
 	mutex_unlock(&clocksource_mutex);
 	return 0;
 }
@@ -991,6 +1049,8 @@ static int clocksource_unbind(struct clocksource *cs)
 {
 	unsigned long flags;
 
+	clocksource_debugfs_remove(cs);
+
 	if (clocksource_is_watchdog(cs)) {
 		/* Select and try to install a replacement watchdog. */
 		clocksource_select_watchdog(true);
-- 
2.24.1

Re: [PATCH] clocksource: Add debugfs support

[John Stultz]

On Tue, Mar 31, 2020 at 2:40 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> Add a top-level "clocksource" directory to debugfs. For each clocksource
> registered with the system, a subdirectory will be added with attributes
> that can be queried to obtain information about the clocksource.
>

Curious, what's the need/planned use for this?  I know in the past
folks have tried to get timekeeping internals exported to userland so
they could create their own parallel userland timekeeping system,
which I worry is a poor idea.

thanks
-john

Re: [PATCH] clocksource: Add debugfs support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

On Tue, Mar 31, 2020 at 02:50:47PM -0700, John Stultz wrote:
> On Tue, Mar 31, 2020 at 2:40 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Add a top-level "clocksource" directory to debugfs. For each clocksource
> > registered with the system, a subdirectory will be added with attributes
> > that can be queried to obtain information about the clocksource.
> >
> 
> Curious, what's the need/planned use for this?  I know in the past
> folks have tried to get timekeeping internals exported to userland so
> they could create their own parallel userland timekeeping system,
> which I worry is a poor idea.

This was meant to be purely for debugging purposes. That is as an easy
way to check that the code was working and that the counter is properly
updated.

I certainly wasn't planning on implementing any userland on top of this.
Well, I guess it could be useful to use these values in test scripts
perhaps, since one of the clock sources exposed by one of the drivers I
have been working on is used across Tegra SoCs for hardware
timestamping. For that it might be interesting to be able to compare
those timestamp snapshots to something that I can read from userspace
during testing.

But that's about as far as I was planning on taking this. I agree that
basing some userland timekeeping system on a debugfs interface sounds
like a very bad idea.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clocksource: Add debugfs support

[John Stultz]

On Tue, Mar 31, 2020 at 3:25 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Mar 31, 2020 at 02:50:47PM -0700, John Stultz wrote:
> > On Tue, Mar 31, 2020 at 2:40 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Add a top-level "clocksource" directory to debugfs. For each clocksource
> > > registered with the system, a subdirectory will be added with attributes
> > > that can be queried to obtain information about the clocksource.
> > >
> >
> > Curious, what's the need/planned use for this?  I know in the past
> > folks have tried to get timekeeping internals exported to userland so
> > they could create their own parallel userland timekeeping system,
> > which I worry is a poor idea.
>
> This was meant to be purely for debugging purposes. That is as an easy
> way to check that the code was working and that the counter is properly
> updated.
>
> I certainly wasn't planning on implementing any userland on top of this.
> Well, I guess it could be useful to use these values in test scripts
> perhaps, since one of the clock sources exposed by one of the drivers I
> have been working on is used across Tegra SoCs for hardware
> timestamping. For that it might be interesting to be able to compare
> those timestamp snapshots to something that I can read from userspace
> during testing.

So, other platforms do similar, but utilizing the ktime_get_snapshot()
interface internally so drivers can share that SoC hardware
timestamping logic and export that via driver interfaces in a cleanly
abstracted way to userland, rather than exposing the timekeping
internals.

thanks
-john

Re: [PATCH] clocksource: Add debugfs support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]

On Tue, Mar 31, 2020 at 03:44:25PM -0700, John Stultz wrote:
> On Tue, Mar 31, 2020 at 3:25 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Mar 31, 2020 at 02:50:47PM -0700, John Stultz wrote:
> > > On Tue, Mar 31, 2020 at 2:40 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Add a top-level "clocksource" directory to debugfs. For each clocksource
> > > > registered with the system, a subdirectory will be added with attributes
> > > > that can be queried to obtain information about the clocksource.
> > > >
> > >
> > > Curious, what's the need/planned use for this?  I know in the past
> > > folks have tried to get timekeeping internals exported to userland so
> > > they could create their own parallel userland timekeeping system,
> > > which I worry is a poor idea.
> >
> > This was meant to be purely for debugging purposes. That is as an easy
> > way to check that the code was working and that the counter is properly
> > updated.
> >
> > I certainly wasn't planning on implementing any userland on top of this.
> > Well, I guess it could be useful to use these values in test scripts
> > perhaps, since one of the clock sources exposed by one of the drivers I
> > have been working on is used across Tegra SoCs for hardware
> > timestamping. For that it might be interesting to be able to compare
> > those timestamp snapshots to something that I can read from userspace
> > during testing.
> 
> So, other platforms do similar, but utilizing the ktime_get_snapshot()
> interface internally so drivers can share that SoC hardware
> timestamping logic and export that via driver interfaces in a cleanly
> abstracted way to userland, rather than exposing the timekeping
> internals.

The hardware timestamping functionality is going to be exposed by a
different driver. I was merely speculating that the debugfs interface
could be used to also read out the counter value for the TSC as a means
of correlating it to the values from the timestamping hardware. On
second thought that may not be very useful given the non-deterministic
delay between the hardware timestamp and the debugfs read.

Like I said, the original intent here was really only to make it easy to
inspect the clocksources. I can add more fields if that's deemed useful.
I can imagine that different engineers keep writing different test code
to verify clock sources and thought that this might be a good addition
to make this easier.

But if you have concerns that this might get abused for something
unintended I understand that, so if you think exposing this is a bad
idea, I'll just drop this patch.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clocksource: Add debugfs support

[Thomas Gleixner]

Thierry,

Thierry Reding <thierry.reding@gmail.com> writes:
> Add a top-level "clocksource" directory to debugfs. For each clocksource
> registered with the system, a subdirectory will be added with attributes
> that can be queried to obtain information about the clocksource.

first of all this does tell what this patch does but omits the more
important information about the WHY.

What's even worse is that the changelog is blantantly wrong.

> +static int clocksource_debugfs_counter_show(struct seq_file *s, void *data)
> +{
> +	struct clocksource *cs = s->private;
> +
> +	seq_printf(s, "%llu\n", cs->read(cs));
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(clocksource_debugfs_counter);
> +
> +static void clocksource_debugfs_add(struct clocksource *cs)
> +{
> +	if (!debugfs_root)
> +		return;
> +
> +	cs->debugfs = debugfs_create_dir(cs->name, debugfs_root);
> +
> +	debugfs_create_file("counter", 0444, cs->debugfs, cs,
> +			    &clocksource_debugfs_counter_fops);
> +}

It does not provide any information about the clocksource, it provides
an interface to read the counter - nothing else.

Try again.

Thanks,

        tglx

Re: [PATCH] clocksource: Add debugfs support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On Wed, Apr 01, 2020 at 12:06:37AM +0200, Thomas Gleixner wrote:
> Thierry,
> 
> Thierry Reding <thierry.reding@gmail.com> writes:
> > Add a top-level "clocksource" directory to debugfs. For each clocksource
> > registered with the system, a subdirectory will be added with attributes
> > that can be queried to obtain information about the clocksource.
> 
> first of all this does tell what this patch does but omits the more
> important information about the WHY.
> 
> What's even worse is that the changelog is blantantly wrong.
> 
> > +static int clocksource_debugfs_counter_show(struct seq_file *s, void *data)
> > +{
> > +	struct clocksource *cs = s->private;
> > +
> > +	seq_printf(s, "%llu\n", cs->read(cs));
> > +
> > +	return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(clocksource_debugfs_counter);
> > +
> > +static void clocksource_debugfs_add(struct clocksource *cs)
> > +{
> > +	if (!debugfs_root)
> > +		return;
> > +
> > +	cs->debugfs = debugfs_create_dir(cs->name, debugfs_root);
> > +
> > +	debugfs_create_file("counter", 0444, cs->debugfs, cs,
> > +			    &clocksource_debugfs_counter_fops);
> > +}
> 
> It does not provide any information about the clocksource, it provides
> an interface to read the counter - nothing else.

The counter is part of the information about a clocksource, isn't it?
But yes, frankly I had anticipated that I'd be adding more files here
and when I ended up not doing that I forgot to update the patch
description.

I can also add some information about what I intend to use this for,
though it'll be a bit boring because I really only want this as a way
of testing that I'm reading from the right registers and that these
counters are running. A debugfs interface seemed like a better and more
widely useful way to achieve that than implementing some one-off hack to
poll those registers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clocksource: Add debugfs support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

On Wed, Apr 01, 2020 at 12:29:17AM +0200, Thierry Reding wrote:
> On Wed, Apr 01, 2020 at 12:06:37AM +0200, Thomas Gleixner wrote:
> > Thierry,
> > 
> > Thierry Reding <thierry.reding@gmail.com> writes:
> > > Add a top-level "clocksource" directory to debugfs. For each clocksource
> > > registered with the system, a subdirectory will be added with attributes
> > > that can be queried to obtain information about the clocksource.
> > 
> > first of all this does tell what this patch does but omits the more
> > important information about the WHY.
> > 
> > What's even worse is that the changelog is blantantly wrong.
> > 
> > > +static int clocksource_debugfs_counter_show(struct seq_file *s, void *data)
> > > +{
> > > +	struct clocksource *cs = s->private;
> > > +
> > > +	seq_printf(s, "%llu\n", cs->read(cs));
> > > +
> > > +	return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(clocksource_debugfs_counter);
> > > +
> > > +static void clocksource_debugfs_add(struct clocksource *cs)
> > > +{
> > > +	if (!debugfs_root)
> > > +		return;
> > > +
> > > +	cs->debugfs = debugfs_create_dir(cs->name, debugfs_root);
> > > +
> > > +	debugfs_create_file("counter", 0444, cs->debugfs, cs,
> > > +			    &clocksource_debugfs_counter_fops);
> > > +}
> > 
> > It does not provide any information about the clocksource, it provides
> > an interface to read the counter - nothing else.
> 
> The counter is part of the information about a clocksource, isn't it?
> But yes, frankly I had anticipated that I'd be adding more files here
> and when I ended up not doing that I forgot to update the patch
> description.

Looking again, I suppose I could add a couple of the fields from struct
clocksource to this as well. The rating seems like it would be useful,
as well as perhaps the mult (and maxadj) and shift fields, which would
give users an easy way of converting the counter value to nanoseconds.
max_idle_ns, max_cycles and mask seem too low-level and are really only
useful for the kernel to deal with the clocksource.

"flags" might also be interesting.

Any other suggestions?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clocksource: Add debugfs support

[Thomas Gleixner]

Thierry Reding <thierry.reding@gmail.com> writes:
> On Wed, Apr 01, 2020 at 12:06:37AM +0200, Thomas Gleixner wrote:
>> It does not provide any information about the clocksource, it provides
>> an interface to read the counter - nothing else.
>
> The counter is part of the information about a clocksource, isn't it?

Sorry to be pedantic, but no. Information about a clocksource is the
name, the type, the frequency, bitwidth etc.

The counter file is not providing information about the
clocksource. It's exposing an accessor to the clocksource itself.

> I can also add some information about what I intend to use this for,
> though it'll be a bit boring because I really only want this as a way
> of testing that I'm reading from the right registers and that these
> counters are running. A debugfs interface seemed like a better and more
> widely useful way to achieve that than implementing some one-off hack to
> poll those registers.

But how much value has this interface beyond the 'hack a driver for a
new clocksource' experience?

To me none, but that might be my personal skewed view.

Thanks,

        tglx

Re: [PATCH] clocksource: Add debugfs support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Wed, Apr 01, 2020 at 01:01:49AM +0200, Thomas Gleixner wrote:
> Thierry Reding <thierry.reding@gmail.com> writes:
> > On Wed, Apr 01, 2020 at 12:06:37AM +0200, Thomas Gleixner wrote:
> >> It does not provide any information about the clocksource, it provides
> >> an interface to read the counter - nothing else.
> >
> > The counter is part of the information about a clocksource, isn't it?
> 
> Sorry to be pedantic, but no. Information about a clocksource is the
> name, the type, the frequency, bitwidth etc.
> 
> The counter file is not providing information about the
> clocksource. It's exposing an accessor to the clocksource itself.

Okay, fair enough.

> > I can also add some information about what I intend to use this for,
> > though it'll be a bit boring because I really only want this as a way
> > of testing that I'm reading from the right registers and that these
> > counters are running. A debugfs interface seemed like a better and more
> > widely useful way to achieve that than implementing some one-off hack to
> > poll those registers.
> 
> But how much value has this interface beyond the 'hack a driver for a
> new clocksource' experience?
> 
> To me none, but that might be my personal skewed view.

No, that's the only intended purpose. I just thought that would be nicer
than everyone having to write their own debug messages to do the same
thing.

If nobody else thinks this is useful I'll just stash it somewhere in
case I ever need it again.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]