Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
On Fri, Nov 22, 2013 at 02:47:38PM -0800, Dmitry Torokhov wrote: > 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? > > Just stumbled upon this thread... > > There is a need for drivers to add driver-specific attributes to a > device. Since they are driver specific they can not go into bus or class > or whatever default attributes that are created when device is > instantiated, but rather attached to the device when a driver binds to > them. An example would be a PS/2 mouse driver allowing user to control > report rate and resolution of the device. Since it only applicable to > PS/2 mice the knob does not belong to the generic serio layer/bus nor > should it go into input layer as it is again PS/2 specific. So psmouse > creates it while binding to a serio port. Yeah, platform drivers also "need" this type of thing as well, so you are right, I can't forbit it for everyone. > Do we send a uevent when driver binds/unbinds from a device? No, we don't, see kobject_actions[] for what we implement. > If not I think we should so that userspace can check for additional > attributes, if any. We might be able to do a "change" event for the device itself after it has been bound / unbound, but I don't know what userspace will do with that information at this point in time (i.e. 10+ years without that information...) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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? Just stumbled upon this thread... There is a need for drivers to add driver-specific attributes to a device. Since they are driver specific they can not go into bus or class or whatever default attributes that are created when device is instantiated, but rather attached to the device when a driver binds to them. An example would be a PS/2 mouse driver allowing user to control report rate and resolution of the device. Since it only applicable to PS/2 mice the knob does not belong to the generic serio layer/bus nor should it go into input layer as it is again PS/2 specific. So psmouse creates it while binding to a serio port. Do we send a uevent when driver binds/unbinds from a device? If not I think we should so that userspace can check for additional attributes, if any. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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? Just stumbled upon this thread... There is a need for drivers to add driver-specific attributes to a device. Since they are driver specific they can not go into bus or class or whatever default attributes that are created when device is instantiated, but rather attached to the device when a driver binds to them. An example would be a PS/2 mouse driver allowing user to control report rate and resolution of the device. Since it only applicable to PS/2 mice the knob does not belong to the generic serio layer/bus nor should it go into input layer as it is again PS/2 specific. So psmouse creates it while binding to a serio port. Do we send a uevent when driver binds/unbinds from a device? If not I think we should so that userspace can check for additional attributes, if any. Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
On Fri, Nov 22, 2013 at 02:47:38PM -0800, Dmitry Torokhov wrote: 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? Just stumbled upon this thread... There is a need for drivers to add driver-specific attributes to a device. Since they are driver specific they can not go into bus or class or whatever default attributes that are created when device is instantiated, but rather attached to the device when a driver binds to them. An example would be a PS/2 mouse driver allowing user to control report rate and resolution of the device. Since it only applicable to PS/2 mice the knob does not belong to the generic serio layer/bus nor should it go into input layer as it is again PS/2 specific. So psmouse creates it while binding to a serio port. Yeah, platform drivers also need this type of thing as well, so you are right, I can't forbit it for everyone. Do we send a uevent when driver binds/unbinds from a device? No, we don't, see kobject_actions[] for what we implement. If not I think we should so that userspace can check for additional attributes, if any. We might be able to do a change event for the device itself after it has been bound / unbound, but I don't know what userspace will do with that information at this point in time (i.e. 10+ years without that information...) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [lm-sensors] [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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 -- To unsubscribe from this list: send the line
Re: [RFC PATCH 0/2] fs: sysfs: Add devres support
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
[RFC PATCH 0/2] fs: sysfs: Add devres support
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 >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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 0/2] fs: sysfs: Add devres support
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
