[PATCH v2 0/4] of: overlay: Assorted fixes

[PATCH v2 0/4] of: overlay: Assorted fixes

[Pantelis Antoniou]

The first patch makes sure that no overlays are being left over from
the unit tests.

The second puts the overlays as objects in the sysfs in
/sys/firmware/devicetree/overlays while the next one adds a master
overlay enable switch (that once is set to disabled can't be re-enabled),
and a few per-overlay attributes.

The last patch updates the ABI docs for the sysfs entries.

The patchset is against Rob Herring's tree at
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux branch for-next

Changes since v1:
* Maintainer requested changes.
* Documented the sysfs entries
* Per overlay sysfs attributes.

Pantelis Antoniou (4):
  of: unittest: overlay: Keep track of created overlays
  of: overlay: kobjectify overlay objects
  of: overlay: Add sysfs attributes
  Documentation: ABI: /sys/firmware/devicetree/overlays

 .../ABI/testing/sysfs-firmware-devicetree-overlays |  23 +++
 drivers/of/base.c                                  |   5 +
 drivers/of/of_private.h                            |   9 +
 drivers/of/overlay.c                               | 217 ++++++++++++++++++++-
 drivers/of/unittest.c                              |  62 ++++++
 5 files changed, 314 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays

-- 
1.7.12

[PATCH v2 1/4] of: unittest: overlay: Keep track of created overlays

[Pantelis Antoniou]

During the course of the overlay selftests some of them remain
applied. While this does not pose a real problem, make sure you track
them and destroy them at the end of the test.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/unittest.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index fdb5977..995cc73 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -23,6 +23,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 
+#include <linux/bitops.h>
+
 #include "of_private.h"
 
 static struct unittest_results {
@@ -1095,6 +1097,59 @@ static const char *overlay_path(int nr)
 
 static const char *bus_path = "/testcase-data/overlay-node/test-bus";
 
+/* it is guaranteed that overlay ids are assigned in sequence */
+#define MAX_UNITTEST_OVERLAYS	256
+static unsigned long overlay_id_bits[BITS_TO_LONGS(MAX_UNITTEST_OVERLAYS)];
+static int overlay_first_id = -1;
+
+static void of_unittest_track_overlay(int id)
+{
+	if (overlay_first_id < 0)
+		overlay_first_id = id;
+	id -= overlay_first_id;
+
+	/* we shouldn't need that many */
+	BUG_ON(id >= MAX_UNITTEST_OVERLAYS);
+	overlay_id_bits[BIT_WORD(id)] |= BIT_MASK(id);
+}
+
+static void of_unittest_untrack_overlay(int id)
+{
+	if (overlay_first_id < 0)
+		return;
+	id -= overlay_first_id;
+	BUG_ON(id >= MAX_UNITTEST_OVERLAYS);
+	overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
+}
+
+static void of_unittest_destroy_tracked_overlays(void)
+{
+	int id, ret, defers;
+
+	if (overlay_first_id < 0)
+		return;
+
+	/* try until no defers */
+	do {
+		defers = 0;
+		/* remove in reverse order */
+		for (id = MAX_UNITTEST_OVERLAYS - 1; id >= 0; id--) {
+			if (!(overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id)))
+				continue;
+
+			ret = of_overlay_destroy(id + overlay_first_id);
+			if (ret != 0) {
+				defers++;
+				pr_warn("%s: overlay destroy failed for #%d\n",
+					__func__, id + overlay_first_id);
+				continue;
+			}
+
+			overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
+		}
+	} while (defers > 0);
+}
+
 static int of_unittest_apply_overlay(int unittest_nr, int overlay_nr,
 		int *overlay_id)
 {
@@ -1116,6 +1171,7 @@ static int of_unittest_apply_overlay(int unittest_nr, int overlay_nr,
 		goto out;
 	}
 	id = ret;
+	of_unittest_track_overlay(id);
 
 	ret = 0;
 
@@ -1329,6 +1385,7 @@ static void of_unittest_overlay_6(void)
 			return;
 		}
 		ov_id[i] = ret;
+		of_unittest_track_overlay(ov_id[i]);
 	}
 
 	for (i = 0; i < 2; i++) {
@@ -1353,6 +1410,7 @@ static void of_unittest_overlay_6(void)
 						PDEV_OVERLAY));
 			return;
 		}
+		of_unittest_untrack_overlay(ov_id[i]);
 	}
 
 	for (i = 0; i < 2; i++) {
@@ -1397,6 +1455,7 @@ static void of_unittest_overlay_8(void)
 			return;
 		}
 		ov_id[i] = ret;
+		of_unittest_track_overlay(ov_id[i]);
 	}
 
 	/* now try to remove first overlay (it should fail) */
@@ -1419,6 +1478,7 @@ static void of_unittest_overlay_8(void)
 						PDEV_OVERLAY));
 			return;
 		}
+		of_unittest_untrack_overlay(ov_id[i]);
 	}
 
 	unittest(1, "overlay test %d passed\n", 8);
@@ -1841,6 +1901,8 @@ static void __init of_unittest_overlay(void)
 	of_unittest_overlay_i2c_cleanup();
 #endif
 
+	of_unittest_destroy_tracked_overlays();
+
 out:
 	of_node_put(bus_np);
 }
-- 
1.7.12

[PATCH v2 2/4] of: overlay: kobjectify overlay objects

[Pantelis Antoniou]

We are going to need the overlays to appear on sysfs with runtime
global properties (like master enable) so turn them into kobjects.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/base.c       |  5 +++++
 drivers/of/of_private.h |  9 +++++++++
 drivers/of/overlay.c    | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 69566b6..808f351 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -192,6 +192,7 @@ int __of_attach_node_sysfs(struct device_node *np)
 static int __init of_init(void)
 {
 	struct device_node *np;
+	int ret;
 
 	/* Create the kset, and register existing nodes */
 	mutex_lock(&of_mutex);
@@ -208,6 +209,10 @@ static int __init of_init(void)
 	if (of_root)
 		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
 
+	ret = of_overlay_init();
+	if (ret != 0)
+		pr_warn("of_init: of_overlay_init failed!\n");
+
 	return 0;
 }
 core_initcall(of_init);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8e882e7..120eb44 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -90,4 +90,13 @@ extern void __of_detach_node_sysfs(struct device_node *np);
 #define for_each_transaction_entry_reverse(_oft, _te) \
 	list_for_each_entry_reverse(_te, &(_oft)->te_list, node)
 
+#if defined(CONFIG_OF_OVERLAY)
+extern int of_overlay_init(void);
+#else
+static inline int of_overlay_init(void)
+{
+	return 0;
+}
+#endif
+
 #endif /* _LINUX_OF_PRIVATE_H */
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index dee9270..f17f5ef 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/idr.h>
+#include <linux/sysfs.h>
 
 #include "of_private.h"
 
@@ -51,6 +52,7 @@ struct of_overlay {
 	int count;
 	struct of_overlay_info *ovinfo_tab;
 	struct of_changeset cset;
+	struct kobject kobj;
 };
 
 static int of_overlay_apply_one(struct of_overlay *ov,
@@ -325,6 +327,24 @@ static int of_free_overlay_info(struct of_overlay *ov)
 static LIST_HEAD(ov_list);
 static DEFINE_IDR(ov_idr);
 
+static inline struct of_overlay *kobj_to_overlay(struct kobject *kobj)
+{
+	return container_of(kobj, struct of_overlay, kobj);
+}
+
+void of_overlay_release(struct kobject *kobj)
+{
+	struct of_overlay *ov = kobj_to_overlay(kobj);
+
+	kfree(ov);
+}
+
+static struct kobj_type of_overlay_ktype = {
+	.release = of_overlay_release,
+};
+
+static struct kset *ov_kset;
+
 /**
  * of_overlay_create() - Create and apply an overlay
  * @tree:	Device node containing all the overlays
@@ -350,6 +370,9 @@ int of_overlay_create(struct device_node *tree)
 
 	of_changeset_init(&ov->cset);
 
+	/* initialize kobject */
+	kobject_init(&ov->kobj, &of_overlay_ktype);
+
 	mutex_lock(&of_mutex);
 
 	id = idr_alloc(&ov_idr, ov, 0, 0, GFP_KERNEL);
@@ -385,6 +408,14 @@ int of_overlay_create(struct device_node *tree)
 		goto err_revert_overlay;
 	}
 
