[RFC PATCH 0/2] Sysfs group create for empty groups.

[RFC PATCH 0/2] Sysfs group create for empty groups.

[Jonathan Cameron]

The following is a quick stab at avoiding a hideous work around
we are currently using in IIO.  In some cases we have entire
attribute groups that are created from descriptions an array
of struct iio_chan_spec.  To keep the reference counts sane
and cause subdirectories to be created we are currently using
dummy groups and dummy attribute arrays (provided once in the
core).  This series is an initial probably stupid approach
to avoiding this.

Greg has expressed some doubts about whether subdirectories are
ever a good idea, but the problem occurs for the top level
directory as well (handled by patch 1).

Note, all attributes are created at probe time.  Ultimately we
are just respinning the create_group code to allow us to create
the attributes from a device description rather than statically
allocating them in each driver (results in a massive reduction
in repeated code).

All opinions welcomed.

(this is definitely an rfc, the code isn't even tested yet)

Jonathan

p.s. Greg, sorry for pestering about this!

Jonathan Cameron (2):
  sysfs: Allow for null group pointer in create_group
  sysfs: Allow for groups with no attributes - grp->attrs = NULL.

 fs/sysfs/group.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] sysfs: Allow for null group pointer in create_group

[Jonathan Cameron]

Needed for the case where all files are added separately rather than
as a group.

