[PATCH v6 0/4] of: overlay: kobject & sysfs'ation

[PATCH v6 0/4] of: overlay: kobject & sysfs'ation

[Pantelis Antoniou]

The first patch puts the overlays as objects in the sysfs in
/sys/firmware/devicetree/overlays.

The next adds a master overlay enable switch (that once is set to
disabled can't be re-enabled), while the one after that
introduces a number of default per overlay attributes.

The patchset is against linus's tree as of today.

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

Changes since v5:
* Does a single kobject_put that suffices
* A per-fragment sysfs directory and a single value target.
* Update in the ABI documention.

Changes since v4:
* Rebased against latest mainline.

Changes since v3:
* Used strtobool instead of kstrtoul
* ABI Documentation includes a pointer to the discussion that
requested the sysfs property.

Changes since v2:
* Removed the unittest patch.
* Split the sysfs attribute patch to a global and a per-overlay
  patch.
* Dropped binary attributes using textual kobj_attributes instead.

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


Pantelis Antoniou (4):
  of: overlay: kobjectify overlay objects
  of: overlay: global sysfs enable attribute
  of: overlay: add per overlay sysfs attributes
  Documentation: ABI: /sys/firmware/devicetree/overlays

 .../ABI/testing/sysfs-firmware-devicetree-overlays |  40 +++++
 drivers/of/base.c                                  |   7 +
 drivers/of/of_private.h                            |   9 +
 drivers/of/overlay.c                               | 194 ++++++++++++++++++++-
 4 files changed, 244 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays

-- 
1.7.12

[PATCH v6 1/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       |  7 +++++++
 drivers/of/of_private.h |  9 +++++++++
 drivers/of/overlay.c    | 50 +++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8b5a187..31c0c8f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -192,6 +192,7 @@ int __of_attach_node_sysfs(struct device_node *np)
 void __init of_core_init(void)
 {
 	struct device_node *np;
+	int ret;
 
 	/* Create the kset, and register existing nodes */
 	mutex_lock(&of_mutex);
@@ -208,6 +209,12 @@ void __init of_core_init(void)
 	/* Symlink in /proc as required by userspace ABI */
 	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;
 }
 
 static struct property *__of_find_property(const struct device_node *np,
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 24e025f..12c3e47 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,8 @@ int of_overlay_destroy(int id)
 	of_free_overlay_info(ov);
 	idr_remove(&ov_idr, id);
 	of_changeset_destroy(&ov->cset);
-	kfree(ov);
+
+	kobject_put(&ov->kobj);
 
 	err = 0;
 
@@ -542,7 +576,7 @@ 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_put(&ov->kobj);
 	}
 
 	mutex_unlock(&of_mutex);
@@ -550,3 +584,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 v6 2/4] of: overlay: global sysfs enable attribute

[Pantelis Antoniou]

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

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 12c3e47..067404e 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)
@@ -339,6 +344,35 @@ void of_overlay_release(struct kobject *kobj)
 	kfree(ov);
 }
 
+static ssize_t enable_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
+}
+
+static ssize_t enable_store(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	int ret;
+	bool new_enable;
+
+	ret = strtobool(buf, &new_enable);
+	if (ret != 0)
+		return ret;
+	/* if we've disabled it, no going back */
+	if (atomic_read(&ov_enable) == 0)
+		return -EPERM;
+	atomic_set(&ov_enable, (int)new_enable);
+	return count;
+}
+
+static struct kobj_attribute enable_attr = __ATTR_RW(enable);
+
+static const struct attribute *overlay_global_attrs[] = {
+	&enable_attr.attr,
+	NULL
+};
+
 static struct kobj_type of_overlay_ktype = {
 	.release = of_overlay_release,
 };
@@ -360,6 +394,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)
@@ -594,5 +632,8 @@ int of_overlay_init(void)
 	if (!ov_kset)
 		return -ENOMEM;
 
-	return 0;
+	rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
+	WARN(rc, "%s: error adding global attributes\n", __func__);
+
+	return rc;
 }
-- 
1.7.12

[PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

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

* A target sysfs attribute listing the target of each fragment,
in a group named after the name of the fragment.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/overlay.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 067404e..2d51d9e2 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -25,8 +25,23 @@
 
 #include "of_private.h"
 
+/* fwd. decl */
+struct of_overlay;
+struct of_overlay_info;
+
+/* an attribute for each fragment */
+struct fragment_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct kobject *kobj, struct fragment_attribute *fattr,
+			char *buf);
+	ssize_t (*store)(struct kobject *kobj, struct fragment_attribute *fattr,
+			 const char *buf, size_t count);
+	struct of_overlay_info *ovinfo;
+};
+
 /**
  * struct of_overlay_info - Holds a single overlay info
+ * @info:	info node that contains the target and overlay
  * @target:	target of the overlay operation
  * @overlay:	pointer to the overlay contents node
  *
@@ -34,8 +49,13 @@
  * records.
  */
 struct of_overlay_info {
+	struct of_overlay *ov;
+	struct device_node *info;
 	struct device_node *target;
 	struct device_node *overlay;
+	struct attribute_group attr_group;
+	struct attribute *attrs[2];
+	struct fragment_attribute target_attr;
 };
 
 /**
@@ -52,6 +72,7 @@ struct of_overlay {
 	struct list_head node;
 	int count;
 	struct of_overlay_info *ovinfo_tab;
+	const struct attribute_group **attr_groups;
 	struct of_changeset cset;
 	struct kobject kobj;
 };
@@ -245,6 +266,8 @@ static int of_fill_overlay_info(struct of_overlay *ov,
 	if (ovinfo->target == NULL)
 		goto err_fail;
 
+	ovinfo->info = of_node_get(info_node);
+
 	return 0;
 
 err_fail:
@@ -255,6 +278,17 @@ err_fail:
 	return -EINVAL;
 }
 
+static ssize_t target_show(struct kobject *kobj,
+		struct fragment_attribute *fattr, char *buf)
+{
+	struct of_overlay_info *ovinfo = fattr->ovinfo;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			of_node_full_name(ovinfo->target));
+}
+
+static const struct fragment_attribute target_template_attr = __ATTR_RO(target);
+
 /**
  * of_build_overlay_info() - Build an overlay info array
  * @ov		Overlay to build
@@ -272,7 +306,7 @@ static int of_build_overlay_info(struct of_overlay *ov,
 {
 	struct device_node *node;
 	struct of_overlay_info *ovinfo;
-	int cnt, err;
+	int i, cnt, err;
 
 	/* worst case; every child is a node */
 	cnt = 0;
