[RFC PATCH 0/5] Introduce and use device_create_groups

[RFC PATCH 0/5] Introduce and use device_create_groups

[Guenter Roeck]

This patch series introduces the new driver core API function
device_create_groups().

device_create_groups() lets callers create devices as well as associated
sysfs attributes with a single call. This avoids potential race conditions
seen if sysfs attributes on new devices are created later.

The rationale for the new API is that sysfs attributes should be created
synchronously with device creation to avoid race conditions, as outlined in
http://www.linuxfoundation.org/news-media/blogs/browse/2013/06/how-create-sysfs-file-correctly.

Unfortunately, the only API function to create a device dynamically is
device_create, which does not support the notion of adding sysfs attributes
when creating a device. The new API call is similar but lets the caller provide
a list of sysfs attribute groups.

The first patch in the series introduces the new driver core API function.

The second patch introduces hwmon_device_register_groups(), which uses the new
API call.

The remaining patches convert some hwmon drivers to use the new API to show and
test its use.

[RFC PATCH 1/5] driver core: Introduce device_create_groups

[Guenter Roeck]

device_create_groups lets callers create devices as well as associated
sysfs attributes with a single call. This avoids race conditions seen
if sysfs attributes on new devices are created later.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/base/core.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/device.h |    4 ++++
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index dc3ea23..dd82645 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1692,9 +1692,11 @@ static void device_create_release(struct device *dev)
  * Note: the struct class passed to this function must have previously
  * been created with a call to class_create().
  */
