Message-ID: <51DF1BDD.6080707@schinagl.nl>
Date:	Thu, 11 Jul 2013 22:55:57 +0200
From:	Oliver Schinagl <oliver+list@...inagl.nl>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	linux-kernel@...r.kernel.org, linux@...ck-us.net,
	khali@...ux-fr.org
Subject: Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro

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@...inagl.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@...inagl.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
>


View attachment "0001-sysfs-add-more-helper-macro-s-for-bin_-attribute-_gr.patch" of type "text/x-patch" (3131 bytes)

Powered by blists - more mailing lists