Provide devres functions for device_create_file, sysfs_create_file,
and sysfs_create_group plus the respective remove functions.
Idea is to be able to drop calls to the remove functions from the various
drivers using those calls.
Potential savings are substantial. There are more than 700 calls to
device_remove_file in the kernel, more than 500 calls to sysfs_remove_group,
and some 50 calls to sysfs_remove_file (though not all of those use dev->kobj
as parameter). Expanding the API to sysfs_create_bin_file would add another 80+
opportunities, and adding sysfs_create_link would create another 100 or so.
The approach used in this patch set is one possible solution.
Another possibility would be to not bother with sysfs and provide
devm_device_create_file, devm_device_create_group, and its remove functions
in drivers/base/core.c instead.
I am not sure which approach is better. The solution presented here is more
aligned with other devm_ functions (I think) and does not require changing
function parameters besides the first one. Providing functions in the driver
core code would mean parameter changes [sysfs_create_file(dev, attr) ->
devm_device_create_file(dev, device_attr)] and thus be more invasive and thus a
bit more risky. It would also create devres data entries even if sysfs
is not configured (if that is even possible nowadays).
One question with the presented API is how the API should look like.
Should it be
int devm_sysfs_create_file(struct device *dev, const struct attribute *attr)
or
int devm_sysfs_create_file(struct device *dev,
struct kobject *kobj,
const struct attribute *attr)
The latter would be more consistent with other devm_ functions, but the
additional parameter seems like a waste, as the kobj would presumably
always be &dev->kobj anyway.
Before I go much further with this, I would like to get some feedback from the
community if this all makes sense or not.
Note that the code is compile tested only at this time - I don't want to spend
too much time on it if turns out to be a bad idea.
Add support for devm_device_create_file and devm_device_remove_file.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/base/core.c | 18 ++++++++++++++++++
include/linux/device.h | 4 ++++
2 files changed, 22 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 56536f4b0..76ff488 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -568,6 +568,16 @@ int device_create_file(struct device *dev,
return error;
}
+int devm_device_create_file(struct device *dev,
+ const struct device_attribute *attr)
+{
+ int error = 0;
+ if (dev)
+ error = devm_sysfs_create_file(dev, &attr->attr);
+ return error;
+}
+EXPORT_SYMBOL_GPL(devm_device_create_file);
+
/**
* device_remove_file - remove sysfs attribute file.
* @dev: device.
@@ -580,6 +590,14 @@ void device_remove_file(struct device *dev,
sysfs_remove_file(&dev->kobj, &attr->attr);
}
+void devm_device_remove_file(struct device *dev,
+ const struct device_attribute *attr)
+{
+ if (dev)
+ devm_sysfs_remove_file(dev, &attr->attr);
+}
+EXPORT_SYMBOL_GPL(devm_device_remove_file);
+
/**
* device_create_bin_file - create sysfs binary attribute file for device.
* @dev: device.
diff --git a/include/linux/device.h b/include/linux/device.h
index 9d6464e..1c56fb7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -521,8 +521,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 devm_device_create_file(struct device *device,
+ const struct device_attribute *entry);
extern void device_remove_file(struct device *dev,
const struct device_attribute *attr);
+extern void devm_device_remove_file(struct device *dev,
+ const struct device_attribute *attr);
extern int __must_check device_create_bin_file(struct device *dev,
const struct bin_attribute *attr);
extern void device_remove_bin_file(struct device *dev,
--
1.7.9.7
Add support for
devm_sysfs_create_file
devm_sysfs_remove_file
devm_sysfs_create_group
devm_sysfs_remove_group
Signed-off-by: Guenter Roeck <[email protected]>
---
fs/sysfs/file.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
fs/sysfs/group.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/sysfs.h | 30 ++++++++++++++++++++++++++++
3 files changed, 134 insertions(+)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 602f56d..d8f4631 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -20,6 +20,7 @@
#include <linux/list.h>
#include <linux/mutex.h>
#include <linux/limits.h>
+#include <linux/device.h>
#include <asm/uaccess.h>
#include "sysfs.h"
@@ -589,6 +590,40 @@ int sysfs_create_files(struct kobject *kobj, const struct attribute **ptr)
return err;
}
+struct sysfs_devres {
+ struct list_head node;
+ const struct attribute *attr;
+};
+
+static void devm_sysfs_file_release(struct device *dev, void *res)
+{
+ struct sysfs_devres *devres = res;
+
+ sysfs_remove_file(&dev->kobj, devres->attr);
+}
+
+int devm_sysfs_create_file(struct device *dev, const struct attribute *attr)
+{
+ struct sysfs_devres *devres;
+ int ret;
+
+ devres = devres_alloc(devm_sysfs_file_release, sizeof(*devres),
+ GFP_KERNEL);
+ if (!devres)
+ return -ENOMEM;
+
+ devres->attr = attr;
+
+ ret = sysfs_create_file(&dev->kobj, attr);
+ if (!ret)
+ devres_add(dev, devres);
+ else
+ devres_free(devres);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_create_file);
+
/**
* sysfs_add_file_to_group - add an attribute file to a pre-existing group.
* @kobj: object we're acting for.
@@ -678,6 +713,22 @@ void sysfs_remove_files(struct kobject * kobj, const struct attribute **ptr)
sysfs_remove_file(kobj, ptr[i]);
}
+static int devm_sysfs_device_match(struct device *dev, void *res, void *data)
+{
+ struct sysfs_devres *devres = res;
+
+ return devres->attr == data;
+}
+
+void devm_sysfs_remove_file(struct device *dev, const struct attribute *attr)
+{
+ WARN_ON(devres_destroy(dev, devm_sysfs_file_release,
+ devm_sysfs_device_match, (void *)attr));
+
+ sysfs_remove_file(&dev->kobj, attr);
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_remove_file);
+
/**
* sysfs_remove_file_from_group - remove an attribute file from a group.
* @kobj: object we're acting for.
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c..a0b8f95 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -13,6 +13,7 @@
#include <linux/dcache.h>
#include <linux/namei.h>
#include <linux/err.h>
+#include <linux/device.h>
#include "sysfs.h"
@@ -104,6 +105,41 @@ int sysfs_create_group(struct kobject *kobj,
return internal_create_group(kobj, 0, grp);
}
+struct sysfs_devres {
+ struct list_head node;
+ const struct attribute_group *grp;
+};
+
+static void devm_sysfs_group_release(struct device *dev, void *res)
+{
+ struct sysfs_devres *devres = res;
+
+ sysfs_remove_group(&dev->kobj, devres->grp);
+}
+
+int devm_sysfs_create_group(struct device *dev,
+ const struct attribute_group *grp)
+{
+ struct sysfs_devres *devres;
+ int ret;
+
+ devres = devres_alloc(devm_sysfs_group_release, sizeof(*devres),
+ GFP_KERNEL);
+ if (!devres)
+ return -ENOMEM;
+
+ devres->grp = grp;
+
+ ret = sysfs_create_group(&dev->kobj, grp);
+ if (!ret)
+ devres_add(dev, devres);
+ else
+ devres_free(devres);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_create_group);
+
/**
* sysfs_update_group - given a directory kobject, update an attribute group
* @kobj: The kobject to update the group on
@@ -152,6 +188,23 @@ void sysfs_remove_group(struct kobject * kobj,
sysfs_put(sd);
}
+static int devm_sysfs_device_match(struct device *dev, void *res, void *data)
+{
+ struct sysfs_devres *devres = res;
+
+ return devres->grp == data;
+}
+
+void devm_sysfs_remove_group(struct device *dev,
+ const struct attribute_group *grp)
+{
+ WARN_ON(devres_destroy(dev, devm_sysfs_group_release,
+ devm_sysfs_device_match, (void *)grp));
+
+ sysfs_remove_group(&dev->kobj, grp);
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_remove_group);
+
/**
* sysfs_merge_group - merge files into a pre-existing attribute group.
* @kobj: The kobject containing the group.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e2cee22..df0fa5a 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -20,6 +20,7 @@
#include <linux/atomic.h>
struct kobject;
+struct device;
struct module;
enum kobj_ns_type;
@@ -142,11 +143,14 @@ int __must_check sysfs_move_dir(struct kobject *kobj,
int __must_check sysfs_create_file(struct kobject *kobj,
const struct attribute *attr);
+int __must_check devm_sysfs_create_file(struct device *dev,
+ const struct attribute *attr);
int __must_check sysfs_create_files(struct kobject *kobj,
const struct attribute **attr);
int __must_check sysfs_chmod_file(struct kobject *kobj,
const struct attribute *attr, umode_t mode);
void sysfs_remove_file(struct kobject *kobj, const struct attribute *attr);
+void devm_sysfs_remove_file(struct device *dev, const struct attribute *attr);
void sysfs_remove_files(struct kobject *kobj, const struct attribute **attr);
int __must_check sysfs_create_bin_file(struct kobject *kobj,
@@ -169,10 +173,14 @@ void sysfs_delete_link(struct kobject *dir, struct kobject *targ,
int __must_check sysfs_create_group(struct kobject *kobj,
const struct attribute_group *grp);
+int __must_check devm_sysfs_create_group(struct device *dev,
+ const struct attribute_group *grp);
int sysfs_update_group(struct kobject *kobj,
const struct attribute_group *grp);
void sysfs_remove_group(struct kobject *kobj,
const struct attribute_group *grp);
+void devm_sysfs_remove_group(struct device *dev,
+ const struct attribute_group *grp);
int sysfs_add_file_to_group(struct kobject *kobj,
const struct attribute *attr, const char *group);
void sysfs_remove_file_from_group(struct kobject *kobj,
@@ -230,6 +238,12 @@ static inline int sysfs_create_file(struct kobject *kobj,
return 0;
}
+static inline int devm_sysfs_create_file(struct device *dev,
+ const struct attribute *attr)
+{
+ return 0;
+}
+
static inline int sysfs_create_files(struct kobject *kobj,
const struct attribute **attr)
{
@@ -247,6 +261,11 @@ static inline void sysfs_remove_file(struct kobject *kobj,
{
}
+static inline void devm_sysfs_remove_file(struct device *dev,
+ const struct attribute *attr)
+{
+}
+
static inline void sysfs_remove_files(struct kobject *kobj,
const struct attribute **attr)
{
@@ -297,6 +316,12 @@ static inline int sysfs_create_group(struct kobject *kobj,
return 0;
}
+static inline int devm_sysfs_create_group(struct device *dev,
+ const struct attribute_group *grp)
+{
+ return 0;
+}
+
static inline int sysfs_update_group(struct kobject *kobj,
const struct attribute_group *grp)
{
@@ -308,6 +333,11 @@ static inline void sysfs_remove_group(struct kobject *kobj,
{
}
+static inline void devm_sysfs_remove_group(struct device *dev,
+ const struct attribute_group *grp)
+{
+}
+
static inline int sysfs_add_file_to_group(struct kobject *kobj,
const struct attribute *attr, const char *group)
{
--
1.7.9.7
On Thu, Mar 14, 2013 at 08:24:45PM -0700, Guenter Roeck wrote:
> Provide devres functions for device_create_file, sysfs_create_file,
> and sysfs_create_group plus the respective remove functions.
>
> Idea is to be able to drop calls to the remove functions from the various
> drivers using those calls.
Hm, despite the fact that almost every driver that makes these calls is
broken? :)
> Potential savings are substantial. There are more than 700 calls to
> device_remove_file in the kernel, more than 500 calls to sysfs_remove_group,
> and some 50 calls to sysfs_remove_file (though not all of those use dev->kobj
> as parameter). Expanding the API to sysfs_create_bin_file would add another 80+
> opportunities, and adding sysfs_create_link would create another 100 or so.
The idea is nice, but why are these drivers adding sysfs files on their
own? Are they doing this in a way that is race-free with userspace
(i.e. creating them before userspace is told about the device), or are
they broken and need to have these calls added to the "default
device/driver/bus" attribute list for them instead?
I think the "we need to fix the drivers" option is the correct one :(
Ideally, I could get rid of those files from being exported at all, but
some busses do do things correctly, so I can't. But they seem to be in
the minority...
So how about we fix up the drivers first, then, if there are valid users
for this type of interface (which I do think there is), we can add it
then?
thanks,
greg k-h
Adding lm-sensors.
On Sat, Mar 16, 2013 at 09:21:40AM -0700, Greg Kroah-Hartman wrote:
> On Thu, Mar 14, 2013 at 08:24:45PM -0700, Guenter Roeck wrote:
> > Provide devres functions for device_create_file, sysfs_create_file,
> > and sysfs_create_group plus the respective remove functions.
> >
> > Idea is to be able to drop calls to the remove functions from the various
> > drivers using those calls.
>
> Hm, despite the fact that almost every driver that makes these calls is
> broken? :)
>
> > Potential savings are substantial. There are more than 700 calls to
> > device_remove_file in the kernel, more than 500 calls to sysfs_remove_group,
> > and some 50 calls to sysfs_remove_file (though not all of those use dev->kobj
> > as parameter). Expanding the API to sysfs_create_bin_file would add another 80+
> > opportunities, and adding sysfs_create_link would create another 100 or so.
>
> The idea is nice, but why are these drivers adding sysfs files on their
> own? Are they doing this in a way that is race-free with userspace
> (i.e. creating them before userspace is told about the device), or are
> they broken and need to have these calls added to the "default
> device/driver/bus" attribute list for them instead?
>
My use case is primarily for hwmon drivers.
hwmon has a separate API call to register a driver with the hwmon subsystem,
which creates a separate hwmon device and provides the user space notification.
As the created attribute files are often conditional on device variant and device
configuration, I don't see how this could be done through a default attribute
list (even though it might be worthwhile exploring if it can be used for some
of the simpler drivers).
The idea was to also provide devm_hwmon_register and devm_hwmon_unregister.
Together that would help us reducing the remove function for most hwmon
drivers to pretty much nothing.
Some other subsystems:
usb: Used widely. From looking into a couple of sources, usage seems to be
questionable, as I don't see how userspace would be notified. I don't know
enough about usb to be sure, though.
mtd: One use case (volume creation) seems to be safe, as it notifies userspace
about its completion. For UBI I am not that sure, as it registers the device
first and then adds the attributes.
regulators: No idea if it does the right thing.
input: Usage in files I looked at seems questionable.
There are others, but it really gets murky and I don't understand
the subsystems well enough to make a call.
> I think the "we need to fix the drivers" option is the correct one :(
>
> Ideally, I could get rid of those files from being exported at all, but
> some busses do do things correctly, so I can't. But they seem to be in
> the minority...
>
> So how about we fix up the drivers first, then, if there are valid users
> for this type of interface (which I do think there is), we can add it
> then?
>
I think hwmon is a valid use case.
For other subsystems, I simply don't know enough about those to do that kind
of work; I think it would make more sense to ask it to be done by people who
are familiar with the respective subsystems or drivers to do it.
Besides, looking through well above 1,000 calls would probably take about
forever if a single person was to do it, even if that person would be available
full-time to do the work.
How about an alternative: Provide the API, then if/when people start using it
wrongly ask them to fix up the drivers instead. That seems to make more sense
to me.
Thanks,
Guenter
On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote:
> Adding lm-sensors.
>
> On Sat, Mar 16, 2013 at 09:21:40AM -0700, Greg Kroah-Hartman wrote:
> > On Thu, Mar 14, 2013 at 08:24:45PM -0700, Guenter Roeck wrote:
> > > Provide devres functions for device_create_file, sysfs_create_file,
> > > and sysfs_create_group plus the respective remove functions.
> > >
> > > Idea is to be able to drop calls to the remove functions from the various
> > > drivers using those calls.
> >
> > Hm, despite the fact that almost every driver that makes these calls is
> > broken? :)
> >
> > > Potential savings are substantial. There are more than 700 calls to
> > > device_remove_file in the kernel, more than 500 calls to sysfs_remove_group,
> > > and some 50 calls to sysfs_remove_file (though not all of those use dev->kobj
> > > as parameter). Expanding the API to sysfs_create_bin_file would add another 80+
> > > opportunities, and adding sysfs_create_link would create another 100 or so.
> >
> > The idea is nice, but why are these drivers adding sysfs files on their
> > own? Are they doing this in a way that is race-free with userspace
> > (i.e. creating them before userspace is told about the device), or are
> > they broken and need to have these calls added to the "default
> > device/driver/bus" attribute list for them instead?
> >
> My use case is primarily for hwmon drivers.
>
> hwmon has a separate API call to register a driver with the hwmon subsystem,
> which creates a separate hwmon device and provides the user space notification.
> As the created attribute files are often conditional on device variant and device
> configuration, I don't see how this could be done through a default attribute
> list (even though it might be worthwhile exploring if it can be used for some
> of the simpler drivers).
The default attribute list functionality offers you the ability to have
callbacks to your driver to validate if you really want this sysfs file
to be created or not. Just use that like other subsystems do, then you
will never have to be making these create and remove calls at all.
> The idea was to also provide devm_hwmon_register and devm_hwmon_unregister.
> Together that would help us reducing the remove function for most hwmon
> drivers to pretty much nothing.
That's a great goal to have, I like it.
> Some other subsystems:
>
> usb: Used widely. From looking into a couple of sources, usage seems to be
> questionable, as I don't see how userspace would be notified. I don't know
> enough about usb to be sure, though.
Which USB drivers do this? The core should be fine, we delay telling
userspace until after the core has create the files it needs. USB
drivers should all be using attribute lists, if not, then they are
probably broken, although it really depends on the subsystem they are
registering themselves with (USB is just the transport layer for lots of
different things, as you know.)
> mtd: One use case (volume creation) seems to be safe, as it notifies userspace
> about its completion. For UBI I am not that sure, as it registers the device
> first and then adds the attributes.
That's not good (the UBI one.) It should be fixed.
> regulators: No idea if it does the right thing.
Ick.
> input: Usage in files I looked at seems questionable.
Really? I thought we fixed those a while ago, but more could have crept
in over time. Which is why I really want to not export those
functions...
> There are others, but it really gets murky and I don't understand
> the subsystems well enough to make a call.
>
> > I think the "we need to fix the drivers" option is the correct one :(
> >
> > Ideally, I could get rid of those files from being exported at all, but
> > some busses do do things correctly, so I can't. But they seem to be in
> > the minority...
> >
> > So how about we fix up the drivers first, then, if there are valid users
> > for this type of interface (which I do think there is), we can add it
> > then?
> >
> I think hwmon is a valid use case.
See above for why I don't think it is.
Bonus is, if you use the attribute callbacks, your code is smaller :)
> For other subsystems, I simply don't know enough about those to do that kind
> of work; I think it would make more sense to ask it to be done by people who
> are familiar with the respective subsystems or drivers to do it.
I agree.
> Besides, looking through well above 1,000 calls would probably take about
> forever if a single person was to do it, even if that person would be available
> full-time to do the work.
>
> How about an alternative: Provide the API, then if/when people start using it
> wrongly ask them to fix up the drivers instead. That seems to make more sense
> to me.
People are using the existing apis "wrongly" and no one noticed,
including me :(
Anyway, see above for how you can change the hwmon subsystem to not need
this at all, so you don't have to add anything new to the core of the
kernel, just fix up the drivers and you will be fine.
thanks,
greg k-h
On Sat, Mar 16, 2013 at 12:50:02PM -0700, Greg Kroah-Hartman wrote:
> On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote:
> > Adding lm-sensors.
> >
> > On Sat, Mar 16, 2013 at 09:21:40AM -0700, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 14, 2013 at 08:24:45PM -0700, Guenter Roeck wrote:
> > > > Provide devres functions for device_create_file, sysfs_create_file,
> > > > and sysfs_create_group plus the respective remove functions.
> > > >
> > > > Idea is to be able to drop calls to the remove functions from the various
> > > > drivers using those calls.
> > >
> > > Hm, despite the fact that almost every driver that makes these calls is
> > > broken? :)
> > >
> > > > Potential savings are substantial. There are more than 700 calls to
> > > > device_remove_file in the kernel, more than 500 calls to sysfs_remove_group,
> > > > and some 50 calls to sysfs_remove_file (though not all of those use dev->kobj
> > > > as parameter). Expanding the API to sysfs_create_bin_file would add another 80+
> > > > opportunities, and adding sysfs_create_link would create another 100 or so.
> > >
> > > The idea is nice, but why are these drivers adding sysfs files on their
> > > own? Are they doing this in a way that is race-free with userspace
> > > (i.e. creating them before userspace is told about the device), or are
> > > they broken and need to have these calls added to the "default
> > > device/driver/bus" attribute list for them instead?
> > >
> > My use case is primarily for hwmon drivers.
> >
> > hwmon has a separate API call to register a driver with the hwmon subsystem,
> > which creates a separate hwmon device and provides the user space notification.
> > As the created attribute files are often conditional on device variant and device
> > configuration, I don't see how this could be done through a default attribute
> > list (even though it might be worthwhile exploring if it can be used for some
> > of the simpler drivers).
>
> The default attribute list functionality offers you the ability to have
> callbacks to your driver to validate if you really want this sysfs file
> to be created or not. Just use that like other subsystems do, then you
> will never have to be making these create and remove calls at all.
>
I thought about it, but right now I have no idea how to make it work.
Initialization sequence in hwmon drivers is
probe():
allocate and initialize local driver data structures
detect configuration and initialize hardware if necessary
create attribute files
register with hwmon subsystem
sometimes do additional work, such as enable interrupts
If I use attribute_group of the device_driver structure to create the attribute
files, my understanding is that those would be created prior to calling
the probe function.
This would be too early, since local data structures do not yet exist, and
the chip configuration is unknown and uninitialized.
On the other side, attribute files must exist before hwmon_device_register()
is called, since otherwise userspace would get confused.
If interrupts are supported, those are typically used to signal attribute
file related events (via udev and/or poll events), meaning the interrupts
must only be enabled after sysfs files were created (becaue the interrupt
acts on it), and should only be enabled after the hwmon device was registered
(because otherwise userspace won't know about the device if the first interrupt
happens after sysfs file creation but before hwmon registration).
I looked into the use of is_visible. The drivers I looked at (ad7877, tsc2005,
lm3533_led, lm3533-core, lm3533_bl) all need data obtained in the probe function
in their is_visible function, meaning the attribute files can not be created
before that data is available. That (and the solution to create the attributes
in the probe function after basic device initialization) is quite similar
to the problem we have in the hwmon subsystem and its current solution.
Overall I have no idea how to make this all fit into the generic attribute
file handling. If you do, please let me know.
> > The idea was to also provide devm_hwmon_register and devm_hwmon_unregister.
> > Together that would help us reducing the remove function for most hwmon
> > drivers to pretty much nothing.
>
> That's a great goal to have, I like it.
>
> > Some other subsystems:
> >
> > usb: Used widely. From looking into a couple of sources, usage seems to be
> > questionable, as I don't see how userspace would be notified. I don't know
> > enough about usb to be sure, though.
>
> Which USB drivers do this? The core should be fine, we delay telling
> userspace until after the core has create the files it needs. USB
> drivers should all be using attribute lists, if not, then they are
> probably broken, although it really depends on the subsystem they are
> registering themselves with (USB is just the transport layer for lots of
> different things, as you know.)
>
Just look for device_create_file in the drivers/usb directory. Maybe I got it
all wrong, and everything is fine. Either some 50 calls to device_create_file
and 10 calls to sysfs_create_groups creeped in, or those (or at least many
of those) are fine. If the calls are fine, introducing devm_ functions to
replace at least many of those calls would make sense again.
> > mtd: One use case (volume creation) seems to be safe, as it notifies userspace
> > about its completion. For UBI I am not that sure, as it registers the device
> > first and then adds the attributes.
>
> That's not good (the UBI one.) It should be fixed.
>
> > regulators: No idea if it does the right thing.
>
> Ick.
>
Not necessarily - just shows that I don't understand the code.
> > input: Usage in files I looked at seems questionable.
>
> Really? I thought we fixed those a while ago, but more could have crept
> in over time. Which is why I really want to not export those
> functions...
>
Maybe it is not a problem because the input device is registered as input device
from its probe function, ie there is a sysfs event after its attribute files
were created. But if the use is ok, so would be supporting the use of devm_
functions in those drivers.
> > There are others, but it really gets murky and I don't understand
> > the subsystems well enough to make a call.
> >
> > > I think the "we need to fix the drivers" option is the correct one :(
> > >
> > > Ideally, I could get rid of those files from being exported at all, but
> > > some busses do do things correctly, so I can't. But they seem to be in
> > > the minority...
> > >
> > > So how about we fix up the drivers first, then, if there are valid users
> > > for this type of interface (which I do think there is), we can add it
> > > then?
> > >
> > I think hwmon is a valid use case.
>
> See above for why I don't think it is.
>
> Bonus is, if you use the attribute callbacks, your code is smaller :)
>
> > For other subsystems, I simply don't know enough about those to do that kind
> > of work; I think it would make more sense to ask it to be done by people who
> > are familiar with the respective subsystems or drivers to do it.
>
> I agree.
>
> > Besides, looking through well above 1,000 calls would probably take about
> > forever if a single person was to do it, even if that person would be available
> > full-time to do the work.
> >
> > How about an alternative: Provide the API, then if/when people start using it
> > wrongly ask them to fix up the drivers instead. That seems to make more sense
> > to me.
>
> People are using the existing apis "wrongly" and no one noticed,
> including me :(
>
> Anyway, see above for how you can change the hwmon subsystem to not need
> this at all, so you don't have to add anything new to the core of the
> kernel, just fix up the drivers and you will be fine.
>
Unfortunately I have no ida how to make that all work. Can you point me
to some examples showing me how other subsystems solved a similar problem ?
I browsed through various drivers using is_visible, but none of the ones I found
really apply; almost all actually call sysfs_create_group in the probe function
and thus could use the devm_ API as well.
Thanks,
Guenter
On Sat, Mar 16, 2013 at 02:25:40PM -0700, Guenter Roeck wrote:
> On Sat, Mar 16, 2013 at 12:50:02PM -0700, Greg Kroah-Hartman wrote:
> > On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote:
> > > Adding lm-sensors.
> > >
> > > On Sat, Mar 16, 2013 at 09:21:40AM -0700, Greg Kroah-Hartman wrote:
> > > > On Thu, Mar 14, 2013 at 08:24:45PM -0700, Guenter Roeck wrote:
> > > > > Provide devres functions for device_create_file, sysfs_create_file,
> > > > > and sysfs_create_group plus the respective remove functions.
> > > > >
> > > > > Idea is to be able to drop calls to the remove functions from the various
> > > > > drivers using those calls.
> > > >
> > > > Hm, despite the fact that almost every driver that makes these calls is
> > > > broken? :)
> > > >
> > > > > Potential savings are substantial. There are more than 700 calls to
> > > > > device_remove_file in the kernel, more than 500 calls to sysfs_remove_group,
> > > > > and some 50 calls to sysfs_remove_file (though not all of those use dev->kobj
> > > > > as parameter). Expanding the API to sysfs_create_bin_file would add another 80+
> > > > > opportunities, and adding sysfs_create_link would create another 100 or so.
> > > >
> > > > The idea is nice, but why are these drivers adding sysfs files on their
> > > > own? Are they doing this in a way that is race-free with userspace
> > > > (i.e. creating them before userspace is told about the device), or are
> > > > they broken and need to have these calls added to the "default
> > > > device/driver/bus" attribute list for them instead?
> > > >
> > > My use case is primarily for hwmon drivers.
> > >
> > > hwmon has a separate API call to register a driver with the hwmon subsystem,
> > > which creates a separate hwmon device and provides the user space notification.
> > > As the created attribute files are often conditional on device variant and device
> > > configuration, I don't see how this could be done through a default attribute
> > > list (even though it might be worthwhile exploring if it can be used for some
> > > of the simpler drivers).
> >
> > The default attribute list functionality offers you the ability to have
> > callbacks to your driver to validate if you really want this sysfs file
> > to be created or not. Just use that like other subsystems do, then you
> > will never have to be making these create and remove calls at all.
> >
>
> I thought about it, but right now I have no idea how to make it work.
> Initialization sequence in hwmon drivers is
> probe():
> allocate and initialize local driver data structures
> detect configuration and initialize hardware if necessary
> create attribute files
> register with hwmon subsystem
> sometimes do additional work, such as enable interrupts
>
> If I use attribute_group of the device_driver structure to create the attribute
> files, my understanding is that those would be created prior to calling
> the probe function.
>
> This would be too early, since local data structures do not yet exist, and
> the chip configuration is unknown and uninitialized.
>
> On the other side, attribute files must exist before hwmon_device_register()
> is called, since otherwise userspace would get confused.
>
> If interrupts are supported, those are typically used to signal attribute
> file related events (via udev and/or poll events), meaning the interrupts
> must only be enabled after sysfs files were created (becaue the interrupt
> acts on it), and should only be enabled after the hwmon device was registered
> (because otherwise userspace won't know about the device if the first interrupt
> happens after sysfs file creation but before hwmon registration).
>
> I looked into the use of is_visible. The drivers I looked at (ad7877, tsc2005,
> lm3533_led, lm3533-core, lm3533_bl) all need data obtained in the probe function
> in their is_visible function, meaning the attribute files can not be created
> before that data is available. That (and the solution to create the attributes
> in the probe function after basic device initialization) is quite similar
> to the problem we have in the hwmon subsystem and its current solution.
>
> Overall I have no idea how to make this all fit into the generic attribute
> file handling. If you do, please let me know.
>
I spent some time implementing two variants of devm_hwmon_device_register().
One is based on the patchset provided here, and uses the following API.
struct device *devm_hwmon_device_register(struct device *dev);
The other is
struct device *devm_hwmon_device_register(struct device *dev, struct
attribute_group **groups);
The second API includes a pointer to the sysfs attribute groups, and creates the
sysfs attributes within the API call. With this approach, conditional attributes
are managed with is_visible.
I then converted a couple of drivers to the new APIs.
For the first approach, conversion was quite simple:
device_create_file -> devm_device_create_file
sysfs_create_group -> devm_sysfs_create_group
hwmon_device_register -> devm_hwmon_device_register
and drop all remove calls.
Result for the lm90 driver as a medium complexity example:
drivers/hwmon/lm90.c | 46 +++++++++++++---------------------------------
1 file changed, 13 insertions(+), 33 deletions(-)
The object size is reduced by approximately 500 bytes (of 20k).
Result for max16065 driver:
drivers/hwmon/max16065.c | 52 +++++++++++-----------------------------------
1 file changed, 12 insertions(+), 40 deletions(-)
Code size is reduced by approximately 450 bytes (of 12k).
For the second approach, I had to convert sysfs groups to use is_visible,
create new groups where those did not exist, implement is_visible functions,
drop calls to device_create_file, sysfs_create_group, and drop the remove
functions.
Result for the lm90 driver:
drivers/hwmon/lm90.c | 137 ++++++++++++++++++++++++++++++--------------------
1 file changed, 83 insertions(+), 54 deletions(-)
The object size increases by about 200 bytes.
Results for max16065 driver:
drivers/hwmon/max16065.c | 142 ++++++++++++++++++++++++++--------------------
1 file changed, 80 insertions(+), 62 deletions(-)
Code size increases by about 160 bytes.
This is after I pulled all tricks I could find to reduce the code size increase.
In both cases, the code ended up being less readable and more confusing
than before, as the validation if an attribute has to be created or not
is now spread across multiple is_visible functions and no longer
in a single block of code, and it is more difficult to understand if an
attribute will be created or not.
For simple drivers which don't need is_visible, the resulting code is
similar with both approaches. Unfortunately, most hwmon drivers are not
that simple. As soon as conditional attributes are needed, the code ends up
being more complex.
In conclusion, I see the second approach (ie using is_visible) as a no-go.
The idea is to reduce complexity, not to increase it. So, ultimately, we can
discuss how the API functions are named and its parameters, but for the hwmon
subsystem conversion to devres to make sense we would need the device/sysfs
API functions along the line I proposed. Otherwise we are better off with
just keeping the current hwmon API and not bother converting it to devres.
Thanks,
Guenter
On Sat, 16 Mar 2013 14:25:40 -0700, Guenter Roeck wrote:
> On Sat, Mar 16, 2013 at 12:50:02PM -0700, Greg Kroah-Hartman wrote:
> > On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote:
> > > My use case is primarily for hwmon drivers.
> > >
> > > hwmon has a separate API call to register a driver with the hwmon subsystem,
> > > which creates a separate hwmon device and provides the user space notification.
> > > As the created attribute files are often conditional on device variant and device
> > > configuration, I don't see how this could be done through a default attribute
> > > list (even though it might be worthwhile exploring if it can be used for some
> > > of the simpler drivers).
> >
> > The default attribute list functionality offers you the ability to have
> > callbacks to your driver to validate if you really want this sysfs file
> > to be created or not. Just use that like other subsystems do, then you
> > will never have to be making these create and remove calls at all.
>
> I thought about it, but right now I have no idea how to make it work.
> Initialization sequence in hwmon drivers is
> probe():
> allocate and initialize local driver data structures
> detect configuration and initialize hardware if necessary
> create attribute files
> register with hwmon subsystem
> sometimes do additional work, such as enable interrupts
>
> If I use attribute_group of the device_driver structure to create the attribute
> files, my understanding is that those would be created prior to calling
> the probe function.
>
> This would be too early, since local data structures do not yet exist, and
> the chip configuration is unknown and uninitialized.
>
> On the other side, attribute files must exist before hwmon_device_register()
> is called, since otherwise userspace would get confused.
I'd like to add something at this point.
We have historically created the hwmon attributes in the hardware (i2c,
platform...) device, and then created an empty hwmon class device on
top of it so that libsensors etc. can locate all hardware monitoring
chips on the system. This is probably wrong and this may explain the
difference of views between Greg and Guenter.
I suspect that ideally all hwmon-related attributes should belong to the
hwmon-class device and not the physical device. Would doing so solve
the problem of is_visible() needing chip-specific information that can
only be gathered during probe()? Sure this is an interface change, but
a few hwmon drivers already do it that way (the ones without an actual
hardware device, e.g. ACPI thermal zones) and libsensors supports this
since version 3.0.3, which was released in September 2008 - 4.5 years
ago.
This would require creating the attributes after calling
hwmon_device_register() rather than before, but from the ongoing
discussion I seem to understand that the driver core supports creating
the attributes for us, possibly at the same time as the class device
will be created. Would this solve the userspace timing issue?
Also note that libsensors is really old fashioned when it comes to
device discovery. It doesn't support hot-plug nor hot-remove. So some
work would be needed in this area anyway if we want libsensors-based
applications to properly cope with device addition and removal.
Just my 2 cents.
--
Jean Delvare
On Sun, Mar 17, 2013 at 01:39:20PM +0100, Jean Delvare wrote:
> On Sat, 16 Mar 2013 14:25:40 -0700, Guenter Roeck wrote:
> > On Sat, Mar 16, 2013 at 12:50:02PM -0700, Greg Kroah-Hartman wrote:
> > > On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote:
> > > > My use case is primarily for hwmon drivers.
> > > >
> > > > hwmon has a separate API call to register a driver with the hwmon subsystem,
> > > > which creates a separate hwmon device and provides the user space notification.
> > > > As the created attribute files are often conditional on device variant and device
> > > > configuration, I don't see how this could be done through a default attribute
> > > > list (even though it might be worthwhile exploring if it can be used for some
> > > > of the simpler drivers).
> > >
> > > The default attribute list functionality offers you the ability to have
> > > callbacks to your driver to validate if you really want this sysfs file
> > > to be created or not. Just use that like other subsystems do, then you
> > > will never have to be making these create and remove calls at all.
> >
> > I thought about it, but right now I have no idea how to make it work.
> > Initialization sequence in hwmon drivers is
> > probe():
> > allocate and initialize local driver data structures
> > detect configuration and initialize hardware if necessary
> > create attribute files
> > register with hwmon subsystem
> > sometimes do additional work, such as enable interrupts
> >
> > If I use attribute_group of the device_driver structure to create the attribute
> > files, my understanding is that those would be created prior to calling
> > the probe function.
> >
> > This would be too early, since local data structures do not yet exist, and
> > the chip configuration is unknown and uninitialized.
> >
> > On the other side, attribute files must exist before hwmon_device_register()
> > is called, since otherwise userspace would get confused.
>
> I'd like to add something at this point.
>
> We have historically created the hwmon attributes in the hardware (i2c,
> platform...) device, and then created an empty hwmon class device on
> top of it so that libsensors etc. can locate all hardware monitoring
> chips on the system. This is probably wrong and this may explain the
> difference of views between Greg and Guenter.
>
> I suspect that ideally all hwmon-related attributes should belong to the
> hwmon-class device and not the physical device. Would doing so solve
> the problem of is_visible() needing chip-specific information that can
> only be gathered during probe()? Sure this is an interface change, but
> a few hwmon drivers already do it that way (the ones without an actual
> hardware device, e.g. ACPI thermal zones) and libsensors supports this
> since version 3.0.3, which was released in September 2008 - 4.5 years
> ago.
>
> This would require creating the attributes after calling
> hwmon_device_register() rather than before, but from the ongoing
> discussion I seem to understand that the driver core supports creating
> the attributes for us, possibly at the same time as the class device
> will be created. Would this solve the userspace timing issue?
>
This is what I had in mind as ultimate possibility when I created
the second API mentioned in my other e-mail.
struct device *devm_hwmon_device_register(struct device *dev,
const struct attribute_group **groups)
The attributes are still attached to dev (ie to the hardware device)
in my current code, but it should be possible to attach them to the
hwmon class device instead.
Problem with that approach is that it makes drivers larger, not smaller,
at least if is_visible is needed. So it kind of defeats the purpose.
We can go along that route anyway if people think it is the right or a better
approach, but I am not sure if it is worth it. I can send out the patches if
there is interest.
> Also note that libsensors is really old fashioned when it comes to
> device discovery. It doesn't support hot-plug nor hot-remove. So some
> work would be needed in this area anyway if we want libsensors-based
> applications to properly cope with device addition and removal.
>
Agreed.
Thanks,
Guenter
On Sun, Mar 17, 2013 at 06:19:33AM -0700, Guenter Roeck wrote:
> On Sun, Mar 17, 2013 at 01:39:20PM +0100, Jean Delvare wrote:
> > On Sat, 16 Mar 2013 14:25:40 -0700, Guenter Roeck wrote:
> > > On Sat, Mar 16, 2013 at 12:50:02PM -0700, Greg Kroah-Hartman wrote:
> > > > On Sat, Mar 16, 2013 at 11:12:53AM -0700, Guenter Roeck wrote:
> > > > > My use case is primarily for hwmon drivers.
> > > > >
> > > > > hwmon has a separate API call to register a driver with the hwmon subsystem,
> > > > > which creates a separate hwmon device and provides the user space notification.
> > > > > As the created attribute files are often conditional on device variant and device
> > > > > configuration, I don't see how this could be done through a default attribute
> > > > > list (even though it might be worthwhile exploring if it can be used for some
> > > > > of the simpler drivers).
> > > >
> > > > The default attribute list functionality offers you the ability to have
> > > > callbacks to your driver to validate if you really want this sysfs file
> > > > to be created or not. Just use that like other subsystems do, then you
> > > > will never have to be making these create and remove calls at all.
> > >
> > > I thought about it, but right now I have no idea how to make it work.
> > > Initialization sequence in hwmon drivers is
> > > probe():
> > > allocate and initialize local driver data structures
> > > detect configuration and initialize hardware if necessary
> > > create attribute files
> > > register with hwmon subsystem
> > > sometimes do additional work, such as enable interrupts
> > >
> > > If I use attribute_group of the device_driver structure to create the attribute
> > > files, my understanding is that those would be created prior to calling
> > > the probe function.
> > >
> > > This would be too early, since local data structures do not yet exist, and
> > > the chip configuration is unknown and uninitialized.
> > >
> > > On the other side, attribute files must exist before hwmon_device_register()
> > > is called, since otherwise userspace would get confused.
> >
> > I'd like to add something at this point.
> >
> > We have historically created the hwmon attributes in the hardware (i2c,
> > platform...) device, and then created an empty hwmon class device on
> > top of it so that libsensors etc. can locate all hardware monitoring
> > chips on the system. This is probably wrong and this may explain the
> > difference of views between Greg and Guenter.
> >
> > I suspect that ideally all hwmon-related attributes should belong to the
> > hwmon-class device and not the physical device. Would doing so solve
> > the problem of is_visible() needing chip-specific information that can
> > only be gathered during probe()? Sure this is an interface change, but
> > a few hwmon drivers already do it that way (the ones without an actual
> > hardware device, e.g. ACPI thermal zones) and libsensors supports this
> > since version 3.0.3, which was released in September 2008 - 4.5 years
> > ago.
> >
> > This would require creating the attributes after calling
> > hwmon_device_register() rather than before, but from the ongoing
> > discussion I seem to understand that the driver core supports creating
> > the attributes for us, possibly at the same time as the class device
> > will be created. Would this solve the userspace timing issue?
> >
> This is what I had in mind as ultimate possibility when I created
> the second API mentioned in my other e-mail.
>
> struct device *devm_hwmon_device_register(struct device *dev,
> const struct attribute_group **groups)
>
> The attributes are still attached to dev (ie to the hardware device)
> in my current code, but it should be possible to attach them to the
> hwmon class device instead.
>
> Problem with that approach is that it makes drivers larger, not smaller,
> at least if is_visible is needed. So it kind of defeats the purpose.
>
> We can go along that route anyway if people think it is the right or a better
> approach, but I am not sure if it is worth it. I can send out the patches if
> there is interest.
I found an alternate way of converting drivers with optional attributes - don't
use is_visible, but collect the attribute groups dynamically. With this,
conversion results look much better. For lm90:
drivers/hwmon/lm90.c | 91 +++++++++++++++++++++-----------------------------
1 file changed, 38 insertions(+), 53 deletions(-)
Code size is reduced by ~550 bytes. With that, this approach is much more
feasible.
I'll figure out how to attach the attributes to the hwmon device and send out
a new set of patches after I get it working.
Thanks,
Guenter
On Sun, 17 Mar 2013 06:19:33 -0700, Guenter Roeck wrote:
> On Sun, Mar 17, 2013 at 01:39:20PM +0100, Jean Delvare wrote:
> > I'd like to add something at this point.
> >
> > We have historically created the hwmon attributes in the hardware (i2c,
> > platform...) device, and then created an empty hwmon class device on
> > top of it so that libsensors etc. can locate all hardware monitoring
> > chips on the system. This is probably wrong and this may explain the
> > difference of views between Greg and Guenter.
> >
> > I suspect that ideally all hwmon-related attributes should belong to the
> > hwmon-class device and not the physical device. Would doing so solve
> > the problem of is_visible() needing chip-specific information that can
> > only be gathered during probe()? Sure this is an interface change, but
> > a few hwmon drivers already do it that way (the ones without an actual
> > hardware device, e.g. ACPI thermal zones) and libsensors supports this
> > since version 3.0.3, which was released in September 2008 - 4.5 years
> > ago.
> >
> > This would require creating the attributes after calling
> > hwmon_device_register() rather than before, but from the ongoing
> > discussion I seem to understand that the driver core supports creating
> > the attributes for us, possibly at the same time as the class device
> > will be created. Would this solve the userspace timing issue?
> >
> This is what I had in mind as ultimate possibility when I created
> the second API mentioned in my other e-mail.
>
> struct device *devm_hwmon_device_register(struct device *dev,
> const struct attribute_group **groups)
>
> The attributes are still attached to dev (ie to the hardware device)
> in my current code, but it should be possible to attach them to the
> hwmon class device instead.
>
> Problem with that approach is that it makes drivers larger, not smaller,
> at least if is_visible is needed. So it kind of defeats the purpose.
>
> We can go along that route anyway if people think it is the right or a better
> approach, but I am not sure if it is worth it. I can send out the patches if
> there is interest.
Really, I don't know. All I know is that I do not have any time to
devote to this ATM.
--
Jean Delvare
On Mon, Mar 18, 2013 at 09:02:41AM +0100, Jean Delvare wrote:
> On Sun, 17 Mar 2013 06:19:33 -0700, Guenter Roeck wrote:
> > On Sun, Mar 17, 2013 at 01:39:20PM +0100, Jean Delvare wrote:
> > > I'd like to add something at this point.
> > >
> > > We have historically created the hwmon attributes in the hardware (i2c,
> > > platform...) device, and then created an empty hwmon class device on
> > > top of it so that libsensors etc. can locate all hardware monitoring
> > > chips on the system. This is probably wrong and this may explain the
> > > difference of views between Greg and Guenter.
> > >
> > > I suspect that ideally all hwmon-related attributes should belong to the
> > > hwmon-class device and not the physical device. Would doing so solve
> > > the problem of is_visible() needing chip-specific information that can
> > > only be gathered during probe()? Sure this is an interface change, but
> > > a few hwmon drivers already do it that way (the ones without an actual
> > > hardware device, e.g. ACPI thermal zones) and libsensors supports this
> > > since version 3.0.3, which was released in September 2008 - 4.5 years
> > > ago.
> > >
> > > This would require creating the attributes after calling
> > > hwmon_device_register() rather than before, but from the ongoing
> > > discussion I seem to understand that the driver core supports creating
> > > the attributes for us, possibly at the same time as the class device
> > > will be created. Would this solve the userspace timing issue?
> > >
> > This is what I had in mind as ultimate possibility when I created
> > the second API mentioned in my other e-mail.
> >
> > struct device *devm_hwmon_device_register(struct device *dev,
> > const struct attribute_group **groups)
> >
> > The attributes are still attached to dev (ie to the hardware device)
> > in my current code, but it should be possible to attach them to the
> > hwmon class device instead.
> >
> > Problem with that approach is that it makes drivers larger, not smaller,
> > at least if is_visible is needed. So it kind of defeats the purpose.
> >
> > We can go along that route anyway if people think it is the right or a better
> > approach, but I am not sure if it is worth it. I can send out the patches if
> > there is interest.
>
> Really, I don't know. All I know is that I do not have any time to
> devote to this ATM.
>
Hi Jean,
Can't help it. Worst case I learned how make better use of is_visible
and how to avoid its pitfalls.
Guenter