-struct device *device_create_vargs(struct class *class, struct device *parent,
-				   dev_t devt, void *drvdata, const char *fmt,
-				   va_list args)
+static struct device *
+device_create_groups_vargs(struct class *class, struct device *parent,
+			   dev_t devt, void *drvdata,
+			   const struct attribute_group **groups,
+			   const char *fmt, va_list args)
 {
 	struct device *dev = NULL;
 	int retval = -ENODEV;
@@ -1711,6 +1713,7 @@ struct device *device_create_vargs(struct class *class, struct device *parent,
 	dev->devt = devt;
 	dev->class = class;
 	dev->parent = parent;
+	dev->groups = groups;
 	dev->release = device_create_release;
 	dev_set_drvdata(dev, drvdata);
 
@@ -1728,6 +1731,14 @@ error:
 	put_device(dev);
 	return ERR_PTR(retval);
 }
+
+struct device *device_create_vargs(struct class *class, struct device *parent,
+				   dev_t devt, void *drvdata, const char *fmt,
+				   va_list args)
+{
+	return device_create_groups_vargs(class, parent, devt, drvdata, NULL,
+					  fmt, args);
+}
 EXPORT_SYMBOL_GPL(device_create_vargs);
 
 /**
@@ -1767,6 +1778,49 @@ struct device *device_create(struct class *class, struct device *parent,
 }
 EXPORT_SYMBOL_GPL(device_create);
 
+/**
+ * device_create_groups - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @groups: NULL-terminated list of attribute groups to be created
+ * @fmt: string for the device's name
+ *
+ * This function can be used by char device classes.  A struct device
+ * will be created in sysfs, registered to the specified class.
+ * Additional attributes specified in the groups parameter will also
+ * be created automatically.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Returns &struct device pointer on success, or ERR_PTR() on error.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_groups(struct class *class, struct device *parent,
+				    dev_t devt, void *drvdata,
+				    const struct attribute_group **groups,
+				    const char *fmt, ...)
+{
+	va_list vargs;
+	struct device *dev;
+
+	va_start(vargs, fmt);
+	dev = device_create_groups_vargs(class, parent, devt, drvdata, groups,
+					 fmt, vargs);
+	va_end(vargs);
+	return dev;
+}
+EXPORT_SYMBOL_GPL(device_create_groups);
+
 static int __match_devt(struct device *dev, const void *data)
 {
 	const dev_t *devt = data;
diff --git a/include/linux/device.h b/include/linux/device.h
index bcf8c0d..573ddb6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -924,6 +924,10 @@ extern __printf(5, 6)
 struct device *device_create(struct class *cls, struct device *parent,
 			     dev_t devt, void *drvdata,
 			     const char *fmt, ...);
+struct device *device_create_groups(struct class *cls, struct device *parent,
+			     dev_t devt, void *drvdata,
+			     const struct attribute_group **groups,
+			     const char *fmt, ...);
 extern void device_destroy(struct class *cls, dev_t devt);
 
 /*
-- 
1.7.9.7

Re: [RFC PATCH 1/5] driver core: Introduce device_create_groups

[Greg Kroah-Hartman]

On Sat, Jul 06, 2013 at 10:24:51AM -0700, Guenter Roeck wrote:
> device_create_groups lets callers create devices as well as associated
> sysfs attributes with a single call. This avoids race conditions seen
> if sysfs attributes on new devices are created later.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/base/core.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/device.h |    4 ++++
>  2 files changed, 61 insertions(+), 3 deletions(-)

Ok, in reading the code, it makes more sense to me now.  That looks
good, but the name is a bit rough.  How many device_create() calls do we
have in the kernel today?

$ git grep -w device_create | wc -l
127

That's really not that many, I wonder how many of those should be using
something like this as well?  A quick look showed that the sound core
should be using this, but maybe not all that many others...

My objection to the name is at first glance, it sounds like you are
creating "groups", not a device + groups.
"device_create_with_groups()"?

Ick, naming is hard...

greg k-h

Re: [RFC PATCH 1/5] driver core: Introduce device_create_groups

[Guenter Roeck]

On Sat, Jul 06, 2013 at 10:47:58AM -0700, Greg Kroah-Hartman wrote:
> On Sat, Jul 06, 2013 at 10:24:51AM -0700, Guenter Roeck wrote:
> > device_create_groups lets callers create devices as well as associated
> > sysfs attributes with a single call. This avoids race conditions seen
> > if sysfs attributes on new devices are created later.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/base/core.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/device.h |    4 ++++
> >  2 files changed, 61 insertions(+), 3 deletions(-)
> 
> Ok, in reading the code, it makes more sense to me now.  That looks
> good, but the name is a bit rough.  How many device_create() calls do we
> have in the kernel today?
> 
> $ git grep -w device_create | wc -l
> 127
> 
> That's really not that many, I wonder how many of those should be using
> something like this as well?  A quick look showed that the sound core
> should be using this, but maybe not all that many others...
> 
Pretty much all hwmon devices. Given that, another option would be to not use
device_create in the hwmon core code, but more or less re-write it there.
I don't think that would be such a good idea, but it is my fallback option.

> My objection to the name is at first glance, it sounds like you are
> creating "groups", not a device + groups.
> "device_create_with_groups()"?
> 
No problem. You pick the name ...

Thanks,
Guenter

[RFC PATCH 2/5] hwmon: Introduce hwmon_device_register_groups

[Guenter Roeck]

hwmon_device_register_groups() lets callers register a hwmon device
together with all sysfs attributes in a single call.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c |   14 +++++++++++---
 include/linux/hwmon.h |    4 ++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 646314f..e94a053 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -38,7 +38,9 @@ static DEFINE_IDA(hwmon_ida);
  *
  * Returns the pointer to the new device.
  */
-struct device *hwmon_device_register(struct device *dev)
+struct device *
+hwmon_device_register_groups(struct device *dev, void *drvdata,
+			     const struct attribute_group **groups)
 {
 	struct device *hwdev;
 	int id;
@@ -47,14 +49,20 @@ struct device *hwmon_device_register(struct device *dev)
 	if (id < 0)
 		return ERR_PTR(id);
 
-	hwdev = device_create(hwmon_class, dev, MKDEV(0, 0), NULL,
-			      HWMON_ID_FORMAT, id);
+	hwdev = device_create_groups(hwmon_class, dev, MKDEV(0, 0), drvdata,
+				     groups, HWMON_ID_FORMAT, id);
 
 	if (IS_ERR(hwdev))
 		ida_simple_remove(&hwmon_ida, id);
 
 	return hwdev;
 }
+EXPORT_SYMBOL_GPL(hwmon_device_register_groups);
+
+struct device *hwmon_device_register(struct device *dev)
+{
+	return hwmon_device_register_groups(dev, NULL, NULL);
+}
 EXPORT_SYMBOL_GPL(hwmon_device_register);
 
 /**
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index b2514f7..ad91c45 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -15,8 +15,12 @@
 #define _HWMON_H_
 
 struct device;
+struct attribute_group;
 
 struct device *hwmon_device_register(struct device *dev);
+struct device *
+hwmon_device_register_groups(struct device *dev, void *drvdata,
+			     const struct attribute_group **groups);
 
 void hwmon_device_unregister(struct device *dev);
 
-- 
1.7.9.7

[RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ds1621.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
index a26ba7a..f1d0fa0 100644
--- a/drivers/hwmon/ds1621.c
+++ b/drivers/hwmon/ds1621.c
@@ -358,11 +358,15 @@ static const struct attribute_group ds1621_group = {
 	.is_visible = ds1621_attribute_visible
 };
 
+static const struct attribute_group *ds1621_groups[] = {
+	&ds1621_group,
+	NULL
+};
+
 static int ds1621_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ds1621_data *data;
-	int err;
 
 	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
 			    GFP_KERNEL);
@@ -377,22 +381,12 @@ static int ds1621_probe(struct i2c_client *client,
 	/* Initialize the DS1621 chip */
 	ds1621_init_client(client);
 
-	/* Register sysfs hooks */
-	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
-	if (err)
-		return err;
-
-	data->hwmon_dev = hwmon_device_register(&client->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove_files;
-	}
+	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
+						       ds1621_groups);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
 
 	return 0;
