Re: [PATCH 1/3] sysfs representation of stacked devices (common) (rev.2)

[Greg KH <gregkh@suse.de>]

On Thu, Feb 23, 2006 at 02:15:45PM -0500, Jun'ichi Nomura wrote:
> Hello Greg,
> 
> >>>>+/* This is a mere directory in sysfs. No methods are needed. */
> >>>>+static struct kobj_type bd_holder_ktype = {
> >>>>+	.release	= NULL,
> >>>>+	.sysfs_ops	= NULL,
> >>>>+	.default_attrs	= NULL,
> >>>>+};
> >>>
> >>>That doesn't look right.  You always need a release function.
> 
> I updated the patch based your comments.
> Could you take a look at this version whether there's
> any problematic use of sysfs/kobjects?
> 
>   - I removed embedded child-kobjects from struct block_device
>     and struct gendisk which I added in my previous patch.
>     Kobject registration occurs when gendisk or hd_struct is
>     registered. Release function of the kobject type is added.
>   - Reference counting of kobjects is done in much symmetric
>     manner than before.
>   - Added bd_claim_by_disk/bd_release_from_disk inline functions
>     to help proper reference counting.

Looks great, only one comment:

> --- linux-2.6.16-rc4/fs/partitions/check.c	2006-02-17 17:23:45.000000000 -0500
> +++ linux-2.6.16-rc4/fs/partitions/check.c	2006-02-22 23:18:06.000000000 -0500
> @@ -297,6 +297,56 @@ struct kobj_type ktype_part = {
>  	.sysfs_ops	= &part_sysfs_ops,
>  };
>  
> +static void dir_release(struct kobject *kobj)
> +{
> +	kfree(kobj);
> +}
> +
> +static struct kobj_type dir_ktype = {
> +	.release	= dir_release,
> +	.sysfs_ops	= NULL,
> +	.default_attrs	= NULL,
> +};
> +
> +static inline struct kobject *add_dir(struct kobject *parent, const char *name)
> +{
> +	struct kobject *k;
> +
> +	if (!parent)
> +		return NULL;
> +
> +	k = kmalloc(sizeof(*k), GFP_KERNEL);
> +	if (!k)
> +		return NULL;
> +
> +	memset(k, 0, sizeof(*k));
> +	k->parent = parent;
> +	k->ktype = &dir_ktype;
> +	kobject_set_name(k, name);
> +	kobject_register(k);
> +
> +	return k;
> +}

This code looks good enough that we should add it to the core kobject
code, don't you think?  Also, you might use kzalloc instead of kmalloc
here.

thanks,

greg k-h