[PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

Hi,

this is a simple patch to add device_create_files() and
device_remove_files() to replace multiple device_create_file() or
_remove() calls with a single shot with the device_attr list.

It's basically just a clean up, but also helps to simplify the error
handling a lot in many existing codes since the function itself does
rollback at error.

The series contains a patch to apply these to drivers/base/node.c.
I have lots of patches (up to 30) to use these in the whole tree, but
maybe it'd be easier too apply once after this stuff is merged at
first.  It's just a cleanup so no urgent task, after all.


thanks,

Takashi

===

Takashi Iwai (2):
  driver core: Add device_create_files() and device_remove_files()
    helpers
  drivers/base/node: Use device_create_files() and device_remove_files()

 drivers/base/core.c    | 36 ++++++++++++++++++++++++++++++++++++
 drivers/base/node.c    | 24 ++++++++++++------------
 include/linux/device.h |  4 ++++
 3 files changed, 52 insertions(+), 12 deletions(-)

-- 
2.2.2

[PATCH 1/2] driver core: Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

There are lots of open codes that call multiple device_create_file()
or device_remove_file() sequentially.  This patch provides the helper
functions for such cases to be performed in a shot.

Also, device_create_files() rolls back at the error path, and this
will simplify the error handling code in many drivers, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/core.c    | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  4 ++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 97e2baf6e5d8..d3fa3d55f1fb 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -557,6 +557,29 @@ int device_create_file(struct device *dev,
 EXPORT_SYMBOL_GPL(device_create_file);
 
 /**
+ * device_create_files - create multiple sysfs attribute files for device.
+ * @dev: device.
+ * @attr: NULL-terminated list of device attribute descriptors.
+ */
+int device_create_files(struct device *dev,
+			const struct device_attribute **attrs)
+{
+	int i, err;
+
+	for (i = 0; attrs[i]; i++) {
+		err = device_create_file(dev, attrs[i]);
+		if (err) {
+			while (--i >= 0)
+				device_remove_file(dev, attrs[i]);
+			return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_create_files);
+
+/**
  * device_remove_file - remove sysfs attribute file.
  * @dev: device.
  * @attr: device attribute descriptor.
@@ -570,6 +593,19 @@ void device_remove_file(struct device *dev,
 EXPORT_SYMBOL_GPL(device_remove_file);
 
 /**
+ * device_remove_file - remove multiple sysfs attribute files.
+ * @dev: device.
+ * @attr: NULL-terminated list of device attribute descriptors.
+ */
+void device_remove_files(struct device *dev,
+			 const struct device_attribute **attrs)
+{
+	for (; *attrs; attrs++)
+		device_remove_file(dev, *attrs);
+}
+EXPORT_SYMBOL_GPL(device_remove_files);
+
+/**
  * device_remove_file_self - remove sysfs attribute file from its own method.
  * @dev: device.
  * @attr: device attribute descriptor.
diff --git a/include/linux/device.h b/include/linux/device.h
index fb506738f7b7..45b6cef1a12b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -559,8 +559,12 @@ ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
 
 extern int device_create_file(struct device *device,
 			      const struct device_attribute *entry);
+extern int device_create_files(struct device *device,
+			       const struct device_attribute **attrs);
 extern void device_remove_file(struct device *dev,
 			       const struct device_attribute *attr);
+extern void device_remove_files(struct device *dev,
+				const struct device_attribute **attrs);
 extern bool device_remove_file_self(struct device *dev,
 				    const struct device_attribute *attr);
 extern int __must_check device_create_bin_file(struct device *dev,
-- 
2.2.2

[PATCH 2/2] drivers/base/node: Use device_create_files() and device_remove_files()

[Takashi Iwai]

Use the new helper functions to simplify the code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/node.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index a3b82e9c7f20..e567e7bde333 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -200,6 +200,16 @@ static ssize_t node_read_distance(struct device *dev,
 }
 static DEVICE_ATTR(distance, S_IRUGO, node_read_distance, NULL);
 
+static const struct device_attribute *node_dev_attrs[] = {
+	&dev_attr_cpumap,
+	&dev_attr_cpulist,
+	&dev_attr_meminfo,
+	&dev_attr_numastat,
+	&dev_attr_distance,
+	&dev_attr_vmstat,
+	NULL
+};
+
 #ifdef CONFIG_HUGETLBFS
 /*
  * hugetlbfs per node attributes registration interface:
@@ -276,12 +286,7 @@ static int register_node(struct node *node, int num, struct node *parent)
 	error = device_register(&node->dev);
 
 	if (!error){
-		device_create_file(&node->dev, &dev_attr_cpumap);
-		device_create_file(&node->dev, &dev_attr_cpulist);
-		device_create_file(&node->dev, &dev_attr_meminfo);
-		device_create_file(&node->dev, &dev_attr_numastat);
-		device_create_file(&node->dev, &dev_attr_distance);
-		device_create_file(&node->dev, &dev_attr_vmstat);
+		device_create_files(&node->dev, node_dev_attrs);
 
 		hugetlb_register_node(node);
 
@@ -299,12 +304,7 @@ static int register_node(struct node *node, int num, struct node *parent)
  */
 void unregister_node(struct node *node)
 {
-	device_remove_file(&node->dev, &dev_attr_cpumap);
-	device_remove_file(&node->dev, &dev_attr_cpulist);
-	device_remove_file(&node->dev, &dev_attr_meminfo);
-	device_remove_file(&node->dev, &dev_attr_numastat);
-	device_remove_file(&node->dev, &dev_attr_distance);
-	device_remove_file(&node->dev, &dev_attr_vmstat);
+	device_remove_files(&node->dev, node_dev_attrs);
 
 	hugetlb_unregister_node(node);		/* no-op, if memoryless node */
 
-- 
2.2.2

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Greg Kroah-Hartman]

On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> Hi,
> 
> this is a simple patch to add device_create_files() and
> device_remove_files() to replace multiple device_create_file() or
> _remove() calls with a single shot with the device_attr list.
> 
> It's basically just a clean up, but also helps to simplify the error
> handling a lot in many existing codes since the function itself does
> rollback at error.
> 
> The series contains a patch to apply these to drivers/base/node.c.
> I have lots of patches (up to 30) to use these in the whole tree, but
> maybe it'd be easier too apply once after this stuff is merged at
> first.  It's just a cleanup so no urgent task, after all.

I'd like to some day be able to drop device_create_file entirely, as it
is almost always used in a racy way (but not always, so we can't get rid
of it today.)

A driver should be using an attribute group and be created/registered
with it if they want any files associated with it, so giving people the
ability to add large numbers of files all at once seems like the wrong
thing to do :)

thanks,

greg k-h

Re: [PATCH 2/2] drivers/base/node: Use device_create_files() and device_remove_files()

[Greg Kroah-Hartman]

On Wed, Jan 28, 2015 at 09:46:14PM +0100, Takashi Iwai wrote:
> Use the new helper functions to simplify the code.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/base/node.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index a3b82e9c7f20..e567e7bde333 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -200,6 +200,16 @@ static ssize_t node_read_distance(struct device *dev,
>  }
>  static DEVICE_ATTR(distance, S_IRUGO, node_read_distance, NULL);
>  
> +static const struct device_attribute *node_dev_attrs[] = {
> +	&dev_attr_cpumap,
> +	&dev_attr_cpulist,
> +	&dev_attr_meminfo,
> +	&dev_attr_numastat,
> +	&dev_attr_distance,
> +	&dev_attr_vmstat,
> +	NULL
> +};
> +
>  #ifdef CONFIG_HUGETLBFS
>  /*
>   * hugetlbfs per node attributes registration interface:
> @@ -276,12 +286,7 @@ static int register_node(struct node *node, int num, struct node *parent)
>  	error = device_register(&node->dev);
>  
>  	if (!error){
> -		device_create_file(&node->dev, &dev_attr_cpumap);
> -		device_create_file(&node->dev, &dev_attr_cpulist);
> -		device_create_file(&node->dev, &dev_attr_meminfo);
> -		device_create_file(&node->dev, &dev_attr_numastat);
> -		device_create_file(&node->dev, &dev_attr_distance);
> -		device_create_file(&node->dev, &dev_attr_vmstat);
> +		device_create_files(&node->dev, node_dev_attrs);

This should just be using an attribute group instead of these files all
being created at once, care to switch the code to use that instead?

thanks,

greg k-h

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

At Wed, 28 Jan 2015 13:05:47 -0800,
Greg Kroah-Hartman wrote:
> 
> On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > Hi,
> > 
> > this is a simple patch to add device_create_files() and
> > device_remove_files() to replace multiple device_create_file() or
> > _remove() calls with a single shot with the device_attr list.
> > 
> > It's basically just a clean up, but also helps to simplify the error
> > handling a lot in many existing codes since the function itself does
> > rollback at error.
> > 
> > The series contains a patch to apply these to drivers/base/node.c.
> > I have lots of patches (up to 30) to use these in the whole tree, but
> > maybe it'd be easier too apply once after this stuff is merged at
> > first.  It's just a cleanup so no urgent task, after all.
> 
> I'd like to some day be able to drop device_create_file entirely, as it
> is almost always used in a racy way (but not always, so we can't get rid
> of it today.)
> 
> A driver should be using an attribute group and be created/registered
> with it if they want any files associated with it, so giving people the
> ability to add large numbers of files all at once seems like the wrong
> thing to do :)

Well, through the glance over many codes using device_create_file(),
I think the problem of the attribute group is that there is little
help for generating the entries dynamically.  For example, if you have
two groups you want to enable conditionally, what would be the best
way to implement?

I don't mean that my device_create_files() is best, but to me it looks
like that proper tools are still missing.


thanks,

Takashi

Re: [PATCH 2/2] drivers/base/node: Use device_create_files() and device_remove_files()

[Takashi Iwai]

At Wed, 28 Jan 2015 13:06:23 -0800,
Greg Kroah-Hartman wrote:
> 
> On Wed, Jan 28, 2015 at 09:46:14PM +0100, Takashi Iwai wrote:
> > Use the new helper functions to simplify the code.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/base/node.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/base/node.c b/drivers/base/node.c
> > index a3b82e9c7f20..e567e7bde333 100644
> > --- a/drivers/base/node.c
> > +++ b/drivers/base/node.c
> > @@ -200,6 +200,16 @@ static ssize_t node_read_distance(struct device *dev,
> >  }
> >  static DEVICE_ATTR(distance, S_IRUGO, node_read_distance, NULL);
> >  
> > +static const struct device_attribute *node_dev_attrs[] = {
> > +	&dev_attr_cpumap,
> > +	&dev_attr_cpulist,
> > +	&dev_attr_meminfo,
> > +	&dev_attr_numastat,
> > +	&dev_attr_distance,
> > +	&dev_attr_vmstat,
> > +	NULL
> > +};
> > +
> >  #ifdef CONFIG_HUGETLBFS
> >  /*
> >   * hugetlbfs per node attributes registration interface:
> > @@ -276,12 +286,7 @@ static int register_node(struct node *node, int num, struct node *parent)
> >  	error = device_register(&node->dev);
> >  
> >  	if (!error){
> > -		device_create_file(&node->dev, &dev_attr_cpumap);
> > -		device_create_file(&node->dev, &dev_attr_cpulist);
> > -		device_create_file(&node->dev, &dev_attr_meminfo);
> > -		device_create_file(&node->dev, &dev_attr_numastat);
> > -		device_create_file(&node->dev, &dev_attr_distance);
> > -		device_create_file(&node->dev, &dev_attr_vmstat);
> > +		device_create_files(&node->dev, node_dev_attrs);
> 
> This should just be using an attribute group instead of these files all
> being created at once, care to switch the code to use that instead?

OK, this would be relatively easily converted to attribute group.
Let me check later...


Takashi

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Greg Kroah-Hartman]

On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> At Wed, 28 Jan 2015 13:05:47 -0800,
> Greg Kroah-Hartman wrote:
> > 
> > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > this is a simple patch to add device_create_files() and
> > > device_remove_files() to replace multiple device_create_file() or
> > > _remove() calls with a single shot with the device_attr list.
> > > 
> > > It's basically just a clean up, but also helps to simplify the error
> > > handling a lot in many existing codes since the function itself does
> > > rollback at error.
> > > 
> > > The series contains a patch to apply these to drivers/base/node.c.
> > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > maybe it'd be easier too apply once after this stuff is merged at
> > > first.  It's just a cleanup so no urgent task, after all.
> > 
> > I'd like to some day be able to drop device_create_file entirely, as it
> > is almost always used in a racy way (but not always, so we can't get rid
> > of it today.)
> > 
> > A driver should be using an attribute group and be created/registered
> > with it if they want any files associated with it, so giving people the
> > ability to add large numbers of files all at once seems like the wrong
> > thing to do :)
> 
> Well, through the glance over many codes using device_create_file(),
> I think the problem of the attribute group is that there is little
> help for generating the entries dynamically.  For example, if you have
> two groups you want to enable conditionally, what would be the best
> way to implement?

Use the is_visable() function callback, that's what it is there for.

thanks,

greg k-h

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

At Wed, 28 Jan 2015 13:34:21 -0800,
Greg Kroah-Hartman wrote:
> 
> On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > At Wed, 28 Jan 2015 13:05:47 -0800,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > Hi,
> > > > 
> > > > this is a simple patch to add device_create_files() and
> > > > device_remove_files() to replace multiple device_create_file() or
> > > > _remove() calls with a single shot with the device_attr list.
> > > > 
> > > > It's basically just a clean up, but also helps to simplify the error
> > > > handling a lot in many existing codes since the function itself does
> > > > rollback at error.
> > > > 
> > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > first.  It's just a cleanup so no urgent task, after all.
> > > 
> > > I'd like to some day be able to drop device_create_file entirely, as it
> > > is almost always used in a racy way (but not always, so we can't get rid
> > > of it today.)
> > > 
> > > A driver should be using an attribute group and be created/registered
> > > with it if they want any files associated with it, so giving people the
> > > ability to add large numbers of files all at once seems like the wrong
> > > thing to do :)
> > 
> > Well, through the glance over many codes using device_create_file(),
> > I think the problem of the attribute group is that there is little
> > help for generating the entries dynamically.  For example, if you have
> > two groups you want to enable conditionally, what would be the best
> > way to implement?
> 
> Use the is_visable() function callback, that's what it is there for.

But if the entries are determined dynamically?  Selecting the enabled
elements from the static list is one way, it'd work in many cases, but
it's not always the most straightforward way.  It often would be
easier to build up the list dynamically.

What if having a link to the chained group for appending entries
dynamically?  Just a wild idea, but it might make things easier.


Takashi

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Greg Kroah-Hartman]

On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote:
> At Wed, 28 Jan 2015 13:34:21 -0800,
> Greg Kroah-Hartman wrote:
> > 
> > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > > At Wed, 28 Jan 2015 13:05:47 -0800,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > > Hi,
> > > > > 
> > > > > this is a simple patch to add device_create_files() and
> > > > > device_remove_files() to replace multiple device_create_file() or
> > > > > _remove() calls with a single shot with the device_attr list.
> > > > > 
> > > > > It's basically just a clean up, but also helps to simplify the error
> > > > > handling a lot in many existing codes since the function itself does
> > > > > rollback at error.
> > > > > 
> > > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > > first.  It's just a cleanup so no urgent task, after all.
> > > > 
> > > > I'd like to some day be able to drop device_create_file entirely, as it
> > > > is almost always used in a racy way (but not always, so we can't get rid
> > > > of it today.)
> > > > 
> > > > A driver should be using an attribute group and be created/registered
> > > > with it if they want any files associated with it, so giving people the
> > > > ability to add large numbers of files all at once seems like the wrong
> > > > thing to do :)
> > > 
> > > Well, through the glance over many codes using device_create_file(),
> > > I think the problem of the attribute group is that there is little
> > > help for generating the entries dynamically.  For example, if you have
> > > two groups you want to enable conditionally, what would be the best
> > > way to implement?
> > 
> > Use the is_visable() function callback, that's what it is there for.
> 
> But if the entries are determined dynamically?  Selecting the enabled
> elements from the static list is one way, it'd work in many cases, but
> it's not always the most straightforward way.  It often would be
> easier to build up the list dynamically.

Do you have an example of this?  Wouldn't it be the same thing to list
them all in an attribute group, but only say "this is valid" in the
is_visable() callback for those that would have been built up
dynamically?

> What if having a link to the chained group for appending entries
> dynamically?  Just a wild idea, but it might make things easier.

We have the ability to pass a group list pointer to device_create
already, and the attribute pointer is a list of groups as well, how can
we change this to be "easier"?

thanks,

greg k-h

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

At Wed, 28 Jan 2015 14:28:51 -0800,
Greg Kroah-Hartman wrote:
> 
> On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote:
> > At Wed, 28 Jan 2015 13:34:21 -0800,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > > > At Wed, 28 Jan 2015 13:05:47 -0800,
> > > > Greg Kroah-Hartman wrote:
> > > > > 
> > > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > this is a simple patch to add device_create_files() and
> > > > > > device_remove_files() to replace multiple device_create_file() or
> > > > > > _remove() calls with a single shot with the device_attr list.
> > > > > > 
> > > > > > It's basically just a clean up, but also helps to simplify the error
> > > > > > handling a lot in many existing codes since the function itself does
> > > > > > rollback at error.
> > > > > > 
> > > > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > > > first.  It's just a cleanup so no urgent task, after all.
> > > > > 
> > > > > I'd like to some day be able to drop device_create_file entirely, as it
> > > > > is almost always used in a racy way (but not always, so we can't get rid
> > > > > of it today.)
> > > > > 
> > > > > A driver should be using an attribute group and be created/registered
> > > > > with it if they want any files associated with it, so giving people the
> > > > > ability to add large numbers of files all at once seems like the wrong
> > > > > thing to do :)
> > > > 
> > > > Well, through the glance over many codes using device_create_file(),
> > > > I think the problem of the attribute group is that there is little
> > > > help for generating the entries dynamically.  For example, if you have
> > > > two groups you want to enable conditionally, what would be the best
> > > > way to implement?
> > > 
> > > Use the is_visable() function callback, that's what it is there for.
> > 
> > But if the entries are determined dynamically?  Selecting the enabled
> > elements from the static list is one way, it'd work in many cases, but
> > it's not always the most straightforward way.  It often would be
> > easier to build up the list dynamically.
> 
> Do you have an example of this?  Wouldn't it be the same thing to list
> them all in an attribute group, but only say "this is valid" in the
> is_visable() callback for those that would have been built up
> dynamically?

One common scene is the case where a device has already the static
group defined in the core helper module while a driver wants to put
additional sysfs entries on it.

A complex case is something in drivers/leds/trigger/ledtrig-*.c.

Another interesting example is drivers/regulator/core.c.  It creates a
bunch of various sysfs files depending on the client's ops presence.
It might be implemented via is_visible, but then it'd become more
lengthy (too many small callback functions).

Also, multiple drivers seem calling device_create_file() from the
array of attributes in a loop.  One reason might be that it's easier
to write for a bunch of entries, without defining many piece of
structs.  An example is found in drivers/gpu/drm/drm_sysfs.c.


> > What if having a link to the chained group for appending entries
> > dynamically?  Just a wild idea, but it might make things easier.
> 
> We have the ability to pass a group list pointer to device_create
> already, and the attribute pointer is a list of groups as well, how can
> we change this to be "easier"?

I guess the order is the problem.  In many cases, you know the
additional entries only after the device creation.  The device
creation is often done by a helper code.  So the driver has no control
to it, just gets the resultant device.


thanks,

Takashi

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Greg Kroah-Hartman]

On Thu, Jan 29, 2015 at 12:11:21AM +0100, Takashi Iwai wrote:
> At Wed, 28 Jan 2015 14:28:51 -0800,
> Greg Kroah-Hartman wrote:
> > 
> > On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote:
> > > At Wed, 28 Jan 2015 13:34:21 -0800,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > > > > At Wed, 28 Jan 2015 13:05:47 -0800,
> > > > > Greg Kroah-Hartman wrote:
> > > > > > 
> > > > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > this is a simple patch to add device_create_files() and
> > > > > > > device_remove_files() to replace multiple device_create_file() or
> > > > > > > _remove() calls with a single shot with the device_attr list.
> > > > > > > 
> > > > > > > It's basically just a clean up, but also helps to simplify the error
> > > > > > > handling a lot in many existing codes since the function itself does
> > > > > > > rollback at error.
> > > > > > > 
> > > > > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > > > > first.  It's just a cleanup so no urgent task, after all.
> > > > > > 
> > > > > > I'd like to some day be able to drop device_create_file entirely, as it
> > > > > > is almost always used in a racy way (but not always, so we can't get rid
> > > > > > of it today.)
> > > > > > 
> > > > > > A driver should be using an attribute group and be created/registered
> > > > > > with it if they want any files associated with it, so giving people the
> > > > > > ability to add large numbers of files all at once seems like the wrong
> > > > > > thing to do :)
> > > > > 
> > > > > Well, through the glance over many codes using device_create_file(),
> > > > > I think the problem of the attribute group is that there is little
> > > > > help for generating the entries dynamically.  For example, if you have
> > > > > two groups you want to enable conditionally, what would be the best
> > > > > way to implement?
> > > > 
> > > > Use the is_visable() function callback, that's what it is there for.
> > > 
> > > But if the entries are determined dynamically?  Selecting the enabled
> > > elements from the static list is one way, it'd work in many cases, but
> > > it's not always the most straightforward way.  It often would be
> > > easier to build up the list dynamically.
> > 
> > Do you have an example of this?  Wouldn't it be the same thing to list
> > them all in an attribute group, but only say "this is valid" in the
> > is_visable() callback for those that would have been built up
> > dynamically?
> 
> One common scene is the case where a device has already the static
> group defined in the core helper module while a driver wants to put
> additional sysfs entries on it.
> 
> A complex case is something in drivers/leds/trigger/ledtrig-*.c.
> 
> Another interesting example is drivers/regulator/core.c.  It creates a
> bunch of various sysfs files depending on the client's ops presence.
> It might be implemented via is_visible, but then it'd become more
> lengthy (too many small callback functions).

Yeah, I'm not saying it's easy, or simple, it's just the only way I know
how to do this in a race-free way.  We have to create the files before
the uevent happens, not after, like these drivers are doing.

If you can think of a way that we can do this in a simpler way, that
would be great.

> Also, multiple drivers seem calling device_create_file() from the
> array of attributes in a loop.  One reason might be that it's easier
> to write for a bunch of entries, without defining many piece of
> structs.  An example is found in drivers/gpu/drm/drm_sysfs.c.

That one should just be adding the whole attribute group, using
device_add_groups, which we have in the driver core, but I didn't export
publicly.  That is if those are being added in a race-free way, I
couldn't unwind the drm mess to see if the uevent is happening after the
files are added or before.

> > > What if having a link to the chained group for appending entries
> > > dynamically?  Just a wild idea, but it might make things easier.
> > 
> > We have the ability to pass a group list pointer to device_create
> > already, and the attribute pointer is a list of groups as well, how can
> > we change this to be "easier"?
> 
> I guess the order is the problem.  In many cases, you know the
> additional entries only after the device creation.  The device
> creation is often done by a helper code.  So the driver has no control
> to it, just gets the resultant device.

Yeah, that's the problem.  And another problem is drivers adding
attributes to devices after they are bound to a device, which is kind of
pointless, as the uevent is long past at that point in time.  I've
cleaned up a bunch of those, but odds are there are still more to fix.

thanks,

greg k-h

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

At Thu, 29 Jan 2015 20:26:26 -0800,
Greg Kroah-Hartman wrote:
> 
> On Thu, Jan 29, 2015 at 12:11:21AM +0100, Takashi Iwai wrote:
> > At Wed, 28 Jan 2015 14:28:51 -0800,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote:
> > > > At Wed, 28 Jan 2015 13:34:21 -0800,
> > > > Greg Kroah-Hartman wrote:
> > > > > 
> > > > > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > > > > > At Wed, 28 Jan 2015 13:05:47 -0800,
> > > > > > Greg Kroah-Hartman wrote:
> > > > > > > 
> > > > > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > this is a simple patch to add device_create_files() and
> > > > > > > > device_remove_files() to replace multiple device_create_file() or
> > > > > > > > _remove() calls with a single shot with the device_attr list.
> > > > > > > > 
> > > > > > > > It's basically just a clean up, but also helps to simplify the error
> > > > > > > > handling a lot in many existing codes since the function itself does
> > > > > > > > rollback at error.
> > > > > > > > 
> > > > > > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > > > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > > > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > > > > > first.  It's just a cleanup so no urgent task, after all.
> > > > > > > 
> > > > > > > I'd like to some day be able to drop device_create_file entirely, as it
> > > > > > > is almost always used in a racy way (but not always, so we can't get rid
> > > > > > > of it today.)
> > > > > > > 
> > > > > > > A driver should be using an attribute group and be created/registered
> > > > > > > with it if they want any files associated with it, so giving people the
> > > > > > > ability to add large numbers of files all at once seems like the wrong
> > > > > > > thing to do :)
> > > > > > 
> > > > > > Well, through the glance over many codes using device_create_file(),
> > > > > > I think the problem of the attribute group is that there is little
> > > > > > help for generating the entries dynamically.  For example, if you have
> > > > > > two groups you want to enable conditionally, what would be the best
> > > > > > way to implement?
> > > > > 
> > > > > Use the is_visable() function callback, that's what it is there for.
> > > > 
> > > > But if the entries are determined dynamically?  Selecting the enabled
> > > > elements from the static list is one way, it'd work in many cases, but
> > > > it's not always the most straightforward way.  It often would be
> > > > easier to build up the list dynamically.
> > > 
> > > Do you have an example of this?  Wouldn't it be the same thing to list
> > > them all in an attribute group, but only say "this is valid" in the
> > > is_visable() callback for those that would have been built up
> > > dynamically?
> > 
> > One common scene is the case where a device has already the static
> > group defined in the core helper module while a driver wants to put
> > additional sysfs entries on it.
> > 
> > A complex case is something in drivers/leds/trigger/ledtrig-*.c.
> > 
> > Another interesting example is drivers/regulator/core.c.  It creates a
> > bunch of various sysfs files depending on the client's ops presence.
> > It might be implemented via is_visible, but then it'd become more
> > lengthy (too many small callback functions).
> 
> Yeah, I'm not saying it's easy, or simple, it's just the only way I know
> how to do this in a race-free way.  We have to create the files before
> the uevent happens, not after, like these drivers are doing.
> 
> If you can think of a way that we can do this in a simpler way, that
> would be great.

The latter one (regulator/core.c) is actually a case where is_visible
callback would work better, I noticed after studying mode code.
Thanks for hints.  I'm going to submit a patch later.

OTOH, the leds class looks not intuitive.  Need more investigation.

> > Also, multiple drivers seem calling device_create_file() from the
> > array of attributes in a loop.  One reason might be that it's easier
> > to write for a bunch of entries, without defining many piece of
> > structs.  An example is found in drivers/gpu/drm/drm_sysfs.c.
> 
> That one should just be adding the whole attribute group, using
> device_add_groups, which we have in the driver core, but I didn't export
> publicly.  That is if those are being added in a race-free way, I
> couldn't unwind the drm mess to see if the uevent is happening after the
> files are added or before.

If we export device_add_groups() and device_remove_groups(), is it
safe to call it before device_add()?  If yes, some drivers/subsystems
can have a code flow like:

some_subsystem_init(struct device *dev)
{
	device_initialize(dev);
	devs->groups = subsystem_groups;
	....
}

driver_init(struct device *dev)
{
	some_subsystem_init(dev);
	device_add_groups(dev, additional_groups);
	....
	device_add(dev);
	....
}

The network device has a own multi dev_groups array so that the driver
can put an own group while the net core fills common groups
dynamically just before the device registration call.  I though of
implementing similar for others (including the sound stuff), but if
the scheme above works, the rewrite will become smaller.

Of corse, the drawback of the explicit device_add_groups() call would
be that you'll have to call device_remove_groups() at removal or error
paths.


> > > > What if having a link to the chained group for appending entries
> > > > dynamically?  Just a wild idea, but it might make things easier.
> > > 
> > > We have the ability to pass a group list pointer to device_create
> > > already, and the attribute pointer is a list of groups as well, how can
> > > we change this to be "easier"?
> > 
> > I guess the order is the problem.  In many cases, you know the
> > additional entries only after the device creation.  The device
> > creation is often done by a helper code.  So the driver has no control
> > to it, just gets the resultant device.
> 
> Yeah, that's the problem.  And another problem is drivers adding
> attributes to devices after they are bound to a device, which is kind of
> pointless, as the uevent is long past at that point in time.  I've
> cleaned up a bunch of those, but odds are there are still more to fix.

Right, there are a bunch of drivers doing it.  I guess partly because
they don't need uevents for creation, but also partly because there is
no way to give attribute groups properly in some cases.  For example,
misc_register() or register_framebuffer() calls device_create() so the
caller can't pass groups.

It'd be trivial to extend struct miscdevice to carry an optional group
field and change the call to device_create_with_groups().  But,
fb_info has also common sysfs entries, so it'd need also the solution
above with device_add_groups() in addition.

The similar pattern is found for many drivers with platform devices.
But I haven't figured out yet what would be a good way...


thanks,

Takashi

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Greg Kroah-Hartman]

On Fri, Jan 30, 2015 at 05:31:51PM +0100, Takashi Iwai wrote:
> At Thu, 29 Jan 2015 20:26:26 -0800,
> Greg Kroah-Hartman wrote:
> > 
> > On Thu, Jan 29, 2015 at 12:11:21AM +0100, Takashi Iwai wrote:
> > > At Wed, 28 Jan 2015 14:28:51 -0800,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote:
> > > > > At Wed, 28 Jan 2015 13:34:21 -0800,
> > > > > Greg Kroah-Hartman wrote:
> > > > > > 
> > > > > > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > > > > > > At Wed, 28 Jan 2015 13:05:47 -0800,
> > > > > > > Greg Kroah-Hartman wrote:
> > > > > > > > 
> > > > > > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > this is a simple patch to add device_create_files() and
> > > > > > > > > device_remove_files() to replace multiple device_create_file() or
> > > > > > > > > _remove() calls with a single shot with the device_attr list.
> > > > > > > > > 
> > > > > > > > > It's basically just a clean up, but also helps to simplify the error
> > > > > > > > > handling a lot in many existing codes since the function itself does
> > > > > > > > > rollback at error.
> > > > > > > > > 
> > > > > > > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > > > > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > > > > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > > > > > > first.  It's just a cleanup so no urgent task, after all.
> > > > > > > > 
> > > > > > > > I'd like to some day be able to drop device_create_file entirely, as it
> > > > > > > > is almost always used in a racy way (but not always, so we can't get rid
> > > > > > > > of it today.)
> > > > > > > > 
> > > > > > > > A driver should be using an attribute group and be created/registered
> > > > > > > > with it if they want any files associated with it, so giving people the
> > > > > > > > ability to add large numbers of files all at once seems like the wrong
> > > > > > > > thing to do :)
> > > > > > > 
> > > > > > > Well, through the glance over many codes using device_create_file(),
> > > > > > > I think the problem of the attribute group is that there is little
> > > > > > > help for generating the entries dynamically.  For example, if you have
> > > > > > > two groups you want to enable conditionally, what would be the best
> > > > > > > way to implement?
> > > > > > 
> > > > > > Use the is_visable() function callback, that's what it is there for.
> > > > > 
> > > > > But if the entries are determined dynamically?  Selecting the enabled
> > > > > elements from the static list is one way, it'd work in many cases, but
> > > > > it's not always the most straightforward way.  It often would be
> > > > > easier to build up the list dynamically.
> > > > 
> > > > Do you have an example of this?  Wouldn't it be the same thing to list
> > > > them all in an attribute group, but only say "this is valid" in the
> > > > is_visable() callback for those that would have been built up
> > > > dynamically?
> > > 
> > > One common scene is the case where a device has already the static
> > > group defined in the core helper module while a driver wants to put
> > > additional sysfs entries on it.
> > > 
> > > A complex case is something in drivers/leds/trigger/ledtrig-*.c.
> > > 
> > > Another interesting example is drivers/regulator/core.c.  It creates a
> > > bunch of various sysfs files depending on the client's ops presence.
> > > It might be implemented via is_visible, but then it'd become more
> > > lengthy (too many small callback functions).
> > 
> > Yeah, I'm not saying it's easy, or simple, it's just the only way I know
> > how to do this in a race-free way.  We have to create the files before
> > the uevent happens, not after, like these drivers are doing.
> > 
> > If you can think of a way that we can do this in a simpler way, that
> > would be great.
> 
> The latter one (regulator/core.c) is actually a case where is_visible
> callback would work better, I noticed after studying mode code.
> Thanks for hints.  I'm going to submit a patch later.
> 
> OTOH, the leds class looks not intuitive.  Need more investigation.
> 
> > > Also, multiple drivers seem calling device_create_file() from the
> > > array of attributes in a loop.  One reason might be that it's easier
> > > to write for a bunch of entries, without defining many piece of
> > > structs.  An example is found in drivers/gpu/drm/drm_sysfs.c.
> > 
> > That one should just be adding the whole attribute group, using
> > device_add_groups, which we have in the driver core, but I didn't export
> > publicly.  That is if those are being added in a race-free way, I
> > couldn't unwind the drm mess to see if the uevent is happening after the
> > files are added or before.
> 
> If we export device_add_groups() and device_remove_groups(), is it
> safe to call it before device_add()?  If yes, some drivers/subsystems
> can have a code flow like:
> 
> some_subsystem_init(struct device *dev)
> {
> 	device_initialize(dev);
> 	devs->groups = subsystem_groups;
> 	....
> }
> 
> driver_init(struct device *dev)
> {
> 	some_subsystem_init(dev);
> 	device_add_groups(dev, additional_groups);
> 	....
> 	device_add(dev);
> 	....
> }
> 
> The network device has a own multi dev_groups array so that the driver
> can put an own group while the net core fills common groups
> dynamically just before the device registration call.  I though of
> implementing similar for others (including the sound stuff), but if
> the scheme above works, the rewrite will become smaller.
> 
> Of corse, the drawback of the explicit device_add_groups() call would
> be that you'll have to call device_remove_groups() at removal or error
> paths.

Right now, no, you can't call device_add_groups() until after
device_add() happens, as it device_initialize() doesn't do enough sysfs
work in order to be able to create the files.

> > > > > What if having a link to the chained group for appending entries
> > > > > dynamically?  Just a wild idea, but it might make things easier.
> > > > 
> > > > We have the ability to pass a group list pointer to device_create
> > > > already, and the attribute pointer is a list of groups as well, how can
> > > > we change this to be "easier"?
> > > 
> > > I guess the order is the problem.  In many cases, you know the
> > > additional entries only after the device creation.  The device
> > > creation is often done by a helper code.  So the driver has no control
> > > to it, just gets the resultant device.
> > 
> > Yeah, that's the problem.  And another problem is drivers adding
> > attributes to devices after they are bound to a device, which is kind of
> > pointless, as the uevent is long past at that point in time.  I've
> > cleaned up a bunch of those, but odds are there are still more to fix.
> 
> Right, there are a bunch of drivers doing it.  I guess partly because
> they don't need uevents for creation, but also partly because there is
> no way to give attribute groups properly in some cases.  For example,
> misc_register() or register_framebuffer() calls device_create() so the
> caller can't pass groups.
> 
> It'd be trivial to extend struct miscdevice to carry an optional group
> field and change the call to device_create_with_groups().  But,
> fb_info has also common sysfs entries, so it'd need also the solution
> above with device_add_groups() in addition.

Your patch to do that looks good, I'll queue them all up after 3.20-rc1
is out as it's too close to 3.19 at the moment.

thanks,

greg k-h

Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai]

At Sat, 7 Feb 2015 18:10:48 +0800,
Greg Kroah-Hartman wrote:
> 
> On Fri, Jan 30, 2015 at 05:31:51PM +0100, Takashi Iwai wrote:
> > If we export device_add_groups() and device_remove_groups(), is it
> > safe to call it before device_add()?  If yes, some drivers/subsystems
> > can have a code flow like:
> > 
> > some_subsystem_init(struct device *dev)
> > {
> > 	device_initialize(dev);
> > 	devs->groups = subsystem_groups;
> > 	....
> > }
> > 
> > driver_init(struct device *dev)
> > {
> > 	some_subsystem_init(dev);
> > 	device_add_groups(dev, additional_groups);
> > 	....
> > 	device_add(dev);
> > 	....
> > }
> > 
> > The network device has a own multi dev_groups array so that the driver
> > can put an own group while the net core fills common groups
> > dynamically just before the device registration call.  I though of
> > implementing similar for others (including the sound stuff), but if
> > the scheme above works, the rewrite will become smaller.
> > 
> > Of corse, the drawback of the explicit device_add_groups() call would
> > be that you'll have to call device_remove_groups() at removal or error
> > paths.
> 
> Right now, no, you can't call device_add_groups() until after
> device_add() happens, as it device_initialize() doesn't do enough sysfs
> work in order to be able to create the files.

OK, that's not so trivial as I hoped.
One can add some list to manage the additional attributes, but it
would put at least a pointer to each struct device, and it's certainly
a waste that doesn't pay enough for the gain.

BTW, I wonder whether we can drop many codes in the remove path.
IIRC, kobject_del() should remove the whole sysfs files recursively,
so we don't have to remove files individually.


> > > > > > What if having a link to the chained group for appending entries
> > > > > > dynamically?  Just a wild idea, but it might make things easier.
> > > > > 
> > > > > We have the ability to pass a group list pointer to device_create
> > > > > already, and the attribute pointer is a list of groups as well, how can
> > > > > we change this to be "easier"?
> > > > 
> > > > I guess the order is the problem.  In many cases, you know the
> > > > additional entries only after the device creation.  The device
> > > > creation is often done by a helper code.  So the driver has no control
> > > > to it, just gets the resultant device.
> > > 
> > > Yeah, that's the problem.  And another problem is drivers adding
> > > attributes to devices after they are bound to a device, which is kind of
> > > pointless, as the uevent is long past at that point in time.  I've
> > > cleaned up a bunch of those, but odds are there are still more to fix.
> > 
> > Right, there are a bunch of drivers doing it.  I guess partly because
> > they don't need uevents for creation, but also partly because there is
> > no way to give attribute groups properly in some cases.  For example,
> > misc_register() or register_framebuffer() calls device_create() so the
> > caller can't pass groups.
> > 
> > It'd be trivial to extend struct miscdevice to carry an optional group
> > field and change the call to device_create_with_groups().  But,
> > fb_info has also common sysfs entries, so it'd need also the solution
> > above with device_add_groups() in addition.
> 
> Your patch to do that looks good, I'll queue them all up after 3.20-rc1
> is out as it's too close to 3.19 at the moment.

Yeah, 3.20 is good enough.  Thanks for picking it up.

I've submitted other cleanup patches to various subsystems, some have
been merged for 3.20 and some are pending to 3.21, as it seems.  There
are still a few easy lowhanging fruits left, but mostly arch stuff (or
arch-specific drivers).

However, there are also many device_create_file() calls that can't be
translated to static attribute groups.  Namely, lots of drivers add
the sysfs files onto the device that is being probed.  That is, in
xxx_probe() for a platform device or a pci device (or others), the
driver puts new files to the probed device itself.

I have no idea how this can be implemented in a better way.


Takashi