Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro

[Oliver Schinagl <oliver+list@schinagl.nl>]

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
>