@@ -293,14 +327,45 @@ static int of_build_overlay_info(struct of_overlay *ov,
 
 	/* if nothing filled, return error */
 	if (cnt == 0) {
-		kfree(ovinfo);
-		return -ENODEV;
+		err = -ENODEV;
+		goto err_free_ovinfo;
 	}
 
 	ov->count = cnt;
 	ov->ovinfo_tab = ovinfo;
 
+	ov->attr_groups = kcalloc(cnt + 1,
+			sizeof(struct attribute_group *), GFP_KERNEL);
+	if (ov->attr_groups == NULL) {
+		err = -ENOMEM;
+		goto err_free_ovinfo;
+	}
+
+	for (i = 0; i < cnt; i++) {
+		ovinfo = &ov->ovinfo_tab[i];
+
+		ov->attr_groups[i] = &ovinfo->attr_group;
+
+		ovinfo->target_attr = target_template_attr;
+		/* make lockdep happy */
+		sysfs_attr_init(&ovinfo->target_attr.attr);
+		ovinfo->target_attr.ovinfo = ovinfo;
+
+		ovinfo->attrs[0] = &ovinfo->target_attr.attr;
+		ovinfo->attrs[1] = NULL;
+
+		/* NOTE: direct reference to the full_name */
+		ovinfo->attr_group.name = kbasename(ovinfo->info->full_name);
+		ovinfo->attr_group.attrs = ovinfo->attrs;
+
+	}
+	ov->attr_groups[i] = NULL;
+
 	return 0;
+
+err_free_ovinfo:
+	kfree(ovinfo);
+	return err;
 }
 
 /**
@@ -317,12 +382,16 @@ static int of_free_overlay_info(struct of_overlay *ov)
 	struct of_overlay_info *ovinfo;
 	int i;
 
+	/* free attribute groups space */
+	kfree(ov->attr_groups);
+
 	/* do it in reverse */
 	for (i = ov->count - 1; i >= 0; i--) {
 		ovinfo = &ov->ovinfo_tab[i];
 
 		of_node_put(ovinfo->target);
 		of_node_put(ovinfo->overlay);
+		of_node_put(ovinfo->info);
 	}
 	kfree(ov->ovinfo_tab);
 
@@ -373,8 +442,25 @@ static const struct attribute *overlay_global_attrs[] = {
 	NULL
 };
 
+static ssize_t can_remove_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct of_overlay *ov = kobj_to_overlay(kobj);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", overlay_removal_is_ok(ov));
+}
+
+static struct kobj_attribute can_remove_attr = __ATTR_RO(can_remove);
+
+static struct attribute *overlay_attrs[] = {
+	&can_remove_attr.attr,
+	NULL
+};
+
 static struct kobj_type of_overlay_ktype = {
 	.release = of_overlay_release,
+	.sysfs_ops = &kobj_sysfs_ops,	/* default kobj sysfs ops */
+	.default_attrs = overlay_attrs,
 };
 
 static struct kset *ov_kset;
@@ -454,13 +540,21 @@ int of_overlay_create(struct device_node *tree)
 		goto err_cancel_overlay;
 	}
 
