Hi Greg,
> On Oct 4, 2015, at 22:22 , Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Sep 16, 2015 at 07:09:08PM +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.
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>> ---
>> 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);
>> @@ -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);
> }
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
