[PATCH 5/6] sysfs: add support for binary attributes in groups

[PATCH 5/6] sysfs: add support for binary attributes in groups

[Greg Kroah-Hartman]

groups should be able to support binary attributes, just like it
supports "normal" attributes.  This lets us only handle one type of
structure, groups, throughout the driver core and subsystems, making
binary attributes a "full fledged" part of the driver model, and not
something just "tacked on".

Reported-by: Oliver Schinagl <oliver+list@schinagl.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/sysfs/group.c      | 18 ++++++++++++++++--
 include/linux/sysfs.h |  4 ++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c..d8d8a8f 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -20,16 +20,19 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			 const struct attribute_group *grp)
 {
 	struct attribute *const* attr;
-	int i;
+	struct bin_attribute *const* bin_attr;
 
-	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+	for (attr = grp->attrs; *attr; attr++)
 		sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
+		sysfs_remove_bin_file(kobj, *bin_attr);
 }
 
 static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 			const struct attribute_group *grp, int update)
 {
 	struct attribute *const* attr;
+	struct bin_attribute *const* bin_attr;
 	int error = 0, i;
 
 	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
@@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 	}
 	if (error)
 		remove_files(dir_sd, kobj, grp);
+
+	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+		if (update)
+			sysfs_remove_bin_file(kobj, *bin_attr);
+		error = sysfs_create_bin_file(kobj, *bin_attr);
+		if (error)
+			break;
+	}
+	if (error)
+		remove_files(dir_sd, kobj, grp);
+
 	return error;
 }
 
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d50a96b..2c3b6a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -21,6 +21,7 @@
 
 struct kobject;
 struct module;
+struct bin_attribute;
 enum kobj_ns_type;
 
 struct attribute {
@@ -59,10 +60,9 @@ struct attribute_group {
 	umode_t			(*is_visible)(struct kobject *,
 					      struct attribute *, int);
 	struct attribute	**attrs;
+	struct bin_attribute	**bin_attrs;
 };
 
-
-
 /**
  * Use these macros to make defining attributes easier. See include/linux/device.h
  * for examples..
-- 
1.8.3.rc0.20.gb99dd2e

Re: [PATCH 5/6] sysfs: add support for binary attributes in groups

[Guenter Roeck]

On Wed, Jul 10, 2013 at 01:05:13PM -0700, Greg Kroah-Hartman wrote:
> groups should be able to support binary attributes, just like it
> supports "normal" attributes.  This lets us only handle one type of
> structure, groups, throughout the driver core and subsystems, making
> binary attributes a "full fledged" part of the driver model, and not
> something just "tacked on".
> 
> Reported-by: Oliver Schinagl <oliver+list@schinagl.nl>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/sysfs/group.c      | 18 ++++++++++++++++--
>  include/linux/sysfs.h |  4 ++--
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index aec3d5c..d8d8a8f 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -20,16 +20,19 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  			 const struct attribute_group *grp)
>  {
>  	struct attribute *const* attr;
> -	int i;
> +	struct bin_attribute *const* bin_attr;

checkpatch doesn't like the above and similar declarations below.

Yes, I know, nitpick, just saying ;).

>  
> -	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> +	for (attr = grp->attrs; *attr; attr++)
>  		sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
> +	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
> +		sysfs_remove_bin_file(kobj, *bin_attr);
>  }
>  
>  static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  			const struct attribute_group *grp, int update)
>  {
>  	struct attribute *const* attr;
> +	struct bin_attribute *const* bin_attr;
>  	int error = 0, i;
>  
>  	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> @@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  	}
>  	if (error)
>  		remove_files(dir_sd, kobj, grp);

You might want to abort here if there was an error.

> +
> +	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {

What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
reference when accessing *bin_attr ?

[ The problem didn't exist before since attr was supposed to be non-NULL.
  It may now be a problem if only bin_attr is provided, ie if there are
  only binary attribute files.
  Or maybe I am just tired and there is no problem... ]

> +		if (update)
> +			sysfs_remove_bin_file(kobj, *bin_attr);
> +		error = sysfs_create_bin_file(kobj, *bin_attr);
> +		if (error)
> +			break;
> +	}
> +	if (error)
> +		remove_files(dir_sd, kobj, grp);
> +
>  	return error;
>  }
>  
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index d50a96b..2c3b6a3 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -21,6 +21,7 @@
>  
>  struct kobject;
>  struct module;
> +struct bin_attribute;
>  enum kobj_ns_type;
>  
>  struct attribute {
> @@ -59,10 +60,9 @@ struct attribute_group {
>  	umode_t			(*is_visible)(struct kobject *,
>  					      struct attribute *, int);
>  	struct attribute	**attrs;
> +	struct bin_attribute	**bin_attrs;
>  };
>  
> -
> -
>  /**
>   * Use these macros to make defining attributes easier. See include/linux/device.h
>   * for examples..
> -- 
> 1.8.3.rc0.20.gb99dd2e
> 
> 

Re: [PATCH 5/6] sysfs: add support for binary attributes in groups

[Greg Kroah-Hartman]

On Wed, Jul 10, 2013 at 04:00:57PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 01:05:13PM -0700, Greg Kroah-Hartman wrote:
> > @@ -52,6 +55,17 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> >  	}
> >  	if (error)
> >  		remove_files(dir_sd, kobj, grp);
> 
> You might want to abort here if there was an error.

Good point, I'll update it.

> > +
> > +	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> 
> What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
> reference when accessing *bin_attr ?
> 
> [ The problem didn't exist before since attr was supposed to be non-NULL.
>   It may now be a problem if only bin_attr is provided, ie if there are
>   only binary attribute files.
>   Or maybe I am just tired and there is no problem... ]

I thought I tested this patch out, but I think you are right, let me go
do a rebuild and test it again...

thanks,

greg k-h

Re: [PATCH 5/6] sysfs: add support for binary attributes in groups

[Greg Kroah-Hartman]

On Wed, Jul 10, 2013 at 04:59:28PM -0700, Greg Kroah-Hartman wrote:
> On Wed, Jul 10, 2013 at 04:00:57PM -0700, Guenter Roeck wrote:
> > > +	for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > 
> > What happens here if grp->bin_attrs is NULL ? Doesn't that cause a NULL
> > reference when accessing *bin_attr ?
> > 
> > [ The problem didn't exist before since attr was supposed to be non-NULL.
> >   It may now be a problem if only bin_attr is provided, ie if there are
> >   only binary attribute files.
> >   Or maybe I am just tired and there is no problem... ]
> 
> I thought I tested this patch out, but I think you are right, let me go
> do a rebuild and test it again...

You were right, good catch, as-is, the kernel dies at boot, I've fixed
this and will resend the series, thanks so much for the review.

greg k-h