+	ov->kobj.kset = ov_kset;
+	err = kobject_add(&ov->kobj, NULL, "%d", id);
+	if (err != 0) {
+		pr_err("%s: kobject_add() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_cancel_overlay;
+	}
+
 	/* add to the tail of the overlay list */
 	list_add_tail(&ov->node, &ov_list);
 
@@ -392,6 +423,8 @@ int of_overlay_create(struct device_node *tree)
 
 	return id;
 
+err_cancel_overlay:
+	of_changeset_revert(&ov->cset);
 err_revert_overlay:
 err_abort_trans:
 	of_free_overlay_info(ov);
@@ -512,7 +545,9 @@ int of_overlay_destroy(int id)
 	of_free_overlay_info(ov);
 	idr_remove(&ov_idr, id);
 	of_changeset_destroy(&ov->cset);
-	kfree(ov);
+
+	kobject_del(&ov->kobj);
+	kobject_put(&ov->kobj);
 
 	err = 0;
 
@@ -542,7 +577,8 @@ int of_overlay_destroy_all(void)
 		of_changeset_revert(&ov->cset);
 		of_free_overlay_info(ov);
 		idr_remove(&ov_idr, ov->id);
-		kfree(ov);
+		kobject_del(&ov->kobj);
+		kobject_put(&ov->kobj);
 	}
 
 	mutex_unlock(&of_mutex);
@@ -550,3 +586,15 @@ int of_overlay_destroy_all(void)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_overlay_destroy_all);
+
+/* called from of_init() */
+int of_overlay_init(void)
+{
+	int rc;
+
+	ov_kset = kset_create_and_add("overlays", NULL, &of_kset->kobj);
+	if (!ov_kset)
+		return -ENOMEM;
+
+	return 0;
+}
-- 
1.7.12

[PATCH v2 3/4] of: overlay: Add sysfs attributes

[Pantelis Antoniou]

Implement a number of sysfs attributes for overlays.

* A throw once master enable switch to protect against any
further overlay applications if the administrator desires so.

* A per overlay targets sysfs attribute listing the targets of
the installed overlay.

* A per overlay can_remove sysfs attribute that reports whether
the overlay can be removed or not due to another overlapping overlay.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 166 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f17f5ef..c54d097 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -21,6 +21,7 @@
 #include <linux/err.h>
 #include <linux/idr.h>
 #include <linux/sysfs.h>
+#include <linux/atomic.h>
 
 #include "of_private.h"
 
@@ -55,8 +56,12 @@ struct of_overlay {
 	struct kobject kobj;
 };
 
+/* master enable switch; once set to 0 can't be re-enabled */
+static atomic_t ov_enable = ATOMIC_INIT(1);
+
 static int of_overlay_apply_one(struct of_overlay *ov,
 		struct device_node *target, const struct device_node *overlay);
+static int overlay_removal_is_ok(struct of_overlay *ov);
 
 static int of_overlay_apply_single_property(struct of_overlay *ov,
 		struct device_node *target, struct property *prop)
@@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
 
 static struct kset *ov_kset;
 
+static ssize_t enable_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *bin_attr, char *buf,
+		loff_t offset, size_t count)
+{
+	char tbuf[3];
+
+	if (offset < 0)
+		return -EINVAL;
+
+	if (offset >= sizeof(tbuf))
+		return 0;
+
+	if (count > sizeof(tbuf) - offset)
+		count = sizeof(tbuf) - offset;
+
+	/* fill in temp */
+	tbuf[0] = '0' + atomic_read(&ov_enable);
+	tbuf[1] = '\n';
+	tbuf[2] = '\0';
+
+	/* copy to buffer */
+	memcpy(buf, tbuf + offset, count);
+
+	return count;
+}
+
+static ssize_t enable_write(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *bin_attr, char *buf,
+		loff_t off, size_t count)
+{
+	unsigned int new_enable;
+
+	if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
+		return -EINVAL;
+
+	new_enable = (unsigned int)(buf[0] - '0');
+	if (new_enable > 1)
+		return -EINVAL;
+
+	/* NOP for same value */
+	if (new_enable == atomic_read(&ov_enable))
+		return count;
+
+	/* if we've disabled it, no going back */
+	if (atomic_read(&ov_enable) == 0)
+		return -EPERM;
+
+	atomic_set(&ov_enable, new_enable);
+	return count;
+}
+
+/* just a single char + '\n' + '\0' */
+static BIN_ATTR_RW(enable, 3);
+
+static ssize_t targets_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *bin_attr, char *buf,
+		loff_t offset, size_t count)
+{
+	struct of_overlay *ov = kobj_to_overlay(kobj);
+	struct of_overlay_info *ovinfo;
+	char *tmpbuf, *s, *e;
+	const char *name;
+	ssize_t ret;
+	int i, len;
+
+	/* allocate work buffer; we know that PAGE_SIZE is enough */
+	tmpbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (tmpbuf == NULL)
+		return -ENOMEM;
+
+	s = tmpbuf;
+	e = tmpbuf + PAGE_SIZE;
+
+	mutex_lock(&of_mutex);
+
+	/* targets */
+	for (i = 0; i < ov->count; i++) {
+		ovinfo = &ov->ovinfo_tab[i];
+
+		name = of_node_full_name(ovinfo->target);
+		len = strlen(name);
+		if (s + len + 1 >= e)
+			return -ENOMEM;
+		memcpy(s, name, len);
+		s += len;
+		*s++ = '\n';
+	}
+	if (s + 1 >= e)
+		return -ENOMEM;
+	*s++ = '\0';
+
+	/* the buffer is zero terminated */
+	len = s - tmpbuf;
+
+	mutex_unlock(&of_mutex);
+
+	/* perform the read */
+	ret = memory_read_from_buffer(buf, count, &offset, tmpbuf, len);
+
+	/* free the temporary buffer */
+	kfree(tmpbuf);
+
+	return ret;
+}
+
+/* targets property */
+static BIN_ATTR_RO(targets, PAGE_SIZE);
+
+static ssize_t can_remove_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *bin_attr, char *buf,
+		loff_t offset, size_t count)
+{
+	struct of_overlay *ov = kobj_to_overlay(kobj);
+	char tbuf[3];
+
+	if (offset < 0)
+		return -EINVAL;
+
+	if (offset >= sizeof(tbuf))
+		return 0;
+
+	if (count > sizeof(tbuf) - offset)
+		count = sizeof(tbuf) - offset;
+
+	/* fill in temp */
+	tbuf[0] = '0' + overlay_removal_is_ok(ov);
+	tbuf[1] = '\n';
+	tbuf[2] = '\0';
+
+	/* copy to buffer */
+	memcpy(buf, tbuf + offset, count);
+
+	return count;
+}
+
+/* can_remove property */
+static BIN_ATTR_RO(can_remove, 3);
+
 /**
  * of_overlay_create() - Create and apply an overlay
  * @tree:	Device node containing all the overlays
@@ -360,6 +503,10 @@ int of_overlay_create(struct device_node *tree)
 	struct of_overlay *ov;
 	int err, id;
 
+	/* administratively disabled */
+	if (!atomic_read(&ov_enable))
+		return -EPERM;
+
 	/* allocate the overlay structure */
 	ov = kzalloc(sizeof(*ov), GFP_KERNEL);
 	if (ov == NULL)