-
- exit_remove_files:
-	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
-	return err;
 }
 
 static int ds1621_remove(struct i2c_client *client)
@@ -400,7 +394,6 @@ static int ds1621_remove(struct i2c_client *client)
 	struct ds1621_data *data = i2c_get_clientdata(client);
 
 	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
 
 	return 0;
 }
-- 
1.7.9.7

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Greg Kroah-Hartman]

On Sat, Jul 06, 2013 at 10:24:53AM -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/ds1621.c |   25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
> index a26ba7a..f1d0fa0 100644
> --- a/drivers/hwmon/ds1621.c
> +++ b/drivers/hwmon/ds1621.c
> @@ -358,11 +358,15 @@ static const struct attribute_group ds1621_group = {
>  	.is_visible = ds1621_attribute_visible
>  };
>  
> +static const struct attribute_group *ds1621_groups[] = {
> +	&ds1621_group,
> +	NULL
> +};
> +
>  static int ds1621_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
>  	struct ds1621_data *data;
> -	int err;
>  
>  	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
>  			    GFP_KERNEL);
> @@ -377,22 +381,12 @@ static int ds1621_probe(struct i2c_client *client,
>  	/* Initialize the DS1621 chip */
>  	ds1621_init_client(client);
>  
> -	/* Register sysfs hooks */
> -	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
> -	if (err)
> -		return err;
> -
> -	data->hwmon_dev = hwmon_device_register(&client->dev);
> -	if (IS_ERR(data->hwmon_dev)) {
> -		err = PTR_ERR(data->hwmon_dev);
> -		goto exit_remove_files;
> -	}
> +	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
> +						       ds1621_groups);
> +	if (IS_ERR(data->hwmon_dev))
> +		return PTR_ERR(data->hwmon_dev);
>  
>  	return 0;
> -
> - exit_remove_files:
> -	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
> -	return err;

Aren't your sysfs files now being created attached to a different device
than they originally were?  Before this patch, they were being attached
to the parent device of the hwmon device being created with the call to
hwmon_device_register(), and now they are attached to the device created
with that call.

Is that what you really want?  Can userspace handle this movement of
files?

thanks,

greg k-h

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Jean Delvare]

On Sat, 6 Jul 2013 10:57:47 -0700, Greg Kroah-Hartman wrote:
> On Sat, Jul 06, 2013 at 10:24:53AM -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/hwmon/ds1621.c |   25 +++++++++----------------
> >  1 file changed, 9 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
> > index a26ba7a..f1d0fa0 100644
> > --- a/drivers/hwmon/ds1621.c
> > +++ b/drivers/hwmon/ds1621.c
> > @@ -358,11 +358,15 @@ static const struct attribute_group ds1621_group = {
> >  	.is_visible = ds1621_attribute_visible
> >  };
> >  
> > +static const struct attribute_group *ds1621_groups[] = {
> > +	&ds1621_group,
> > +	NULL
> > +};
> > +
> >  static int ds1621_probe(struct i2c_client *client,
> >  			const struct i2c_device_id *id)
> >  {
> >  	struct ds1621_data *data;
> > -	int err;
> >  
> >  	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
> >  			    GFP_KERNEL);
> > @@ -377,22 +381,12 @@ static int ds1621_probe(struct i2c_client *client,
> >  	/* Initialize the DS1621 chip */
> >  	ds1621_init_client(client);
> >  
> > -	/* Register sysfs hooks */
> > -	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
> > -	if (err)
> > -		return err;
> > -
> > -	data->hwmon_dev = hwmon_device_register(&client->dev);
> > -	if (IS_ERR(data->hwmon_dev)) {
> > -		err = PTR_ERR(data->hwmon_dev);
> > -		goto exit_remove_files;
> > -	}
> > +	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
> > +						       ds1621_groups);
> > +	if (IS_ERR(data->hwmon_dev))
> > +		return PTR_ERR(data->hwmon_dev);
> >  
> >  	return 0;
> > -
> > - exit_remove_files:
> > -	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
> > -	return err;
> 
> Aren't your sysfs files now being created attached to a different device
> than they originally were?  Before this patch, they were being attached
> to the parent device of the hwmon device being created with the call to
> hwmon_device_register(), and now they are attached to the device created
> with that call.
> 
> Is that what you really want?  Can userspace handle this movement of
> files?

