Driver core and sysfs changes for attribute groups

Hi all,

Guenter and Oliver have been pointing out a few limitations of the driver core's ability to create files properly (i.e. in a way that doesn't race with userspace.) The driver core allows this, but it doesn't export that ability to drivers very easily, and for binary files, not at all.

So here's a set of 6 patches that I'll be queueing up to go to Linus in time for 3.11 so that people can start using them in their driver subsystems. It adds some new macros to make using attributes and attribute groups easier, adds binary file capabilities to attribute groups, and finally, lets subsystems (like platform drivers) set a attribute group for when their device is created.

If anyone has any problems with these patches, please let me know.

Guenter, I've tweaked your original patch a bit, changing the name of the function and putting the kernel doc comments in the correct place so the build doesn't complain about it.

I also have a set of follow-on patches, about 50+ big so far, that goes through the kernel and converts different drivers and subsystems to properly use attribute groups, instead of open-coding binary files and attributes. Those patches will be sent out later, and will be for 3.12 as they aren't needed at the moment, this infrastructure changes are needed first.

thanks,

greg k-h

Hi all,

Guenter and Oliver have been pointing out a few limitations of the driver core's ability to create files properly (i.e. in a way that doesn't race with userspace.) The driver core allows this, but it doesn't export that ability to drivers very easily, and for binary files, not at all.

So here's a set of 6 patches that I'll be queueing up to go to Linus in time for 3.11 so that people can start using them in their driver subsystems. It adds some new macros to make using attributes and attribute groups easier, adds binary file capabilities to attribute groups, and finally, lets subsystems (like platform drivers) set a attribute group for when their device is created.

If anyone has any problems with these patches, please let me know.

Guenter, I've tweaked your original patch a bit, changing the name of the function and putting the kernel doc comments in the correct place so the build doesn't complain about it.

I also have a set of follow-on patches, about 50+ big so far, that goes through the kernel and converts different drivers and subsystems to properly use attribute groups, instead of open-coding binary files and attributes. Those patches will be sent out later, and will be for 3.12 as they aren't needed at the moment, this infrastructure changes are needed first.

Quoted 17 lines · Greg Kroah-Hartman

Hi all,

Guenter and Oliver have been pointing out a few limitations of the driver core's ability to create files properly (i.e. in a way that doesn't race with userspace.) The driver core allows this, but it doesn't export that ability to drivers very easily, and for binary files, not at all.

So here's a set of 6 patches that I'll be queueing up to go to Linus in time for 3.11 so that people can start using them in their driver subsystems. It adds some new macros to make using attributes and attribute groups easier, adds binary file capabilities to attribute groups, and finally, lets subsystems (like platform drivers) set a attribute group for when their device is created.

If anyone has any problems with these patches, please let me know.

Quoted 4 lines

Guenter, I've tweaked your original patch a bit, changing the name of the function and putting the kernel doc comments in the correct place so the build doesn't complain about it.

I like it - thanks a lot for picking that up. I think there are several drivers who could and should use the new API call (gpio, infiniband/umad, c2port, ptp, scsi/osst, android/timed_output, asus_oled are some examples).

For hwmon, I ended up having to handle device creation internally after all. Reason is that I also had to set device->type to a new hwmon device type. That in turn was necessary to be able to support the hwmon 'name' attribute properly (it has to be optional, meaning I could not use class->dev_attrs but had to use device_type->groups instead). I hope I can send out a revised series of patches soon .. only I'll leave for pto next week and I am bugged down with other work right now, so it may have to wait until after I return.

Wonder if it would make sense to support a core driver API call for that purpose as well - ie one that not only has the additional groups argument, but one that (also ?) accepts a pointer to the the device type. I didn't spend any time tracking down potential users, though, so I may be off track here.

Quoted 18 lines · Guenter Roeck

Quoted 17 lines · Greg Kroah-Hartman

Hi all,

Guenter and Oliver have been pointing out a few limitations of the driver core's ability to create files properly (i.e. in a way that doesn't race with userspace.) The driver core allows this, but it doesn't export that ability to drivers very easily, and for binary files, not at all.

So here's a set of 6 patches that I'll be queueing up to go to Linus in time for 3.11 so that people can start using them in their driver subsystems. It adds some new macros to make using attributes and attribute groups easier, adds binary file capabilities to attribute groups, and finally, lets subsystems (like platform drivers) set a attribute group for when their device is created.

If anyone has any problems with these patches, please let me know.

Quoted 7 lines

Quoted 4 lines

Guenter, I've tweaked your original patch a bit, changing the name of the function and putting the kernel doc comments in the correct place so the build doesn't complain about it.

I like it - thanks a lot for picking that up. I think there are several drivers who could and should use the new API call (gpio, infiniband/umad, c2port, ptp, scsi/osst, android/timed_output, asus_oled are some examples).

Yes, I'll be sweeping the tree for the next few released doing all of this. The amount of sysfs "cruft" that has accumulated in the past few years since I last looked is crazy...

