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

Hi Greg,

On Oct 4, 2015, at 22:22 , Greg Kroah-Hartman [off-list ref] wrote:

On Wed, Sep 16, 2015 at 07:09:08PM +0300, Pantelis Antoniou wrote:

quoted

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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
drivers/of/base.c       |  7 +++++++
drivers/of/of_private.h |  9 +++++++++
drivers/of/overlay.c    | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 66 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..eeb3ec2 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);

NULL for the parent?  Really?  Where does that cause this kobject to
show up in the sysfs tree?

It shows up in the root of it’s kset, i.e. /sys/firmware/devicetree/overlays

It’s a common idiom in the kernel as evident by:

$ grep 'kobject_add([^,]*,[^,]NULL' drivers/ -r
drivers/of/base.c:		rc = kobject_add(&np->kobj, NULL, "%s",
drivers/firmware/memmap.c:	if (kobject_add(&entry->kobj, NULL, "%d", map_entries_nr++))
drivers/firmware/efi/runtime-map.c:	ret = kobject_add(&entry->kobj, NULL, "%d", nr);

quoted

@@ -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);

Two drops of the reference?

There’s a single drop. Only kobject_put drops the reference. But you’re right,
the kobject_del is not required; it’s performed implicitly by the kobj_cleanup()
method.

Fixing in the new patch.

void of_overlay_release(struct kobject *kobj)
{
        struct of_overlay *ov = kobj_to_overlay(kobj);

        of_node_put(ov->target_root);
        kfree(ov->indirect_id);
        kfree(ov);
}

quoted

	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);

Again, why 2?

thanks,

greg k-h

Regards

— Pantelis