@@ -416,6 +563,22 @@ int of_overlay_create(struct device_node *tree)
 		goto err_cancel_overlay;
 	}
 
+	/* create targets file */
+	err = sysfs_create_bin_file(&ov->kobj, &bin_attr_targets);
+	if (err != 0) {
+		pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_cancel_overlay;
+	}
+
+	/* create can_remove file */
+	err = sysfs_create_bin_file(&ov->kobj, &bin_attr_can_remove);
+	if (err != 0) {
+		pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_cancel_overlay;
+	}
+
 	/* add to the tail of the overlay list */
 	list_add_tail(&ov->node, &ov_list);
 
@@ -596,5 +759,7 @@ int of_overlay_init(void)
 	if (!ov_kset)
 		return -ENOMEM;
 
-	return 0;
+	rc = sysfs_create_bin_file(&ov_kset->kobj, &bin_attr_enable);
+	WARN(rc, "%s: error adding enable attribute\n", __func__);
+	return rc;
 }
-- 
1.7.12

[PATCH v2 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Pantelis Antoniou]

Documentation ABI entry for overlays sysfs entries.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 .../ABI/testing/sysfs-firmware-devicetree-overlays | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays

diff --git a/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
new file mode 100644
index 0000000..6b81f1c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
@@ -0,0 +1,23 @@
+What:		/sys/firmware/devicetree/overlays/
+Date:		March 2015
+Contact:	Pantelis Antoniou <pantelis.antoniou@konsulko.com>
+Description:
+		This directory contains the applied device tree overlays of
+		the running system, as directories of the overlay id.
+
+		enable: The master enable switch, by default is 1, and when
+		        set to 0 it cannot be re-enabled for security reasons.
+
+What:		/sys/firmware/devicetree/overlays/<id>
+Date:		March 2015
+Contact:	Pantelis Antoniou <pantelis.antoniou@konsulko.com>
+Description:
+		Each directory represents an applied overlay, containing
+		the following attribute files.
+
+		targets: A file containing the list of targets of each overlay
+		         with each line containing a target.
+
+		can_remove: The attribute set to 1 means that the overlay can
+		            be removed, while 0 means that the overlay is being
+			    overlapped therefore removal is prohibited.
-- 
1.7.12

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Rob Herring]

On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Implement a number of sysfs attributes for overlays.
>
> * A throw once master enable switch to protect against any
> further overlay applications if the administrator desires so.

This one should be a separate patch.

> * A per overlay targets sysfs attribute listing the targets of
> the installed overlay.

What are targets? "targets lists targets" does not help me. The
documentation doesn't help me either.

> * A per overlay can_remove sysfs attribute that reports whether
> the overlay can be removed or not due to another overlapping overlay.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 166 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f17f5ef..c54d097 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -21,6 +21,7 @@
>  #include <linux/err.h>
>  #include <linux/idr.h>
>  #include <linux/sysfs.h>
> +#include <linux/atomic.h>
>
>  #include "of_private.h"
>
> @@ -55,8 +56,12 @@ struct of_overlay {
>         struct kobject kobj;
>  };
>
> +/* master enable switch; once set to 0 can't be re-enabled */
> +static atomic_t ov_enable = ATOMIC_INIT(1);
> +
>  static int of_overlay_apply_one(struct of_overlay *ov,
>                 struct device_node *target, const struct device_node *overlay);
> +static int overlay_removal_is_ok(struct of_overlay *ov);
>
>  static int of_overlay_apply_single_property(struct of_overlay *ov,
>                 struct device_node *target, struct property *prop)
> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
>
>  static struct kset *ov_kset;
>
> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
> +               struct bin_attribute *bin_attr, char *buf,
> +               loff_t offset, size_t count)
> +{
> +       char tbuf[3];
> +
> +       if (offset < 0)
> +               return -EINVAL;
> +
> +       if (offset >= sizeof(tbuf))
> +               return 0;
> +
> +       if (count > sizeof(tbuf) - offset)
> +               count = sizeof(tbuf) - offset;
> +
> +       /* fill in temp */
> +       tbuf[0] = '0' + atomic_read(&ov_enable);
> +       tbuf[1] = '\n';
> +       tbuf[2] = '\0';
> +
> +       /* copy to buffer */
> +       memcpy(buf, tbuf + offset, count);
> +
> +       return count;
> +}
> +
> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
> +               struct bin_attribute *bin_attr, char *buf,
> +               loff_t off, size_t count)
> +{
> +       unsigned int new_enable;
> +
> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
> +               return -EINVAL;
> +
> +       new_enable = (unsigned int)(buf[0] - '0');
> +       if (new_enable > 1)
> +               return -EINVAL;
> +
> +       /* NOP for same value */
> +       if (new_enable == atomic_read(&ov_enable))
> +               return count;
> +
> +       /* if we've disabled it, no going back */
> +       if (atomic_read(&ov_enable) == 0)
> +               return -EPERM;
> +
> +       atomic_set(&ov_enable, new_enable);
> +       return count;
> +}
> +
> +/* just a single char + '\n' + '\0' */
> +static BIN_ATTR_RW(enable, 3);

Why are you using bin attribute? You are complicating the
implementation needlessly.

> +
> +static ssize_t targets_read(struct file *filp, struct kobject *kobj,
> +               struct bin_attribute *bin_attr, char *buf,
> +               loff_t offset, size_t count)
> +{
> +       struct of_overlay *ov = kobj_to_overlay(kobj);
> +       struct of_overlay_info *ovinfo;
> +       char *tmpbuf, *s, *e;
> +       const char *name;
> +       ssize_t ret;
> +       int i, len;
> +
> +       /* allocate work buffer; we know that PAGE_SIZE is enough */
> +       tmpbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +       if (tmpbuf == NULL)
> +               return -ENOMEM;
> +
> +       s = tmpbuf;
> +       e = tmpbuf + PAGE_SIZE;
> +
> +       mutex_lock(&of_mutex);
> +
> +       /* targets */
> +       for (i = 0; i < ov->count; i++) {
> +               ovinfo = &ov->ovinfo_tab[i];
> +
> +               name = of_node_full_name(ovinfo->target);
> +               len = strlen(name);
> +               if (s + len + 1 >= e)
> +                       return -ENOMEM;

Leaking memory here and holding the mutex.

> +               memcpy(s, name, len);
> +               s += len;
> +               *s++ = '\n';
> +       }
> +       if (s + 1 >= e)
> +               return -ENOMEM;

And here.

> +       *s++ = '\0';
> +
> +       /* the buffer is zero terminated */
> +       len = s - tmpbuf;
> +
> +       mutex_unlock(&of_mutex);
> +
> +       /* perform the read */
> +       ret = memory_read_from_buffer(buf, count, &offset, tmpbuf, len);
> +
> +       /* free the temporary buffer */
> +       kfree(tmpbuf);
> +
> +       return ret;
> +}
> +
> +/* targets property */
> +static BIN_ATTR_RO(targets, PAGE_SIZE);
> +
> +static ssize_t can_remove_read(struct file *filp, struct kobject *kobj,
> +               struct bin_attribute *bin_attr, char *buf,
> +               loff_t offset, size_t count)
> +{
> +       struct of_overlay *ov = kobj_to_overlay(kobj);
> +       char tbuf[3];
> +
> +       if (offset < 0)
> +               return -EINVAL;
> +
> +       if (offset >= sizeof(tbuf))
> +               return 0;
> +
> +       if (count > sizeof(tbuf) - offset)
> +               count = sizeof(tbuf) - offset;
> +
> +       /* fill in temp */
> +       tbuf[0] = '0' + overlay_removal_is_ok(ov);
> +       tbuf[1] = '\n';
> +       tbuf[2] = '\0';
> +
> +       /* copy to buffer */
> +       memcpy(buf, tbuf + offset, count);
> +
> +       return count;
> +}
> +
> +/* can_remove property */
> +static BIN_ATTR_RO(can_remove, 3);