Quoted 8 lines

For hwmon, I ended up having to handle device creation internally after all. Reason is that I also had to set device->type to a new hwmon device type. That in turn was necessary to be able to support the hwmon 'name' attribute properly (it has to be optional, meaning I could not use class->dev_attrs but had to use device_type->groups instead). I hope I can send out a revised series of patches soon .. only I'll leave for pto next week and I am bugged down with other work right now, so it may have to wait until after I return.

Quoted 4 lines

Wonder if it would make sense to support a core driver API call for that purpose as well - ie one that not only has the additional groups argument, but one that (also ?) accepts a pointer to the the device type. I didn't spend any time tracking down potential users, though, so I may be off track here.

I don't understand the problem, for USB we have different "types" of devices on the bus just fine. Or are you saying that the function you created isn't going to be used in the hwmon core in the end?

I doubt device_create() really needs to worry about different types, but I could be wrong :)

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.

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
Forgot this one in the series, it adds support to classes for groups, so that we can handle binary attributes properly for them as well.

 drivers/base/core.c    |    8 +++++++-
 include/linux/device.h |    4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

drivers/base/core.c +7 -1

@@ -528,9 +528,12 @@ static int device_add_attrs(struct devic
 	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 devic
  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;
 }

include/linux/device.h +3 -1

@@ -313,6 +313,7 @@ int subsys_virtual_register(struct bus_t
  * @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.

@@ -342,7 +343,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;
 

Quoted 5 lines · 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.

Quoted 4 lines

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

Signed-off-by Greg Kroah-Hartman <[email protected]>

Nice one ... that solves my problem.

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.

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
  include/linux/sysfs.h | 9 +++++++++
  1 file changed, 9 insertions(+)

include/linux/sysfs.h +9 -0

index 9cd20c8..f62ff01 100644
@@ -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;

From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001

Quoted 3 lines · 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.

But binary groups are discriminated against :(

The attached patch should help here.

I suppose one more additional helper wouldn't be bad to have:

#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)

Quoted 1 line

From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001

From: Oliver Schinagl <[email protected]>
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)

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.

Additionally there are no BIN_ group helpers so there's that aswell Moreso, if both bin and normal attribute groups are used, there's a simple helper for that, though the naming code be better. _TXT_ for the show/store ones and neither TXT or BIN for both, but that would change things to extensivly.

Finally there's also helpers for ATTRIBUTE_ATTRS.

After this patch, create default attributes can be as easy as:

ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);

Signed-off-by Oliver Schinagl <[email protected]>

---
 include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 84 insertions(+), 12 deletions(-)

include/linux/sysfs.h +15 -5

index 2c3b6a3..0ebed11 100644
@@ -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,32 @@ struct attribute_group {
 #define __ATTR_IGNORE_LOCKDEP	__ATTR
 #endif
 
-#define ATTRIBUTE_GROUPS(name)					\
-static const struct attribute_group name##_group = {		\
-	.attrs = name##_attrs,					\
+#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,					\
 };								\
-static const struct attribute_group *name##_groups[] = {	\
-	&name##_group,						\
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)				\
+struct bin_attribute *_name##_attrs[] = {			\
+	&_name##_attr,						\

include/linux/sysfs.h +20 -7

+
 #define attr_name(_attr) (_attr).attr.name
 
 struct file;

@@ -132,15 +150,69 @@ 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

include/linux/sysfs.h +21 -0

+
+#define __ATTRIBUTE_BIN_GROUPS(_name)					\
+static const struct attribute_group *_name##_bin_groups[] = {		\
+	&_name##_bin_group,						\
+	NULL,								\
 }
 
+#define ATTRIBUTE_BIN_GROUPS(_name)					\
+static const struct attribute_group _name##_bin_group = {		\
+	.bin_attrs = _name##_bin_attrs,					\
+};									\
+__ATTRIBUTE_BIN_GROUPS(_name)
+
+#define ATTRIBUTE_FULL_GROUPS(_name)					\
+static const struct attribute_group _name##_full_group = {		\
+	.attrs = _name##_attrs,						\
+	.bin_attrs = _name##_bin_attrs,					\
+};									\
+__ATTRIBUTE_GROUPS(_name); __ATTRIBUTE_BIN_GROUPS(_name)
+
+#define __ATTRIBUTE_BIN_ATTRS(_name)					\
+struct bin_attribute *_name##_bin_attrs[] = {				\
+	&_name##_bin_attr,						\

Quoted 5 lines · Oliver Schinagl

Quoted 3 lines · 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.

But binary groups are discriminated against :(

Yes, as they are "rarer" by far, as they should be. binary sysfs files should almost never be used, as they are only "pass-through" files to the hardware, so I want to see you do more work in order to use them, as they should not be created lightly.

Quoted 2 lines

The attached patch should help here.

Can you give me an example of using these macros? I seem to be lost in them somehow, or maybe my morning coffee just hasn't kicked in...

Quoted 6 lines

I suppose one more additional helper wouldn't be bad to have:

#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)

Would that ever be needed?

Quoted 54 lines

Quoted 2 lines

From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001

From: Oliver Schinagl <[email protected]>
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)

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.

