On Sun, Oct 13, 2024 at 1:07 PM Richard Weinberger <richard@xxxxxx> 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@xxxxxx>
> ---
> 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
>