libsensors supports both, as well as pwmconfig/fancontrol. Some drivers
are already doing that so every application dealing with hwmon devices
should handle it already.

-- 
Jean Delvare

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Guenter Roeck]

On Sat, Jul 06, 2013 at 08:01:51PM +0200, Jean Delvare wrote:
> On Sat, 6 Jul 2013 10:57:47 -0700, Greg Kroah-Hartman wrote:
> > On Sat, Jul 06, 2013 at 10:24:53AM -0700, Guenter Roeck wrote:
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  drivers/hwmon/ds1621.c |   25 +++++++++----------------
> > >  1 file changed, 9 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
> > > index a26ba7a..f1d0fa0 100644
> > > --- a/drivers/hwmon/ds1621.c
> > > +++ b/drivers/hwmon/ds1621.c
> > > @@ -358,11 +358,15 @@ static const struct attribute_group ds1621_group = {
> > >  	.is_visible = ds1621_attribute_visible
> > >  };
> > >  
> > > +static const struct attribute_group *ds1621_groups[] = {
> > > +	&ds1621_group,
> > > +	NULL
> > > +};
> > > +
> > >  static int ds1621_probe(struct i2c_client *client,
> > >  			const struct i2c_device_id *id)
> > >  {
> > >  	struct ds1621_data *data;
> > > -	int err;
> > >  
> > >  	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
> > >  			    GFP_KERNEL);
> > > @@ -377,22 +381,12 @@ static int ds1621_probe(struct i2c_client *client,
> > >  	/* Initialize the DS1621 chip */
> > >  	ds1621_init_client(client);
> > >  
> > > -	/* Register sysfs hooks */
> > > -	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
> > > -	if (err)
> > > -		return err;
> > > -
> > > -	data->hwmon_dev = hwmon_device_register(&client->dev);
> > > -	if (IS_ERR(data->hwmon_dev)) {
> > > -		err = PTR_ERR(data->hwmon_dev);
> > > -		goto exit_remove_files;
> > > -	}
> > > +	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
> > > +						       ds1621_groups);
> > > +	if (IS_ERR(data->hwmon_dev))
> > > +		return PTR_ERR(data->hwmon_dev);
> > >  
> > >  	return 0;
> > > -
> > > - exit_remove_files:
> > > -	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
> > > -	return err;
> > 
> > Aren't your sysfs files now being created attached to a different device
> > than they originally were?  Before this patch, they were being attached
> > to the parent device of the hwmon device being created with the call to
> > hwmon_device_register(), and now they are attached to the device created
> > with that call.
> > 
> > Is that what you really want?  Can userspace handle this movement of
> > files?
> 
> libsensors supports both, as well as pwmconfig/fancontrol. Some drivers
> are already doing that so every application dealing with hwmon devices
> should handle it already.
> 
Yes, and I tested it with the new nct6775 driver to make sure that it works.
The i2c and spi drivers need some more work, though - the name attribute is
now missing and will have to be created for the hwmon device itself. If you have
an idea how to do that without adding it in every driver, please let me know.

Thanks,
Guenter

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Greg Kroah-Hartman]