Additionally there are no BIN_ group helpers so there's that aswell Moreso, if both bin and normal attribute groups are used, there's a simple helper for that, though the naming code be better. _TXT_ for the show/store ones and neither TXT or BIN for both, but that would change things to extensivly.

Finally there's also helpers for ATTRIBUTE_ATTRS.

After this patch, create default attributes can be as easy as:

ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);

Signed-off-by Oliver Schinagl <[email protected]>

---
  include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 84 insertions(+), 12 deletions(-)

include/linux/sysfs.h +9 -3

index 2c3b6a3..0ebed11 100644
@@ -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,32 @@ struct attribute_group {
  #define __ATTR_IGNORE_LOCKDEP	__ATTR
  #endif
 
-#define ATTRIBUTE_GROUPS(name)					\
-static const struct attribute_group name##_group = {		\
-	.attrs = name##_attrs,					\
+#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 = {		\

include/linux/sysfs.h struct attribute_group { · line 111

Quoted 8 lines

include/linux/sysfs.h +5 -2

+	.attrs = _name##_attrs,					\
  };								\
-static const struct attribute_group *name##_groups[] = {	\
-	&name##_group,						\
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)				\
+struct bin_attribute *_name##_attrs[] = {			\

Quoted 4 lines

include/linux/sysfs.h +1 -0

+	&_name##_attr,						\
  	NULL,							\
  }
 

include/linux/sysfs.h struct attribute_group { · line 122

Quoted 7 lines

include/linux/sysfs.h +7 -0

+#define ATTRIBUTE_ATTR_RO(_name, _size)				\
+struct attribute _name##_attr = __ATTR_RO(_name, _size);	\
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)				\
+struct attribute _name##_attr = __ATTR_RW(_name, _size);	\
+__ATTRIBUTE_ATTRS(_name)

What do these two help out with? "attribute attribute read-write" seems a bit "clunky", don't you think? :)

Quoted 39 lines

include/linux/sysfs.h +24 -7

+
  #define attr_name(_attr) (_attr).attr.name
 
  struct file;

@@ -132,15 +150,69 @@ 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)

include/linux/sysfs.h struct bin_attribute { · line 181

Quoted 6 lines

include/linux/sysfs.h +6 -0

+
+#define BIN_RO_ATTR(_name, _size)					\
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_RO(_name, _size)
+
+#define BIN_RW_ATTR(_name, _size)					\
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_RW(_name, _size)

To be consistent, these shoudl be BIN_ATTR_RO and BIN_ATTR_RW, right?

These all look fine to me, thanks.

It's these that I'm confused about:

include/linux/sysfs.h struct bin_attribute { · line 187

Quoted 6 lines

include/linux/sysfs.h +5 -0

+
+#define __ATTRIBUTE_BIN_GROUPS(_name)					\
+static const struct attribute_group *_name##_bin_groups[] = {		\
+	&_name##_bin_group,						\
+	NULL,								\
  }

include/linux/sysfs.h struct bin_attribute { · line 193

Quoted 6 lines

include/linux/sysfs.h +5 -0

+#define ATTRIBUTE_BIN_GROUPS(_name)					\
+static const struct attribute_group _name##_bin_group = {		\
+	.bin_attrs = _name##_bin_attrs,					\
+};									\
+__ATTRIBUTE_BIN_GROUPS(_name)

include/linux/sysfs.h struct bin_attribute { · line 200

Quoted 7 lines

include/linux/sysfs.h +7 -0

+
+#define ATTRIBUTE_FULL_GROUPS(_name)					\
+static const struct attribute_group _name##_full_group = {		\
+	.attrs = _name##_attrs,						\
+	.bin_attrs = _name##_bin_attrs,					\
+};									\
+__ATTRIBUTE_GROUPS(_name); __ATTRIBUTE_BIN_GROUPS(_name)

include/linux/sysfs.h struct bin_attribute { · line 206

Quoted 6 lines

include/linux/sysfs.h +6 -0

+
+#define __ATTRIBUTE_BIN_ATTRS(_name)					\
+struct bin_attribute *_name##_bin_attrs[] = {				\
+	&_name##_bin_attr,						\
+	NULL,								\
+}

include/linux/sysfs.h struct bin_attribute { · line 214

Quoted 8 lines

include/linux/sysfs.h +8 -0

+
+#define ATTRIBUTE_BIN_ATTR_RO(_name, _size)				\
+struct bin_attribute _name##_bin_attr = __BIN_ATTR_RO(_name, _size);	\
+__ATTRIBUTE_BIN_ATTRS(_name)
+
+#define ATTRIBUTE_BIN_ATTR_RW(_name, _size)				\
+struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size);	\
+__ATTRIBUTE_BIN_ATTRS(_name)

Can you show me how these would be used in a real-world example?

thanks,

greg k-h

Quoted 7 lines · Greg Kroah-Hartman

Quoted 5 lines · Oliver Schinagl

Quoted 3 lines · 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.

But binary groups are discriminated against :(

Quoted 4 lines

Yes, as they are "rarer" by far, as they should be. binary sysfs files should almost never be used, as they are only "pass-through" files to the hardware, so I want to see you do more work in order to use them, as they should not be created lightly.

I guess I can see a valid reason here, but they are only helper macro's making life easier for people who do need to use these and are on par with the non-binary versions. And we already have quite some binary attributes, probably far less then normal ones :)

Anyway, wouldn't all users be reviewed anyway? But I guess it's a small safety net to make it not TOO easy.

Quoted 4 lines

Quoted 1 line

The attached patch should help here.

Can you give me an example of using these macros? I seem to be lost in them somehow, or maybe my morning coffee just hasn't kicked in...

Yeah, I kinda added the whole shebang there :) I was trying being helpful :(

Quoted 7 lines

Quoted 6 lines

I suppose one more additional helper wouldn't be bad to have:

#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)

Quoted 1 line

Would that ever be needed?

Of ourse, by the lazy :)

I think now you create an attribute in a group as this (with this patch):

ATTRIBUTE_ATTR_RO(name, SIZE);

ATTRIBUTE_GROUPS(name);

.group = name;

After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);

