Hi,
This patch against 2.6.12-rc4 adds a void * field to the base sysfs
attribute, and updates all the derived attributes (device_attribute,
etc) to pass the void * to their sysfs callbacks. This facilitates a
number of things:
- reduce the size of much of the sysfs attribute code significantly by
allowing the replacement of the large number of macro generated
virtually identical sysfs callbacks with a single sysfs callback that
uses the void * parameter to decide what to do. For example, adapting
adm1026 to take advantage of this patch:
-------------------2.6.11.7--------------------
Module Size Used by
adm1026 44692 0
--------2.6.12-rc3-devdyncallback-----
Module Size Used by
adm1026 33172 0
(see http://archives.andrew.net.au/lm-sensors/msg31310.html)
- allow the creation of a non-predetermined and potentially unlimited
number of sysfs attributes, this is required by bmcsensors - a driver
I am porting from lm_sensors 2.4 to 2.6.
(see http://archives.andrew.net.au/lm-sensors/msg31225.html ,
http://bmcsensors-26.sourceforge.net/)
This first patch changes the core sysfs attribute and derived attributes.
Signed-off-by: Yani Ioannou <[email protected]>
On Sat, May 07, 2005 at 09:21:34AM -0400, Yani Ioannou wrote:
> Hi,
>
> This patch against 2.6.12-rc4 adds a void * field to the base sysfs
> attribute, and updates all the derived attributes (device_attribute,
> etc) to pass the void * to their sysfs callbacks. This facilitates a
> number of things:
Woah. Why change _every_ type of attribute callback? Why not only
change the ones that actually need this kind of change? That would
reduce your patch immensly, and allow us to actually see what is going
on.
And, in looking at the attribute structure, I see we already have a
private pointer in the bin_attribute structure, so we might as well
collapse them together.
So, here's a first patch to base your next changes off of. It merely
adds a private pointer to the struct attribute. It breaks no code, and
requires no other changes to get it to work properly. Can you now work
off of it and start changing _only_ the attribute types that need this
change for now? That should reduce your changes a lot. I'll add this
patch to my quilt tree so it will show up in the next -mm releases.
thanks,
greg k-h
Subject: sysfs: make attribute contain a private pointer.
Lets us get rid of the bin_attribute's pointer at the same time.
This is being done so that sysfs code can determine what attribute is being
accessed to allow smaller amounts of kernel code to be written.
Based on a patch from Yani Ioannou <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/pci/pci-sysfs.c | 4 ++--
include/linux/sysfs.h | 30 ++++++++++++++++++++----------
2 files changed, 22 insertions(+), 12 deletions(-)
--- gregkh-2.6.orig/include/linux/sysfs.h 2005-05-10 14:15:04.000000000 -0700
+++ gregkh-2.6/include/linux/sysfs.h 2005-05-10 15:10:31.000000000 -0700
@@ -18,6 +18,7 @@
struct attribute {
const char * name;
struct module * owner;
+ void * private;
mode_t mode;
};
@@ -33,15 +34,25 @@
* for examples..
*/
-#define __ATTR(_name,_mode,_show,_store) { \
- .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE }, \
- .show = _show, \
- .store = _store, \
-}
-
-#define __ATTR_RO(_name) { \
- .attr = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE }, \
- .show = _name##_show, \
+#define __ATTR(_name,_mode,_show,_store) { \
+ .attr = { \
+ .name = __stringify(_name), \
+ .mode = _mode, \
+ .private = NULL, \
+ .owner = THIS_MODULE, \
+ }, \
+ .show = _show, \
+ .store = _store, \
+}
+
+#define __ATTR_RO(_name) { \
+ .attr = { \
+ .name = __stringify(_name), \
+ .mode = 0444, \
+ .private = NULL, \
+ .owner = THIS_MODULE, \
+ }, \
+ .show = _name##_show, \
}
#define __ATTR_NULL { .attr = { .name = NULL } }
@@ -53,7 +64,6 @@
struct bin_attribute {
struct attribute attr;
size_t size;
- void *private;
ssize_t (*read)(struct kobject *, char *, loff_t, size_t);
ssize_t (*write)(struct kobject *, char *, loff_t, size_t);
int (*mmap)(struct kobject *, struct bin_attribute *attr,
--- gregkh-2.6.orig/drivers/pci/pci-sysfs.c 2005-05-10 14:15:04.000000000 -0700
+++ gregkh-2.6/drivers/pci/pci-sysfs.c 2005-05-10 15:06:29.000000000 -0700
@@ -299,7 +299,7 @@
{
struct pci_dev *pdev = to_pci_dev(container_of(kobj,
struct device, kobj));
- struct resource *res = (struct resource *)attr->private;
+ struct resource *res = (struct resource *)attr->attr.private;
enum pci_mmap_state mmap_type;
vma->vm_pgoff += res->start >> PAGE_SHIFT;
@@ -337,9 +337,9 @@
res_attr->attr.name = res_attr_name;
res_attr->attr.mode = S_IRUSR | S_IWUSR;
res_attr->attr.owner = THIS_MODULE;
+ res_attr->attr.private = &pdev->resource[i];
res_attr->size = pci_resource_len(pdev, i);
res_attr->mmap = pci_mmap_resource;
- res_attr->private = &pdev->resource[i];
sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
}
}
That just looks like a subset of the core patch except for the
member's name and the bin_attribute change I didn't notice (which
looks very useful), so yes that is good (although I don't see any
reason to change the name).
Truth is after looking at how all the sysfs derived attributes are
used its hard not to believe that all the attributes should be changed
for exactly the same reasons as device_attribute. For example my patch
against net-sysfs.c uses class_attributes, and that cleans up the code
nicely and saves space. What attribute type wouldn't benefit from this
change? I know its a lot of work to change the driver's to benefit
from it but it breaks nothing in the mean-time and I'm committed to
doing a fair bit of that work if need be.
If you think the patch should be split up more and incrementally
implemented I can understand that, but we obviously need to change the
derived attributes eventually to make use of it, why not now? It
doesn't really break anything, my large update is just to remove the
harmless warnings.
Thanks,
Yani
On 5/10/05, Greg KH <[email protected]> wrote:
> On Sat, May 07, 2005 at 09:21:34AM -0400, Yani Ioannou wrote:
> > Hi,
> >
> > This patch against 2.6.12-rc4 adds a void * field to the base sysfs
> > attribute, and updates all the derived attributes (device_attribute,
> > etc) to pass the void * to their sysfs callbacks. This facilitates a
> > number of things:
>
> Woah. Why change _every_ type of attribute callback? Why not only
> change the ones that actually need this kind of change? That would
> reduce your patch immensly, and allow us to actually see what is going
> on.
>
> And, in looking at the attribute structure, I see we already have a
> private pointer in the bin_attribute structure, so we might as well
> collapse them together.
>
> So, here's a first patch to base your next changes off of. It merely
> adds a private pointer to the struct attribute. It breaks no code, and
> requires no other changes to get it to work properly. Can you now work
> off of it and start changing _only_ the attribute types that need this
> change for now? That should reduce your changes a lot. I'll add this
> patch to my quilt tree so it will show up in the next -mm releases.
>
> thanks,
>
> greg k-h
>
> Subject: sysfs: make attribute contain a private pointer.
>
> Lets us get rid of the bin_attribute's pointer at the same time.
> This is being done so that sysfs code can determine what attribute is being
> accessed to allow smaller amounts of kernel code to be written.
>
> Based on a patch from Yani Ioannou <[email protected]>
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> ---
> drivers/pci/pci-sysfs.c | 4 ++--
> include/linux/sysfs.h | 30 ++++++++++++++++++++----------
> 2 files changed, 22 insertions(+), 12 deletions(-)
>
> --- gregkh-2.6.orig/include/linux/sysfs.h 2005-05-10 14:15:04.000000000 -0700
> +++ gregkh-2.6/include/linux/sysfs.h 2005-05-10 15:10:31.000000000 -0700
> @@ -18,6 +18,7 @@
> struct attribute {
> const char * name;
> struct module * owner;
> + void * private;
> mode_t mode;
> };
>
> @@ -33,15 +34,25 @@
> * for examples..
> */
>
> -#define __ATTR(_name,_mode,_show,_store) { \
> - .attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE }, \
> - .show = _show, \
> - .store = _store, \
> -}
> -
> -#define __ATTR_RO(_name) { \
> - .attr = { .name = __stringify(_name), .mode = 0444, .owner = THIS_MODULE }, \
> - .show = _name##_show, \
> +#define __ATTR(_name,_mode,_show,_store) { \
> + .attr = { \
> + .name = __stringify(_name), \
> + .mode = _mode, \
> + .private = NULL, \
> + .owner = THIS_MODULE, \
> + }, \
> + .show = _show, \
> + .store = _store, \
> +}
> +
> +#define __ATTR_RO(_name) { \
> + .attr = { \
> + .name = __stringify(_name), \
> + .mode = 0444, \
> + .private = NULL, \
> + .owner = THIS_MODULE, \
> + }, \
> + .show = _name##_show, \
> }
>
> #define __ATTR_NULL { .attr = { .name = NULL } }
> @@ -53,7 +64,6 @@
> struct bin_attribute {
> struct attribute attr;
> size_t size;
> - void *private;
> ssize_t (*read)(struct kobject *, char *, loff_t, size_t);
> ssize_t (*write)(struct kobject *, char *, loff_t, size_t);
> int (*mmap)(struct kobject *, struct bin_attribute *attr,
> --- gregkh-2.6.orig/drivers/pci/pci-sysfs.c 2005-05-10 14:15:04.000000000 -0700
> +++ gregkh-2.6/drivers/pci/pci-sysfs.c 2005-05-10 15:06:29.000000000 -0700
> @@ -299,7 +299,7 @@
> {
> struct pci_dev *pdev = to_pci_dev(container_of(kobj,
> struct device, kobj));
> - struct resource *res = (struct resource *)attr->private;
> + struct resource *res = (struct resource *)attr->attr.private;
> enum pci_mmap_state mmap_type;
>
> vma->vm_pgoff += res->start >> PAGE_SHIFT;
> @@ -337,9 +337,9 @@
> res_attr->attr.name = res_attr_name;
> res_attr->attr.mode = S_IRUSR | S_IWUSR;
> res_attr->attr.owner = THIS_MODULE;
> + res_attr->attr.private = &pdev->resource[i];
> res_attr->size = pci_resource_len(pdev, i);
> res_attr->mmap = pci_mmap_resource;
> - res_attr->private = &pdev->resource[i];
> sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
> }
> }
>
On further reflection, how about I work off this patch for now and for
each of the derived attributes submit a separate patch implementing
the callbacks, etc for that attribute along with an example patch
showing how it can be used to benefit some existing code (these
already exist for device and class attributes, so I'll resubmit those
examples).
This way we can be sure that we aren't changing any of the derived
attributes needlessly, and it presents a better view of exactly what
changes I'm making to others I suppose :-).
We should probably document this change somehow in the sysfs
documentation at some point too.
Thanks,
Yani
On Tue, May 10, 2005 at 08:59:32PM -0400, Yani Ioannou wrote:
> On further reflection, how about I work off this patch for now and for
> each of the derived attributes submit a separate patch implementing
> the callbacks, etc for that attribute along with an example patch
> showing how it can be used to benefit some existing code (these
> already exist for device and class attributes, so I'll resubmit those
> examples).
>
> This way we can be sure that we aren't changing any of the derived
> attributes needlessly, and it presents a better view of exactly what
> changes I'm making to others I suppose :-).
That sounds great, and is what I was trying to get at.
> We should probably document this change somehow in the sysfs
> documentation at some point too.
Yeah, documentation, what a concept... :)
thanks,
greg k-h
On Tue, May 10, 2005 at 03:16:15PM -0700, Greg KH wrote:
> +#define __ATTR(_name,_mode,_show,_store) { \
> + .attr = { \
> + .name = __stringify(_name), \
> + .mode = _mode, \
> + .private = NULL, \
We don't specifically initialise elements to NULL or zero. Is this a
change of policy?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Wed, May 11, 2005 at 07:51:00AM +0100, Russell King wrote:
> On Tue, May 10, 2005 at 03:16:15PM -0700, Greg KH wrote:
> > +#define __ATTR(_name,_mode,_show,_store) { \
> > + .attr = { \
> > + .name = __stringify(_name), \
> > + .mode = _mode, \
> > + .private = NULL, \
>
> We don't specifically initialise elements to NULL or zero. Is this a
> change of policy?
Doh, no, it isn't, you are correct. I've updated the patch, removing
the changes to the __ATTR() and other macro that I modified.
Thanks for catching this.
greg k-h