On Sat, Jul 06, 2013 at 12:18:16PM -0700, Guenter Roeck wrote:
> On Sat, Jul 06, 2013 at 08:01:51PM +0200, Jean Delvare wrote:
> > On Sat, 6 Jul 2013 10:57:47 -0700, Greg Kroah-Hartman wrote:
> > > On Sat, Jul 06, 2013 at 10:24:53AM -0700, Guenter Roeck wrote:
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > ---
> > > >  drivers/hwmon/ds1621.c |   25 +++++++++----------------
> > > >  1 file changed, 9 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
> > > > index a26ba7a..f1d0fa0 100644
> > > > --- a/drivers/hwmon/ds1621.c
> > > > +++ b/drivers/hwmon/ds1621.c
> > > > @@ -358,11 +358,15 @@ static const struct attribute_group ds1621_group = {
> > > >  	.is_visible = ds1621_attribute_visible
> > > >  };
> > > >  
> > > > +static const struct attribute_group *ds1621_groups[] = {
> > > > +	&ds1621_group,
> > > > +	NULL
> > > > +};
> > > > +
> > > >  static int ds1621_probe(struct i2c_client *client,
> > > >  			const struct i2c_device_id *id)
> > > >  {
> > > >  	struct ds1621_data *data;
> > > > -	int err;
> > > >  
> > > >  	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
> > > >  			    GFP_KERNEL);
> > > > @@ -377,22 +381,12 @@ static int ds1621_probe(struct i2c_client *client,
> > > >  	/* Initialize the DS1621 chip */
> > > >  	ds1621_init_client(client);
> > > >  
> > > > -	/* Register sysfs hooks */
> > > > -	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
> > > > -	if (err)
> > > > -		return err;
> > > > -
> > > > -	data->hwmon_dev = hwmon_device_register(&client->dev);
> > > > -	if (IS_ERR(data->hwmon_dev)) {
> > > > -		err = PTR_ERR(data->hwmon_dev);
> > > > -		goto exit_remove_files;
> > > > -	}
> > > > +	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
> > > > +						       ds1621_groups);
> > > > +	if (IS_ERR(data->hwmon_dev))
> > > > +		return PTR_ERR(data->hwmon_dev);
> > > >  
> > > >  	return 0;
> > > > -
> > > > - exit_remove_files:
> > > > -	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
> > > > -	return err;
> > > 
> > > Aren't your sysfs files now being created attached to a different device
> > > than they originally were?  Before this patch, they were being attached
> > > to the parent device of the hwmon device being created with the call to
> > > hwmon_device_register(), and now they are attached to the device created
> > > with that call.
> > > 
> > > Is that what you really want?  Can userspace handle this movement of
> > > files?
> > 
> > libsensors supports both, as well as pwmconfig/fancontrol. Some drivers
> > are already doing that so every application dealing with hwmon devices
> > should handle it already.
> > 
> Yes, and I tested it with the new nct6775 driver to make sure that it works.
> The i2c and spi drivers need some more work, though - the name attribute is
> now missing and will have to be created for the hwmon device itself. If you have
> an idea how to do that without adding it in every driver, please let me know.

Why do you need a name attribute at all?  Shouldn't that just be the
name of the device, or the parent?

Can you make the name be part of the groups always?

greg k-h

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Guenter Roeck]

