drivers/of/Kconfig | 9 +++++ drivers/of/Makefile | 1 + drivers/of/base.c | 2 + drivers/of/debug.c | 83 +++++++++++++++++++++++++++++++++++++++++ drivers/of/of_private.h | 6 +++ include/linux/of.h | 3 ++ 6 files changed, 104 insertions(+) create mode 100644 drivers/of/debug.c
This is a proof-of-concept patch that introduces a debug feature I find
particularly useful. I frequently encounter situations where I'm
uncertain if my device tree configuration is correct or being utilized
by the kernel. This is especially common when porting device trees
from vendor kernels, as some properties may have slightly different
names in the upstream kernel, or upstream drivers may not use certain
properties at all.
By writing 'y' to <debugfs>/of_mark_queried, every queried device tree
property will gain S_IWUSR in sysfs. While abusing S_IWUSR is
admittedly a crude hack, it works for now. I'm open to better ideas,
perhaps using an xattr?
That way, dtc can easily add an annotation to unused device trees when
reading from /proc/device-tree.
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/of/Kconfig | 9 +++++
drivers/of/Makefile | 1 +
drivers/of/base.c | 2 +
drivers/of/debug.c | 83 +++++++++++++++++++++++++++++++++++++++++
drivers/of/of_private.h | 6 +++
include/linux/of.h | 3 ++
6 files changed, 104 insertions(+)
create mode 100644 drivers/of/debug.c
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 0e2d608c3e207..39079ab9f1dc9 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -90,6 +90,15 @@ config OF_IRQ
def_bool y
depends on !SPARC && IRQ_DOMAIN
+config OF_DEBUG
+ bool "Device Tree debug features"
+ select DEBUG_FS
+ help
+ This option enables device tree debug features.
+ Currently only <debugfs>/of_mark_queried, writing 'y' to this file
+ causes setting S_IWUSR on each device tree property in sysfs that
+ was queried by a device driver. This is useful to find dead properties.
+
config OF_RESERVED_MEM
def_bool OF_EARLY_FLATTREE
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 379a0afcbdc0b..041502125e897 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -25,3 +25,4 @@ obj-$(CONFIG_OF_OVERLAY_KUNIT_TEST) += overlay-test.o
overlay-test-y := overlay_test.o kunit_overlay_test.dtbo.o
obj-$(CONFIG_OF_UNITTEST) += unittest-data/
+obj-$(CONFIG_OF_DEBUG) += debug.o
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20603d3c9931b..00807da2187aa 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -202,6 +202,8 @@ static struct property *__of_find_property(const struct device_node *np,
if (of_prop_cmp(pp->name, name) == 0) {
if (lenp)
*lenp = pp->length;
+ of_debug_mark_queried(pp);
+
break;
}
}
diff --git a/drivers/of/debug.c b/drivers/of/debug.c
new file mode 100644
index 0000000000000..ceb88062e9dec
--- /dev/null
+++ b/drivers/of/debug.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/debugfs.h>
+#include <linux/kstrtox.h>
+#include <linux/of.h>
+
+#include "of_private.h"
+
+void of_debug_mark_queried(struct property *pp)
+{
+ pp->queried = true;
+}
+
+static int dtmq_update_node_sysfs(struct device_node *np)
+{
+ struct property *pp;
+ int ret = 0;
+
+ if (!IS_ENABLED(CONFIG_SYSFS) || !of_kset)
+ goto out;
+
+ for_each_property_of_node(np, pp) {
+ if (pp->queried) {
+ ret = sysfs_chmod_file(&np->kobj, &pp->attr.attr,
+ pp->attr.attr.mode | S_IWUSR);
+ if (ret)
+ break;
+ }
+ }
+
+out:
+ return ret;
+}
+
+static int dtmq_update_sysfs(void)
+{
+ struct device_node *np;
+ int ret = 0;
+
+ mutex_lock(&of_mutex);
+ for_each_of_allnodes(np) {
+ ret = dtmq_update_node_sysfs(np);
+ if (ret)
+ break;
+ }
+ mutex_unlock(&of_mutex);
+
+ return ret;
+}
+
+static ssize_t dtmq_file_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ bool do_it;
+ int ret;
+
+ ret = kstrtobool_from_user(user_buf, count, &do_it);
+ if (ret)
+ goto out;
+
+ if (do_it) {
+ ret = dtmq_update_sysfs();
+ if (!ret)
+ ret = count;
+ } else {
+ ret = -EINVAL;
+ }
+
+out:
+ return ret;
+}
+
+static const struct file_operations dtmq_fops = {
+ .write = dtmq_file_write,
+ .open = simple_open,
+ .owner = THIS_MODULE,
+};
+
+static int __init of_debug_init(void)
+{
+ return PTR_ERR_OR_ZERO(debugfs_create_file("of_mark_queried", 0644, NULL, NULL,
+ &dtmq_fops));
+}
+late_initcall(of_debug_init);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 04aa2a91f851a..55a21ef292064 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -184,4 +184,10 @@ void fdt_init_reserved_mem(void);
bool of_fdt_device_is_available(const void *blob, unsigned long node);
+#if defined(CONFIG_OF_DEBUG)
+void of_debug_mark_queried(struct property *pp);
+#else
+static inline void of_debug_mark_queried(struct property *pp) { }
+#endif
+
#endif /* _LINUX_OF_PRIVATE_H */
diff --git a/include/linux/of.h b/include/linux/of.h
index 85b60ac9eec50..3b7afa252fca3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -39,6 +39,9 @@ struct property {
#if defined(CONFIG_OF_KOBJ)
struct bin_attribute attr;
#endif
+#if defined(CONFIG_OF_DEBUG)
+ bool queried;
+#endif
};
#if defined(CONFIG_SPARC)
--
2.35.3On Sun, Oct 13, 2024 at 10:07:30PM +0200, Richard Weinberger wrote: > This is a proof-of-concept patch that introduces a debug feature I find > particularly useful. I frequently encounter situations where I'm > uncertain if my device tree configuration is correct or being utilized > by the kernel. This is especially common when porting device trees > from vendor kernels, as some properties may have slightly different > names in the upstream kernel, or upstream drivers may not use certain > properties at all. In general I don't mind, but I have a comment about above rationale. It's just wrong. The point of DT is to describe hardware, not the one given, fixed in time implementation. What's more, writing bindings mentions this explicit: make binding complete, even if it is not used. Best regards, Krzysztof
Krzysztof, Am Montag, 14. Oktober 2024, 09:49:14 CEST schrieb 'Krzysztof Kozlowski' via upstream: > On Sun, Oct 13, 2024 at 10:07:30PM +0200, Richard Weinberger wrote: > > This is a proof-of-concept patch that introduces a debug feature I find > > particularly useful. I frequently encounter situations where I'm > > uncertain if my device tree configuration is correct or being utilized > > by the kernel. This is especially common when porting device trees > > from vendor kernels, as some properties may have slightly different > > names in the upstream kernel, or upstream drivers may not use certain > > properties at all. > > In general I don't mind, but I have a comment about above rationale. > It's just wrong. The point of DT is to describe hardware, not the one > given, fixed in time implementation. I agree with you, sorry for being imprecise. > What's more, writing bindings mentions this explicit: make binding > complete, even if it is not used. Yes, with this aid, it is IMHO easier to find bindings that need attention. Just as an example, lately the device tree of a vendor used the property "timers", but in mainline it is "ti,timers". With this debug feature, it is easy to see that "timers" is not being used, and somebody has to decide whether the property is really not used by a driver, or if the binding needs more work. Thanks, //richard -- sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT UID/VAT Nr: ATU 66964118 | FN: 374287y
On Mon, Oct 14, 2024 at 3:51 AM Richard Weinberger <richard@sigma-star.at> wrote: > > Krzysztof, > > Am Montag, 14. Oktober 2024, 09:49:14 CEST schrieb 'Krzysztof Kozlowski' via upstream: > > On Sun, Oct 13, 2024 at 10:07:30PM +0200, Richard Weinberger wrote: > > > This is a proof-of-concept patch that introduces a debug feature I find > > > particularly useful. I frequently encounter situations where I'm > > > uncertain if my device tree configuration is correct or being utilized > > > by the kernel. This is especially common when porting device trees > > > from vendor kernels, as some properties may have slightly different > > > names in the upstream kernel, or upstream drivers may not use certain > > > properties at all. > > > > In general I don't mind, but I have a comment about above rationale. > > It's just wrong. The point of DT is to describe hardware, not the one > > given, fixed in time implementation. > > I agree with you, sorry for being imprecise. > > > What's more, writing bindings mentions this explicit: make binding > > complete, even if it is not used. > > Yes, with this aid, it is IMHO easier to find bindings that need attention. > Just as an example, lately the device tree of a vendor used the property "timers", > but in mainline it is "ti,timers". With this debug feature, it is easy to see that > "timers" is not being used, and somebody has to decide whether the property is > really not used by a driver, or if the binding needs more work. Paying attention to the schema warnings would have found this issue. Assuming there is a schema for the node... That's not to say this type of run-time check is not also useful. Rob
On Sun, Oct 13, 2024 at 1:07 PM Richard Weinberger <richard@nod.at> wrote:
>
> This is a proof-of-concept patch that introduces a debug feature I find
> particularly useful. I frequently encounter situations where I'm
> uncertain if my device tree configuration is correct or being utilized
> by the kernel. This is especially common when porting device trees
> from vendor kernels, as some properties may have slightly different
> names in the upstream kernel, or upstream drivers may not use certain
> properties at all.
Why not just add debug logs? You can print the full path of the
properties being read and it should be easy to grep for the property
you care about.
> By writing 'y' to <debugfs>/of_mark_queried, every queried device tree
A lot of querying is going to happen at boot time. So, I'm not sure if
this method of enabling it is helpful. If we do this, make it a kernel
command line.
> property will gain S_IWUSR in sysfs. While abusing S_IWUSR is
> admittedly a crude hack, it works for now. I'm open to better ideas,
> perhaps using an xattr?
This seems quite convoluted. Why not just add another file per node
that lists all the queried properties?
> That way, dtc can easily add an annotation to unused device trees when
> reading from /proc/device-tree.
I'm not too familiar with this part. Can you elaborate more?
-Saravana
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> drivers/of/Kconfig | 9 +++++
> drivers/of/Makefile | 1 +
> drivers/of/base.c | 2 +
> drivers/of/debug.c | 83 +++++++++++++++++++++++++++++++++++++++++
> drivers/of/of_private.h | 6 +++
> include/linux/of.h | 3 ++
> 6 files changed, 104 insertions(+)
> create mode 100644 drivers/of/debug.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 0e2d608c3e207..39079ab9f1dc9 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -90,6 +90,15 @@ config OF_IRQ
> def_bool y
> depends on !SPARC && IRQ_DOMAIN
>
> +config OF_DEBUG
> + bool "Device Tree debug features"
> + select DEBUG_FS
> + help
> + This option enables device tree debug features.
> + Currently only <debugfs>/of_mark_queried, writing 'y' to this file
> + causes setting S_IWUSR on each device tree property in sysfs that
> + was queried by a device driver. This is useful to find dead properties.
> +
> config OF_RESERVED_MEM
> def_bool OF_EARLY_FLATTREE
>
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 379a0afcbdc0b..041502125e897 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_OF_OVERLAY_KUNIT_TEST) += overlay-test.o
> overlay-test-y := overlay_test.o kunit_overlay_test.dtbo.o
>
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-$(CONFIG_OF_DEBUG) += debug.o
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 20603d3c9931b..00807da2187aa 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -202,6 +202,8 @@ static struct property *__of_find_property(const struct device_node *np,
> if (of_prop_cmp(pp->name, name) == 0) {
> if (lenp)
> *lenp = pp->length;
> + of_debug_mark_queried(pp);
> +
> break;
> }
> }
> diff --git a/drivers/of/debug.c b/drivers/of/debug.c
> new file mode 100644
> index 0000000000000..ceb88062e9dec
> --- /dev/null
> +++ b/drivers/of/debug.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/debugfs.h>
> +#include <linux/kstrtox.h>
> +#include <linux/of.h>
> +
> +#include "of_private.h"
> +
> +void of_debug_mark_queried(struct property *pp)
> +{
> + pp->queried = true;
> +}
> +
> +static int dtmq_update_node_sysfs(struct device_node *np)
> +{
> + struct property *pp;
> + int ret = 0;
> +
> + if (!IS_ENABLED(CONFIG_SYSFS) || !of_kset)
> + goto out;
> +
> + for_each_property_of_node(np, pp) {
> + if (pp->queried) {
> + ret = sysfs_chmod_file(&np->kobj, &pp->attr.attr,
> + pp->attr.attr.mode | S_IWUSR);
> + if (ret)
> + break;
> + }
> + }
> +
> +out:
> + return ret;
> +}
> +
> +static int dtmq_update_sysfs(void)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + mutex_lock(&of_mutex);
> + for_each_of_allnodes(np) {
> + ret = dtmq_update_node_sysfs(np);
> + if (ret)
> + break;
> + }
> + mutex_unlock(&of_mutex);
> +
> + return ret;
> +}
> +
> +static ssize_t dtmq_file_write(struct file *file, const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + bool do_it;
> + int ret;
> +
> + ret = kstrtobool_from_user(user_buf, count, &do_it);
> + if (ret)
> + goto out;
> +
> + if (do_it) {
> + ret = dtmq_update_sysfs();
> + if (!ret)
> + ret = count;
> + } else {
> + ret = -EINVAL;
> + }
> +
> +out:
> + return ret;
> +}
> +
> +static const struct file_operations dtmq_fops = {
> + .write = dtmq_file_write,
> + .open = simple_open,
> + .owner = THIS_MODULE,
> +};
> +
> +static int __init of_debug_init(void)
> +{
> + return PTR_ERR_OR_ZERO(debugfs_create_file("of_mark_queried", 0644, NULL, NULL,
> + &dtmq_fops));
> +}
> +late_initcall(of_debug_init);
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 04aa2a91f851a..55a21ef292064 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -184,4 +184,10 @@ void fdt_init_reserved_mem(void);
>
> bool of_fdt_device_is_available(const void *blob, unsigned long node);
>
> +#if defined(CONFIG_OF_DEBUG)
> +void of_debug_mark_queried(struct property *pp);
> +#else
> +static inline void of_debug_mark_queried(struct property *pp) { }
> +#endif
> +
> #endif /* _LINUX_OF_PRIVATE_H */
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 85b60ac9eec50..3b7afa252fca3 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -39,6 +39,9 @@ struct property {
> #if defined(CONFIG_OF_KOBJ)
> struct bin_attribute attr;
> #endif
> +#if defined(CONFIG_OF_DEBUG)
> + bool queried;
> +#endif
> };
>
> #if defined(CONFIG_SPARC)
> --
> 2.35.3
>
Saravana,
Am Montag, 14. Oktober 2024, 04:37:10 CEST schrieb 'Saravana Kannan' via upstream:
> On Sun, Oct 13, 2024 at 1:07 PM Richard Weinberger <richard@nod.at> wrote:
> >
> > This is a proof-of-concept patch that introduces a debug feature I find
> > particularly useful. I frequently encounter situations where I'm
> > uncertain if my device tree configuration is correct or being utilized
> > by the kernel. This is especially common when porting device trees
> > from vendor kernels, as some properties may have slightly different
> > names in the upstream kernel, or upstream drivers may not use certain
> > properties at all.
>
> Why not just add debug logs? You can print the full path of the
> properties being read and it should be easy to grep for the property
> you care about.
This approach works only well when I know what property I care about.
The problem I'm addressing is more like importing a device tree (usually
something wonky from vendor tree) and want check whether all properties
stated in the device tree get actually used.
If something is not being used, I need to investigate...
>
> > By writing 'y' to <debugfs>/of_mark_queried, every queried device tree
>
> A lot of querying is going to happen at boot time. So, I'm not sure if
> this method of enabling it is helpful. If we do this, make it a kernel
> command line.
It works differently.
As soon CONFIG_OF_DEBUG is enabled, every queried property gets ->queried
set to true.
Writing to of_mark_queried updates just the sysfs representation of the
device tree.
That way one can analyze the tree also step by step.
E.g. learning what properties have been queried while bootup and later,
what was queried after loading kernel modules.
>
> > property will gain S_IWUSR in sysfs. While abusing S_IWUSR is
> > admittedly a crude hack, it works for now. I'm open to better ideas,
> > perhaps using an xattr?
>
> This seems quite convoluted. Why not just add another file per node
> that lists all the queried properties?
AFAICT, adding another file will confuse dtc since dtc treats every file
in sysfs as property.
>
> > That way, dtc can easily add an annotation to unused device trees when
> > reading from /proc/device-tree.
>
> I'm not too familiar with this part. Can you elaborate more?
E.g.
$ ./dtc/dtc -I fs -O dts /proc/device-tree
...
intc@8000000 {
...
v2m@8020000 {
msi-controller;
compatible = "arm,gic-v2m-frame";
reg = <0x00 0x8020000 0x00 0x1000>;
lala = "lulu"; /* not queried */
};
};
...
With my kernel patch and minimal changes to dtc, it is able to tell me that
nobody has read the property "lala" in my device tree.
Thanks,
//richard
--
sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y
© 2016 - 2024 Red Hat, Inc.