[PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Greg Kroah-Hartman]

Hi all,

Here is the third iteration of the patchset to add better attribute
group support to the driver core and sysfs.

Others have now tested it, and we got 3 more patches fixing a warning
that was showing up when binary attributes were in a group with no
"normal" attributes, and some more helper macros were added and cleaned
up.

I've booted this successfully against 3.11-rc1 with no problems (am
sending these from that machine.)

Ian, I didn't add a DEVICE_PATTR() macro yet, that can come later.  I
considered it, but that would also mean a BUS_PATTR() and the like, which I
guess works, but I hate even a _hint_ of "Hungarian" notation, which this
implies.

Anyone else have a better name for this macro:
	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
that they can think of?  It will cut down on the typing done for
attribute lists.

changes from v3:
	- tested by Guenter
	- patches from Oliver added
	- refreshed on 3.11-rc1 (minor fuzz)
	- Ian glanced at them.

changes from v2:
	- actually boots
	- 7th patch added properly
	- added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
	  to help with converting code to use attributes properly.

thanks,

greg k-h

[PATCH 01/10] sysfs.h: add __ATTR_RW() macro

[Greg Kroah-Hartman]

A number of parts of the kernel created their own version of this, might
as well have the sysfs core provide it instead.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/sysfs.h | 2 ++
 kernel/events/core.c  | 2 --
 mm/backing-dev.c      | 2 --
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e2cee22f..9cd20c84 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -79,6 +79,8 @@ struct attribute_group {
 	.show	= _name##_show,					\
 }
 
+#define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
+
 #define __ATTR_NULL { .attr = { .name = NULL } }
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
diff --git a/kernel/events/core.c b/kernel/events/core.c
index eba8fb58..dd987802 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6234,8 +6234,6 @@ perf_event_mux_interval_ms_store(struct device *dev,
 	return count;
 }
 
-#define __ATTR_RW(attr) __ATTR(attr, 0644, attr##_show, attr##_store)
-
 static struct device_attribute pmu_dev_attrs[] = {
 	__ATTR_RO(type),
 	__ATTR_RW(perf_event_mux_interval_ms),
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index d014ee5f..e04454cd 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -232,8 +232,6 @@ static ssize_t stable_pages_required_show(struct device *dev,
 			bdi_cap_stable_pages_required(bdi) ? 1 : 0);
 }
 
-#define __ATTR_RW(attr) __ATTR(attr, 0644, attr##_show, attr##_store)
-
 static struct device_attribute bdi_dev_attrs[] = {
 	__ATTR_RW(read_ahead_kb),
 	__ATTR_RW(min_ratio),
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 02/10] sysfs.h: add ATTRIBUTE_GROUPS() macro

[Greg Kroah-Hartman]

To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/sysfs.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c84..f62ff01e 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
 #define __ATTR_IGNORE_LOCKDEP	__ATTR
 #endif
 
+#define ATTRIBUTE_GROUPS(name)					\
+static const struct attribute_group name##_group = {		\
+	.attrs = name##_attrs,					\
+};								\
+static const struct attribute_group *name##_groups[] = {	\
+	&name##_group,						\
+	NULL,							\
+}
+
 #define attr_name(_attr) (_attr).attr.name
 
 struct file;
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 03/10] sysfs.h: add BIN_ATTR macro

[Greg Kroah-Hartman]

This makes it easier to create static binary attributes, which is needed
in a number of drivers, instead of "open coding" them.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/sysfs.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f62ff01e..d50a96b9 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -132,6 +132,15 @@ struct bin_attribute {
  */
 #define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
 
+/* macro to create static binary attributes easier */
+#define BIN_ATTR(_name, _mode, _read, _write, _size)		\
+struct bin_attribute bin_attr_##_name = {			\
+	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.read	= _read,					\
+	.write	= _write,					\
+	.size	= _size,					\
+}
+
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 04/10] driver core: device.h: add RW and RO attribute macros

[Greg Kroah-Hartman]

Make it easier to create attributes without having to always audit the
mode settings.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/device.h | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index bcf8c0d4..f207a8f4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -47,7 +47,11 @@ struct bus_attribute {
 };
 
 #define BUS_ATTR(_name, _mode, _show, _store)	\