Same question about bin attr here.

> +
>  /**
>   * of_overlay_create() - Create and apply an overlay
>   * @tree:      Device node containing all the overlays
> @@ -360,6 +503,10 @@ int of_overlay_create(struct device_node *tree)
>         struct of_overlay *ov;
>         int err, id;
>
> +       /* administratively disabled */
> +       if (!atomic_read(&ov_enable))
> +               return -EPERM;
> +
>         /* allocate the overlay structure */
>         ov = kzalloc(sizeof(*ov), GFP_KERNEL);
>         if (ov == NULL)
> @@ -416,6 +563,22 @@ int of_overlay_create(struct device_node *tree)
>                 goto err_cancel_overlay;
>         }
>
> +       /* create targets file */
> +       err = sysfs_create_bin_file(&ov->kobj, &bin_attr_targets);
> +       if (err != 0) {
> +               pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n",
> +                               __func__, tree->full_name);
> +               goto err_cancel_overlay;
> +       }
> +
> +       /* create can_remove file */
> +       err = sysfs_create_bin_file(&ov->kobj, &bin_attr_can_remove);
> +       if (err != 0) {
> +               pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n",
> +                               __func__, tree->full_name);
> +               goto err_cancel_overlay;
> +       }
> +
>         /* add to the tail of the overlay list */
>         list_add_tail(&ov->node, &ov_list);
>
> @@ -596,5 +759,7 @@ int of_overlay_init(void)
>         if (!ov_kset)
>                 return -ENOMEM;
>
> -       return 0;
> +       rc = sysfs_create_bin_file(&ov_kset->kobj, &bin_attr_enable);
> +       WARN(rc, "%s: error adding enable attribute\n", __func__);
> +       return rc;
>  }
> --
> 1.7.12
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] of: unittest: overlay: Keep track of created overlays

[Rob Herring]

On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> During the course of the overlay selftests some of them remain
> applied. While this does not pose a real problem, make sure you track
> them and destroy them at the end of the test.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

I've applied this one. Patch 2 is okay, but the rest still needs some work.

Rob

> ---
>  drivers/of/unittest.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index fdb5977..995cc73 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -23,6 +23,8 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>
> +#include <linux/bitops.h>
> +
>  #include "of_private.h"
>
>  static struct unittest_results {
> @@ -1095,6 +1097,59 @@ static const char *overlay_path(int nr)
>
>  static const char *bus_path = "/testcase-data/overlay-node/test-bus";
>
> +/* it is guaranteed that overlay ids are assigned in sequence */
> +#define MAX_UNITTEST_OVERLAYS  256
> +static unsigned long overlay_id_bits[BITS_TO_LONGS(MAX_UNITTEST_OVERLAYS)];
> +static int overlay_first_id = -1;
> +
> +static void of_unittest_track_overlay(int id)
> +{
> +       if (overlay_first_id < 0)
> +               overlay_first_id = id;
> +       id -= overlay_first_id;
> +
> +       /* we shouldn't need that many */
> +       BUG_ON(id >= MAX_UNITTEST_OVERLAYS);
> +       overlay_id_bits[BIT_WORD(id)] |= BIT_MASK(id);
> +}
> +
> +static void of_unittest_untrack_overlay(int id)
> +{
> +       if (overlay_first_id < 0)
> +               return;
> +       id -= overlay_first_id;
> +       BUG_ON(id >= MAX_UNITTEST_OVERLAYS);
> +       overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
> +}
> +
> +static void of_unittest_destroy_tracked_overlays(void)
> +{
> +       int id, ret, defers;
> +
> +       if (overlay_first_id < 0)
> +               return;
> +
> +       /* try until no defers */
> +       do {
> +               defers = 0;
> +               /* remove in reverse order */
> +               for (id = MAX_UNITTEST_OVERLAYS - 1; id >= 0; id--) {
> +                       if (!(overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id)))
> +                               continue;
> +
> +                       ret = of_overlay_destroy(id + overlay_first_id);
> +                       if (ret != 0) {
> +                               defers++;
> +                               pr_warn("%s: overlay destroy failed for #%d\n",
> +                                       __func__, id + overlay_first_id);
> +                               continue;
> +                       }
> +
> +                       overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
> +               }
> +       } while (defers > 0);
> +}
> +
>  static int of_unittest_apply_overlay(int unittest_nr, int overlay_nr,
>                 int *overlay_id)
>  {
> @@ -1116,6 +1171,7 @@ static int of_unittest_apply_overlay(int unittest_nr, int overlay_nr,
>                 goto out;
>         }
>         id = ret;
> +       of_unittest_track_overlay(id);
>
>         ret = 0;
>
> @@ -1329,6 +1385,7 @@ static void of_unittest_overlay_6(void)
>                         return;
>                 }
>                 ov_id[i] = ret;
> +               of_unittest_track_overlay(ov_id[i]);
>         }
>
>         for (i = 0; i < 2; i++) {
> @@ -1353,6 +1410,7 @@ static void of_unittest_overlay_6(void)
>                                                 PDEV_OVERLAY));
>                         return;
>                 }
> +               of_unittest_untrack_overlay(ov_id[i]);
>         }
>
>         for (i = 0; i < 2; i++) {
> @@ -1397,6 +1455,7 @@ static void of_unittest_overlay_8(void)
>                         return;
>                 }
>                 ov_id[i] = ret;
> +               of_unittest_track_overlay(ov_id[i]);
>         }
>
>         /* now try to remove first overlay (it should fail) */
> @@ -1419,6 +1478,7 @@ static void of_unittest_overlay_8(void)
>                                                 PDEV_OVERLAY));
>                         return;
>                 }
> +               of_unittest_untrack_overlay(ov_id[i]);
>         }
>
>         unittest(1, "overlay test %d passed\n", 8);
> @@ -1841,6 +1901,8 @@ static void __init of_unittest_overlay(void)
>         of_unittest_overlay_i2c_cleanup();
>  #endif
>
> +       of_unittest_destroy_tracked_overlays();
> +
>  out:
>         of_node_put(bus_np);
>  }
> --
> 1.7.12
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Pantelis Antoniou]

Hi Rob,

> On Apr 15, 2015, at 04:27 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> Implement a number of sysfs attributes for overlays.
>> 
>> * A throw once master enable switch to protect against any
>> further overlay applications if the administrator desires so.
> 
> This one should be a separate patch.
> 

OK.

>> * A per overlay targets sysfs attribute listing the targets of
>> the installed overlay.
> 
> What are targets? "targets lists targets" does not help me. The
> documentation doesn't help me either.
> 

It lists the targets of the overlay that has been applied. What do
you need in order to be helped? I mean what do you want listed?