+	err = sysfs_create_groups(&ov->kobj, ov->attr_groups);
+	if (err != 0) {
+		pr_err("%s: sysfs_create_groups() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_remove_kobj;
+	}
+
 	/* add to the tail of the overlay list */
 	list_add_tail(&ov->node, &ov_list);
 
 	mutex_unlock(&of_mutex);
 
 	return id;
-
+err_remove_kobj:
+	kobject_put(&ov->kobj);
 err_cancel_overlay:
 	of_changeset_revert(&ov->cset);
 err_revert_overlay:
@@ -579,6 +673,7 @@ int of_overlay_destroy(int id)
 
 
 	list_del(&ov->node);
+	sysfs_remove_groups(&ov->kobj, ov->attr_groups);
 	of_changeset_revert(&ov->cset);
 	of_free_overlay_info(ov);
 	idr_remove(&ov_idr, id);
-- 
1.7.12

[PATCH v6 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 | 40 ++++++++++++++++++++++
 1 file changed, 40 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..adc4068
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
@@ -0,0 +1,40 @@
+What:		/sys/firmware/devicetree/overlays/
+Date:		October 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.
+
+		The discussion about this switch takes place in:
+		http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
+
+		Kees Cook:
+		"Coming from the perspective of drawing a bright line between
+		kernel and the root user (which tends to start with disabling
+		kernel module loading), I would say that there at least needs
+		to be a high-level one-way "off" switch for the interface so
+		that systems that have this interface can choose to turn it off
+		during initial boot, etc."
+
+What:		/sys/firmware/devicetree/overlays/<id>
+Date:		October 2015
+Contact:	Pantelis Antoniou <pantelis.antoniou@konsulko.com>
+Description:
+		Each directory represents an applied overlay, containing
+		the following attribute files.
+
+		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.
+
+What:		/sys/firmware/devicetree/overlays/<id>/<fragment-name>/
+Date:		October 2015
+Contact:	Pantelis Antoniou <pantelis.antoniou@konsulko.com>
+Description:
+		Each of these directories contain information about of the
+		particular overlay fragment.
+
+		target: The full-path of the target of the fragment
-- 
1.7.12

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

[Rob Herring]

On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Documentation ABI entry for overlays sysfs entries.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  .../ABI/testing/sysfs-firmware-devicetree-overlays | 40 ++++++++++++++++++++++
>  1 file changed, 40 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..adc4068
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> @@ -0,0 +1,40 @@
> +What:          /sys/firmware/devicetree/overlays/
> +Date:          October 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.
> +
> +               The discussion about this switch takes place in:
> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
> +
> +               Kees Cook:
> +               "Coming from the perspective of drawing a bright line between
> +               kernel and the root user (which tends to start with disabling
> +               kernel module loading), I would say that there at least needs
> +               to be a high-level one-way "off" switch for the interface so
> +               that systems that have this interface can choose to turn it off
> +               during initial boot, etc."
> +
> +What:          /sys/firmware/devicetree/overlays/<id>
> +Date:          October 2015
> +Contact:       Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> +Description:
> +               Each directory represents an applied overlay, containing
> +               the following attribute files.
> +
> +               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.
> +
> +What:          /sys/firmware/devicetree/overlays/<id>/<fragment-name>/
> +Date:          October 2015
> +Contact:       Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> +Description:
> +               Each of these directories contain information about of the
> +               particular overlay fragment.
> +
> +               target: The full-path of the target of the fragment
> --

What happened to attributes within the fragment dir?

Rob

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

[Pantelis Antoniou]

Hi Rob,

> On Oct 20, 2015, at 23:56 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Documentation ABI entry for overlays sysfs entries.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 40 ++++++++++++++++++++++
>> 1 file changed, 40 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..adc4068
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>> @@ -0,0 +1,40 @@
>> +What:          /sys/firmware/devicetree/overlays/
>> +Date:          October 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.
>> +
>> +               The discussion about this switch takes place in:
>> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
>> +
>> +               Kees Cook:
>> +               "Coming from the perspective of drawing a bright line between
>> +               kernel and the root user (which tends to start with disabling
>> +               kernel module loading), I would say that there at least needs
>> +               to be a high-level one-way "off" switch for the interface so
>> +               that systems that have this interface can choose to turn it off
>> +               during initial boot, etc."
>> +
>> +What:          /sys/firmware/devicetree/overlays/<id>
>> +Date:          October 2015
>> +Contact:       Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> +Description:
>> +               Each directory represents an applied overlay, containing
>> +               the following attribute files.
>> +
>> +               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.
>> +
>> +What:          /sys/firmware/devicetree/overlays/<id>/<fragment-name>/
>> +Date:          October 2015
>> +Contact:       Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> +Description:
>> +               Each of these directories contain information about of the
>> +               particular overlay fragment.
>> +
>> +               target: The full-path of the target of the fragment
>> --
> 
> What happened to attributes within the fragment dir?
> 

There’s a single attribute named target that contains the target of the fragment.

At the moment this is the only attribute. I eventually intent to put the full contents of the overlay fragment
there as /sysfs/firmware/devicetree/base does, but this would make things quite complicated for now.

Should I add a What: line for that too? 

> Rob

Regards

— Pantelis

Re: [PATCH v6 1/4] of: overlay: kobjectify overlay objects

[Greg Kroah-Hartman]

On Tue, Oct 20, 2015 at 10:13:14PM +0300, Pantelis Antoniou wrote:
> We are going to need the overlays to appear on sysfs with runtime
> global properties (like master enable) so turn them into kobjects.

Why kobjects and not 'struct device'?

Why even have them in sysfs at all?  You need more information here as
to why you want to do this.

thanks,

greg k-h

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Rob Herring]

On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> * A per overlay can_remove sysfs attribute that reports whether
> the overlay can be removed or not due to another overlapping overlay.
>
> * A target sysfs attribute listing the target of each fragment,
> in a group named after the name of the fragment.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/overlay.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 067404e..2d51d9e2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -25,8 +25,23 @@
>
>  #include "of_private.h"
>
> +/* fwd. decl */
> +struct of_overlay;
> +struct of_overlay_info;
> +
> +/* an attribute for each fragment */
> +struct fragment_attribute {
> +       struct attribute attr;
> +       ssize_t (*show)(struct kobject *kobj, struct fragment_attribute *fattr,
> +                       char *buf);
> +       ssize_t (*store)(struct kobject *kobj, struct fragment_attribute *fattr,
> +                        const char *buf, size_t count);
> +       struct of_overlay_info *ovinfo;
> +};
> +
>  /**
>   * struct of_overlay_info - Holds a single overlay info
> + * @info:      info node that contains the target and overlay
>   * @target:    target of the overlay operation
>   * @overlay:   pointer to the overlay contents node
>   *
> @@ -34,8 +49,13 @@
>   * records.
>   */
>  struct of_overlay_info {
> +       struct of_overlay *ov;
> +       struct device_node *info;
>         struct device_node *target;
>         struct device_node *overlay;
> +       struct attribute_group attr_group;
> +       struct attribute *attrs[2];
> +       struct fragment_attribute target_attr;
>  };
>
>  /**
> @@ -52,6 +72,7 @@ struct of_overlay {
>         struct list_head node;
>         int count;
>         struct of_overlay_info *ovinfo_tab;
> +       const struct attribute_group **attr_groups;
>         struct of_changeset cset;
>         struct kobject kobj;
>  };
> @@ -245,6 +266,8 @@ static int of_fill_overlay_info(struct of_overlay *ov,
>         if (ovinfo->target == NULL)
>                 goto err_fail;
>
> +       ovinfo->info = of_node_get(info_node);
> +
>         return 0;
>
>  err_fail:
> @@ -255,6 +278,17 @@ err_fail:
>         return -EINVAL;
>  }
>
> +static ssize_t target_show(struct kobject *kobj,
> +               struct fragment_attribute *fattr, char *buf)
> +{
> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
> +
> +       return snprintf(buf, PAGE_SIZE, "%s\n",
> +                       of_node_full_name(ovinfo->target));

This can be a link to the node itself, can't it?

Rob

Re: [PATCH v6 2/4] of: overlay: global sysfs enable attribute

[Greg Kroah-Hartman]

On Tue, Oct 20, 2015 at 10:13:15PM +0300, Pantelis Antoniou wrote:
> A throw once master enable switch to protect against any
> further overlay applications if the administrator desires so.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 12c3e47..067404e 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)
> @@ -339,6 +344,35 @@ void of_overlay_release(struct kobject *kobj)
>  	kfree(ov);
>  }
>  
> +static ssize_t enable_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
> +}
> +
> +static ssize_t enable_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	int ret;
> +	bool new_enable;
> +
> +	ret = strtobool(buf, &new_enable);
> +	if (ret != 0)
> +		return ret;
> +	/* if we've disabled it, no going back */
> +	if (atomic_read(&ov_enable) == 0)
> +		return -EPERM;
> +	atomic_set(&ov_enable, (int)new_enable);
> +	return count;
> +}
> +
> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
> +
> +static const struct attribute *overlay_global_attrs[] = {
> +	&enable_attr.attr,
> +	NULL
> +};
> +
>  static struct kobj_type of_overlay_ktype = {
>  	.release = of_overlay_release,
>  };
> @@ -360,6 +394,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)
> @@ -594,5 +632,8 @@ int of_overlay_init(void)
>  	if (!ov_kset)
>  		return -ENOMEM;
>  
> -	return 0;
> +	rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
> +	WARN(rc, "%s: error adding global attributes\n", __func__);
> +
> +	return rc;
>  }