-struct bus_attribute bus_attr_##_name = __ATTR(_name, _mode, _show, _store)
+	struct bus_attribute bus_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define BUS_ATTR_RW(_name) \
+	struct bus_attribute bus_attr_##_name = __ATTR_RW(_name)
+#define BUS_ATTR_RO(_name) \
+	struct bus_attribute bus_attr_##_name = __ATTR_RO(_name)
 
 extern int __must_check bus_create_file(struct bus_type *,
 					struct bus_attribute *);
@@ -261,9 +265,12 @@ struct driver_attribute {
 			 size_t count);
 };
 
-#define DRIVER_ATTR(_name, _mode, _show, _store)	\
-struct driver_attribute driver_attr_##_name =		\
-	__ATTR(_name, _mode, _show, _store)
+#define DRIVER_ATTR(_name, _mode, _show, _store) \
+	struct driver_attribute driver_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define DRIVER_ATTR_RW(_name) \
+	struct driver_attribute driver_attr_##_name = __ATTR_RW(_name)
+#define DRIVER_ATTR_RO(_name) \
+	struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
 
 extern int __must_check driver_create_file(struct device_driver *driver,
 					const struct driver_attribute *attr);
@@ -414,8 +421,12 @@ struct class_attribute {
 				 const struct class_attribute *attr);
 };
 
-#define CLASS_ATTR(_name, _mode, _show, _store)			\
-struct class_attribute class_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define CLASS_ATTR(_name, _mode, _show, _store) \
+	struct class_attribute class_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define CLASS_ATTR_RW(_name) \
+	struct class_attribute class_attr_##_name = __ATTR_RW(_name)
+#define CLASS_ATTR_RO(_name) \
+	struct class_attribute class_attr_##_name = __ATTR_RO(_name)
 
 extern int __must_check class_create_file(struct class *class,
 					  const struct class_attribute *attr);
@@ -423,7 +434,6 @@ extern void class_remove_file(struct class *class,
 			      const struct class_attribute *attr);
 
 /* Simple class attribute that is just a static string */
-
 struct class_attribute_string {
 	struct class_attribute attr;
 	char *str;
@@ -512,6 +522,10 @@ ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
 
 #define DEVICE_ATTR(_name, _mode, _show, _store) \
 	struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)
+#define DEVICE_ATTR_RW(_name) \
+	struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
+#define DEVICE_ATTR_RO(_name) \
+	struct device_attribute dev_attr_##_name = __ATTR_RO(_name)
 #define DEVICE_ULONG_ATTR(_name, _mode, _var) \
 	struct dev_ext_attribute dev_attr_##_name = \
 		{ __ATTR(_name, _mode, device_show_ulong, device_store_ulong), &(_var) }
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 05/10] 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@schinagl.nl>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/sysfs/group.c      | 66 +++++++++++++++++++++++++++++++++++----------------
 include/linux/sysfs.h |  4 ++--
 2 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c9..e5719c60 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -20,38 +20,64 @@ 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++)
-		sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+	if (grp->attrs)
+		for (attr = grp->attrs; *attr; attr++)
+			sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+	if (grp->bin_attrs)
+		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++) {
-		umode_t mode = 0;
+	if (grp->attrs) {
+		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
+			umode_t mode = 0;
+
+			/*
+			 * In update mode, we're changing the permissions or
+			 * visibility.  Do this by first removing then
+			 * re-adding (if required) the file.
+			 */
+			if (update)
+				sysfs_hash_and_remove(dir_sd, NULL,
+						      (*attr)->name);
+			if (grp->is_visible) {
+				mode = grp->is_visible(kobj, *attr, i);
+				if (!mode)
+					continue;
+			}
+			error = sysfs_add_file_mode(dir_sd, *attr,
+						    SYSFS_KOBJ_ATTR,
+						    (*attr)->mode | mode);
+			if (unlikely(error))
+				break;
+		}
+		if (error) {
+			remove_files(dir_sd, kobj, grp);
+			goto exit;
+		}
+	}
 