On Sat, Jul 06, 2013 at 12:31:42PM -0700, Greg Kroah-Hartman wrote:
> On Sat, Jul 06, 2013 at 12:18:16PM -0700, Guenter Roeck wrote:
> > On Sat, Jul 06, 2013 at 08:01:51PM +0200, Jean Delvare wrote:
> > > On Sat, 6 Jul 2013 10:57:47 -0700, Greg Kroah-Hartman wrote:
> > > > On Sat, Jul 06, 2013 at 10:24:53AM -0700, Guenter Roeck wrote:
> > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > ---
> > > > >  drivers/hwmon/ds1621.c |   25 +++++++++----------------
> > > > >  1 file changed, 9 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
> > > > > index a26ba7a..f1d0fa0 100644
> > > > > --- a/drivers/hwmon/ds1621.c
> > > > > +++ b/drivers/hwmon/ds1621.c
> > > > > @@ -358,11 +358,15 @@ static const struct attribute_group ds1621_group = {
> > > > >  	.is_visible = ds1621_attribute_visible
> > > > >  };
> > > > >  
> > > > > +static const struct attribute_group *ds1621_groups[] = {
> > > > > +	&ds1621_group,
> > > > > +	NULL
> > > > > +};
> > > > > +
> > > > >  static int ds1621_probe(struct i2c_client *client,
> > > > >  			const struct i2c_device_id *id)
> > > > >  {
> > > > >  	struct ds1621_data *data;
> > > > > -	int err;
> > > > >  
> > > > >  	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
> > > > >  			    GFP_KERNEL);
> > > > > @@ -377,22 +381,12 @@ static int ds1621_probe(struct i2c_client *client,
> > > > >  	/* Initialize the DS1621 chip */
> > > > >  	ds1621_init_client(client);
> > > > >  
> > > > > -	/* Register sysfs hooks */
> > > > > -	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
> > > > > -	if (err)
> > > > > -		return err;
> > > > > -
> > > > > -	data->hwmon_dev = hwmon_device_register(&client->dev);
> > > > > -	if (IS_ERR(data->hwmon_dev)) {
> > > > > -		err = PTR_ERR(data->hwmon_dev);
> > > > > -		goto exit_remove_files;
> > > > > -	}
> > > > > +	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
> > > > > +						       ds1621_groups);
> > > > > +	if (IS_ERR(data->hwmon_dev))
> > > > > +		return PTR_ERR(data->hwmon_dev);
> > > > >  
> > > > >  	return 0;
> > > > > -
> > > > > - exit_remove_files:
> > > > > -	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
> > > > > -	return err;
> > > > 
> > > > Aren't your sysfs files now being created attached to a different device
> > > > than they originally were?  Before this patch, they were being attached
> > > > to the parent device of the hwmon device being created with the call to
> > > > hwmon_device_register(), and now they are attached to the device created
> > > > with that call.
> > > > 
> > > > Is that what you really want?  Can userspace handle this movement of
> > > > files?
> > > 
> > > libsensors supports both, as well as pwmconfig/fancontrol. Some drivers
> > > are already doing that so every application dealing with hwmon devices
> > > should handle it already.
> > > 
> > Yes, and I tested it with the new nct6775 driver to make sure that it works.
> > The i2c and spi drivers need some more work, though - the name attribute is
> > now missing and will have to be created for the hwmon device itself. If you have
> > an idea how to do that without adding it in every driver, please let me know.
> 
> Why do you need a name attribute at all?  Shouldn't that just be the
> name of the device, or the parent?
> 
It is part of the hwmon ABI and, at least in almost all cases, does return
the name of the device.

It is kind of similar to the 'name' attribute in i2c and spi drivers - in fact,
the drivers just use that attribute since the hwmon attributes are currently
associated with the parent device.

> Can you make the name be part of the groups always?
> 
Yes, that would be one option. All drivers except for spi and i2c drivers
already do that.

Another option would be to create a class attribute to return the parent driver
name for spi and i2c devices and otherwise the hwmon driver name. This would
reduce the amount of code needed in the drivers. Jean, do you see a problem with
that approach (except that we would have to remove explicit name attributes from
existing drivers) ?

Thanks,
Guenter

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Jean Delvare]

On Sat, 6 Jul 2013 12:48:03 -0700, Guenter Roeck wrote:
> Another option would be to create a class attribute to return the parent driver
> name for spi and i2c devices and otherwise the hwmon driver name. This would
> reduce the amount of code needed in the drivers. Jean, do you see a problem with
> that approach (except that we would have to remove explicit name attributes from
> existing drivers) ?

A class attribute is what I had in mind in my previous reply. However I
do not understand the "hwmon driver name" in your sentence above.
Device name != driver name. But well I'm sure I'll understand what you
mean as soon as you show us a candidate implementation ;)

-- 
Jean Delvare

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Guenter Roeck]

On Sat, Jul 06, 2013 at 09:59:57PM +0200, Jean Delvare wrote:
> On Sat, 6 Jul 2013 12:48:03 -0700, Guenter Roeck wrote:
> > Another option would be to create a class attribute to return the parent driver
> > name for spi and i2c devices and otherwise the hwmon driver name. This would
> > reduce the amount of code needed in the drivers. Jean, do you see a problem with
> > that approach (except that we would have to remove explicit name attributes from
> > existing drivers) ?
> 
> A class attribute is what I had in mind in my previous reply. However I
> do not understand the "hwmon driver name" in your sentence above.

s/driver/device/

> Device name != driver name. But well I'm sure I'll understand what you
> mean as soon as you show us a candidate implementation ;)
> 
Guess so :)

Guenter

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Jean Delvare]