Usecase - iio_chan_spec based registration of IIO devices where
in some drivers all sysfs attributes are generated from these
descriptive structures on probe rather than using a static table.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 fs/sysfs/group.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 194414f..65d6d78 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -68,17 +68,19 @@ static int internal_create_group(struct kobject *kobj, int update,
 	if (unlikely(update && !kobj->sd))
 		return -EINVAL;
 
-	if (grp->name) {
+	if (grp && grp->name) {
 		error = sysfs_create_subdir(kobj, grp->name, &sd);
 		if (error)
 			return error;
 	} else
 		sd = kobj->sd;
 	sysfs_get(sd);
-	error = create_files(sd, kobj, grp, update);
-	if (error) {
-		if (grp->name)
-			sysfs_remove_subdir(sd);
+	if (grp) {
+		error = create_files(sd, kobj, grp, update);
+		if (error) {
+			if (grp->name)
+				sysfs_remove_subdir(sd);
+		}
 	}
 	sysfs_put(sd);
 	return error;
@@ -131,7 +133,7 @@ void sysfs_remove_group(struct kobject * kobj,
 	struct sysfs_dirent *dir_sd = kobj->sd;
 	struct sysfs_dirent *sd;
 
-	if (grp->name) {
+	if (grp && grp->name) {
 		sd = sysfs_get_dirent(dir_sd, NULL, grp->name);
 		if (!sd) {
 			WARN(!sd, KERN_WARNING "sysfs group %p not found for "
@@ -140,11 +142,11 @@ void sysfs_remove_group(struct kobject * kobj,
 		}
 	} else
 		sd = sysfs_get(dir_sd);
-
-	remove_files(sd, kobj, grp);
-	if (grp->name)
-		sysfs_remove_subdir(sd);
-
+	if (grp) {
+	 	remove_files(sd, kobj, grp);
+		if (grp->name)
+			sysfs_remove_subdir(sd);
+	}
 	sysfs_put(sd);
 }
 
-- 
1.7.3.4

[PATCH 2/2] sysfs: Allow for groups with no attributes - grp->attrs = NULL.

[Jonathan Cameron]

One approach to handling case where all attributes of a subdir are
added independently via sysfs_add_file.

Usecase - iio_chan_spec device registration sometimes creates all the
elements of a subdirectory (e.g. scan_elements) rather than adding
them to an existing subdirectory. Right now the core provides 'dummy'
groups for this purpose, but that's uggly.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 fs/sysfs/group.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 65d6d78..25fe548 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -22,6 +22,8 @@ static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 	struct attribute *const* attr;
 	int i;
 
+	if (grp->attrs == NULL)
+		return;
 	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
 		sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
 }
@@ -32,6 +34,8 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
 	struct attribute *const* attr;
 	int error = 0, i;
 
+	if (grp->attrs == NULL)
+		return 0;
 	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
 		mode_t mode = 0;
 
-- 
1.7.3.4

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Jonathan Cameron]

On 08/17/11 11:17, Jonathan Cameron wrote:
> The following is a quick stab at avoiding a hideous work around
> we are currently using in IIO.  In some cases we have entire
> attribute groups that are created from descriptions an array
> of struct iio_chan_spec.  To keep the reference counts sane
> and cause subdirectories to be created we are currently using
> dummy groups and dummy attribute arrays (provided once in the
> core).  This series is an initial probably stupid approach
> to avoiding this.
> 
> Greg has expressed some doubts about whether subdirectories are
> ever a good idea, but the problem occurs for the top level
> directory as well (handled by patch 1).
> 
> Note, all attributes are created at probe time.  Ultimately we
> are just respinning the create_group code to allow us to create
> the attributes from a device description rather than statically
> allocating them in each driver (results in a massive reduction
> in repeated code).
> 
> All opinions welcomed.
> 
> (this is definitely an rfc, the code isn't even tested yet)
Now tested and looks fine, so I've pushed it to our iio dev tree
(which is re based a few times a day currently so I can always
drop it again ;)
> 
> Jonathan
> 
> p.s. Greg, sorry for pestering about this!
> 
> Jonathan Cameron (2):
>   sysfs: Allow for null group pointer in create_group
>   sysfs: Allow for groups with no attributes - grp->attrs = NULL.
> 
>  fs/sysfs/group.c |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Greg KH]

On Wed, Aug 17, 2011 at 05:27:41PM +0100, Jonathan Cameron wrote:
> On 08/17/11 11:17, Jonathan Cameron wrote:
> > The following is a quick stab at avoiding a hideous work around
> > we are currently using in IIO.  In some cases we have entire
> > attribute groups that are created from descriptions an array
> > of struct iio_chan_spec.  To keep the reference counts sane
> > and cause subdirectories to be created we are currently using
> > dummy groups and dummy attribute arrays (provided once in the
> > core).  This series is an initial probably stupid approach
> > to avoiding this.
> > 
> > Greg has expressed some doubts about whether subdirectories are
> > ever a good idea, but the problem occurs for the top level
> > directory as well (handled by patch 1).
> > 
> > Note, all attributes are created at probe time.  Ultimately we
> > are just respinning the create_group code to allow us to create
> > the attributes from a device description rather than statically
> > allocating them in each driver (results in a massive reduction
> > in repeated code).
> > 
> > All opinions welcomed.
> > 
> > (this is definitely an rfc, the code isn't even tested yet)
> Now tested and looks fine, so I've pushed it to our iio dev tree
> (which is re based a few times a day currently so I can always
> drop it again ;)

Can you show me how you would use this so I could try to understand it a
bit better?

confused,

greg k-h

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Jonathan Cameron]

On 08/23/11 01:33, Greg KH wrote:
> On Wed, Aug 17, 2011 at 05:27:41PM +0100, Jonathan Cameron wrote:
>> On 08/17/11 11:17, Jonathan Cameron wrote:
>>> The following is a quick stab at avoiding a hideous work around
>>> we are currently using in IIO.  In some cases we have entire
>>> attribute groups that are created from descriptions an array
>>> of struct iio_chan_spec.  To keep the reference counts sane
>>> and cause subdirectories to be created we are currently using
>>> dummy groups and dummy attribute arrays (provided once in the
>>> core).  This series is an initial probably stupid approach
>>> to avoiding this.
>>>
>>> Greg has expressed some doubts about whether subdirectories are
>>> ever a good idea, but the problem occurs for the top level
>>> directory as well (handled by patch 1).
>>>
>>> Note, all attributes are created at probe time.  Ultimately we
>>> are just respinning the create_group code to allow us to create
>>> the attributes from a device description rather than statically
>>> allocating them in each driver (results in a massive reduction
>>> in repeated code).
>>>
>>> All opinions welcomed.
>>>
>>> (this is definitely an rfc, the code isn't even tested yet)
>> Now tested and looks fine, so I've pushed it to our iio dev tree
>> (which is re based a few times a day currently so I can always
>> drop it again ;)
> 
> Can you show me how you would use this so I could try to understand it a
> bit better?
Sorry, should have pointed you at an example.

See our iio-dev tree for the full code (some of it is in your staging tree
as well).
http://git.kernel.org/?p=linux/kernel/git/jic23/iio-blue.git;a=summary

Summary:

Sysfs attributes are created from struct iio_chan_spec arrays. These structures
describe the facilities and characteristics of a physical device channel in
a compact fashion. Sometimes there are no other attributes thus we currently
end up with dummy attribute_groups in the core that are registered.  The
purpose of this is to keep the reference counts consistent.

Details:

The core of the matter is that we have channel descriptions for every
channel on a device.  This encodes most of the information about the
channel in a consistent compact way (there are a few more unusual cases
we are still working out how to handle).  The sysfs control and data reading
attributes are generated from these struct iio_chan_spec structure arrays
rather than existing as a static set of attributes as they previously did.

This change (suggested by Arnd) has two big advantages:
1) Massive reduction in boilerplate code.
2) Enforced consistency of interface across drivers.

The base directory contains simple read attributes for the files and
stuff like calibration offsets.  In many cases there are other elements
in there not handled by iio_chan_spec. When that happens we register the
group of other attributes first and then use sysfs_add_file_to_group to
add the other attributes as they are created from the description.
A klist keeps track of the attributes added so they can be cleanly
removed later.

Key function is __iio_add_chan_devattr in industrialio-core.c
int __iio_add_chan_devattr(const char *postfix,
			   const char *group,
			   struct iio_chan_spec const *chan,
			   ssize_t (*readfunc)(struct device *dev,
					       struct device_attribute *attr,
					       char *buf),
			   ssize_t (*writefunc)(struct device *dev,
						struct device_attribute *attr,
						const char *buf,
						size_t len),
			   u64 mask,
			   bool generic,
			   struct device *dev,
			   struct list_head *attr_list)
{
	int ret;
	struct iio_dev_attr *iio_attr, *t;

	iio_attr = kzalloc(sizeof *iio_attr, GFP_KERNEL);
	if (iio_attr == NULL) {
		ret = -ENOMEM;
		goto error_ret;
	}
	ret = __iio_device_attr_init(&iio_attr->dev_attr,
				     postfix, chan,
				     readfunc, writefunc, generic);
	if (ret)
		goto error_iio_dev_attr_free;
	iio_attr->c = chan;
	iio_attr->address = mask;
	list_for_each_entry(t, attr_list, l)
		if (strcmp(t->dev_attr.attr.name,
			   iio_attr->dev_attr.attr.name) == 0) {
			if (!generic)
				dev_err(dev, "tried to double register : %s\n",
					t->dev_attr.attr.name);
			ret = -EBUSY;
			goto error_device_attr_deinit;
		}

	ret = sysfs_add_file_to_group(&dev->kobj,
				      &iio_attr->dev_attr.attr, group);
	if (ret < 0)
		goto error_device_attr_deinit;

	list_add(&iio_attr->l, attr_list);

	return 0;

error_device_attr_deinit:
	__iio_device_attr_deinit(&iio_attr->dev_attr);
error_iio_dev_attr_free:
	kfree(iio_attr);
error_ret:
	return ret;
}


Call to __iio_device_attr_init builds the attribute name up and then
it performs checks for double registration (valid to try for 'generic'
attributes - but not others. When it is marked generic it means
it applies to a number of channels in_accel_scale for example applies to
all registered in_accel channels and it simply will not be added if
already there).

The file is added with sysfs_add_file_to_group.

So ultimately we have a dynamic equivalent of what goes on in
sysfs_create_group with a const group.  We could in theory
do two passes - first to work out what space is needed and second
to create a suitable attribute_group, but it's a lot uglier than
doing it in a single pass through the chan_spec structure array (made
so by the need to handle those 'generic' attributes.)  An alternative
is to do all but the sysfs_add_file_to_group and then build an
attribute group from our existing klist of attributes.

Basically I'm happy to do anything that makes this work. If we
agree there is a reason to have sysfs_add_file_to_group available
then to my mind we should also allow for empty groups. Right now
it looks like there are only a few users of that (I count about 6 with
2 of them in IIO).

The group add of an empty group is simply to get the reference count
consistent. 

The slightly more involved case is for the scanelements subdirectory.
This describes the contents of the any buffers that the driver supports
and is often (but not quite always) generated entirely from the iio_chan_spec
structure arrays provided by the drivers.  It's in a subdirectory as purely
a means of grouping elements related to a particular task (buffer description)
rather than any inherent requirement. This case motivates the allowing a group
with a null pointer in .attrs to avoid having a
struct attribute *dummy_attrs[] = {NULL};
which is ugly.

So what do you recommend?

Thanks,

Jonathan

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Jonathan Cameron]

On 08/23/11 12:01, Jonathan Cameron wrote:
> On 08/23/11 01:33, Greg KH wrote:
>> On Wed, Aug 17, 2011 at 05:27:41PM +0100, Jonathan Cameron wrote:
>>> On 08/17/11 11:17, Jonathan Cameron wrote:
>>>> The following is a quick stab at avoiding a hideous work around
>>>> we are currently using in IIO.  In some cases we have entire
>>>> attribute groups that are created from descriptions an array
>>>> of struct iio_chan_spec.  To keep the reference counts sane
>>>> and cause subdirectories to be created we are currently using
>>>> dummy groups and dummy attribute arrays (provided once in the
>>>> core).  This series is an initial probably stupid approach
>>>> to avoiding this.
>>>>
>>>> Greg has expressed some doubts about whether subdirectories are
>>>> ever a good idea, but the problem occurs for the top level
>>>> directory as well (handled by patch 1).
>>>>
>>>> Note, all attributes are created at probe time.  Ultimately we
>>>> are just respinning the create_group code to allow us to create
>>>> the attributes from a device description rather than statically
>>>> allocating them in each driver (results in a massive reduction
>>>> in repeated code).
>>>>
>>>> All opinions welcomed.
>>>>
>>>> (this is definitely an rfc, the code isn't even tested yet)
>>> Now tested and looks fine, so I've pushed it to our iio dev tree
>>> (which is re based a few times a day currently so I can always
>>> drop it again ;)
>>
>> Can you show me how you would use this so I could try to understand it a
>> bit better?
> Sorry, should have pointed you at an example.
> 
> See our iio-dev tree for the full code (some of it is in your staging tree
> as well).
> http://git.kernel.org/?p=linux/kernel/git/jic23/iio-blue.git;a=summary
> 
> Summary:
> 
> Sysfs attributes are created from struct iio_chan_spec arrays. These structures
> describe the facilities and characteristics of a physical device channel in
> a compact fashion. Sometimes there are no other attributes thus we currently
> end up with dummy attribute_groups in the core that are registered.  The
> purpose of this is to keep the reference counts consistent.
> 
> Details:
> 
> The core of the matter is that we have channel descriptions for every
> channel on a device.  This encodes most of the information about the
> channel in a consistent compact way (there are a few more unusual cases
> we are still working out how to handle).  The sysfs control and data reading
> attributes are generated from these struct iio_chan_spec structure arrays
> rather than existing as a static set of attributes as they previously did.
> 
> This change (suggested by Arnd) has two big advantages:
> 1) Massive reduction in boilerplate code.
> 2) Enforced consistency of interface across drivers.
> 
> The base directory contains simple read attributes for the files and
> stuff like calibration offsets.  In many cases there are other elements
> in there not handled by iio_chan_spec. When that happens we register the
> group of other attributes first and then use sysfs_add_file_to_group to
> add the other attributes as they are created from the description.
> A klist keeps track of the attributes added so they can be cleanly
> removed later.
> 
> Key function is __iio_add_chan_devattr in industrialio-core.c
> int __iio_add_chan_devattr(const char *postfix,
> 			   const char *group,
> 			   struct iio_chan_spec const *chan,
> 			   ssize_t (*readfunc)(struct device *dev,
> 					       struct device_attribute *attr,
> 					       char *buf),
> 			   ssize_t (*writefunc)(struct device *dev,
> 						struct device_attribute *attr,
> 						const char *buf,
> 						size_t len),
> 			   u64 mask,
> 			   bool generic,
> 			   struct device *dev,
> 			   struct list_head *attr_list)
> {
> 	int ret;
> 	struct iio_dev_attr *iio_attr, *t;
> 
> 	iio_attr = kzalloc(sizeof *iio_attr, GFP_KERNEL);
> 	if (iio_attr == NULL) {
> 		ret = -ENOMEM;
> 		goto error_ret;
> 	}
> 	ret = __iio_device_attr_init(&iio_attr->dev_attr,
> 				     postfix, chan,
> 				     readfunc, writefunc, generic);
> 	if (ret)
> 		goto error_iio_dev_attr_free;
> 	iio_attr->c = chan;
> 	iio_attr->address = mask;
> 	list_for_each_entry(t, attr_list, l)
> 		if (strcmp(t->dev_attr.attr.name,
> 			   iio_attr->dev_attr.attr.name) == 0) {
> 			if (!generic)
> 				dev_err(dev, "tried to double register : %s\n",
> 					t->dev_attr.attr.name);
> 			ret = -EBUSY;
> 			goto error_device_attr_deinit;
> 		}
> 
> 	ret = sysfs_add_file_to_group(&dev->kobj,
> 				      &iio_attr->dev_attr.attr, group);
> 	if (ret < 0)
> 		goto error_device_attr_deinit;
> 
> 	list_add(&iio_attr->l, attr_list);
> 
> 	return 0;
> 
> error_device_attr_deinit:
> 	__iio_device_attr_deinit(&iio_attr->dev_attr);
> error_iio_dev_attr_free:
> 	kfree(iio_attr);
> error_ret:
> 	return ret;
> }
> 
> 
> Call to __iio_device_attr_init builds the attribute name up and then
> it performs checks for double registration (valid to try for 'generic'
> attributes - but not others. When it is marked generic it means
> it applies to a number of channels in_accel_scale for example applies to
> all registered in_accel channels and it simply will not be added if
> already there).
> 
> The file is added with sysfs_add_file_to_group.
> 
> So ultimately we have a dynamic equivalent of what goes on in
> sysfs_create_group with a const group.  We could in theory
> do two passes - first to work out what space is needed and second
> to create a suitable attribute_group, but it's a lot uglier than
> doing it in a single pass through the chan_spec structure array (made
> so by the need to handle those 'generic' attributes.)  An alternative
> is to do all but the sysfs_add_file_to_group and then build an
> attribute group from our existing klist of attributes.
> 
> Basically I'm happy to do anything that makes this work. If we
> agree there is a reason to have sysfs_add_file_to_group available
> then to my mind we should also allow for empty groups. Right now
> it looks like there are only a few users of that (I count about 6 with
> 2 of them in IIO).
> 
> The group add of an empty group is simply to get the reference count
> consistent. 
> 
> The slightly more involved case is for the scanelements subdirectory.
> This describes the contents of the any buffers that the driver supports
> and is often (but not quite always) generated entirely from the iio_chan_spec
> structure arrays provided by the drivers.  It's in a subdirectory as purely
> a means of grouping elements related to a particular task (buffer description)
> rather than any inherent requirement. This case motivates the allowing a group
> with a null pointer in .attrs to avoid having a
> struct attribute *dummy_attrs[] = {NULL};
> which is ugly.
> 
> So what do you recommend?
Having just come across Bart's documentation patches to the driver-model
docs I now understand the userspace issue (misunderstood what you said
the other day).  Basically if it isn't done through the groups element
of struct device userspace won't know the attributes have been created.

Will see if I can rework the registration code to build up a suitable cache
of what will be in the attribute groups for each device. This dynamic
cache can then be used to build up everything needed before the
device_register occurs.  It's going to be somewhat clunky but contained
within the IIO core so not too bad.

So upshot is ignore this patch set.  The hack with the dummy groups
will have to stand for 3.1 in IIO.

Thanks,

Jonathan

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Greg KH]