Shouldn't this also be allowed to be overridden as a boot and build time
parameter to prevent any races on systems that don't want this?

thanks,

greg k-h

Re: [PATCH v6 0/4] of: overlay: kobject & sysfs'ation

[Rob Herring]

On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> The first patch puts the overlays as objects in the sysfs in
> /sys/firmware/devicetree/overlays.
>
> The next adds a master overlay enable switch (that once is set to
> disabled can't be re-enabled), while the one after that
> introduces a number of default per overlay attributes.
>
> The patchset is against linus's tree as of today.
>
> The last patch updates the ABI docs for the sysfs entries.

I think I told you I would take patches 1 and 2 if you split out the
sysfs documentation for that part of it.

Rob

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Greg Kroah-Hartman]

On Tue, Oct 20, 2015 at 10:13:16PM +0300, Pantelis Antoniou wrote:
> * A per overlay can_remove sysfs attribute that reports whether
> the overlay can be removed or not due to another overlapping overlay.
> 
> * A target sysfs attribute listing the target of each fragment,
> in a group named after the name of the fragment.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/overlay.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 067404e..2d51d9e2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -25,8 +25,23 @@
>  
>  #include "of_private.h"
>  
> +/* fwd. decl */
> +struct of_overlay;
> +struct of_overlay_info;
> +
> +/* an attribute for each fragment */
> +struct fragment_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct kobject *kobj, struct fragment_attribute *fattr,
> +			char *buf);
> +	ssize_t (*store)(struct kobject *kobj, struct fragment_attribute *fattr,
> +			 const char *buf, size_t count);
> +	struct of_overlay_info *ovinfo;
> +};
> +
>  /**
>   * struct of_overlay_info - Holds a single overlay info
> + * @info:	info node that contains the target and overlay
>   * @target:	target of the overlay operation
>   * @overlay:	pointer to the overlay contents node
>   *
> @@ -34,8 +49,13 @@
>   * records.
>   */
>  struct of_overlay_info {
> +	struct of_overlay *ov;
> +	struct device_node *info;
>  	struct device_node *target;
>  	struct device_node *overlay;
> +	struct attribute_group attr_group;
> +	struct attribute *attrs[2];

Why both 2 attributes _and_ an attribute group?  Why not put the
attributes in the attribute group?

And why just one attribute group?  Why not an array of them like the
rest of the kernel is used to handle?

thanks,

greg k-h

Re: [PATCH v6 0/4] of: overlay: kobject & sysfs'ation

[Pantelis Antoniou]

Hi Rob,

> On Oct 21, 2015, at 00:06 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> The first patch puts the overlays as objects in the sysfs in
>> /sys/firmware/devicetree/overlays.
>> 
>> The next adds a master overlay enable switch (that once is set to
>> disabled can't be re-enabled), while the one after that
>> introduces a number of default per overlay attributes.
>> 
>> The patchset is against linus's tree as of today.
>> 
>> The last patch updates the ABI docs for the sysfs entries.
> 
> I think I told you I would take patches 1 and 2 if you split out the
> sysfs documentation for that part of it.
> 

Sorry, but I haven’t seen them being picked up anywhere, so I resent them to be on the safe side.

> Rob

Regards

— Pantelis

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

Hi Rob,

> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> * A per overlay can_remove sysfs attribute that reports whether
>> the overlay can be removed or not due to another overlapping overlay.
>> 
>> * A target sysfs attribute listing the target of each fragment,
>> in a group named after the name of the fragment.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/overlay.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 99 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 067404e..2d51d9e2 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -25,8 +25,23 @@
>> 
>> #include "of_private.h"
>> 
>> +/* fwd. decl */
>> +struct of_overlay;
>> +struct of_overlay_info;
>> +
>> +/* an attribute for each fragment */
>> +struct fragment_attribute {
>> +       struct attribute attr;
>> +       ssize_t (*show)(struct kobject *kobj, struct fragment_attribute *fattr,
>> +                       char *buf);
>> +       ssize_t (*store)(struct kobject *kobj, struct fragment_attribute *fattr,
>> +                        const char *buf, size_t count);
>> +       struct of_overlay_info *ovinfo;
>> +};
>> +
>> /**
>>  * struct of_overlay_info - Holds a single overlay info
>> + * @info:      info node that contains the target and overlay
>>  * @target:    target of the overlay operation
>>  * @overlay:   pointer to the overlay contents node
>>  *
>> @@ -34,8 +49,13 @@
>>  * records.
>>  */
>> struct of_overlay_info {
>> +       struct of_overlay *ov;
>> +       struct device_node *info;
>>        struct device_node *target;
>>        struct device_node *overlay;
>> +       struct attribute_group attr_group;
>> +       struct attribute *attrs[2];
>> +       struct fragment_attribute target_attr;
>> };
>> 
>> /**
>> @@ -52,6 +72,7 @@ struct of_overlay {
>>        struct list_head node;
>>        int count;
>>        struct of_overlay_info *ovinfo_tab;
>> +       const struct attribute_group **attr_groups;
>>        struct of_changeset cset;
>>        struct kobject kobj;
>> };
>> @@ -245,6 +266,8 @@ static int of_fill_overlay_info(struct of_overlay *ov,
>>        if (ovinfo->target == NULL)
>>                goto err_fail;
>> 
>> +       ovinfo->info = of_node_get(info_node);
>> +
>>        return 0;
>> 
>> err_fail:
>> @@ -255,6 +278,17 @@ err_fail:
>>        return -EINVAL;
>> }
>> 
>> +static ssize_t target_show(struct kobject *kobj,
>> +               struct fragment_attribute *fattr, char *buf)
>> +{
>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>> +
>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>> +                       of_node_full_name(ovinfo->target));
> 
> This can be a link to the node itself, can't it?
> 

Yes. Do you want it like this?

> Rob

Regards

— Pantelis

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

Hi Greg,

> On Oct 21, 2015, at 00:08 , Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Oct 20, 2015 at 10:13:16PM +0300, Pantelis Antoniou wrote:
>> * A per overlay can_remove sysfs attribute that reports whether
>> the overlay can be removed or not due to another overlapping overlay.
>> 
>> * A target sysfs attribute listing the target of each fragment,
>> in a group named after the name of the fragment.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/overlay.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 99 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 067404e..2d51d9e2 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -25,8 +25,23 @@
>> 
>> #include "of_private.h"
>> 
>> +/* fwd. decl */
>> +struct of_overlay;
>> +struct of_overlay_info;
>> +
>> +/* an attribute for each fragment */
>> +struct fragment_attribute {
>> +	struct attribute attr;
>> +	ssize_t (*show)(struct kobject *kobj, struct fragment_attribute *fattr,
>> +			char *buf);
>> +	ssize_t (*store)(struct kobject *kobj, struct fragment_attribute *fattr,
>> +			 const char *buf, size_t count);
>> +	struct of_overlay_info *ovinfo;
>> +};
>> +
>> /**
>>  * struct of_overlay_info - Holds a single overlay info
>> + * @info:	info node that contains the target and overlay
>>  * @target:	target of the overlay operation
>>  * @overlay:	pointer to the overlay contents node
>>  *
>> @@ -34,8 +49,13 @@
>>  * records.
>>  */
>> struct of_overlay_info {
>> +	struct of_overlay *ov;
>> +	struct device_node *info;
>> 	struct device_node *target;
>> 	struct device_node *overlay;
>> +	struct attribute_group attr_group;
>> +	struct attribute *attrs[2];
> 
> Why both 2 attributes _and_ an attribute group?  Why not put the
> attributes in the attribute group?
> 
> And why just one attribute group?  Why not an array of them like the
> rest of the kernel is used to handle?
> 

Because this makes it easier to add all the attributes for all the fragments in a single
sysfs_create_groups() call, once for each overlay, instead of having a call to 
sysfs_create_group() for each fragment of the overlay.

Reusing driver core helpers is good, no?

> thanks,
> 
> greg k-h

Regards

— Pantelis

Re: [PATCH v6 2/4] of: overlay: global sysfs enable attribute

[Rob Herring]

On Tue, Oct 20, 2015 at 4:06 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 20, 2015 at 10:13:15PM +0300, Pantelis Antoniou wrote:
>> A throw once master enable switch to protect against any
>> further overlay applications if the administrator desires so.
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>>  drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 12c3e47..067404e 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)
>> @@ -339,6 +344,35 @@ void of_overlay_release(struct kobject *kobj)
>>       kfree(ov);
>>  }
>>
>> +static ssize_t enable_show(struct kobject *kobj,
>> +             struct kobj_attribute *attr, char *buf)
>> +{
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
>> +}
>> +
>> +static ssize_t enable_store(struct kobject *kobj,
>> +             struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> +     int ret;
>> +     bool new_enable;
>> +
>> +     ret = strtobool(buf, &new_enable);
>> +     if (ret != 0)
>> +             return ret;
>> +     /* if we've disabled it, no going back */
>> +     if (atomic_read(&ov_enable) == 0)
>> +             return -EPERM;
>> +     atomic_set(&ov_enable, (int)new_enable);
>> +     return count;
>> +}
>> +
>> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
>> +
>> +static const struct attribute *overlay_global_attrs[] = {
>> +     &enable_attr.attr,
>> +     NULL
>> +};
>> +
>>  static struct kobj_type of_overlay_ktype = {
>>       .release = of_overlay_release,
>>  };
>> @@ -360,6 +394,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)
>> @@ -594,5 +632,8 @@ int of_overlay_init(void)
>>       if (!ov_kset)
>>               return -ENOMEM;
>>
>> -     return 0;
>> +     rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
>> +     WARN(rc, "%s: error adding global attributes\n", __func__);
>> +
>> +     return rc;
>>  }
>
> Shouldn't this also be allowed to be overridden as a boot and build time
> parameter to prevent any races on systems that don't want this?