-		/* in update mode, we're changing the permissions or
-		 * visibility.  Do this by first removing then
-		 * re-adding (if required) the file */
-		if (update)
-			sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
-		if (grp->is_visible) {
-			mode = grp->is_visible(kobj, *attr, i);
-			if (!mode)
-				continue;
+	if (grp->bin_attrs) {
+		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;
 		}
-		error = sysfs_add_file_mode(dir_sd, *attr, SYSFS_KOBJ_ATTR,
-					    (*attr)->mode | mode);
-		if (unlikely(error))
-			break;
+		if (error)
+			remove_files(dir_sd, kobj, grp);
 	}
-	if (error)
-		remove_files(dir_sd, kobj, grp);
+exit:
 	return error;
 }
 
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d50a96b9..2c3b6a30 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

[PATCH 06/10] sysfs: prevent warning when only using binary attributes

[Greg Kroah-Hartman]

From: Oliver Schinagl <oliver@schinagl.nl>

When only using bin_attrs instead of attrs the kernel prints a warning
and refuses to create the sysfs entry. This fixes that.

Signed-off-by: Oliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/sysfs/group.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index e5719c60..09a1a25c 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -93,8 +93,8 @@ static int internal_create_group(struct kobject *kobj, int update,
 	/* Updates may happen before the object has been instantiated */
 	if (unlikely(update && !kobj->sd))
 		return -EINVAL;
-	if (!grp->attrs) {
-		WARN(1, "sysfs: attrs not set by subsystem for group: %s/%s\n",
+	if (!grp->attrs && !grp->bin_attrs) {
+		WARN(1, "sysfs: (bin_)attrs not set by subsystem for group: %s/%s\n",
 			kobj->name, grp->name ? "" : grp->name);
 		return -EINVAL;
 	}
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 07/10] driver core: Introduce device_create_groups

[Greg Kroah-Hartman]

From: Guenter Roeck <linux@roeck-us.net>

device_create_groups lets callers create devices as well as associated
sysfs attributes with a single call. This avoids race conditions seen
if sysfs attributes on new devices are created later.

[fixed up comment block placement and add checks for printk buffer
formats - gregkh]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/core.c    | 111 ++++++++++++++++++++++++++++++++++++-------------
 include/linux/device.h |   5 +++
 2 files changed, 88 insertions(+), 28 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index dc3ea237..a8aae182 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1667,34 +1667,11 @@ static void device_create_release(struct device *dev)
 	kfree(dev);
 }
 
-/**
- * device_create_vargs - creates a device and registers it with sysfs
- * @class: pointer to the struct class that this device should be registered to
- * @parent: pointer to the parent struct device of this new device, if any
- * @devt: the dev_t for the char device to be added
- * @drvdata: the data to be added to the device for callbacks
- * @fmt: string for the device's name
- * @args: va_list for the device's name
- *
- * This function can be used by char device classes.  A struct device
- * will be created in sysfs, registered to the specified class.
- *
- * A "dev" file will be created, showing the dev_t for the device, if
- * the dev_t is not 0,0.
- * If a pointer to a parent struct device is passed in, the newly created
- * struct device will be a child of that device in sysfs.
- * The pointer to the struct device will be returned from the call.
- * Any further sysfs files that might be required can be created using this
- * pointer.
- *
- * Returns &struct device pointer on success, or ERR_PTR() on error.
- *
- * Note: the struct class passed to this function must have previously
- * been created with a call to class_create().
- */
-struct device *device_create_vargs(struct class *class, struct device *parent,
-				   dev_t devt, void *drvdata, const char *fmt,
-				   va_list args)
+static struct device *
+device_create_groups_vargs(struct class *class, struct device *parent,
+			   dev_t devt, void *drvdata,
+			   const struct attribute_group **groups,
+			   const char *fmt, va_list args)
 {
 	struct device *dev = NULL;
 	int retval = -ENODEV;
@@ -1711,6 +1688,7 @@ struct device *device_create_vargs(struct class *class, struct device *parent,
 	dev->devt = devt;
 	dev->class = class;
 	dev->parent = parent;
+	dev->groups = groups;
 	dev->release = device_create_release;
 	dev_set_drvdata(dev, drvdata);
 
@@ -1728,6 +1706,39 @@ error:
 	put_device(dev);
 	return ERR_PTR(retval);
 }
+
+/**
+ * device_create_vargs - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @fmt: string for the device's name
+ * @args: va_list for the device's name
+ *
+ * This function can be used by char device classes.  A struct device
+ * will be created in sysfs, registered to the specified class.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_vargs(struct class *class, struct device *parent,
+				   dev_t devt, void *drvdata, const char *fmt,
+				   va_list args)
+{
+	return device_create_groups_vargs(class, parent, devt, drvdata, NULL,
+					  fmt, args);
+}
 EXPORT_SYMBOL_GPL(device_create_vargs);
 
 /**
@@ -1767,6 +1778,50 @@ struct device *device_create(struct class *class, struct device *parent,
 }
 EXPORT_SYMBOL_GPL(device_create);
 
+/**
+ * device_create_with_groups - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @groups: NULL-terminated list of attribute groups to be created
+ * @fmt: string for the device's name
+ *
+ * This function can be used by char device classes.  A struct device
+ * will be created in sysfs, registered to the specified class.
+ * Additional attributes specified in the groups parameter will also
+ * be created automatically.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_with_groups(struct class *class,
+					 struct device *parent, dev_t devt,
+					 void *drvdata,
+					 const struct attribute_group **groups,
+					 const char *fmt, ...)
+{
+	va_list vargs;
+	struct device *dev;
+
+	va_start(vargs, fmt);
+	dev = device_create_groups_vargs(class, parent, devt, drvdata, groups,
+					 fmt, vargs);
+	va_end(vargs);
+	return dev;
+}
+EXPORT_SYMBOL_GPL(device_create_with_groups);
+
 static int __match_devt(struct device *dev, const void *data)
 {
 	const dev_t *devt = data;
diff --git a/include/linux/device.h b/include/linux/device.h
index f207a8f4..bd5931e8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -938,6 +938,11 @@ extern __printf(5, 6)
 struct device *device_create(struct class *cls, struct device *parent,
 			     dev_t devt, void *drvdata,
 			     const char *fmt, ...);
+extern __printf(6, 7)
+struct device *device_create_with_groups(struct class *cls,
+			     struct device *parent, dev_t devt, void *drvdata,
+			     const struct attribute_group **groups,
+			     const char *fmt, ...);
 extern void device_destroy(struct class *cls, dev_t devt);
 
 /*
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 08/10] driver core: add default groups to struct class

[Greg Kroah-Hartman]

We should be using groups, not attribute lists, for classes to allow
subdirectories, and soon, binary files.  Groups are just more flexible
overall, so add them.

The dev_attrs list will go away after all in-kernel users are converted
to use dev_groups.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/core.c    | 9 ++++++++-
 include/linux/device.h | 4 +++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index a8aae182..8856d745 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -528,9 +528,12 @@ static int device_add_attrs(struct device *dev)
 	int error;
 
 	if (class) {
-		error = device_add_attributes(dev, class->dev_attrs);
+		error = device_add_groups(dev, class->dev_groups);
 		if (error)
 			return error;
+		error = device_add_attributes(dev, class->dev_attrs);
+		if (error)
+			goto err_remove_class_groups;
 		error = device_add_bin_attributes(dev, class->dev_bin_attrs);
 		if (error)
 			goto err_remove_class_attrs;
@@ -563,6 +566,9 @@ static int device_add_attrs(struct device *dev)
  err_remove_class_attrs:
 	if (class)
 		device_remove_attributes(dev, class->dev_attrs);
+ err_remove_class_groups:
+	if (class)
+		device_remove_groups(dev, class->dev_groups);
 
 	return error;
 }
@@ -581,6 +587,7 @@ static void device_remove_attrs(struct device *dev)
 	if (class) {
 		device_remove_attributes(dev, class->dev_attrs);
 		device_remove_bin_attributes(dev, class->dev_bin_attrs);
+		device_remove_groups(dev, class->dev_groups);
 	}
 }
 
diff --git a/include/linux/device.h b/include/linux/device.h
index bd5931e8..22b546a5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -320,6 +320,7 @@ int subsys_virtual_register(struct bus_type *subsys,
  * @name:	Name of the class.
  * @owner:	The module owner.
  * @class_attrs: Default attributes of this class.
+ * @dev_groups:	Default attributes of the devices that belong to the class.
  * @dev_attrs:	Default attributes of the devices belong to the class.
  * @dev_bin_attrs: Default binary attributes of the devices belong to the class.
  * @dev_kobj:	The kobject that represents this class and links it into the hierarchy.
@@ -349,7 +350,8 @@ struct class {
 	struct module		*owner;
 
 	struct class_attribute		*class_attrs;
-	struct device_attribute		*dev_attrs;
+	struct device_attribute		*dev_attrs;	/* use dev_groups instead */
+	const struct attribute_group	**dev_groups;
 	struct bin_attribute		*dev_bin_attrs;
 	struct kobject			*dev_kobj;
 
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 09/10] sysfs: add more helper macro's for (bin_)attribute(_groups)

[Greg Kroah-Hartman]

From: Oliver Schinagl <oliver@schinagl.nl>

With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.

Signed-off-by: Oliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/sysfs.h | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a30..d907a732 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/lockdep.h>
 #include <linux/kobject_ns.h>
+#include <linux/stat.h>
 #include <linux/atomic.h>
 
 struct kobject;
@@ -94,15 +95,18 @@ struct attribute_group {
 #define __ATTR_IGNORE_LOCKDEP	__ATTR
 #endif
 
-#define ATTRIBUTE_GROUPS(name)					\
-static const struct attribute_group name##_group = {		\
-	.attrs = name##_attrs,					\
-};								\
-static const struct attribute_group *name##_groups[] = {	\
-	&name##_group,						\
+#define __ATTRIBUTE_GROUPS(_name)				\
+static const struct attribute_group *_name##_groups[] = {	\
+	&_name##_group,						\
 	NULL,							\
 }
 