>> * A per overlay can_remove sysfs attribute that reports whether
>> the overlay can be removed or not due to another overlapping overlay.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>> drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 166 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index f17f5ef..c54d097 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -21,6 +21,7 @@
>> #include <linux/err.h>
>> #include <linux/idr.h>
>> #include <linux/sysfs.h>
>> +#include <linux/atomic.h>
>> 
>> #include "of_private.h"
>> 
>> @@ -55,8 +56,12 @@ struct of_overlay {
>>        struct kobject kobj;
>> };
>> 
>> +/* master enable switch; once set to 0 can't be re-enabled */
>> +static atomic_t ov_enable = ATOMIC_INIT(1);
>> +
>> static int of_overlay_apply_one(struct of_overlay *ov,
>>                struct device_node *target, const struct device_node *overlay);
>> +static int overlay_removal_is_ok(struct of_overlay *ov);
>> 
>> static int of_overlay_apply_single_property(struct of_overlay *ov,
>>                struct device_node *target, struct property *prop)
>> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
>> 
>> static struct kset *ov_kset;
>> 
>> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
>> +               struct bin_attribute *bin_attr, char *buf,
>> +               loff_t offset, size_t count)
>> +{
>> +       char tbuf[3];
>> +
>> +       if (offset < 0)
>> +               return -EINVAL;
>> +
>> +       if (offset >= sizeof(tbuf))
>> +               return 0;
>> +
>> +       if (count > sizeof(tbuf) - offset)
>> +               count = sizeof(tbuf) - offset;
>> +
>> +       /* fill in temp */
>> +       tbuf[0] = '0' + atomic_read(&ov_enable);
>> +       tbuf[1] = '\n';
>> +       tbuf[2] = '\0';
>> +
>> +       /* copy to buffer */
>> +       memcpy(buf, tbuf + offset, count);
>> +
>> +       return count;
>> +}
>> +
>> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
>> +               struct bin_attribute *bin_attr, char *buf,
>> +               loff_t off, size_t count)
>> +{
>> +       unsigned int new_enable;
>> +
>> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
>> +               return -EINVAL;
>> +
>> +       new_enable = (unsigned int)(buf[0] - '0');
>> +       if (new_enable > 1)
>> +               return -EINVAL;
>> +
>> +       /* NOP for same value */
>> +       if (new_enable == atomic_read(&ov_enable))
>> +               return count;
>> +
>> +       /* if we've disabled it, no going back */
>> +       if (atomic_read(&ov_enable) == 0)
>> +               return -EPERM;
>> +
>> +       atomic_set(&ov_enable, new_enable);
>> +       return count;
>> +}
>> +
>> +/* just a single char + '\n' + '\0' */
>> +static BIN_ATTR_RW(enable, 3);
> 
> Why are you using bin attribute? You are complicating the
> implementation needlessly.
> 

It’s the same reason that the device tree core is using it.

Believe it or not, this is the simplest way to do it.
If you take a look at the sysfs attribute implementation, the binary
implementation is the one that’s using the least amount of code.

To use a non-binary method we have to register per ktype sysfs_ops
and duplicate the way the non-binary attribute works.

For the gory details look at sysfs_add_file_mode_ns() in fs/sysfs/file.c

I can add the sysfs_ops but that’s going to be more complicated not less.

>> +
>> +static ssize_t targets_read(struct file *filp, struct kobject *kobj,
>> +               struct bin_attribute *bin_attr, char *buf,
>> +               loff_t offset, size_t count)
>> +{
>> +       struct of_overlay *ov = kobj_to_overlay(kobj);
>> +       struct of_overlay_info *ovinfo;
>> +       char *tmpbuf, *s, *e;
>> +       const char *name;
>> +       ssize_t ret;
>> +       int i, len;
>> +
>> +       /* allocate work buffer; we know that PAGE_SIZE is enough */
>> +       tmpbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> +       if (tmpbuf == NULL)
>> +               return -ENOMEM;
>> +
>> +       s = tmpbuf;
>> +       e = tmpbuf + PAGE_SIZE;
>> +
>> +       mutex_lock(&of_mutex);
>> +
>> +       /* targets */
>> +       for (i = 0; i < ov->count; i++) {
>> +               ovinfo = &ov->ovinfo_tab[i];
>> +
>> +               name = of_node_full_name(ovinfo->target);
>> +               len = strlen(name);
>> +               if (s + len + 1 >= e)
>> +                       return -ENOMEM;
> 
> Leaking memory here and holding the mutex.
> 

OK

>> +               memcpy(s, name, len);
>> +               s += len;
>> +               *s++ = '\n';
>> +       }
>> +       if (s + 1 >= e)
>> +               return -ENOMEM;

> And here.
> 

OK

>> +       *s++ = '\0';
>> +
>> +       /* the buffer is zero terminated */
>> +       len = s - tmpbuf;
>> +
>> +       mutex_unlock(&of_mutex);
>> +
>> +       /* perform the read */
>> +       ret = memory_read_from_buffer(buf, count, &offset, tmpbuf, len);
>> +
>> +       /* free the temporary buffer */
>> +       kfree(tmpbuf);
>> +
>> +       return ret;
>> +}
>> +
>> +/* targets property */
>> +static BIN_ATTR_RO(targets, PAGE_SIZE);
>> +
>> +static ssize_t can_remove_read(struct file *filp, struct kobject *kobj,
>> +               struct bin_attribute *bin_attr, char *buf,
>> +               loff_t offset, size_t count)
>> +{
>> +       struct of_overlay *ov = kobj_to_overlay(kobj);
>> +       char tbuf[3];
>> +
>> +       if (offset < 0)
>> +               return -EINVAL;
>> +
>> +       if (offset >= sizeof(tbuf))
>> +               return 0;
>> +
>> +       if (count > sizeof(tbuf) - offset)
>> +               count = sizeof(tbuf) - offset;
>> +
>> +       /* fill in temp */
>> +       tbuf[0] = '0' + overlay_removal_is_ok(ov);
>> +       tbuf[1] = '\n';
>> +       tbuf[2] = '\0';
>> +
>> +       /* copy to buffer */
>> +       memcpy(buf, tbuf + offset, count);
>> +
>> +       return count;
>> +}
>> +
>> +/* can_remove property */
>> +static BIN_ATTR_RO(can_remove, 3);
> 
> Same question about bin attr here.
> 

Same answer as above.

>> +
>> /**
>>  * of_overlay_create() - Create and apply an overlay
>>  * @tree:      Device node containing all the overlays
>> @@ -360,6 +503,10 @@ int of_overlay_create(struct device_node *tree)
>>        struct of_overlay *ov;
>>        int err, id;
>> 
>> +       /* administratively disabled */
>> +       if (!atomic_read(&ov_enable))
>> +               return -EPERM;
>> +
>>        /* allocate the overlay structure */
>>        ov = kzalloc(sizeof(*ov), GFP_KERNEL);
>>        if (ov == NULL)
>> @@ -416,6 +563,22 @@ int of_overlay_create(struct device_node *tree)
>>                goto err_cancel_overlay;
>>        }
>> 
>> +       /* create targets file */
>> +       err = sysfs_create_bin_file(&ov->kobj, &bin_attr_targets);
>> +       if (err != 0) {
>> +               pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n",
>> +                               __func__, tree->full_name);
>> +               goto err_cancel_overlay;
>> +       }
>> +
>> +       /* create can_remove file */
>> +       err = sysfs_create_bin_file(&ov->kobj, &bin_attr_can_remove);
>> +       if (err != 0) {
>> +               pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n",
>> +                               __func__, tree->full_name);
>> +               goto err_cancel_overlay;
>> +       }
>> +
>>        /* add to the tail of the overlay list */
>>        list_add_tail(&ov->node, &ov_list);
>> 
>> @@ -596,5 +759,7 @@ int of_overlay_init(void)
>>        if (!ov_kset)
>>                return -ENOMEM;
>> 
>> -       return 0;
>> +       rc = sysfs_create_bin_file(&ov_kset->kobj, &bin_attr_enable);
>> +       WARN(rc, "%s: error adding enable attribute\n", __func__);
>> +       return rc;
>> }
>> --
>> 1.7.12

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Greg KH]