On Tue, Aug 23, 2011 at 08:25:29PM +0100, Jonathan Cameron wrote:
> On 08/23/11 12:01, Jonathan Cameron wrote:
> > On 08/23/11 01:33, Greg KH wrote:
> >> On Wed, Aug 17, 2011 at 05:27:41PM +0100, Jonathan Cameron wrote:
> >>> On 08/17/11 11:17, Jonathan Cameron wrote:
> >>>> The following is a quick stab at avoiding a hideous work around
> >>>> we are currently using in IIO.  In some cases we have entire
> >>>> attribute groups that are created from descriptions an array
> >>>> of struct iio_chan_spec.  To keep the reference counts sane
> >>>> and cause subdirectories to be created we are currently using
> >>>> dummy groups and dummy attribute arrays (provided once in the
> >>>> core).  This series is an initial probably stupid approach
> >>>> to avoiding this.
> >>>>
> >>>> Greg has expressed some doubts about whether subdirectories are
> >>>> ever a good idea, but the problem occurs for the top level
> >>>> directory as well (handled by patch 1).
> >>>>
> >>>> Note, all attributes are created at probe time.  Ultimately we
> >>>> are just respinning the create_group code to allow us to create
> >>>> the attributes from a device description rather than statically
> >>>> allocating them in each driver (results in a massive reduction
> >>>> in repeated code).
> >>>>
> >>>> All opinions welcomed.
> >>>>
> >>>> (this is definitely an rfc, the code isn't even tested yet)
> >>> Now tested and looks fine, so I've pushed it to our iio dev tree
> >>> (which is re based a few times a day currently so I can always
> >>> drop it again ;)
> >>
> >> Can you show me how you would use this so I could try to understand it a
> >> bit better?
> > Sorry, should have pointed you at an example.
> > 
> > See our iio-dev tree for the full code (some of it is in your staging tree
> > as well).
> > http://git.kernel.org/?p=linux/kernel/git/jic23/iio-blue.git;a=summary
> > 
> > Summary:
> > 
> > Sysfs attributes are created from struct iio_chan_spec arrays. These structures
> > describe the facilities and characteristics of a physical device channel in
> > a compact fashion. Sometimes there are no other attributes thus we currently
> > end up with dummy attribute_groups in the core that are registered.  The
> > purpose of this is to keep the reference counts consistent.
> > 
> > Details:
> > 
> > The core of the matter is that we have channel descriptions for every
> > channel on a device.  This encodes most of the information about the
> > channel in a consistent compact way (there are a few more unusual cases
> > we are still working out how to handle).  The sysfs control and data reading
> > attributes are generated from these struct iio_chan_spec structure arrays
> > rather than existing as a static set of attributes as they previously did.
> > 
> > This change (suggested by Arnd) has two big advantages:
> > 1) Massive reduction in boilerplate code.
> > 2) Enforced consistency of interface across drivers.
> > 
> > The base directory contains simple read attributes for the files and
> > stuff like calibration offsets.  In many cases there are other elements
> > in there not handled by iio_chan_spec. When that happens we register the
> > group of other attributes first and then use sysfs_add_file_to_group to
> > add the other attributes as they are created from the description.
> > A klist keeps track of the attributes added so they can be cleanly
> > removed later.
> > 
> > Key function is __iio_add_chan_devattr in industrialio-core.c
> > int __iio_add_chan_devattr(const char *postfix,
> > 			   const char *group,
> > 			   struct iio_chan_spec const *chan,
> > 			   ssize_t (*readfunc)(struct device *dev,
> > 					       struct device_attribute *attr,
> > 					       char *buf),
> > 			   ssize_t (*writefunc)(struct device *dev,
> > 						struct device_attribute *attr,
> > 						const char *buf,
> > 						size_t len),
> > 			   u64 mask,
> > 			   bool generic,
> > 			   struct device *dev,
> > 			   struct list_head *attr_list)
> > {
> > 	int ret;
> > 	struct iio_dev_attr *iio_attr, *t;
> > 
> > 	iio_attr = kzalloc(sizeof *iio_attr, GFP_KERNEL);
> > 	if (iio_attr == NULL) {
> > 		ret = -ENOMEM;
> > 		goto error_ret;
> > 	}
> > 	ret = __iio_device_attr_init(&iio_attr->dev_attr,
> > 				     postfix, chan,
> > 				     readfunc, writefunc, generic);
> > 	if (ret)
> > 		goto error_iio_dev_attr_free;
> > 	iio_attr->c = chan;
> > 	iio_attr->address = mask;
> > 	list_for_each_entry(t, attr_list, l)
> > 		if (strcmp(t->dev_attr.attr.name,
> > 			   iio_attr->dev_attr.attr.name) == 0) {
> > 			if (!generic)
> > 				dev_err(dev, "tried to double register : %s\n",
> > 					t->dev_attr.attr.name);
> > 			ret = -EBUSY;
> > 			goto error_device_attr_deinit;
> > 		}
> > 
> > 	ret = sysfs_add_file_to_group(&dev->kobj,
> > 				      &iio_attr->dev_attr.attr, group);
> > 	if (ret < 0)
> > 		goto error_device_attr_deinit;
> > 
> > 	list_add(&iio_attr->l, attr_list);
> > 
> > 	return 0;
> > 
> > error_device_attr_deinit:
> > 	__iio_device_attr_deinit(&iio_attr->dev_attr);
> > error_iio_dev_attr_free:
> > 	kfree(iio_attr);
> > error_ret:
> > 	return ret;
> > }
> > 
> > 
> > Call to __iio_device_attr_init builds the attribute name up and then
> > it performs checks for double registration (valid to try for 'generic'
> > attributes - but not others. When it is marked generic it means
> > it applies to a number of channels in_accel_scale for example applies to
> > all registered in_accel channels and it simply will not be added if
> > already there).
> > 
> > The file is added with sysfs_add_file_to_group.
> > 
> > So ultimately we have a dynamic equivalent of what goes on in
> > sysfs_create_group with a const group.  We could in theory
> > do two passes - first to work out what space is needed and second
> > to create a suitable attribute_group, but it's a lot uglier than
> > doing it in a single pass through the chan_spec structure array (made
> > so by the need to handle those 'generic' attributes.)  An alternative
> > is to do all but the sysfs_add_file_to_group and then build an
> > attribute group from our existing klist of attributes.
> > 
> > Basically I'm happy to do anything that makes this work. If we
> > agree there is a reason to have sysfs_add_file_to_group available
> > then to my mind we should also allow for empty groups. Right now
> > it looks like there are only a few users of that (I count about 6 with
> > 2 of them in IIO).
> > 
> > The group add of an empty group is simply to get the reference count
> > consistent. 
> > 
> > The slightly more involved case is for the scanelements subdirectory.
> > This describes the contents of the any buffers that the driver supports
> > and is often (but not quite always) generated entirely from the iio_chan_spec
> > structure arrays provided by the drivers.  It's in a subdirectory as purely
> > a means of grouping elements related to a particular task (buffer description)
> > rather than any inherent requirement. This case motivates the allowing a group
> > with a null pointer in .attrs to avoid having a
> > struct attribute *dummy_attrs[] = {NULL};
> > which is ugly.
> > 
> > So what do you recommend?
> Having just come across Bart's documentation patches to the driver-model
> docs I now understand the userspace issue (misunderstood what you said
> the other day).  Basically if it isn't done through the groups element
> of struct device userspace won't know the attributes have been created.
> 
> Will see if I can rework the registration code to build up a suitable cache
> of what will be in the attribute groups for each device. This dynamic
> cache can then be used to build up everything needed before the
> device_register occurs.  It's going to be somewhat clunky but contained
> within the IIO core so not too bad.
> 
> So upshot is ignore this patch set.  The hack with the dummy groups
> will have to stand for 3.1 in IIO.