+#define ATTRIBUTE_GROUPS(_name)					\
+static const struct attribute_group _name##_group = {		\
+	.attrs = _name##_attrs,					\
+};								\
+__ATTRIBUTE_GROUPS(_name)
+
 #define attr_name(_attr) (_attr).attr.name
 
 struct file;
@@ -132,15 +136,36 @@ struct bin_attribute {
  */
 #define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
 
-/* macro to create static binary attributes easier */
-#define BIN_ATTR(_name, _mode, _read, _write, _size)		\
-struct bin_attribute bin_attr_##_name = {			\
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
-	.read	= _read,					\
-	.write	= _write,					\
-	.size	= _size,					\
+/* macros to create static binary attributes easier */
+#define __BIN_ATTR(_name, _mode, _read, _write, _size) {		\
+	.attr = { .name = __stringify(_name), .mode = _mode },		\
+	.read	= _read,						\
+	.write	= _write,						\
+	.size	= _size,						\
+}
+
+#define __BIN_ATTR_RO(_name, _size) {					\
+	.attr	= { .name = __stringify(_name), .mode = S_IRUGO },	\
+	.read	= _name##_read,						\
+	.size	= _size,						\
 }
 
+#define __BIN_ATTR_RW(_name, _size) __BIN_ATTR(_name,			\
+				   (S_IWUSR | S_IRUGO), _name##_read,	\
+				   _name##_write)
+
+#define __BIN_ATTR_NULL __ATTR_NULL
+
+#define BIN_ATTR(_name, _mode, _read, _write, _size)			\
+struct bin_attribute bin_attr_##_name = __BIN_ATTR(_name, _mode, _read,	\
+					_write, _size)
+
+#define BIN_ATTR_RO(_name, _size)					\
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_RO(_name, _size)
+
+#define BIN_ATTR_RW(_name, _size)					\
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_RW(_name, _size)
+
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
-- 
1.8.3.rc0.20.gb99dd2e