Build time is already there to disable overlays. A command line option
would be good though.

Rob

Re: [PATCH v6 0/4] of: overlay: kobject & sysfs'ation

[Rob Herring]

On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Oct 21, 2015, at 00:06 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> The first patch puts the overlays as objects in the sysfs in
>>> /sys/firmware/devicetree/overlays.
>>>
>>> The next adds a master overlay enable switch (that once is set to
>>> disabled can't be re-enabled), while the one after that
>>> introduces a number of default per overlay attributes.
>>>
>>> The patchset is against linus's tree as of today.
>>>
>>> The last patch updates the ABI docs for the sysfs entries.
>>
>> I think I told you I would take patches 1 and 2 if you split out the
>> sysfs documentation for that part of it.
>>
>
> Sorry, but I haven’t seen them being picked up anywhere, so I resent them to be on the safe side.

You would need to separate the kill switch sysfs documentation in
patch 4 for me to take just the global part. Or just continue to send
them all. :)

Rob

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Rob Herring]

On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> * A per overlay can_remove sysfs attribute that reports whether
>>> the overlay can be removed or not due to another overlapping overlay.
>>>
>>> * A target sysfs attribute listing the target of each fragment,
>>> in a group named after the name of the fragment.

[...]

>>> @@ -255,6 +278,17 @@ err_fail:
>>>        return -EINVAL;
>>> }
>>>
>>> +static ssize_t target_show(struct kobject *kobj,
>>> +               struct fragment_attribute *fattr, char *buf)
>>> +{
>>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>>> +
>>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>>> +                       of_node_full_name(ovinfo->target));
>>
>> This can be a link to the node itself, can't it?
>>
>
> Yes. Do you want it like this?

Yes, hence the suggestion. Unless you see some reason why not.

Rob

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

[Rob Herring]