.group = name;

saves a whole line :)

Quoted 53 lines

Quoted 53 lines

Quoted 1 line

From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001

From: Oliver Schinagl <[email protected]>
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)

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.

Additionally there are no BIN_ group helpers so there's that aswell Moreso, if both bin and normal attribute groups are used, there's a simple helper for that, though the naming code be better. _TXT_ for the show/store ones and neither TXT or BIN for both, but that would change things to extensivly.

Finally there's also helpers for ATTRIBUTE_ATTRS.

After this patch, create default attributes can be as easy as:

ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);

Signed-off-by Oliver Schinagl <[email protected]>

---
 include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 84 insertions(+), 12 deletions(-)

include/linux/sysfs.h +9 -3

index 2c3b6a3..0ebed11 100644
@@ -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,32 @@ struct attribute_group {
 #define __ATTR_IGNORE_LOCKDEP	__ATTR
 #endif
 
-#define ATTRIBUTE_GROUPS(name)					\
-static const struct attribute_group name##_group = {		\
-	.attrs = name##_attrs,					\
+#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 = {		\

Quoted 8 lines

include/linux/sysfs.h struct attribute_group { · line 111

Quoted 8 lines

include/linux/sysfs.h +5 -2

+	.attrs = _name##_attrs,					\
 };								\
-static const struct attribute_group *name##_groups[] = {	\
-	&name##_group,						\
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)				\
+struct bin_attribute *_name##_attrs[] = {			\

typo here, scrap bin_ copy paste fail!

Quoted 4 lines

Quoted 4 lines

include/linux/sysfs.h +1 -0

+	&_name##_attr,						\
 	NULL,							\
 }
 

Quoted 10 lines

include/linux/sysfs.h struct attribute_group { · line 122

Quoted 7 lines

include/linux/sysfs.h +7 -0

+#define ATTRIBUTE_ATTR_RO(_name, _size)				\
+struct attribute _name##_attr = __ATTR_RO(_name, _size);	\
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)				\
+struct attribute _name##_attr = __ATTR_RW(_name, _size);	\
+__ATTRIBUTE_ATTRS(_name)

What do these two help out with? "attribute attribute read-write" seems a bit "clunky", don't you think? :)

I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and that's the best I could come up with.

Unless I completely misunderstood (which isn't all that unlikely) the following is needed to create a group using a .group.

So you pass group an array of attribute_group pointers. The ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating the array of groups and adding groups to that.

So a group consists of an array of attributes if I understood right and that array needs to be filled with pointers attributes? well those ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is poor, but just ATTRS() felt to short.

Quoted 5 lines

include/linux/sysfs.h struct bin_attribute { · line 193

Quoted 5 lines

include/linux/sysfs.h +5 -0

+#define ATTRIBUTE_BIN_GROUPS(_name)					\
+static const struct attribute_group _name##_bin_group = {		\
+	.bin_attrs = _name##_bin_attrs,					\
+};									\
+__ATTRIBUTE_BIN_GROUPS(_name)

This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an attribute group, with only a binary attribute instead.

Quoted 3 lines

include/linux/sysfs.h struct bin_attribute { · line 214

Quoted 3 lines

include/linux/sysfs.h +3 -0

+#define ATTRIBUTE_BIN_ATTR_RW(_name, _size)				\
+struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size);	\
+__ATTRIBUTE_BIN_ATTRS(_name)

These I guess are the equivialent what ATTRIBUTE_GROUP is for groups, but now for the attributes that go in groups?

Quoted 2 lines

Can you show me how these would be used in a real-world example?

Well my real world is currently limited by my own driver. If I may copy paste from there:

ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
ATTRIBUTE_BIN_GROUPS(sunxi_sid);

static struct platform_driver sunxi_sid_driver = {
.probe = sunxi_sid_probe,
.remove = sunxi_sid_remove,
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE, .of_match_table = sunxi_sid_of_match, .groups = sunxi_sid_bin_groups,
},
};
module_platform_driver(sunxi_sid_driver);

