[v3,5/5] clk: export tree topology and clk data via sysfs

Introduces kobject support for the common struct clk, exports per-clk
data via read-only callbacks and models the clk tree topology in sysfs.

Also adds support for generating the clk tree in clk_init and migrating
nodes when input sources are switches in clk_set_parent.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/Kconfig     |   10 +++
 drivers/clk/Makefile    |    1 +
 drivers/clk/clk-sysfs.c |  199 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c       |    5 +-
 include/linux/clk.h     |   36 ++++++++-
 5 files changed, 248 insertions(+), 3 deletions(-)
 create mode 100644 drivers/clk/clk-sysfs.c

On Tuesday 22 November 2011, Mike Turquette wrote:
> Introduces kobject support for the common struct clk, exports per-clk
> data via read-only callbacks and models the clk tree topology in sysfs.
> 
> Also adds support for generating the clk tree in clk_init and migrating
> nodes when input sources are switches in clk_set_parent.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
>  drivers/clk/Kconfig     |   10 +++
>  drivers/clk/Makefile    |    1 +
>  drivers/clk/clk-sysfs.c |  199 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c       |    5 +-
>  include/linux/clk.h     |   36 ++++++++-
>  5 files changed, 248 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/clk/clk-sysfs.c

Since this introduces a new top-level hierarchy in sysfs, it certainly needs
to be reviewed by Greg K-H (on Cc now). Also, you have to add a proper
documentation for every new node and attribute in Documentation/ABI along
with the code.

> +static struct kobject *clk_kobj;
> +LIST_HEAD(kobj_list);

The list head should be static.

	Arnd

On Mon, Nov 21, 2011 at 05:40:47PM -0800, Mike Turquette wrote:
> Introduces kobject support for the common struct clk, exports per-clk
> data via read-only callbacks and models the clk tree topology in sysfs.
> 
> Also adds support for generating the clk tree in clk_init and migrating
> nodes when input sources are switches in clk_set_parent.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>

Thanks Arnd for pointing me at this.

> +#define MAX_STRING_LENGTH	32

Why?

> +static struct kobject *clk_kobj;
> +LIST_HEAD(kobj_list);

Um, no, please NEVER use raw kobjects, you should be using struct device
instead.  Please change the code to do that.