On Tue, Oct 20, 2015 at 4:02 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Oct 20, 2015, at 23:56 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Documentation ABI entry for overlays sysfs entries.
>>>
>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>> ---
>>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 40 ++++++++++++++++++++++
>>> 1 file changed, 40 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..adc4068
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>>> @@ -0,0 +1,40 @@
>>> +What:          /sys/firmware/devicetree/overlays/
>>> +Date:          October 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.
>>> +
>>> +               The discussion about this switch takes place in:
>>> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
>>> +
>>> +               Kees Cook:
>>> +               "Coming from the perspective of drawing a bright line between
>>> +               kernel and the root user (which tends to start with disabling
>>> +               kernel module loading), I would say that there at least needs
>>> +               to be a high-level one-way "off" switch for the interface so
>>> +               that systems that have this interface can choose to turn it off
>>> +               during initial boot, etc."
>>> +
>>> +What:          /sys/firmware/devicetree/overlays/<id>
>>> +Date:          October 2015
>>> +Contact:       Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>> +Description:
>>> +               Each directory represents an applied overlay, containing
>>> +               the following attribute files.
>>> +
>>> +               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.
>>> +
>>> +What:          /sys/firmware/devicetree/overlays/<id>/<fragment-name>/
>>> +Date:          October 2015
>>> +Contact:       Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>> +Description:
>>> +               Each of these directories contain information about of the
>>> +               particular overlay fragment.
>>> +
>>> +               target: The full-path of the target of the fragment
>>> --
>>
>> What happened to attributes within the fragment dir?
>>
>
> There’s a single attribute named target that contains the target of the fragment.
>
> At the moment this is the only attribute. I eventually intent to put the full contents of the overlay fragment
> there as /sysfs/firmware/devicetree/base does, but this would make things quite complicated for now.
>
> Should I add a What: line for that too?

There should be an entry for every file. These should not be in the
description. So "enable" and "can_remove" need entries too.

Rob

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Greg Kroah-Hartman]

On Wed, Oct 21, 2015 at 12:15:00AM +0300, Pantelis Antoniou wrote:
> Hi Greg,
> 
> > On Oct 21, 2015, at 00:08 , Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > On Tue, Oct 20, 2015 at 10:13:16PM +0300, Pantelis Antoniou wrote:
> >> * A per overlay can_remove sysfs attribute that reports whether
> >> the overlay can be removed or not due to another overlapping overlay.
> >> 
> >> * A target sysfs attribute listing the target of each fragment,
> >> in a group named after the name of the fragment.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >> ---
> >> drivers/of/overlay.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 99 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >> index 067404e..2d51d9e2 100644
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -25,8 +25,23 @@
> >> 
> >> #include "of_private.h"
> >> 
> >> +/* fwd. decl */
> >> +struct of_overlay;
> >> +struct of_overlay_info;
> >> +
> >> +/* an attribute for each fragment */
> >> +struct fragment_attribute {
> >> +	struct attribute attr;
> >> +	ssize_t (*show)(struct kobject *kobj, struct fragment_attribute *fattr,
> >> +			char *buf);
> >> +	ssize_t (*store)(struct kobject *kobj, struct fragment_attribute *fattr,
> >> +			 const char *buf, size_t count);
> >> +	struct of_overlay_info *ovinfo;
> >> +};
> >> +
> >> /**
> >>  * struct of_overlay_info - Holds a single overlay info
> >> + * @info:	info node that contains the target and overlay
> >>  * @target:	target of the overlay operation
> >>  * @overlay:	pointer to the overlay contents node
> >>  *
> >> @@ -34,8 +49,13 @@
> >>  * records.
> >>  */
> >> struct of_overlay_info {
> >> +	struct of_overlay *ov;
> >> +	struct device_node *info;
> >> 	struct device_node *target;
> >> 	struct device_node *overlay;
> >> +	struct attribute_group attr_group;
> >> +	struct attribute *attrs[2];
> > 
> > Why both 2 attributes _and_ an attribute group?  Why not put the
> > attributes in the attribute group?
> > 
> > And why just one attribute group?  Why not an array of them like the
> > rest of the kernel is used to handle?
> > 
> 
> Because this makes it easier to add all the attributes for all the fragments in a single
> sysfs_create_groups() call, once for each overlay, instead of having a call to 
> sysfs_create_group() for each fragment of the overlay.
> 
> Reusing driver core helpers is good, no?

Yes it is, sorry, I missed how you used these later on, and the
attribute_groups usage there, nice job, sorry for the noise.

greg k-h

Re: [PATCH v6 2/4] of: overlay: global sysfs enable attribute

[Pantelis Antoniou]

Hi Rob,

> On Oct 21, 2015, at 00:50 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Tue, Oct 20, 2015 at 4:06 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Tue, Oct 20, 2015 at 10:13:15PM +0300, Pantelis Antoniou wrote:
>>> A throw once master enable switch to protect against any
>>> further overlay applications if the administrator desires so.
>>> 
>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>> ---
>>> drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 42 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 12c3e47..067404e 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)
>>> @@ -339,6 +344,35 @@ void of_overlay_release(struct kobject *kobj)
>>>      kfree(ov);
>>> }
>>> 
>>> +static ssize_t enable_show(struct kobject *kobj,
>>> +             struct kobj_attribute *attr, char *buf)
>>> +{
>>> +     return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
>>> +}
>>> +
>>> +static ssize_t enable_store(struct kobject *kobj,
>>> +             struct kobj_attribute *attr, const char *buf, size_t count)
>>> +{
>>> +     int ret;
>>> +     bool new_enable;
>>> +
>>> +     ret = strtobool(buf, &new_enable);
>>> +     if (ret != 0)
>>> +             return ret;
>>> +     /* if we've disabled it, no going back */
>>> +     if (atomic_read(&ov_enable) == 0)
>>> +             return -EPERM;
>>> +     atomic_set(&ov_enable, (int)new_enable);
>>> +     return count;
>>> +}
>>> +
>>> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
>>> +
>>> +static const struct attribute *overlay_global_attrs[] = {
>>> +     &enable_attr.attr,
>>> +     NULL
>>> +};
>>> +
>>> static struct kobj_type of_overlay_ktype = {
>>>      .release = of_overlay_release,
>>> };
>>> @@ -360,6 +394,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)
>>> @@ -594,5 +632,8 @@ int of_overlay_init(void)
>>>      if (!ov_kset)
>>>              return -ENOMEM;
>>> 
>>> -     return 0;
>>> +     rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
>>> +     WARN(rc, "%s: error adding global attributes\n", __func__);
>>> +
>>> +     return rc;
>>> }
>> 
>> Shouldn't this also be allowed to be overridden as a boot and build time
>> parameter to prevent any races on systems that don't want this?
> 
> Build time is already there to disable overlays. A command line option
> would be good though.
> 