On Sat, 6 Jul 2013 12:31:42 -0700, Greg Kroah-Hartman wrote:
> On Sat, Jul 06, 2013 at 12:18:16PM -0700, Guenter Roeck wrote:
> > Yes, and I tested it with the new nct6775 driver to make sure that it works.
> > The i2c and spi drivers need some more work, though - the name attribute is
> > now missing and will have to be created for the hwmon device itself. If you have
> > an idea how to do that without adding it in every driver, please let me know.
> 
> Why do you need a name attribute at all?  Shouldn't that just be the
> name of the device, or the parent?

libsensors expects the hwmon attributes and the name attribute at the
same place.

> Can you make the name be part of the groups always?

Sounds good, but then we have to make the hwmon core handles it. So far
each driver (or its bus type subsystem) was responsible for it.

-- 
Jean Delvare

Re: [RFC PATCH 3/5] hwmon: (ds1621) Convert to use hwmon_device_register_groups

[Guenter Roeck]

On Sat, Jul 06, 2013 at 09:54:58PM +0200, Jean Delvare wrote:
> On Sat, 6 Jul 2013 12:31:42 -0700, Greg Kroah-Hartman wrote:
> > On Sat, Jul 06, 2013 at 12:18:16PM -0700, Guenter Roeck wrote:
> > > Yes, and I tested it with the new nct6775 driver to make sure that it works.
> > > The i2c and spi drivers need some more work, though - the name attribute is
> > > now missing and will have to be created for the hwmon device itself. If you have
> > > an idea how to do that without adding it in every driver, please let me know.
> > 
> > Why do you need a name attribute at all?  Shouldn't that just be the
> > name of the device, or the parent?
> 
> libsensors expects the hwmon attributes and the name attribute at the
> same place.
> 
> > Can you make the name be part of the groups always?
> 
> Sounds good, but then we have to make the hwmon core handles it. So far
> each driver (or its bus type subsystem) was responsible for it.
> 
A quick scan suggests this affects some 50+ drivers, which all implement the name
attribute. On the downside a lot of work, on the upside there will be lots of
removed code.

> -- 
> Jean Delvare
> 

[RFC PATCH 4/5] hwmon: (gpio-fan) Convert to use hwmon_device_register_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gpio-fan.c |   23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 3104149..7953d99 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -358,6 +358,11 @@ static const struct attribute_group gpio_fan_group = {
 	.is_visible = gpio_fan_is_visible,
 };
 
+static const struct attribute_group *gpio_fan_groups[] = {
+	&gpio_fan_group,
+	NULL
+};
+
 static int fan_ctrl_init(struct gpio_fan_data *fan_data,
 			 struct gpio_fan_platform_data *pdata)
 {
@@ -539,24 +544,15 @@ static int gpio_fan_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	err = sysfs_create_group(&pdev->dev.kobj, &gpio_fan_group);
-	if (err)
-		return err;
-
 	/* Make this driver part of hwmon class. */
-	fan_data->hwmon_dev = hwmon_device_register(&pdev->dev);
-	if (IS_ERR(fan_data->hwmon_dev)) {
-		err = PTR_ERR(fan_data->hwmon_dev);
-		goto err_remove;
-	}
+	fan_data->hwmon_dev = hwmon_device_register_groups(&pdev->dev, fan_data,
+							   gpio_fan_groups);
+	if (IS_ERR(fan_data->hwmon_dev))
+		return PTR_ERR(fan_data->hwmon_dev);
 
 	dev_info(&pdev->dev, "GPIO fan initialized\n");
 
 	return 0;
-
-err_remove:
-	sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_group);
-	return err;
 }
 
 static int gpio_fan_remove(struct platform_device *pdev)
@@ -564,7 +560,6 @@ static int gpio_fan_remove(struct platform_device *pdev)
 	struct gpio_fan_data *fan_data = platform_get_drvdata(pdev);
 
 	hwmon_device_unregister(fan_data->hwmon_dev);
-	sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_group);
 
 	return 0;
 }
-- 
1.7.9.7

[RFC PATCH 5/5] hwmon: (ltc4245) Convert to use hwmon_device_register_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ltc4245.c |   58 +++++++++++------------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