But if you say, you want to be a little less complete, we can drop ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:

struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);

struct bin_attribute *sunxi_sid_bin_attrs[] = {
&sunxi_sid_bin_attr,
NULL,
};

Which requires some manual labor yet still has the __BIN_ATTR_R[OW] macro's to help with some of the more tedious work and allowing you to name the binary attributes nicer?

Quoted 12 lines · Oliver Schinagl

Quoted 11 lines · Greg Kroah-Hartman

Quoted 5 lines · Oliver Schinagl

Quoted 3 lines · 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.

But binary groups are discriminated against :(

Yes, as they are "rarer" by far, as they should be. binary sysfs files should almost never be used, as they are only "pass-through" files to the hardware, so I want to see you do more work in order to use them, as they should not be created lightly.

Quoted 4 lines

I guess I can see a valid reason here, but they are only helper macro's making life easier for people who do need to use these and are on par with the non-binary versions. And we already have quite some binary attributes, probably far less then normal ones :)

I only count about 100 valid binary files in the tree at the moment, that's not really all that many to handle.

Quoted 3 lines

Anyway, wouldn't all users be reviewed anyway? But I guess it's a small safety net to make it not TOO easy.

exactly :)

Quoted 15 lines

Quoted 5 lines

Quoted 2 lines

The attached patch should help here.

Can you give me an example of using these macros? I seem to be lost in them somehow, or maybe my morning coffee just hasn't kicked in...

Yeah, I kinda added the whole shebang there :) I was trying being helpful :(

Quoted 8 lines

Quoted 6 lines

I suppose one more additional helper wouldn't be bad to have:

#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)

Would that ever be needed?

Quoted 5 lines

Of ourse, by the lazy :)

I think now you create an attribute in a group as this (with this patch):

ATTRIBUTE_ATTR_RO(name, SIZE);

but "raw" attributes are rare, you really want a "device", "class", or "bus" attribute, right?

Quoted 11 lines

ATTRIBUTE_GROUPS(name);

.group = name;

After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);

.group = name;

saves a whole line :)

Not worth it :)

Quoted 83 lines

Quoted 53 lines

Quoted 53 lines

Quoted 1 line

From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001

From: Oliver Schinagl <[email protected]>
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)

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.

Additionally there are no BIN_ group helpers so there's that aswell Moreso, if both bin and normal attribute groups are used, there's a simple helper for that, though the naming code be better. _TXT_ for the show/store ones and neither TXT or BIN for both, but that would change things to extensivly.

Finally there's also helpers for ATTRIBUTE_ATTRS.

After this patch, create default attributes can be as easy as:

ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);

Signed-off-by Oliver Schinagl <[email protected]>

---
  include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 84 insertions(+), 12 deletions(-)

include/linux/sysfs.h +9 -3

index 2c3b6a3..0ebed11 100644
@@ -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,32 @@ struct attribute_group {
  #define __ATTR_IGNORE_LOCKDEP	__ATTR
  #endif
 
-#define ATTRIBUTE_GROUPS(name)					\
-static const struct attribute_group name##_group = {		\
-	.attrs = name##_attrs,					\
+#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 = {		\

Quoted 8 lines

include/linux/sysfs.h struct attribute_group { · line 111

Quoted 8 lines

include/linux/sysfs.h +5 -2

+	.attrs = _name##_attrs,					\
  };								\
-static const struct attribute_group *name##_groups[] = {	\
-	&name##_group,						\
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)				\
+struct bin_attribute *_name##_attrs[] = {			\

typo here, scrap bin_ copy paste fail!

Quoted 4 lines

Quoted 4 lines

include/linux/sysfs.h +1 -0

+	&_name##_attr,						\
  	NULL,							\
  }
 

Quoted 10 lines

include/linux/sysfs.h struct attribute_group { · line 122

Quoted 7 lines

include/linux/sysfs.h +7 -0

+#define ATTRIBUTE_ATTR_RO(_name, _size)				\
+struct attribute _name##_attr = __ATTR_RO(_name, _size);	\
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)				\
+struct attribute _name##_attr = __ATTR_RW(_name, _size);	\
+__ATTRIBUTE_ATTRS(_name)

What do these two help out with? "attribute attribute read-write" seems a bit "clunky", don't you think? :)

I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and that's the best I could come up with.