No problemo. I’ll include it in the next version to go out today.

> Rob

Regards

— Pantelis

Re: [PATCH v6 1/4] of: overlay: kobjectify overlay objects

[Pantelis Antoniou]

Hi Greg,

> On Oct 21, 2015, at 00:03 , Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Oct 20, 2015 at 10:13:14PM +0300, Pantelis Antoniou wrote:
>> We are going to need the overlays to appear on sysfs with runtime
>> global properties (like master enable) so turn them into kobjects.
> 
> Why kobjects and not 'struct device’?
> 

Cause it’s overkill.

There is no hardware/abstract device connection between an overlay and a device as what’s being used right now in driver core.

kobjs are enough to present them in the filesystem hierarchy.

> Why even have them in sysfs at all?  You need more information here as
> to why you want to do this.
> 

They have to be in sysfs so that people can have information about the overlays applied in the system, i.e. where their targets are and whether removal is possible. That’s what’s possible for now; in the future we might present the full contents of the overlay there, and what changes to the live tree were made.

> thanks,
> 
> greg k-h

Regards

— Pantelis

Re: [PATCH v6 1/4] of: overlay: kobjectify overlay objects

[Greg Kroah-Hartman]

On Wed, Oct 21, 2015 at 04:28:33PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
> 
> > On Oct 21, 2015, at 00:03 , Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > On Tue, Oct 20, 2015 at 10:13:14PM +0300, Pantelis Antoniou wrote:
> >> We are going to need the overlays to appear on sysfs with runtime
> >> global properties (like master enable) so turn them into kobjects.
> > 
> > Why kobjects and not 'struct device’?
> > 
> 
> Cause it’s overkill.
> 
> There is no hardware/abstract device connection between an overlay and a device as what’s being used right now in driver core.
> 
> kobjs are enough to present them in the filesystem hierarchy.
> 
> > Why even have them in sysfs at all?  You need more information here as
> > to why you want to do this.
> > 
> 
> They have to be in sysfs so that people can have information about the overlays applied in the system, i.e. where their targets are and whether removal is possible. That’s what’s possible for now; in the future we might present the full contents of the overlay there, and what changes to the live tree were made.

Ok, then say that in the changelog to explain what you are doing here :)

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

Hi Rob,

> On Oct 21, 2015, at 00:54 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi Rob,
>> 
>>> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
>>> 
>>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>> the overlay can be removed or not due to another overlapping overlay.
>>>> 
>>>> * A target sysfs attribute listing the target of each fragment,
>>>> in a group named after the name of the fragment.
> 
> [...]
> 
>>>> @@ -255,6 +278,17 @@ err_fail:
>>>>       return -EINVAL;
>>>> }
>>>> 
>>>> +static ssize_t target_show(struct kobject *kobj,
>>>> +               struct fragment_attribute *fattr, char *buf)
>>>> +{
>>>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>>>> +
>>>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>>>> +                       of_node_full_name(ovinfo->target));
>>> 
>>> This can be a link to the node itself, can't it?
>>> 
>> 
>> Yes. Do you want it like this?
> 
> Yes, hence the suggestion. Unless you see some reason why not.
> 

Nope, can’t be done. The sysfs API only allows linking one kobj to another.
The kobj is the overlay but the target is in the fragment attribute group.

Sorry, I really tried, but can’t be done without hacking in a new link sysfs API. 

> Rob

Regards

— Pantelis

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Rob Herring]

On Wed, Oct 21, 2015 at 2:37 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Oct 21, 2015, at 00:54 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi Rob,
>>>
>>>> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
>>>>
>>>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>>> the overlay can be removed or not due to another overlapping overlay.
>>>>>
>>>>> * A target sysfs attribute listing the target of each fragment,
>>>>> in a group named after the name of the fragment.
>>
>> [...]
>>
>>>>> @@ -255,6 +278,17 @@ err_fail:
>>>>>       return -EINVAL;
>>>>> }
>>>>>
>>>>> +static ssize_t target_show(struct kobject *kobj,
>>>>> +               struct fragment_attribute *fattr, char *buf)
>>>>> +{
>>>>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>>>>> +
>>>>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>>>>> +                       of_node_full_name(ovinfo->target));
>>>>
>>>> This can be a link to the node itself, can't it?
>>>>
>>>
>>> Yes. Do you want it like this?
>>
>> Yes, hence the suggestion. Unless you see some reason why not.
>>
>
> Nope, can’t be done. The sysfs API only allows linking one kobj to another.
> The kobj is the overlay but the target is in the fragment attribute group.

Can't we make the fragments kobj's as well?

Rob

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

Hi Rob,

> On Oct 22, 2015, at 00:52 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Wed, Oct 21, 2015 at 2:37 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi Rob,
>> 
>>> On Oct 21, 2015, at 00:54 , Rob Herring <robherring2@gmail.com> wrote:
>>> 
>>> On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> Hi Rob,
>>>> 
>>>>> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
>>>>> 
>>>>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>>>> the overlay can be removed or not due to another overlapping overlay.
>>>>>> 
>>>>>> * A target sysfs attribute listing the target of each fragment,
>>>>>> in a group named after the name of the fragment.
>>> 
>>> [...]
>>> 
>>>>>> @@ -255,6 +278,17 @@ err_fail:
>>>>>>      return -EINVAL;
>>>>>> }
>>>>>> 
>>>>>> +static ssize_t target_show(struct kobject *kobj,
>>>>>> +               struct fragment_attribute *fattr, char *buf)
>>>>>> +{
>>>>>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>>>>>> +
>>>>>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>>>>>> +                       of_node_full_name(ovinfo->target));
>>>>> 
>>>>> This can be a link to the node itself, can't it?
>>>>> 
>>>> 
>>>> Yes. Do you want it like this?
>>> 
>>> Yes, hence the suggestion. Unless you see some reason why not.
>>> 
>> 
>> Nope, can’t be done. The sysfs API only allows linking one kobj to another.
>> The kobj is the overlay but the target is in the fragment attribute group.
> 
> Can't we make the fragments kobj's as well?
> 