diff --git a/drivers/hwmon/ltc4245.c b/drivers/hwmon/ltc4245.c
index cdc1ecc..53b01abe 100644
--- a/drivers/hwmon/ltc4245.c
+++ b/drivers/hwmon/ltc4245.c
@@ -53,6 +53,8 @@ enum ltc4245_cmd {
 struct ltc4245_data {
 	struct device *hwmon_dev;
 
+	const struct attribute_group *groups[3];
+
 	struct mutex update_lock;
 	bool valid;
 	unsigned long last_updated; /* in jiffies */
@@ -455,41 +457,16 @@ static const struct attribute_group ltc4245_gpio_group = {
 	.attrs = ltc4245_gpio_attributes,
 };
 
-static int ltc4245_sysfs_create_groups(struct i2c_client *client)
+static void ltc4245_sysfs_add_groups(struct i2c_client *client)
 {
 	struct ltc4245_data *data = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
-	int ret;
-
-	/* register the standard sysfs attributes */
-	ret = sysfs_create_group(&dev->kobj, &ltc4245_std_group);
-	if (ret) {
-		dev_err(dev, "unable to register standard attributes\n");
-		return ret;
-	}
-
-	/* if we're using the extra gpio support, register it's attributes */
-	if (data->use_extra_gpios) {
-		ret = sysfs_create_group(&dev->kobj, &ltc4245_gpio_group);
-		if (ret) {
-			dev_err(dev, "unable to register gpio attributes\n");
-			sysfs_remove_group(&dev->kobj, &ltc4245_std_group);
-			return ret;
-		}
-	}
 
-	return 0;
-}
-
-static void ltc4245_sysfs_remove_groups(struct i2c_client *client)
-{
-	struct ltc4245_data *data = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
+	/* standard sysfs attributes */
+	data->groups[0] = &ltc4245_std_group;
 
+	/* if we're using the extra gpio support, register it's attributes */
 	if (data->use_extra_gpios)
-		sysfs_remove_group(&dev->kobj, &ltc4245_gpio_group);
-
-	sysfs_remove_group(&dev->kobj, &ltc4245_std_group);
+		data->groups[1] = &ltc4245_gpio_group;
 }
 
 static bool ltc4245_use_extra_gpios(struct i2c_client *client)
@@ -517,7 +494,6 @@ static int ltc4245_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = client->adapter;
 	struct ltc4245_data *data;
-	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
@@ -534,22 +510,15 @@ static int ltc4245_probe(struct i2c_client *client,
 	i2c_smbus_write_byte_data(client, LTC4245_FAULT1, 0x00);
 	i2c_smbus_write_byte_data(client, LTC4245_FAULT2, 0x00);
 
-	/* Register sysfs hooks */
-	ret = ltc4245_sysfs_create_groups(client);
-	if (ret)
-		return ret;
+	/* Add sysfs hooks */
+	ltc4245_sysfs_add_groups(client);
 
-	data->hwmon_dev = hwmon_device_register(&client->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		ret = PTR_ERR(data->hwmon_dev);
-		goto out_hwmon_device_register;
-	}
+	data->hwmon_dev = hwmon_device_register_groups(&client->dev, data,
+						       data->groups);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
 
 	return 0;
-
-out_hwmon_device_register:
-	ltc4245_sysfs_remove_groups(client);
-	return ret;
 }
 
 static int ltc4245_remove(struct i2c_client *client)
@@ -557,7 +526,6 @@ static int ltc4245_remove(struct i2c_client *client)
 	struct ltc4245_data *data = i2c_get_clientdata(client);
 
 	hwmon_device_unregister(data->hwmon_dev);
-	ltc4245_sysfs_remove_groups(client);
 
 	return 0;
 }
-- 
1.7.9.7

Re: [RFC PATCH 0/5] Introduce and use device_create_groups

[Greg Kroah-Hartman]

On Sat, Jul 06, 2013 at 10:24:50AM -0700, Guenter Roeck wrote:
> This patch series introduces the new driver core API function
> device_create_groups().
> 
> device_create_groups() lets callers create devices as well as associated
> sysfs attributes with a single call. This avoids potential race conditions
> seen if sysfs attributes on new devices are created later.
> 
> The rationale for the new API is that sysfs attributes should be created
> synchronously with device creation to avoid race conditions, as outlined in
> http://www.linuxfoundation.org/news-media/blogs/browse/2013/06/how-create-sysfs-file-correctly.
> 
> Unfortunately, the only API function to create a device dynamically is
> device_create, which does not support the notion of adding sysfs attributes
> when creating a device. The new API call is similar but lets the caller provide
> a list of sysfs attribute groups.

What's wrong with the default attribute groups that all devices, busses,
and classes can define to be properly created when the device is added
to the driver core?  How does that not already provide this
functionality?

greg k-h