Ok, consider it ignored :)

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Jonathan Cameron]

I've dropped lkml on this discussion seeing as it will now be all linux-iio
stuff.

Anyhow, I've put together a patch set that does this on top of iio-blue.git.

Unfortunately it requires changes in a lot of drivers.

Basically you have to do iio_device_register last - after all ring and trigger
registrations (previously it had to be first).

Has the nice side effect of removing the need for that nasty regdone trick
that quite a lot of drivers use.

Sorry all but this will break almost all drivers currently out of tree.

I wasn't too nasty to implement, though it does mean embedding attribute_group
structures in struct iio_dev, struct iio_ring_buffer and struct iio_event_interface
and dynamically allocating the struct attribute pointer arrays + remembering to
free them.  That's all in the core though so as long as I haven't messed up shouldn't
effect anyone.

+ now we should get all the right notifications for file creation.

Will post patches once I've done all the driver reorderings as stated above.

Jonathan

Re: [RFC PATCH 0/2] Sysfs group create for empty groups.

[Greg KH]

On Wed, Aug 24, 2011 at 01:16:37PM +0100, Jonathan Cameron wrote:
> I've dropped lkml on this discussion seeing as it will now be all linux-iio
> stuff.
> 
> Anyhow, I've put together a patch set that does this on top of iio-blue.git.
> 
> Unfortunately it requires changes in a lot of drivers.
> 
> Basically you have to do iio_device_register last - after all ring and trigger
> registrations (previously it had to be first).
> 
> Has the nice side effect of removing the need for that nasty regdone trick
> that quite a lot of drivers use.
> 
> Sorry all but this will break almost all drivers currently out of tree.

That provides a good goal for people to get their driver into the tree
:)

> I wasn't too nasty to implement, though it does mean embedding attribute_group
> structures in struct iio_dev, struct iio_ring_buffer and struct iio_event_interface
> and dynamically allocating the struct attribute pointer arrays + remembering to
> free them.  That's all in the core though so as long as I haven't messed up shouldn't
> effect anyone.
> 
> + now we should get all the right notifications for file creation.
> 
> Will post patches once I've done all the driver reorderings as stated above.

Sounds good, thanks for letting me know.

greg k-h