We could, but it break the mental model of what a kobj should represent.
An overlay is an object which can be address, a fragment is never directly
exposed.

TBH a link attribute is indeed better than a path attribute, but marginally so.
It’s not worth the trouble IMO.
 
> Rob

Regards

— Pantelis

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Rob Herring]

On Thu, Oct 22, 2015 at 11:15 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Oct 22, 2015, at 00:52 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Wed, Oct 21, 2015 at 2:37 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi Rob,
>>>
>>>> On Oct 21, 2015, at 00:54 , Rob Herring <robherring2@gmail.com> wrote:
>>>>
>>>> On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>> Hi Rob,
>>>>>
>>>>>> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>>>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>>>>> the overlay can be removed or not due to another overlapping overlay.
>>>>>>>
>>>>>>> * A target sysfs attribute listing the target of each fragment,
>>>>>>> in a group named after the name of the fragment.
>>>>
>>>> [...]
>>>>
>>>>>>> @@ -255,6 +278,17 @@ err_fail:
>>>>>>>      return -EINVAL;
>>>>>>> }
>>>>>>>
>>>>>>> +static ssize_t target_show(struct kobject *kobj,
>>>>>>> +               struct fragment_attribute *fattr, char *buf)
>>>>>>> +{
>>>>>>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>>>>>>> +
>>>>>>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>>>>>>> +                       of_node_full_name(ovinfo->target));
>>>>>>
>>>>>> This can be a link to the node itself, can't it?
>>>>>>
>>>>>
>>>>> Yes. Do you want it like this?
>>>>
>>>> Yes, hence the suggestion. Unless you see some reason why not.
>>>>
>>>
>>> Nope, can’t be done. The sysfs API only allows linking one kobj to another.
>>> The kobj is the overlay but the target is in the fragment attribute group.
>>
>> Can't we make the fragments kobj's as well?
>>
>
> We could, but it break the mental model of what a kobj should represent.
> An overlay is an object which can be address, a fragment is never directly
> exposed.

But you are exposing fragments as there is a directory for each one.

Overlays are just logical collections of fragments. You could go as
far to say overlays don't need to be exposed at all and only fragments
need to be (not that we should). We already have overlays and nodes as
kobjs, so I don't think fragments as kobjs breaks the mental model.

> TBH a link attribute is indeed better than a path attribute, but marginally so.
> It’s not worth the trouble IMO.

It is just an ABI we have to support forever...

Rob

Re: [PATCH v6 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

Hi Rob,

> On Nov 5, 2015, at 21:52 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Thu, Oct 22, 2015 at 11:15 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi Rob,
>> 
>>> On Oct 22, 2015, at 00:52 , Rob Herring <robherring2@gmail.com> wrote:
>>> 
>>> On Wed, Oct 21, 2015 at 2:37 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> Hi Rob,
>>>> 
>>>>> On Oct 21, 2015, at 00:54 , Rob Herring <robherring2@gmail.com> wrote:
>>>>> 
>>>>> On Tue, Oct 20, 2015 at 4:11 PM, Pantelis Antoniou
>>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>>> Hi Rob,
>>>>>> 
>>>>>>> On Oct 21, 2015, at 00:04 , Rob Herring <robherring2@gmail.com> wrote:
>>>>>>> 
>>>>>>> On Tue, Oct 20, 2015 at 2:13 PM, Pantelis Antoniou
>>>>>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>>>>> * A per overlay can_remove sysfs attribute that reports whether
>>>>>>>> the overlay can be removed or not due to another overlapping overlay.
>>>>>>>> 
>>>>>>>> * A target sysfs attribute listing the target of each fragment,
>>>>>>>> in a group named after the name of the fragment.
>>>>> 
>>>>> [...]
>>>>> 
>>>>>>>> @@ -255,6 +278,17 @@ err_fail:
>>>>>>>>     return -EINVAL;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> +static ssize_t target_show(struct kobject *kobj,
>>>>>>>> +               struct fragment_attribute *fattr, char *buf)
>>>>>>>> +{
>>>>>>>> +       struct of_overlay_info *ovinfo = fattr->ovinfo;
>>>>>>>> +
>>>>>>>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>>>>>>>> +                       of_node_full_name(ovinfo->target));
>>>>>>> 
>>>>>>> This can be a link to the node itself, can't it?
>>>>>>> 
>>>>>> 
>>>>>> Yes. Do you want it like this?
>>>>> 
>>>>> Yes, hence the suggestion. Unless you see some reason why not.
>>>>> 
>>>> 
>>>> Nope, can’t be done. The sysfs API only allows linking one kobj to another.
>>>> The kobj is the overlay but the target is in the fragment attribute group.
>>> 
>>> Can't we make the fragments kobj's as well?
>>> 
>> 
>> We could, but it break the mental model of what a kobj should represent.
>> An overlay is an object which can be address, a fragment is never directly
>> exposed.
> 
> But you are exposing fragments as there is a directory for each one.
> 
> Overlays are just logical collections of fragments. You could go as
> far to say overlays don't need to be exposed at all and only fragments
> need to be (not that we should). We already have overlays and nodes as
> kobjs, so I don't think fragments as kobjs breaks the mental model.
> 

I disagree; fragments are merely an internal implementation detail, namely that an overlay
is comprised of a collection of fragments.

Being able to address fragments individually, which is what making a fragment a kobj does,
conceptually, is not making sense.

We are being driven to consider having them as kobjs as a consequence of the gaps in the
sysfs internal kernel API, that is that we can only make links between kobjs’ only not in
arbitrary points from one sysfs directory to another.

>> TBH a link attribute is indeed better than a path attribute, but marginally so.
>> It’s not worth the trouble IMO.
> 
> It is just an ABI we have to support forever…
> 

I don’t think that’s that a big deal IMO. Following a link is no different than reading the path attribute.

> Rob

Regards


— Pantelis