> +static ssize_t clk_show(struct kobject *kobj, struct attribute *attr,
> +		char *buf)
> +{
> +	struct clk *clk;
> +	struct clk_attribute *clk_attr;
> +	ssize_t ret = -EINVAL;
> +
> +	clk = container_of(kobj, struct clk, kobj);
> +	clk_attr = container_of(attr, struct clk_attribute, attr);
> +
> +	if (!clk || !clk_attr)
> +		goto out;
> +
> +	/* we don't do any locking for debug operations */
> +
> +	/* refcount++ */
> +	kobject_get(&clk->kobj);
> +
> +	if (clk_attr->show)
> +		ret = clk_attr->show(clk, buf);
> +	else
> +		ret = -EIO;
> +
> +	/* refcount-- */
> +	kobject_put(&clk->kobj);

Why in the world would you be incrementing and decrementing the
reference count of the kobject in the show function?  What are you
trying to protect here that is not already protected by the core?


> +static void clk_release(struct kobject *kobj)
> +{
> +	struct clk *clk;
> +
> +	clk = container_of(kobj, struct clk, kobj);
> +
> +	complete(&clk->kobj_unregister);
> +}

Look Ma, a memory leak!

> +static struct kobj_type clk_ktype = {
> +	.sysfs_ops	= &clk_ops,
> +	.default_attrs	= clk_default_attrs,
> +	.release	= clk_release,
> +};
> +
> +int clk_kobj_add(struct clk *clk)
> +{
> +	if (IS_ERR(clk))
> +		return -EINVAL;
> +
> +	/*
> +	 * Some kobject trickery!
> +	 *
> +	 * We want to (ab)use the kobject infrastructure to track our
> +	 * tree topology for us, specifically the root clocks (which are
> +	 * otherwise not remembered in a global list).
> +	 * Unfortunately we might not be able to allocate memory yet
> +	 * when this path is hit.  This pretty much rules out anything
> +	 * that looks or smells like kobject_add, since there are
> +	 * allocations for kobject->name and a dependency on sysfs being
> +	 * initialized.
> +	 *
> +	 * To get around this we initialize the kobjects and (ab)use
> +	 * struct kobject's list_head member, "entry".  Later on we walk
> +	 * this list in clk_sysfs_tree_create() to make proper
> +	 * kobject_add calls once it is safe to do so.
> +	 *
> +	 * FIXME - this is starting to smell alot like clkdev (i.e.
> +	 * tracking the clocks in a list)

Ah, comments like this warm my heart.

Come on, no abusing the kobject code please, if have problems with how
the kernel core works, and it doesn't do things you want it to, then why
not change it to work properly for you, or at the least, ASK ME!!!

> +	 */
> +
> +	kobject_init(&clk->kobj, &clk_ktype);
> +	list_add_tail(&clk->kobj.entry, &kobj_list);

Yeah, no kobject lists for you, sorry, DO NOT DO THIS!

> +static int __init clk_sysfs_init(void)
> +{
> +	struct list_head *tmp;
> +
> +	clk_kobj = kobject_create_and_add("clk", NULL);
> +
> +	WARN_ON(!clk_kobj);

Why?  What is this helping with?

Please rewrite to use 'struct device'.

thanks,

greg k-h

On Tue, Nov 22, 2011 at 5:07 AM, Arnd Bergmann <arnd.bergmann@linaro.org> wrote:
> On Tuesday 22 November 2011, Mike Turquette wrote:
>> Introduces kobject support for the common struct clk, exports per-clk
>> data via read-only callbacks and models the clk tree topology in sysfs.
>>
>> Also adds support for generating the clk tree in clk_init and migrating
>> nodes when input sources are switches in clk_set_parent.
>>
>> Signed-off-by: Mike Turquette <mturquette@linaro.org>
>> ---
>>  drivers/clk/Kconfig     |   10 +++
>>  drivers/clk/Makefile    |    1 +
>>  drivers/clk/clk-sysfs.c |  199 +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/clk.c       |    5 +-
>>  include/linux/clk.h     |   36 ++++++++-
>>  5 files changed, 248 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/clk/clk-sysfs.c
>
> Since this introduces a new top-level hierarchy in sysfs, it certainly needs
> to be reviewed by Greg K-H (on Cc now). Also, you have to add a proper
> documentation for every new node and attribute in Documentation/ABI along
> with the code.

I don't intend to keep it in the top-level /sys/ directory.  I
mentioned that in the coverletter, but not here.  I wasn't sure where
to put it, and I knew reviewers would be happy to point the right
place out to me.

>> +static struct kobject *clk_kobj;
>> +LIST_HEAD(kobj_list);
>
> The list head should be static.

Will put into V4.

Thanks,
Mike

>
>        Arnd
>

On Tue, Nov 22, 2011 at 7:49 AM, Greg KH <greg@kroah.com> wrote:
> On Mon, Nov 21, 2011 at 05:40:47PM -0800, Mike Turquette wrote:
>> Introduces kobject support for the common struct clk, exports per-clk
>> data via read-only callbacks and models the clk tree topology in sysfs.
>>
>> Also adds support for generating the clk tree in clk_init and migrating
>> nodes when input sources are switches in clk_set_parent.
>>
>> Signed-off-by: Mike Turquette <mturquette@linaro.org>
>
> Thanks Arnd for pointing me at this.
>
>> +#define MAX_STRING_LENGTH    32
>
> Why?

Copy paste from other implementations.  What do you recommend?

>
>> +static struct kobject *clk_kobj;
>> +LIST_HEAD(kobj_list);
>
> Um, no, please NEVER use raw kobjects, you should be using struct device
> instead.  Please change the code to do that.

Today the clk tree doesn't create a struct device for each clk, which
I assume would be necessary to do what you're talking about.  Is that
right?

Maybe that will be necessary with the device tree stuff anyways... any
feedback from Sascha or Grant on that?

>
>> +static ssize_t clk_show(struct kobject *kobj, struct attribute *attr,
>> +             char *buf)
>> +{
>> +     struct clk *clk;
>> +     struct clk_attribute *clk_attr;
>> +     ssize_t ret = -EINVAL;
>> +
>> +     clk = container_of(kobj, struct clk, kobj);
>> +     clk_attr = container_of(attr, struct clk_attribute, attr);
>> +
>> +     if (!clk || !clk_attr)
>> +             goto out;
>> +
>> +     /* we don't do any locking for debug operations */
>> +
>> +     /* refcount++ */
>> +     kobject_get(&clk->kobj);
>> +
>> +     if (clk_attr->show)
>> +             ret = clk_attr->show(clk, buf);
>> +     else
>> +             ret = -EIO;
>> +
>> +     /* refcount-- */
>> +     kobject_put(&clk->kobj);
>
> Why in the world would you be incrementing and decrementing the
> reference count of the kobject in the show function?  What are you
> trying to protect here that is not already protected by the core?

Will remove in v4.

>
>
>> +static void clk_release(struct kobject *kobj)
>> +{
>> +     struct clk *clk;
>> +
>> +     clk = container_of(kobj, struct clk, kobj);
>> +
>> +     complete(&clk->kobj_unregister);
>> +}
>
> Look Ma, a memory leak!

Oops.  Will fix in V4.

>
>> +static struct kobj_type clk_ktype = {
>> +     .sysfs_ops      = &clk_ops,
>> +     .default_attrs  = clk_default_attrs,
>> +     .release        = clk_release,
>> +};
>> +
>> +int clk_kobj_add(struct clk *clk)
>> +{
>> +     if (IS_ERR(clk))
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * Some kobject trickery!
>> +      *
>> +      * We want to (ab)use the kobject infrastructure to track our
>> +      * tree topology for us, specifically the root clocks (which are
>> +      * otherwise not remembered in a global list).
>> +      * Unfortunately we might not be able to allocate memory yet
>> +      * when this path is hit.  This pretty much rules out anything
>> +      * that looks or smells like kobject_add, since there are
>> +      * allocations for kobject->name and a dependency on sysfs being
>> +      * initialized.
>> +      *
>> +      * To get around this we initialize the kobjects and (ab)use
>> +      * struct kobject's list_head member, "entry".  Later on we walk
>> +      * this list in clk_sysfs_tree_create() to make proper
>> +      * kobject_add calls once it is safe to do so.
>> +      *
>> +      * FIXME - this is starting to smell alot like clkdev (i.e.
>> +      * tracking the clocks in a list)
>
> Ah, comments like this warm my heart.
>
> Come on, no abusing the kobject code please, if have problems with how
> the kernel core works, and it doesn't do things you want it to, then why
> not change it to work properly for you, or at the least, ASK ME!!!

Ok, I'm asking you now.  There are two ways to solve this problem:

1) have kobject core create the lists linking the objects but defer
allocations and any interactions with sysfs until later in the boot
sequence, OR

2) my code can create a list of clks (the same way that clkdev does)
and defer kobject/sysfs stuff until later, which walks the list made
during early-boot

#1 is most closely aligned with the code I have here, #2 presents
challenges that I haven't really though through.  I know that OMAP
uses the clk framework VERY early in it's boot sequence, but as long
as the per-clk data is properly initialized then it should be OK.

What do you think?

Regards,
Mike

On Tue, Nov 22, 2011 at 09:57:41AM -0800, Mike Turquette wrote:
> > Ah, comments like this warm my heart.
> >
> > Come on, no abusing the kobject code please, if have problems with how
> > the kernel core works, and it doesn't do things you want it to, then why
> > not change it to work properly for you, or at the least, ASK ME!!!
> 
> Ok, I'm asking you now.  There are two ways to solve this problem:
> 
> 1) have kobject core create the lists linking the objects but defer
> allocations and any interactions with sysfs until later in the boot
> sequence, OR
> 
> 2) my code can create a list of clks (the same way that clkdev does)
> and defer kobject/sysfs stuff until later, which walks the list made
> during early-boot
> 
> #1 is most closely aligned with the code I have here, #2 presents
> challenges that I haven't really though through.  I know that OMAP
> uses the clk framework VERY early in it's boot sequence, but as long
> as the per-clk data is properly initialized then it should be OK.
> 
> What do you think?

#3 - use debugfs and don't try to create a sysfs interface for the clock
structures :)

thanks,

greg k-h