* [PATCH v2 1/7] sysfs.h: add __ATTR_RW() macro
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
@ 2013-07-11 0:35 ` Greg Kroah-Hartman
2013-07-11 0:36 ` [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro Greg Kroah-Hartman
` (7 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:35 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, 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.
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 e2cee22..9cd20c8 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 1833bc5..95191bf 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6212,8 +6212,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 d014ee5..e04454c 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
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
2013-07-11 0:35 ` [PATCH v2 1/7] sysfs.h: add __ATTR_RW() macro Greg Kroah-Hartman
@ 2013-07-11 0:36 ` Greg Kroah-Hartman
2013-07-11 11:58 ` Oliver Schinagl
2013-07-11 12:03 ` Oliver Schinagl
2013-07-11 0:36 ` [PATCH v2 3/7] sysfs.h: add BIN_ATTR macro Greg Kroah-Hartman
` (6 subsequent siblings)
8 siblings, 2 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:36 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, 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.
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 9cd20c8..f62ff01 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
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 0:36 ` [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro Greg Kroah-Hartman
@ 2013-07-11 11:58 ` Oliver Schinagl
2013-07-11 17:06 ` Greg Kroah-Hartman
2013-07-11 12:03 ` Oliver Schinagl
1 sibling, 1 reply; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-11 11:58 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux, khali
[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> 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)
but wasn't sure if that wasn't a little overkill. I gladly add it in an
additional patch.
>
> 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 9cd20c8..f62ff01 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;
>
[-- Attachment #2: 0001-sysfs-add-more-helper-macro-s-for-bin_-attribute-_gr.patch --]
[-- Type: text/x-patch, Size: 5056 bytes --]
>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl <oliver@schinagl.nl>
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 <oliver@schinagl.nl>
---
include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 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,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, \
NULL, \
}
+#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)
+
#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)
+
+#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)
+
+#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, \
+ NULL, \
+}
+
+#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)
+
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *,char *);
ssize_t (*store)(struct kobject *,struct attribute *,const char *, size_t);
--
1.8.1.5
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 11:58 ` Oliver Schinagl
@ 2013-07-11 17:06 ` Greg Kroah-Hartman
2013-07-11 20:09 ` Oliver Schinagl
0 siblings, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 17:06 UTC (permalink / raw)
To: Oliver Schinagl; +Cc: linux-kernel, linux, khali
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >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.
> 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...
> 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?
> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
> From: Oliver Schinagl <oliver@schinagl.nl>
> 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 <oliver@schinagl.nl>
> ---
> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 84 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 2c3b6a3..0ebed11 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,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, \
> NULL, \
> }
>
> +#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? :)
> +
> #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)
> +
> +#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:
> +
> +#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, \
> + NULL, \
> +}
> +
> +#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
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 17:06 ` Greg Kroah-Hartman
@ 2013-07-11 20:09 ` Oliver Schinagl
2013-07-11 20:26 ` Greg Kroah-Hartman
0 siblings, 1 reply; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-11 20:09 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux, khali
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
> On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
>> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
>>> 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.
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.
>
>> 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 :(
>
>> 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?
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 :)
>
>> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
>> From: Oliver Schinagl <oliver@schinagl.nl>
>> 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 <oliver@schinagl.nl>
>> ---
>> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 84 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>> index 2c3b6a3..0ebed11 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,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[] = { \
typo here, scrap bin_ copy paste fail!
>> + &_name##_attr, \
>> NULL, \
>> }
>>
>> +#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.
>
>> +
>> #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)
>> +
>> +#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?
Yes, I too did this before morning coffee, and I don't drink coffee!
>
> These all look fine to me, thanks.
>
> It's these that I'm confused about:
>
>> +
>> +#define __ATTRIBUTE_BIN_GROUPS(_name) \
>> +static const struct attribute_group *_name##_bin_groups[] = { \
>> + &_name##_bin_group, \
>> + NULL, \
>> }
This is just a helper for the ones below.
>>
>> +#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.
>> +
>> +#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)
This one probably should go, it defines both, and since binaries should
be used cautiously, it's not really needed I guess.
>> +
>> +#define __ATTRIBUTE_BIN_ATTRS(_name) \
>> +struct bin_attribute *_name##_bin_attrs[] = { \
>> + &_name##_bin_attr, \
>> + NULL, \
>> +}
Helper macro again for below
>> +
>> +#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)
These I guess are the equivialent what ATTRIBUTE_GROUP is for groups,
but now for the attributes that go in groups?
>
> 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?
>
> thanks,
Sorry if I sound a little confusing, it made a lot of sense this morning :(
Oliver
>
> greg k-h
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 20:09 ` Oliver Schinagl
@ 2013-07-11 20:26 ` Greg Kroah-Hartman
2013-07-11 20:55 ` Oliver Schinagl
0 siblings, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 20:26 UTC (permalink / raw)
To: Oliver Schinagl; +Cc: linux-kernel, linux, khali
On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
> On 07/11/13 19:06, Greg Kroah-Hartman wrote:
> >On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
> >>On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >>>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.
> 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.
> Anyway, wouldn't all users be reviewed anyway? But I guess it's a
> small safety net to make it not TOO easy.
exactly :)
> >>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 :(
>
> >
> >>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?
> 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?
> 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 :)
> >>>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
> >>From: Oliver Schinagl <oliver@schinagl.nl>
> >>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 <oliver@schinagl.nl>
> >>---
> >> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
> >> 1 file changed, 84 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> >>index 2c3b6a3..0ebed11 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,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[] = { \
> typo here, scrap bin_ copy paste fail!
>
> >>+ &_name##_attr, \
> >> NULL, \
> >> }
> >>
> >>+#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.
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.
> >>+#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 :)
> >>+#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?
>
> >
> >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?
BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting
confusing enough as-is :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 20:26 ` Greg Kroah-Hartman
@ 2013-07-11 20:55 ` Oliver Schinagl
2013-07-14 22:55 ` Oliver Schinagl
0 siblings, 1 reply; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-11 20:55 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux, khali
[-- Attachment #1: Type: text/plain, Size: 8861 bytes --]
On 07/11/13 22:26, Greg Kroah-Hartman wrote:
> On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
>> On 07/11/13 19:06, Greg Kroah-Hartman wrote:
>>> On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
>>>> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
>>>>> 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.
>> 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.
>
>> 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.
>
>>>> 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 :(
>>
>>>
>>>> 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?
>> 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 ;)
>
>> 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 :)
>
>>>> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
>>>> From: Oliver Schinagl <oliver@schinagl.nl>
>>>> 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 <oliver@schinagl.nl>
>>>> ---
>>>> include/linux/sysfs.h | 96 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>> 1 file changed, 84 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>>> index 2c3b6a3..0ebed11 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,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[] = { \
>> typo here, scrap bin_ copy paste fail!
>>
>>>> + &_name##_attr, \
>>>> NULL, \
>>>> }
>>>>
>>>> +#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.
>
> 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.
Heh, they can't I don't think.
>
>
>>>> +#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 :)
>
>>>> +#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?
>>
>>>
>>> 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?
>
> BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting
> confusing enough as-is :)
Agreed and attached.
>
> thanks,
>
> greg k-h
>
[-- Attachment #2: 0001-sysfs-add-more-helper-macro-s-for-bin_-attribute-_gr.patch --]
[-- Type: text/x-patch, Size: 3130 bytes --]
>From 669597be47d042ee28abe5f7e172054043b003e7 Mon Sep 17 00:00:00 2001
From: Oliver Schinagl <oliver@schinagl.nl>
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH 1/2] 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.
Signed-off-by: Oliver Schinagl <oliver@schinagl.nl>
---
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 2c3b6a3..5cf502f 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_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)
+
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *,char *);
ssize_t (*store)(struct kobject *,struct attribute *,const char *, size_t);
--
1.8.1.5
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 20:55 ` Oliver Schinagl
@ 2013-07-14 22:55 ` Oliver Schinagl
2013-07-14 23:06 ` Greg Kroah-Hartman
0 siblings, 1 reply; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-14 22:55 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux, khali
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.
Oliver
On 11-07-13 22:55, Oliver Schinagl wrote:
> On 07/11/13 22:26, Greg Kroah-Hartman wrote:
>> On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
>>> On 07/11/13 19:06, Greg Kroah-Hartman wrote:
>>>> On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
>>>>> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
>>>>>> 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.
>>> 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.
>>
>>> 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.
>
>>
>>>>> 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 :(
>>>
>>>>
>>>>> 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?
>>> 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 ;)
>
>>
>>> 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 :)
>>
>>>>> >From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00
>>>>> 2001
>>>>> From: Oliver Schinagl <oliver@schinagl.nl>
>>>>> 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 <oliver@schinagl.nl>
>>>>> ---
>>>>> include/linux/sysfs.h | 96
>>>>> ++++++++++++++++++++++++++++++++++++++++++++-------
>>>>> 1 file changed, 84 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>>>> index 2c3b6a3..0ebed11 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,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[] = { \
>>> typo here, scrap bin_ copy paste fail!
>>>
>>>>> + &_name##_attr, \
>>>>> NULL, \
>>>>> }
>>>>>
>>>>> +#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.
>>
>> 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.
> Heh, they can't I don't think.
>
>>
>>
>>>>> +#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 :)
>>
>>>>> +#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?
>>>
>>>>
>>>> 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?
>>
>> BIN_ATTR_RO() is fine, I'd stick with that for now, it's getting
>> confusing enough as-is :)
> Agreed and attached.
>
>>
>> thanks,
>>
>> greg k-h
>>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-14 22:55 ` Oliver Schinagl
@ 2013-07-14 23:06 ` Greg Kroah-Hartman
0 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-14 23:06 UTC (permalink / raw)
To: Oliver Schinagl; +Cc: linux-kernel, linux, khali
On Mon, Jul 15, 2013 at 12:55:22AM +0200, Oliver Schinagl wrote:
> 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.
Just refreshed them against 3.11-rc1 and sent them out a mere seconds
ago :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 0:36 ` [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro Greg Kroah-Hartman
2013-07-11 11:58 ` Oliver Schinagl
@ 2013-07-11 12:03 ` Oliver Schinagl
2013-07-11 17:08 ` Greg Kroah-Hartman
1 sibling, 1 reply; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-11 12:03 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux, khali
[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> 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).
Oliver
>
> 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 9cd20c8..f62ff01 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;
>
[-- Attachment #2: 0001-sysfs-use-file-mode-defines-from-stat.h.patch --]
[-- Type: text/x-patch, Size: 1494 bytes --]
>From a67de9f026363ce821c72a807d98830abece3cf7 Mon Sep 17 00:00:00 2001
From: Oliver Schinagl <oliver@schinagl.nl>
Date: Thu, 11 Jul 2013 13:57:42 +0200
Subject: [PATCH] sysfs: use file mode defines from stat.h
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>
---
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 0ebed11..8f820c7 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.1.5
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
2013-07-11 12:03 ` Oliver Schinagl
@ 2013-07-11 17:08 ` Greg Kroah-Hartman
0 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 17:08 UTC (permalink / raw)
To: Oliver Schinagl; +Cc: linux-kernel, linux, khali
On Thu, Jul 11, 2013 at 02:03:34PM +0200, Oliver Schinagl wrote:
> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >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).
No problem, I can merge this with your other one when I can figure out
what that one is really doing :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 3/7] sysfs.h: add BIN_ATTR macro
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
2013-07-11 0:35 ` [PATCH v2 1/7] sysfs.h: add __ATTR_RW() macro Greg Kroah-Hartman
2013-07-11 0:36 ` [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro Greg Kroah-Hartman
@ 2013-07-11 0:36 ` Greg Kroah-Hartman
2013-07-11 0:36 ` [PATCH v2 4/7] driver core: device.h: add RW and RO attribute macros Greg Kroah-Hartman
` (5 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:36 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, 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.
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 f62ff01..d50a96b 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
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 4/7] driver core: device.h: add RW and RO attribute macros
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
` (2 preceding siblings ...)
2013-07-11 0:36 ` [PATCH v2 3/7] sysfs.h: add BIN_ATTR macro Greg Kroah-Hartman
@ 2013-07-11 0:36 ` Greg Kroah-Hartman
2013-07-11 0:36 ` [PATCH v2 5/7] sysfs: add support for binary attributes in groups Greg Kroah-Hartman
` (4 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:36 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, Greg Kroah-Hartman
Make it easier to create attributes without having to always audit the
mode settings.
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 bcf8c0d..f207a8f 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
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 5/7] sysfs: add support for binary attributes in groups
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
` (3 preceding siblings ...)
2013-07-11 0:36 ` [PATCH v2 4/7] driver core: device.h: add RW and RO attribute macros Greg Kroah-Hartman
@ 2013-07-11 0:36 ` Greg Kroah-Hartman
2013-07-11 11:45 ` Oliver Schinagl
2013-07-11 0:36 ` [PATCH v2 6/7] driver core: Introduce device_create_groups Greg Kroah-Hartman
` (3 subsequent siblings)
8 siblings, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:36 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, Greg Kroah-Hartman
groups should be able to support binary attributes, just like it
supports "normal" attributes. This lets us only handle one type of
structure, groups, throughout the driver core and subsystems, making
binary attributes a "full fledged" part of the driver model, and not
something just "tacked on".
Reported-by: Oliver Schinagl <oliver+list@schinagl.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
fs/sysfs/group.c | 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 aec3d5c..e5719c6 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 d50a96b..2c3b6a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -21,6 +21,7 @@
struct kobject;
struct module;
+struct bin_attribute;
enum kobj_ns_type;
struct attribute {
@@ -59,10 +60,9 @@ struct attribute_group {
umode_t (*is_visible)(struct kobject *,
struct attribute *, int);
struct attribute **attrs;
+ struct bin_attribute **bin_attrs;
};
-
-
/**
* Use these macros to make defining attributes easier. See include/linux/device.h
* for examples..
--
1.8.3.rc0.20.gb99dd2e
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 5/7] sysfs: add support for binary attributes in groups
2013-07-11 0:36 ` [PATCH v2 5/7] sysfs: add support for binary attributes in groups Greg Kroah-Hartman
@ 2013-07-11 11:45 ` Oliver Schinagl
2013-07-11 16:59 ` Greg Kroah-Hartman
0 siblings, 1 reply; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-11 11:45 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux, khali
[-- Attachment #1: Type: text/plain, Size: 4291 bytes --]
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> groups should be able to support binary attributes, just like it
> supports "normal" attributes. This lets us only handle one type of
> structure, groups, throughout the driver core and subsystems, making
> binary attributes a "full fledged" part of the driver model, and not
> something just "tacked on".
However when only using binary attributes it warns and doesn't create
anything. The attached patch fixes that.
>
> Reported-by: Oliver Schinagl <oliver+list@schinagl.nl>
If I may be so bold and ask to change my e-mail address to
<oliver@schinagl.nl> that would be kind. I use the +list delimiter to
put all the mailing list mails in a separate mailbox.
> 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 aec3d5c..e5719c6 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 d50a96b..2c3b6a3 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -21,6 +21,7 @@
>
> struct kobject;
> struct module;
> +struct bin_attribute;
> enum kobj_ns_type;
>
> struct attribute {
> @@ -59,10 +60,9 @@ struct attribute_group {
> umode_t (*is_visible)(struct kobject *,
> struct attribute *, int);
> struct attribute **attrs;
> + struct bin_attribute **bin_attrs;
> };
>
> -
> -
> /**
> * Use these macros to make defining attributes easier. See include/linux/device.h
> * for examples..
>
[-- Attachment #2: 0001-sysfs-prevent-warning-when-only-using-binary-attribu.patch --]
[-- Type: text/x-patch, Size: 1104 bytes --]
>From 5a4066011878134c1ab9412bc147c28c30f0fa4b Mon Sep 17 00:00:00 2001
From: Oliver Schinagl <oliver@schinagl.nl>
Date: Thu, 11 Jul 2013 13:40:20 +0200
Subject: [PATCH] sysfs: prevent warning when only using binary attributes
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>
---
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 e5719c6..09a1a25 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.1.5
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 5/7] sysfs: add support for binary attributes in groups
2013-07-11 11:45 ` Oliver Schinagl
@ 2013-07-11 16:59 ` Greg Kroah-Hartman
0 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 16:59 UTC (permalink / raw)
To: Oliver Schinagl; +Cc: linux-kernel, linux, khali
On Thu, Jul 11, 2013 at 01:45:53PM +0200, Oliver Schinagl wrote:
> On 11-07-13 02:36, Greg Kroah-Hartman wrote:
> >groups should be able to support binary attributes, just like it
> >supports "normal" attributes. This lets us only handle one type of
> >structure, groups, throughout the driver core and subsystems, making
> >binary attributes a "full fledged" part of the driver model, and not
> >something just "tacked on".
> However when only using binary attributes it warns and doesn't
> create anything. The attached patch fixes that.
Ah, missed that, I didn't have a binary-only attribute group to test
that out, thanks.
> >Reported-by: Oliver Schinagl <oliver+list@schinagl.nl>
> If I may be so bold and ask to change my e-mail address to
> <oliver@schinagl.nl> that would be kind. I use the +list delimiter
> to put all the mailing list mails in a separate mailbox.
Sure, no problem at all, will do so. And I'll queue up your attached
patch as well, it looks great.
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 6/7] driver core: Introduce device_create_groups
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
` (4 preceding siblings ...)
2013-07-11 0:36 ` [PATCH v2 5/7] sysfs: add support for binary attributes in groups Greg Kroah-Hartman
@ 2013-07-11 0:36 ` Greg Kroah-Hartman
2013-07-11 0:36 ` [PATCH v2 7/7] driver core: add default groups to struct class Greg Kroah-Hartman
` (2 subsequent siblings)
8 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:36 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, 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 dc3ea23..a8aae18 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 f207a8f..bd5931e 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
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 7/7] driver core: add default groups to struct class
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
` (5 preceding siblings ...)
2013-07-11 0:36 ` [PATCH v2 6/7] driver core: Introduce device_create_groups Greg Kroah-Hartman
@ 2013-07-11 0:36 ` Greg Kroah-Hartman
2013-07-11 5:23 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Guenter Roeck
2013-07-14 17:27 ` Guenter Roeck
8 siblings, 0 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 0:36 UTC (permalink / raw)
To: linux-kernel; +Cc: oliver+list, linux, khali, 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.
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 a8aae18..8856d74 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 bd5931e..22b546a 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
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
` (6 preceding siblings ...)
2013-07-11 0:36 ` [PATCH v2 7/7] driver core: add default groups to struct class Greg Kroah-Hartman
@ 2013-07-11 5:23 ` Guenter Roeck
2013-07-11 6:28 ` Greg Kroah-Hartman
2013-07-14 17:27 ` Guenter Roeck
8 siblings, 1 reply; 40+ messages in thread
From: Guenter Roeck @ 2013-07-11 5:23 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, oliver+list, khali
On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> 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.
>
Looks good this time. And, yes, it does boot and work, including patch #7.
Feel free to add
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
and if you like
Tested-by: Guenter Roeck <linux@roeck-us.net>
to the series.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
2013-07-11 5:23 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Guenter Roeck
@ 2013-07-11 6:28 ` Greg Kroah-Hartman
2013-07-11 11:14 ` Oliver Schinagl
0 siblings, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-11 6:28 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-kernel, oliver+list, khali
On Wed, Jul 10, 2013 at 10:23:57PM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> > 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.
> >
> Looks good this time. And, yes, it does boot and work, including patch #7.
> Feel free to add
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> and if you like
>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
>
> to the series.
Thanks for the testing and review, I'll add this when I commit them
sometime next week.
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
2013-07-11 6:28 ` Greg Kroah-Hartman
@ 2013-07-11 11:14 ` Oliver Schinagl
0 siblings, 0 replies; 40+ messages in thread
From: Oliver Schinagl @ 2013-07-11 11:14 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Guenter Roeck, linux-kernel, khali
On 11-07-13 08:28, Greg Kroah-Hartman wrote:
> On Wed, Jul 10, 2013 at 10:23:57PM -0700, Guenter Roeck wrote:
>> On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
>>> 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.
It still crashes, but haven't read back all. I do have some extra helper
macro's patch incoming once i make sure it boots.
(it boots but segfaults) give me a few hours to report back.
oliver
>>>
>>> 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 <linux@roeck-us.net>
>>
>> and if you like
>>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> to the series.
>
> Thanks for the testing and review, I'll add this when I commit them
> sometime next week.
>
> greg k-h
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
2013-07-11 0:35 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Greg Kroah-Hartman
` (7 preceding siblings ...)
2013-07-11 5:23 ` [PATCH v2 0/7] Driver core and sysfs changes for attribute groups Guenter Roeck
@ 2013-07-14 17:27 ` Guenter Roeck
2013-07-14 17:32 ` Greg Kroah-Hartman
8 siblings, 1 reply; 40+ messages in thread
From: Guenter Roeck @ 2013-07-14 17:27 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, oliver+list, khali
On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> 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.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
2013-07-14 17:27 ` Guenter Roeck
@ 2013-07-14 17:32 ` Greg Kroah-Hartman
2013-07-14 18:32 ` Guenter Roeck
0 siblings, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2013-07-14 17:32 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-kernel, oliver+list, khali
On Sun, Jul 14, 2013 at 10:27:03AM -0700, Guenter Roeck wrote:
> On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> > 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.
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 :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
2013-07-14 17:32 ` Greg Kroah-Hartman
@ 2013-07-14 18:32 ` Guenter Roeck
0 siblings, 0 replies; 40+ messages in thread
From: Guenter Roeck @ 2013-07-14 18:32 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, oliver+list, khali
On Sun, Jul 14, 2013 at 10:32:58AM -0700, Greg Kroah-Hartman wrote:
> On Sun, Jul 14, 2013 at 10:27:03AM -0700, Guenter Roeck wrote:
> > On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote:
> > > 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.
>
> 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.
>
Sounds good.
> Got sucked into the "discussion" of the stable kernels on Friday and
> didn't do the update like I was wanting to do so :)
>
In this context ... you are doing an amazing job with -stable.
I have no idea how you manage to do that, especially with all your other
responsibilities. If anything, it is unfair to talk about an allegedly
"broken" process if so much of it depends on a single person.
stable_kernel_rules talks about a review committee. Unless I am missing
something, that committee consists of one person. Maybe it would help
to add a couple more. Good candidates might be the people who are
complaining about a broken process ;).
Guenter
^ permalink raw reply [flat|nested] 40+ messages in thread