On Thu, Apr 23, 2015 at 03:00:03PM +0300, Pantelis Antoniou wrote:
> Hi Rob,
> 
> > On Apr 15, 2015, at 04:27 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
> > <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >> Implement a number of sysfs attributes for overlays.
> >> 
> >> * A throw once master enable switch to protect against any
> >> further overlay applications if the administrator desires so.
> > 
> > This one should be a separate patch.
> > 
> 
> OK.
> 
> >> * A per overlay targets sysfs attribute listing the targets of
> >> the installed overlay.
> > 
> > What are targets? "targets lists targets" does not help me. The
> > documentation doesn't help me either.
> > 
> 
> It lists the targets of the overlay that has been applied. What do
> you need in order to be helped? I mean what do you want listed?
> 
> >> * A per overlay can_remove sysfs attribute that reports whether
> >> the overlay can be removed or not due to another overlapping overlay.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> ---
> >> drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 166 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >> index f17f5ef..c54d097 100644
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -21,6 +21,7 @@
> >> #include <linux/err.h>
> >> #include <linux/idr.h>
> >> #include <linux/sysfs.h>
> >> +#include <linux/atomic.h>
> >> 
> >> #include "of_private.h"
> >> 
> >> @@ -55,8 +56,12 @@ struct of_overlay {
> >>        struct kobject kobj;
> >> };
> >> 
> >> +/* master enable switch; once set to 0 can't be re-enabled */
> >> +static atomic_t ov_enable = ATOMIC_INIT(1);
> >> +
> >> static int of_overlay_apply_one(struct of_overlay *ov,
> >>                struct device_node *target, const struct device_node *overlay);
> >> +static int overlay_removal_is_ok(struct of_overlay *ov);
> >> 
> >> static int of_overlay_apply_single_property(struct of_overlay *ov,
> >>                struct device_node *target, struct property *prop)
> >> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
> >> 
> >> static struct kset *ov_kset;
> >> 
> >> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
> >> +               struct bin_attribute *bin_attr, char *buf,
> >> +               loff_t offset, size_t count)
> >> +{
> >> +       char tbuf[3];
> >> +
> >> +       if (offset < 0)
> >> +               return -EINVAL;
> >> +
> >> +       if (offset >= sizeof(tbuf))
> >> +               return 0;
> >> +
> >> +       if (count > sizeof(tbuf) - offset)
> >> +               count = sizeof(tbuf) - offset;
> >> +
> >> +       /* fill in temp */
> >> +       tbuf[0] = '0' + atomic_read(&ov_enable);
> >> +       tbuf[1] = '\n';
> >> +       tbuf[2] = '\0';
> >> +
> >> +       /* copy to buffer */
> >> +       memcpy(buf, tbuf + offset, count);
> >> +
> >> +       return count;
> >> +}
> >> +
> >> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
> >> +               struct bin_attribute *bin_attr, char *buf,
> >> +               loff_t off, size_t count)
> >> +{
> >> +       unsigned int new_enable;
> >> +
> >> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
> >> +               return -EINVAL;
> >> +
> >> +       new_enable = (unsigned int)(buf[0] - '0');
> >> +       if (new_enable > 1)
> >> +               return -EINVAL;
> >> +
> >> +       /* NOP for same value */
> >> +       if (new_enable == atomic_read(&ov_enable))
> >> +               return count;
> >> +
> >> +       /* if we've disabled it, no going back */
> >> +       if (atomic_read(&ov_enable) == 0)
> >> +               return -EPERM;
> >> +
> >> +       atomic_set(&ov_enable, new_enable);
> >> +       return count;
> >> +}
> >> +
> >> +/* just a single char + '\n' + '\0' */
> >> +static BIN_ATTR_RW(enable, 3);
> > 
> > Why are you using bin attribute? You are complicating the
> > implementation needlessly.
> > 
> 
> It’s the same reason that the device tree core is using it.

It is doing that for "raw" device tree files, not individual attributes,
right?

> Believe it or not, this is the simplest way to do it.
> If you take a look at the sysfs attribute implementation, the binary
> implementation is the one that’s using the least amount of code.

Then something is really wrong here.

> To use a non-binary method we have to register per ktype sysfs_ops
> and duplicate the way the non-binary attribute works.

really?  Again, something must be wrong.

> For the gory details look at sysfs_add_file_mode_ns() in fs/sysfs/file.c
> 
> I can add the sysfs_ops but that’s going to be more complicated not less.

Only use binary sysfs files if you are accepting binary data directly
from userspace and using it as a "pass-through" to the kernel.

Otherwise just use a "normal" sysfs file.  I don't understand the
problem here, sysfs shouldn't be hard to use for simple attributes, that
was not the goal here at all.

thanks,

greg k-h

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Pantelis Antoniou]

Hi Greg,

> On Apr 23, 2015, at 15:33 , Greg KH <greg@kroah.com> wrote:
> 
> On Thu, Apr 23, 2015 at 03:00:03PM +0300, Pantelis Antoniou wrote:
>> Hi Rob,
>> 
>>> On Apr 15, 2015, at 04:27 , Rob Herring <robherring2@gmail.com> wrote:
>>> 
>>> On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> Implement a number of sysfs attributes for overlays.
>>>> 
>>>> * A throw once master enable switch to protect against any
>>>> further overlay applications if the administrator desires so.
>>> 
>>> This one should be a separate patch.
>>> 
>> 
>> OK.
>> 
>>>> * A per overlay targets sysfs attribute listing the targets of
>>>> the installed overlay.
>>> 
>>> What are targets? "targets lists targets" does not help me. The
>>> documentation doesn't help me either.
>>> 
>> 
>> It lists the targets of the overlay that has been applied. What do
>> you need in order to be helped? I mean what do you want listed?
>> 
>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>> the overlay can be removed or not due to another overlapping overlay.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>>> ---
>>>> drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 166 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index f17f5ef..c54d097 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -21,6 +21,7 @@
>>>> #include <linux/err.h>
>>>> #include <linux/idr.h>
>>>> #include <linux/sysfs.h>
>>>> +#include <linux/atomic.h>
>>>> 
>>>> #include "of_private.h"
>>>> 
>>>> @@ -55,8 +56,12 @@ struct of_overlay {
>>>>       struct kobject kobj;
>>>> };
>>>> 
>>>> +/* master enable switch; once set to 0 can't be re-enabled */
>>>> +static atomic_t ov_enable = ATOMIC_INIT(1);
>>>> +
>>>> static int of_overlay_apply_one(struct of_overlay *ov,
>>>>               struct device_node *target, const struct device_node *overlay);
>>>> +static int overlay_removal_is_ok(struct of_overlay *ov);
>>>> 
>>>> static int of_overlay_apply_single_property(struct of_overlay *ov,
>>>>               struct device_node *target, struct property *prop)
>>>> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
>>>> 
>>>> static struct kset *ov_kset;
>>>> 
>>>> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
>>>> +               struct bin_attribute *bin_attr, char *buf,
>>>> +               loff_t offset, size_t count)
>>>> +{
>>>> +       char tbuf[3];
>>>> +
>>>> +       if (offset < 0)
>>>> +               return -EINVAL;
>>>> +
>>>> +       if (offset >= sizeof(tbuf))
>>>> +               return 0;
>>>> +
>>>> +       if (count > sizeof(tbuf) - offset)
>>>> +               count = sizeof(tbuf) - offset;
>>>> +
>>>> +       /* fill in temp */
>>>> +       tbuf[0] = '0' + atomic_read(&ov_enable);
>>>> +       tbuf[1] = '\n';
>>>> +       tbuf[2] = '\0';
>>>> +
>>>> +       /* copy to buffer */
>>>> +       memcpy(buf, tbuf + offset, count);
>>>> +
>>>> +       return count;
>>>> +}
>>>> +
>>>> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
>>>> +               struct bin_attribute *bin_attr, char *buf,
>>>> +               loff_t off, size_t count)
>>>> +{
>>>> +       unsigned int new_enable;
>>>> +
>>>> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
>>>> +               return -EINVAL;
>>>> +
>>>> +       new_enable = (unsigned int)(buf[0] - '0');
>>>> +       if (new_enable > 1)
>>>> +               return -EINVAL;
>>>> +
>>>> +       /* NOP for same value */
>>>> +       if (new_enable == atomic_read(&ov_enable))
>>>> +               return count;
>>>> +
>>>> +       /* if we've disabled it, no going back */
>>>> +       if (atomic_read(&ov_enable) == 0)
>>>> +               return -EPERM;
>>>> +
>>>> +       atomic_set(&ov_enable, new_enable);
>>>> +       return count;
>>>> +}
>>>> +
>>>> +/* just a single char + '\n' + '\0' */
>>>> +static BIN_ATTR_RW(enable, 3);
>>> 
>>> Why are you using bin attribute? You are complicating the
>>> implementation needlessly.
>>> 
>> 
>> It’s the same reason that the device tree core is using it.
> 
> It is doing that for "raw" device tree files, not individual attributes,
> right?
> 