[PATCH 10/10] sysfs: use file mode defines from stat.h

[Greg Kroah-Hartman]

From: Oliver Schinagl <oliver@schinagl.nl>

With the last patches stat.h was included to the header, and thus those
permission defines should be used.

Signed-off-by: Oliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/sysfs.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d907a732..9e8a9b55 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -69,18 +69,19 @@ struct attribute_group {
  * for examples..
  */
 
-#define __ATTR(_name,_mode,_show,_store) { \
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
-	.show	= _show,					\
-	.store	= _store,					\
+#define __ATTR(_name,_mode,_show,_store) { 				\
+	.attr = {.name = __stringify(_name), .mode = _mode },		\
+	.show	= _show,						\
+	.store	= _store,						\
 }
 
-#define __ATTR_RO(_name) { \
-	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
-	.show	= _name##_show,					\
+#define __ATTR_RO(_name) {						\
+	.attr	= { .name = __stringify(_name), .mode = S_IRUGO },	\
+	.show	= _name##_show,						\
 }
 
-#define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
+#define __ATTR_RW(_name) __ATTR(_name, (S_IWUSR | S_IRUGO),		\
+			 _name##_show, _name##_store)
 
 #define __ATTR_NULL { .attr = { .name = NULL } }
 
-- 
1.8.3.rc0.20.gb99dd2e