Unless I completely misunderstood (which isn't all that unlikely) the following is needed to create a group using a .group.

Quoted 8 lines

So you pass group an array of attribute_group pointers. The ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating the array of groups and adding groups to that.

So a group consists of an array of attributes if I understood right and that array needs to be filled with pointers attributes? well those ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is poor, but just ATTRS() felt to short.

Here's an example of a file I converted to use the ATTRIBUTE_GROUP() macro attached below (net/wireless/sysfs). As is, it's an increase of only 2 lines to the file overall, which is about normal for the conversion. As you can see, you still need a list of attributes (which someone has already said I need another macro for, to stop typing "&dev_attr*.attr" all the time).

With your macros, how would a file be converted to use them? Perhaps that will help explain things to me better.

Quoted 9 lines

Quoted 7 lines

include/linux/sysfs.h struct attribute_group {

Quoted 7 lines

include/linux/sysfs.h +5 -0

+#define ATTRIBUTE_BIN_GROUPS(_name)					\
+static const struct attribute_group _name##_bin_group = {		\
+	.bin_attrs = _name##_bin_attrs,					\
+};									\
+__ATTRIBUTE_BIN_GROUPS(_name)

This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an attribute group, with only a binary attribute instead.

Again, binary files are rare, and should be rare, don't make it too easy to create them :)

Quoted 36 lines

Quoted 3 lines

include/linux/sysfs.h

Quoted 3 lines

include/linux/sysfs.h +3 -0

+#define ATTRIBUTE_BIN_ATTR_RW(_name, _size)				\
+struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, _size);	\
+__ATTRIBUTE_BIN_ATTRS(_name)

These I guess are the equivialent what ATTRIBUTE_GROUP is for groups, but now for the attributes that go in groups?

Quoted 2 lines

Can you show me how these would be used in a real-world example?

Well my real world is currently limited by my own driver. If I may copy paste from there:

ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
ATTRIBUTE_BIN_GROUPS(sunxi_sid);

static struct platform_driver sunxi_sid_driver = {
.probe = sunxi_sid_probe,
.remove = sunxi_sid_remove,
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE, .of_match_table = sunxi_sid_of_match, .groups = sunxi_sid_bin_groups,
},
};
module_platform_driver(sunxi_sid_driver);

But if you say, you want to be a little less complete, we can drop ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:

struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);

struct bin_attribute *sunxi_sid_bin_attrs[] = {
&sunxi_sid_bin_attr,
NULL,
};

Quoted 3 lines

Which requires some manual labor yet still has the __BIN_ATTR_R[OW] macro's to help with some of the more tedious work and allowing you to name the binary attributes nicer?

BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting confusing enough as-is :)

thanks,

greg k-h

From 669597be47d042ee28abe5f7e172054043b003e7 Mon Sep 17 00:00:00 2001

Quoted 13 lines · Greg Kroah-Hartman

Quoted 12 lines · Oliver Schinagl

Quoted 11 lines · Greg Kroah-Hartman

Quoted 5 lines · Oliver Schinagl

Quoted 3 lines · 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.

But binary groups are discriminated against :(

Yes, as they are "rarer" by far, as they should be. binary sysfs files should almost never be used, as they are only "pass-through" files to the hardware, so I want to see you do more work in order to use them, as they should not be created lightly.

Quoted 7 lines

Quoted 4 lines

I guess I can see a valid reason here, but they are only helper macro's making life easier for people who do need to use these and are on par with the non-binary versions. And we already have quite some binary attributes, probably far less then normal ones :)

I only count about 100 valid binary files in the tree at the moment, that's not really all that many to handle.

100 is quite a few :) But point taken.

Quoted 5 lines

Quoted 3 lines

Anyway, wouldn't all users be reviewed anyway? But I guess it's a small safety net to make it not TOO easy.

exactly :)

I aggree and this is a v2 that strips all the additional bits.

A few comments left below.

Quoted 15 lines

Quoted 15 lines

Quoted 5 lines

Quoted 2 lines

The attached patch should help here.

Can you give me an example of using these macros? I seem to be lost in them somehow, or maybe my morning coffee just hasn't kicked in...

Yeah, I kinda added the whole shebang there :) I was trying being helpful :(

Quoted 8 lines

Quoted 6 lines

I suppose one more additional helper wouldn't be bad to have:

#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)

Would that ever be needed?

Quoted 8 lines

Quoted 5 lines

Of ourse, by the lazy :)

I think now you create an attribute in a group as this (with this patch):

ATTRIBUTE_ATTR_RO(name, SIZE);

but "raw" attributes are rare, you really want a "device", "class", or "bus" attribute, right?

I suppose so, But I got stuck in the rare case some how initially. I registered my driver with module_platform_driver(); and in that struct i had the "device_driver" which had a group attribute so I used that. I already learned from maxime that that is the wrong way :) and hopefully I'll figure out what the right way will be soon ;)

Quoted 115 lines

Quoted 11 lines

ATTRIBUTE_GROUPS(name);

.group = name;

After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);

.group = name;

saves a whole line :)

Not worth it :)