Each property of a device tree is a binary attribute.

>> Believe it or not, this is the simplest way to do it.
>> If you take a look at the sysfs attribute implementation, the binary
>> implementation is the one that’s using the least amount of code.
> 
> Then something is really wrong here.
> 
>> To use a non-binary method we have to register per ktype sysfs_ops
>> and duplicate the way the non-binary attribute works.
> 
> really?  Again, something must be wrong.
> 
>> For the gory details look at sysfs_add_file_mode_ns() in fs/sysfs/file.c
>> 
>> I can add the sysfs_ops but that’s going to be more complicated not less.
> 

Please take a look in linux/sysfs.h.
The non-binary sysfs accessors are all using some kind of other kobj; 
for instance DEVICE_ATTR is using a device_attribute, etc.

For the overlay case, I’d have to create a of_overlay_attribute and work from
there.

> Only use binary sysfs files if you are accepting binary data directly
> from userspace and using it as a "pass-through" to the kernel.
> 
> Otherwise just use a "normal" sysfs file.  I don't understand the
> problem here, sysfs shouldn't be hard to use for simple attributes, that
> was not the goal here at all.
> 

There is no generic (i.e. not kobj type specific), non-binary sysfs file interface right now.
I can add one for my case, but that’s more code.

It’s your call.

> thanks,
> 
> greg k-h

Regards

— Pantelis

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Greg KH]

On Thu, Apr 23, 2015 at 03:39:21PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
> 
> > On Apr 23, 2015, at 15:33 , Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > On Thu, Apr 23, 2015 at 03:00:03PM +0300, Pantelis Antoniou wrote:
> >> Hi Rob,
> >> 
> >>> On Apr 15, 2015, at 04:27 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> 
> >>> On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
> >>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >>>> Implement a number of sysfs attributes for overlays.
> >>>> 
> >>>> * A throw once master enable switch to protect against any
> >>>> further overlay applications if the administrator desires so.
> >>> 
> >>> This one should be a separate patch.
> >>> 
> >> 
> >> OK.
> >> 
> >>>> * A per overlay targets sysfs attribute listing the targets of
> >>>> the installed overlay.
> >>> 
> >>> What are targets? "targets lists targets" does not help me. The
> >>> documentation doesn't help me either.
> >>> 
> >> 
> >> It lists the targets of the overlay that has been applied. What do
> >> you need in order to be helped? I mean what do you want listed?
> >> 
> >>>> * A per overlay can_remove sysfs attribute that reports whether
> >>>> the overlay can be removed or not due to another overlapping overlay.
> >>>> 
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >>>> ---
> >>>> drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 166 insertions(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >>>> index f17f5ef..c54d097 100644
> >>>> --- a/drivers/of/overlay.c
> >>>> +++ b/drivers/of/overlay.c
> >>>> @@ -21,6 +21,7 @@
> >>>> #include <linux/err.h>
> >>>> #include <linux/idr.h>
> >>>> #include <linux/sysfs.h>
> >>>> +#include <linux/atomic.h>
> >>>> 
> >>>> #include "of_private.h"
> >>>> 
> >>>> @@ -55,8 +56,12 @@ struct of_overlay {
> >>>>       struct kobject kobj;
> >>>> };
> >>>> 
> >>>> +/* master enable switch; once set to 0 can't be re-enabled */
> >>>> +static atomic_t ov_enable = ATOMIC_INIT(1);
> >>>> +
> >>>> static int of_overlay_apply_one(struct of_overlay *ov,
> >>>>               struct device_node *target, const struct device_node *overlay);
> >>>> +static int overlay_removal_is_ok(struct of_overlay *ov);
> >>>> 
> >>>> static int of_overlay_apply_single_property(struct of_overlay *ov,
> >>>>               struct device_node *target, struct property *prop)
> >>>> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
> >>>> 
> >>>> static struct kset *ov_kset;
> >>>> 
> >>>> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
> >>>> +               struct bin_attribute *bin_attr, char *buf,
> >>>> +               loff_t offset, size_t count)
> >>>> +{
> >>>> +       char tbuf[3];
> >>>> +
> >>>> +       if (offset < 0)
> >>>> +               return -EINVAL;
> >>>> +
> >>>> +       if (offset >= sizeof(tbuf))
> >>>> +               return 0;
> >>>> +
> >>>> +       if (count > sizeof(tbuf) - offset)
> >>>> +               count = sizeof(tbuf) - offset;
> >>>> +
> >>>> +       /* fill in temp */
> >>>> +       tbuf[0] = '0' + atomic_read(&ov_enable);
> >>>> +       tbuf[1] = '\n';
> >>>> +       tbuf[2] = '\0';
> >>>> +
> >>>> +       /* copy to buffer */
> >>>> +       memcpy(buf, tbuf + offset, count);
> >>>> +
> >>>> +       return count;
> >>>> +}
> >>>> +
> >>>> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
> >>>> +               struct bin_attribute *bin_attr, char *buf,
> >>>> +               loff_t off, size_t count)
> >>>> +{
> >>>> +       unsigned int new_enable;
> >>>> +
> >>>> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
> >>>> +               return -EINVAL;
> >>>> +
> >>>> +       new_enable = (unsigned int)(buf[0] - '0');
> >>>> +       if (new_enable > 1)
> >>>> +               return -EINVAL;
> >>>> +
> >>>> +       /* NOP for same value */
> >>>> +       if (new_enable == atomic_read(&ov_enable))
> >>>> +               return count;
> >>>> +
> >>>> +       /* if we've disabled it, no going back */
> >>>> +       if (atomic_read(&ov_enable) == 0)
> >>>> +               return -EPERM;
> >>>> +
> >>>> +       atomic_set(&ov_enable, new_enable);
> >>>> +       return count;
> >>>> +}
> >>>> +
> >>>> +/* just a single char + '\n' + '\0' */
> >>>> +static BIN_ATTR_RW(enable, 3);
> >>> 
> >>> Why are you using bin attribute? You are complicating the
> >>> implementation needlessly.
> >>> 
> >> 
> >> It’s the same reason that the device tree core is using it.
> > 
> > It is doing that for "raw" device tree files, not individual attributes,
> > right?
> > 
> 
> Each property of a device tree is a binary attribute.

Because they export binary data, right?  I don't have access to a
machine that uses device tree at the moment to check this...

Any specific file/function you are referring to?

> >> Believe it or not, this is the simplest way to do it.
> >> If you take a look at the sysfs attribute implementation, the binary
> >> implementation is the one that’s using the least amount of code.
> > 
> > Then something is really wrong here.
> > 
> >> To use a non-binary method we have to register per ktype sysfs_ops
> >> and duplicate the way the non-binary attribute works.
> > 
> > really?  Again, something must be wrong.
> > 
> >> For the gory details look at sysfs_add_file_mode_ns() in fs/sysfs/file.c
> >> 
> >> I can add the sysfs_ops but that’s going to be more complicated not less.
> > 
> 
> Please take a look in linux/sysfs.h.
> The non-binary sysfs accessors are all using some kind of other kobj; 
> for instance DEVICE_ATTR is using a device_attribute, etc.
> 
> For the overlay case, I’d have to create a of_overlay_attribute and work from
> there.

Yes, that is what you should be doing here as well.

That's just the model we have to work with, the uses of "raw" kobjects
are very limited, so it does take a bit more wrapper code to use them,
sorry.