Re: [PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Hi Greg,

On Sun, 14 Jul 2013 16:05:50 -0700 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> Anyone else have a better name for this macro:
> 	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
> that they can think of?  It will cut down on the typing done for
> attribute lists.

Just one small peeve I have about all these places we do symbol pasting,
is that it makes it impossible to find some things because grep complete
fails us.  We type them once (often by cutting and pasting), but try to
find them who knows how many times?

Just saying ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Greg Kroah-Hartman]

On Mon, Jul 15, 2013 at 10:27:00AM +1000, Stephen Rothwell wrote:
> Hi Greg,
> 
> On Sun, 14 Jul 2013 16:05:50 -0700 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> > Anyone else have a better name for this macro:
> > 	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
> > that they can think of?  It will cut down on the typing done for
> > attribute lists.
> 
> Just one small peeve I have about all these places we do symbol pasting,
> is that it makes it impossible to find some things because grep complete
> fails us.  We type them once (often by cutting and pasting), but try to
> find them who knows how many times?
> 
> Just saying ...

Yeah, I know.  But realize that the dev_attr_##_name variable was
originally created with a:
	DEV_ATTR_RO(sysfs_file_name);
macro in the first place, so even trying to grep for dev_attr* brings up
nothing.  At least this way the grep would find the same thing, so it
does make a little bit sense in a way, right?

Or am I just trying to justify it as I'm tired of typing all that...

greg k-h

Re: [PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Ian Abbott]

On 2013-07-15 00:05, Greg Kroah-Hartman wrote:
> Ian, I didn't add a DEVICE_PATTR() macro yet, that can come later.  I
> considered it, but that would also mean a BUS_PATTR() and the like, which I
> guess works, but I hate even a _hint_ of "Hungarian" notation, which this
> implies.
>
> Anyone else have a better name for this macro:
> 	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
> that they can think of?  It will cut down on the typing done for
> attribute lists.

DEVICE_ATTR_P or DEV_ATTR_P to save typing.  Or would that be confused 
with the P convention[1]?  If so, DEVICE_ATTR_PTR or DEV_ATTR_PTR could 
be used.

[1] see Jargon File.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Ian Abbott]

On 2013-07-15 10:41, Ian Abbott wrote:
> On 2013-07-15 00:05, Greg Kroah-Hartman wrote:
>> Ian, I didn't add a DEVICE_PATTR() macro yet, that can come later.  I
>> considered it, but that would also mean a BUS_PATTR() and the like, which I
>> guess works, but I hate even a _hint_ of "Hungarian" notation, which this
>> implies.
>>
>> Anyone else have a better name for this macro:
>> 	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
>> that they can think of?  It will cut down on the typing done for
>> attribute lists.
>
> DEVICE_ATTR_P or DEV_ATTR_P to save typing.  Or would that be confused
> with the P convention[1]?  If so, DEVICE_ATTR_PTR or DEV_ATTR_PTR could
> be used.
>
> [1] see Jargon File.