Quoted 85 lines

Quoted 55 lines

Quoted 55 lines

Quoted 1 line

From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00

2001
From: Oliver Schinagl <[email protected]>
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)

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.

Additionally there are no BIN_ group helpers so there's that aswell Moreso, if both bin and normal attribute groups are used, there's a simple helper for that, though the naming code be better. _TXT_ for the show/store ones and neither TXT or BIN for both, but that would change things to extensivly.

Finally there's also helpers for ATTRIBUTE_ATTRS.

After this patch, create default attributes can be as easy as:

ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);

Signed-off-by Oliver Schinagl <[email protected]>

---
  include/linux/sysfs.h | 96 
++++++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 84 insertions(+), 12 deletions(-)

include/linux/sysfs.h +9 -3

index 2c3b6a3..0ebed11 100644
@@ -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,32 @@ struct attribute_group {
  #define __ATTR_IGNORE_LOCKDEP    __ATTR
  #endif
 
-#define ATTRIBUTE_GROUPS(name)                    \
-static const struct attribute_group name##_group = {        \
-    .attrs = name##_attrs,                    \
+#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 = {        \

Quoted 8 lines

include/linux/sysfs.h struct attribute_group {

Quoted 8 lines

include/linux/sysfs.h +5 -2

+    .attrs = _name##_attrs,                    \
  };                                \
-static const struct attribute_group *name##_groups[] = {    \
-    &name##_group,                        \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)                \
+struct bin_attribute *_name##_attrs[] = {            \

typo here, scrap bin_ copy paste fail!

Quoted 4 lines

Quoted 4 lines

include/linux/sysfs.h +1 -0

+ &_name##_attr,                        \
      NULL,                            \
  }
 

Quoted 10 lines

include/linux/sysfs.h struct attribute_group { · line 122

Quoted 7 lines

include/linux/sysfs.h +7 -0

+#define ATTRIBUTE_ATTR_RO(_name, _size)                \
+struct attribute _name##_attr = __ATTR_RO(_name, _size);    \
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)                \
+struct attribute _name##_attr = __ATTR_RW(_name, _size);    \
+__ATTRIBUTE_ATTRS(_name)

What do these two help out with? "attribute attribute read-write" seems a bit "clunky", don't you think? :)

I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and that's the best I could come up with.

Unless I completely misunderstood (which isn't all that unlikely) the following is needed to create a group using a .group.

Quoted 8 lines

So you pass group an array of attribute_group pointers. The ATTRIBUTE_GROUPS helps there, right? It saves the typing of creating the array of groups and adding groups to that.

So a group consists of an array of attributes if I understood right and that array needs to be filled with pointers attributes? well those ATTRIBUTE_ATTR's do just that. Granted, maybe the naming is poor, but just ATTRS() felt to short.

Here's an example of a file I converted to use the ATTRIBUTE_GROUP() macro attached below (net/wireless/sysfs). As is, it's an increase of only 2 lines to the file overall, which is about normal for the conversion. As you can see, you still need a list of attributes (which someone has already said I need another macro for, to stop typing "&dev_attr*.attr" all the time).

Quoted 2 lines

With your macros, how would a file be converted to use them? Perhaps that will help explain things to me better.

Heh, they can't I don't think.

Quoted 50 lines

Quoted 9 lines

Quoted 7 lines

include/linux/sysfs.h struct attribute_group {

Quoted 7 lines

include/linux/sysfs.h +5 -0

+#define ATTRIBUTE_BIN_GROUPS(_name)                    \
+static const struct attribute_group _name##_bin_group = {        \
+    .bin_attrs = _name##_bin_attrs,                    \
+};                                    \
+__ATTRIBUTE_BIN_GROUPS(_name)

This is the equiv. of ATTRIBUTE_GROUPS(_name) which creates an attribute group, with only a binary attribute instead.

Again, binary files are rare, and should be rare, don't make it too easy to create them :)

Quoted 37 lines

Quoted 4 lines

Quoted 4 lines

include/linux/sysfs.h +2 -0

+#define ATTRIBUTE_BIN_ATTR_RW(_name, _size)                \
+struct bin_attribute _name##_bin_attr = __BIN_ATTR_RW(_name, 

_size); \
+__ATTRIBUTE_BIN_ATTRS(_name)

These I guess are the equivialent what ATTRIBUTE_GROUP is for groups, but now for the attributes that go in groups?

Quoted 2 lines

Can you show me how these would be used in a real-world example?

Well my real world is currently limited by my own driver. If I may copy paste from there:

ATTRIBUTE_BIN_ATTR_RO(sunxi_sid, SID_SIZE);
ATTRIBUTE_BIN_GROUPS(sunxi_sid);

static struct platform_driver sunxi_sid_driver = {
.probe = sunxi_sid_probe,
.remove = sunxi_sid_remove,
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE, .of_match_table = sunxi_sid_of_match, .groups = sunxi_sid_bin_groups,
},
}; module_platform_driver(sunxi_sid_driver);