You need access to the kobject anyway, which is why you need to provide
a type of attribute function, so that you get the right kobject.

Or just use an attribute group, would that be simpler?  If you have more
than one sysfs file, that's usually the best way to do things.

thanks,

greg k-h

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Pantelis Antoniou]

Hi Greg,

> On Apr 23, 2015, at 15:54 , Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Thu, Apr 23, 2015 at 03:39:21PM +0300, Pantelis Antoniou wrote:
>> Hi Greg,
>> 
>>> On Apr 23, 2015, at 15:33 , Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> wrote:
>>> 
>>> On Thu, Apr 23, 2015 at 03:00:03PM +0300, Pantelis Antoniou wrote:
>>>> Hi Rob,
>>>> 
>>>>> On Apr 15, 2015, at 04:27 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> 
>>>>> On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
>>>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>>>> Implement a number of sysfs attributes for overlays.
>>>>>> 
>>>>>> * A throw once master enable switch to protect against any
>>>>>> further overlay applications if the administrator desires so.
>>>>> 
>>>>> This one should be a separate patch.
>>>>> 
>>>> 
>>>> OK.
>>>> 
>>>>>> * A per overlay targets sysfs attribute listing the targets of
>>>>>> the installed overlay.
>>>>> 
>>>>> What are targets? "targets lists targets" does not help me. The
>>>>> documentation doesn't help me either.
>>>>> 
>>>> 
>>>> It lists the targets of the overlay that has been applied. What do
>>>> you need in order to be helped? I mean what do you want listed?
>>>> 
>>>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>>>> the overlay can be removed or not due to another overlapping overlay.
>>>>>> 
>>>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>>> ---
>>>>>> drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>> 1 file changed, 166 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>>> index f17f5ef..c54d097 100644
>>>>>> --- a/drivers/of/overlay.c
>>>>>> +++ b/drivers/of/overlay.c
>>>>>> @@ -21,6 +21,7 @@
>>>>>> #include <linux/err.h>
>>>>>> #include <linux/idr.h>
>>>>>> #include <linux/sysfs.h>
>>>>>> +#include <linux/atomic.h>
>>>>>> 
>>>>>> #include "of_private.h"
>>>>>> 
>>>>>> @@ -55,8 +56,12 @@ struct of_overlay {
>>>>>>      struct kobject kobj;
>>>>>> };
>>>>>> 
>>>>>> +/* master enable switch; once set to 0 can't be re-enabled */
>>>>>> +static atomic_t ov_enable = ATOMIC_INIT(1);
>>>>>> +
>>>>>> static int of_overlay_apply_one(struct of_overlay *ov,
>>>>>>              struct device_node *target, const struct device_node *overlay);
>>>>>> +static int overlay_removal_is_ok(struct of_overlay *ov);
>>>>>> 
>>>>>> static int of_overlay_apply_single_property(struct of_overlay *ov,
>>>>>>              struct device_node *target, struct property *prop)
>>>>>> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
>>>>>> 
>>>>>> static struct kset *ov_kset;
>>>>>> 
>>>>>> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
>>>>>> +               struct bin_attribute *bin_attr, char *buf,
>>>>>> +               loff_t offset, size_t count)
>>>>>> +{
>>>>>> +       char tbuf[3];
>>>>>> +
>>>>>> +       if (offset < 0)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       if (offset >= sizeof(tbuf))
>>>>>> +               return 0;
>>>>>> +
>>>>>> +       if (count > sizeof(tbuf) - offset)
>>>>>> +               count = sizeof(tbuf) - offset;
>>>>>> +
>>>>>> +       /* fill in temp */
>>>>>> +       tbuf[0] = '0' + atomic_read(&ov_enable);
>>>>>> +       tbuf[1] = '\n';
>>>>>> +       tbuf[2] = '\0';
>>>>>> +
>>>>>> +       /* copy to buffer */
>>>>>> +       memcpy(buf, tbuf + offset, count);
>>>>>> +
>>>>>> +       return count;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
>>>>>> +               struct bin_attribute *bin_attr, char *buf,
>>>>>> +               loff_t off, size_t count)
>>>>>> +{
>>>>>> +       unsigned int new_enable;
>>>>>> +
>>>>>> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       new_enable = (unsigned int)(buf[0] - '0');
>>>>>> +       if (new_enable > 1)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       /* NOP for same value */
>>>>>> +       if (new_enable == atomic_read(&ov_enable))
>>>>>> +               return count;
>>>>>> +
>>>>>> +       /* if we've disabled it, no going back */
>>>>>> +       if (atomic_read(&ov_enable) == 0)
>>>>>> +               return -EPERM;
>>>>>> +
>>>>>> +       atomic_set(&ov_enable, new_enable);
>>>>>> +       return count;
>>>>>> +}
>>>>>> +
>>>>>> +/* just a single char + '\n' + '\0' */
>>>>>> +static BIN_ATTR_RW(enable, 3);
>>>>> 
>>>>> Why are you using bin attribute? You are complicating the
>>>>> implementation needlessly.
>>>>> 
>>>> 
>>>> It’s the same reason that the device tree core is using it.
>>> 
>>> It is doing that for "raw" device tree files, not individual attributes,
>>> right?
>>> 
>> 
>> Each property of a device tree is a binary attribute.
> 
> Because they export binary data, right?  I don't have access to a
> machine that uses device tree at the moment to check this...
> 
> Any specific file/function you are referring to?
> 

Yes, they export binary data. It works because the device tree nodes
are raw kobjs.

>>>> Believe it or not, this is the simplest way to do it.
>>>> If you take a look at the sysfs attribute implementation, the binary
>>>> implementation is the one that’s using the least amount of code.
>>> 
>>> Then something is really wrong here.
>>> 
>>>> To use a non-binary method we have to register per ktype sysfs_ops
>>>> and duplicate the way the non-binary attribute works.
>>> 
>>> really?  Again, something must be wrong.
>>> 
>>>> For the gory details look at sysfs_add_file_mode_ns() in fs/sysfs/file.c
>>>> 
>>>> I can add the sysfs_ops but that’s going to be more complicated not less.
>>> 
>> 
>> Please take a look in linux/sysfs.h.
>> The non-binary sysfs accessors are all using some kind of other kobj; 
>> for instance DEVICE_ATTR is using a device_attribute, etc.
>> 
>> For the overlay case, I’d have to create a of_overlay_attribute and work from
>> there.
> 
> Yes, that is what you should be doing here as well.
> 
> That's just the model we have to work with, the uses of "raw" kobjects
> are very limited, so it does take a bit more wrapper code to use them,
> sorry.
> 

That’s fine, I can work with this. I was trying to avoid creating overlay
attributes but...

> You need access to the kobject anyway, which is why you need to provide
> a type of attribute function, so that you get the right kobject.
> 
> Or just use an attribute group, would that be simpler?  If you have more
> than one sysfs file, that's usually the best way to do things.
> 

Attribute groups might work, but I have some more work to do to get them to
work.

Thanks for answering definitively this.
 
> thanks,
> 
> greg k-h

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Greg KH]

On Thu, Apr 23, 2015 at 03:59:02PM +0300, Pantelis Antoniou wrote:
> >>>> It’s the same reason that the device tree core is using it.
> >>> 
> >>> It is doing that for "raw" device tree files, not individual attributes,
> >>> right?
> >>> 
> >> 
> >> Each property of a device tree is a binary attribute.
> > 
> > Because they export binary data, right?  I don't have access to a
> > machine that uses device tree at the moment to check this...
> > 
> > Any specific file/function you are referring to?
> > 
> 
> Yes, they export binary data. It works because the device tree nodes
> are raw kobjs.

Great, then the device tree is doing this correctly :)

For text files, use real attribute files, that's the proper way to
handle sysfs from kobjects.

thanks,

greg k-h