Then again, those follow the naming of DEVICE_ATTR_RO and DEVICE_ATTR_RW 
too closely.  Time for someone else to do a bit of bike-shedding.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Guenter Roeck]

On Sun, Jul 14, 2013 at 04:05:50PM -0700, Greg Kroah-Hartman wrote:
> Hi all,
> 
> Here is the third iteration of the patchset to add better attribute
> group support to the driver core and sysfs.
> 
> Others have now tested it, and we got 3 more patches fixing a warning
> that was showing up when binary attributes were in a group with no
> "normal" attributes, and some more helper macros were added and cleaned
> up.
> 
> I've booted this successfully against 3.11-rc1 with no problems (am
> sending these from that machine.)
> 
> Ian, I didn't add a DEVICE_PATTR() macro yet, that can come later.  I
> considered it, but that would also mean a BUS_PATTR() and the like, which I
> guess works, but I hate even a _hint_ of "Hungarian" notation, which this
> implies.
> 
> Anyone else have a better name for this macro:
> 	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
> that they can think of?  It will cut down on the typing done for
> attribute lists.
> 
> changes from v3:
> 	- tested by Guenter
> 	- patches from Oliver added
> 	- refreshed on 3.11-rc1 (minor fuzz)
> 	- Ian glanced at them.
> 
> changes from v2:
> 	- actually boots
> 	- 7th patch added properly
> 	- added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
> 	  to help with converting code to use attributes properly.
> 
Hi Greg,

thanks a lot for this series of patches, and for pushing it upstream.

Do you by any chance have a repository for the cleanup patches depending on this
series ? It looks like a good area to get involved with when I am tired/annoyed
by my other work, but I would not want to duplicate your work.

Thanks,
Guenter

Re: [PATCH v3 01/10] Driver core and sysfs changes for attribute groups

[Greg Kroah-Hartman]

On Thu, Jul 18, 2013 at 04:54:17PM -0700, Guenter Roeck wrote:
> On Sun, Jul 14, 2013 at 04:05:50PM -0700, Greg Kroah-Hartman wrote:
> > Hi all,
> > 
> > Here is the third iteration of the patchset to add better attribute
> > group support to the driver core and sysfs.
> > 
> > Others have now tested it, and we got 3 more patches fixing a warning
> > that was showing up when binary attributes were in a group with no
> > "normal" attributes, and some more helper macros were added and cleaned
> > up.
> > 
> > I've booted this successfully against 3.11-rc1 with no problems (am
> > sending these from that machine.)
> > 
> > Ian, I didn't add a DEVICE_PATTR() macro yet, that can come later.  I
> > considered it, but that would also mean a BUS_PATTR() and the like, which I
> > guess works, but I hate even a _hint_ of "Hungarian" notation, which this
> > implies.
> > 
> > Anyone else have a better name for this macro:
> > 	#define DEVICE_PATTR(_name)	&dev_attr_##_name.attr
> > that they can think of?  It will cut down on the typing done for
> > attribute lists.
> > 
> > changes from v3:
> > 	- tested by Guenter
> > 	- patches from Oliver added
> > 	- refreshed on 3.11-rc1 (minor fuzz)
> > 	- Ian glanced at them.
> > 
> > changes from v2:
> > 	- actually boots
> > 	- 7th patch added properly
> > 	- added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros
> > 	  to help with converting code to use attributes properly.
> > 
> Hi Greg,
> 
> thanks a lot for this series of patches, and for pushing it upstream.
> 
> Do you by any chance have a repository for the cleanup patches depending on this
> series ? It looks like a good area to get involved with when I am tired/annoyed
> by my other work, but I would not want to duplicate your work.

Give me a week or so to get them out, I have 1500+ "normal" patches to
wade through, and 150+ stable patches to deal with first.

But, if you want to look, I do keep my patches in a quilt series on
git.kernel.org in my patches.git repo, feel free to grab those to play
with.

thanks,

greg k-h