But if you say, you want to be a little less complete, we can drop ATTRIBUTE_BIN_ATTR_R[OW]() forcing you to do this instead:

struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);

struct bin_attribute *sunxi_sid_bin_attrs[] = {
&sunxi_sid_bin_attr,
NULL,
};

Quoted 6 lines

Quoted 3 lines

Which requires some manual labor yet still has the __BIN_ATTR_R[OW] macro's to help with some of the more tedious work and allowing you to name the binary attributes nicer?

BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting confusing enough as-is :)

Agreed and attached.

Quoted 5 lines

thanks,

greg k-h

Greg,

Haven't heard anything about this v2 patch, I know you are very busy but with the merge window closing today/yesterday just wondering about the status of it all.

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.

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
  include/linux/sysfs.h | 9 +++++++++
  1 file changed, 9 insertions(+)

include/linux/sysfs.h +9 -0

index 9cd20c8..f62ff01 100644
@@ -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;

From a67de9f026363ce821c72a807d98830abece3cf7 Mon Sep 17 00:00:00 2001

Quoted 3 lines · 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.

Forgot to add this trivial little cleanup patch with my previous mail, I know you don't like patch dependancies, but this one depends on the previous one as that included stat.h, if you decide my previous patch isn't good, i can redo this one with stat.h included (and move atomic.h to the top to keep includes alphabetically ordered).

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 <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
  fs/sysfs/group.c      | 66 +++++++++++++++++++++++++++++++++++----------------
  include/linux/sysfs.h |  4 ++--
  2 files changed, 48 insertions(+), 22 deletions(-)

fs/sysfs/group.c +46 -20

index aec3d5c..e5719c6 100644
@@ -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;
  }
 

include/linux/sysfs.h +2 -2

index d50a96b..2c3b6a3 100644
@@ -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..

From 5a4066011878134c1ab9412bc147c28c30f0fa4b Mon Sep 17 00:00:00 2001

Quoted 5 lines · 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".

However when only using binary attributes it warns and doesn't create anything. The attached patch fixes that.

Quoted 1 line

Reported-by Oliver Schinagl <[email protected]>

If I may be so bold and ask to change my e-mail address to <[email protected]> that would be kind. I use the +list delimiter to put all the mailing list mails in a separate mailbox.

Hi all,

Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs.

I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.)

I'd like to send these to Linus for 3.11 unless someone objects.

Oliver, please use this series instead of the last one, it has fixes that Guenter pointed out that would have crashed your box at boot.

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.

Quoted 10 lines · Greg Kroah-Hartman

Hi all,

Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs.

I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.)

I'd like to send these to Linus for 3.11 unless someone objects.

Quoted 9 lines

Oliver, please use this series instead of the last one, it has fixes that Guenter pointed out that would have crashed your box at boot.

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.

Looks good this time. And, yes, it does boot and work, including patch #7. Feel free to add

Reviewed-by Guenter Roeck <[email protected]>

and if you like

Tested-by Guenter Roeck <[email protected]>

to the series.

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Greg Kroah-Hartman

Hi all,

Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs.

Quoted 7 lines

Quoted 7 lines

I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.)

I'd like to send these to Linus for 3.11 unless someone objects.

Oliver, please use this series instead of the last one, it has fixes that Guenter pointed out that would have crashed your box at boot.

Quoted 10 lines

Quoted 7 lines

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.

Looks good this time. And, yes, it does boot and work, including patch #7. Feel free to add

Quoted 7 lines

Reviewed-by Guenter Roeck <[email protected]>

and if you like

Tested-by Guenter Roeck <[email protected]>

to the series.

Thanks for the testing and review, I'll add this when I commit them sometime next week.

greg k-h

Hi all,

Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs.

I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.)

I'd like to send these to Linus for 3.11 unless someone objects.

Quoted 10 lines · Greg Kroah-Hartman

Hi all,

Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs.

I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.)

I'd like to send these to Linus for 3.11 unless someone objects.

Hi Greg,

I have not seen the series in the upstream kernel. Do you have an updated plan to get it upstream ?

If needed I can carry the patches I need in my branch for now, so it doesn't matter too much if the code misses 3.11. Still, it would help to know the plan going forward.

Quoted 16 lines · Guenter Roeck

Quoted 10 lines · Greg Kroah-Hartman

Hi all,

Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs.

I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.)

I'd like to send these to Linus for 3.11 unless someone objects.

Hi Greg,

I have not seen the series in the upstream kernel. Do you have an updated plan to get it upstream ?

Quoted 3 lines

If needed I can carry the patches I need in my branch for now, so it doesn't matter too much if the code misses 3.11. Still, it would help to know the plan going forward.

I was planning on doing one more respin of the patches on Monday, and then getting them into 3.11-rc2 if possible so that everyone can start taking advantage of them.

Got sucked into the "discussion" of the stable kernels on Friday and didn't do the update like I was wanting to do so :)