rationale for sysfs attr store/show accepting kobj_attribute?

rationale for sysfs attr store/show accepting kobj_attribute?

[Robert P. J. Day]

  more just a curiosity than anything else, but i'm perusing the
kobject sample programs in the kernel source directory and in
kobject-example.c, there's one thing about which i'm a bit puzzled.

  the kernel header file sysfs.h defines the basic attribute structure
as:

struct attribute {
        const char              *name;
        mode_t                  mode;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
        struct lock_class_key   *key;
        struct lock_class_key   skey;
#endif
};

in other words, simply the attribute name and mode.  no problem.

  however, the header file kobject.h defines a more informative
kobj_attribute structure thusly:

struct kobj_attribute {
        struct attribute attr;
        ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,
                        char *buf);
        ssize_t (*store)(struct kobject *kobj, struct kobj_attribute *attr,
                         const char *buf, size_t count);
};

as you can see, it's just a regular sysfs attribute, bundled with the
respective show and store routines.

  what puzzles me is why those show() and store() routines are defined
as being passed the larger kobj_attribute structure, rather than the
simpler sysfs attribute structure.  it doesn't seem as if there's much
value in having the show() and store() callbacks getting those two
extra pieces of information.

  in fact, in the sample program kobject-example.c, in the more
general example, the callback routine needs to check the name of the
attribute to know how to process it properly so it has to use
"attr->attr.name" to access the field of the simpler "struct
attribute" anyway.

  long question short -- even though it's clearly an arbitrary
decision, was there some rationale for having those callback show()
and store() routines be passed a pointer to the larger attribute
structure containing the show() and store() routine addresses
themselves (in this case, kobj_attribute)?  or is everything those
callback routines might be interested in available in the simpler
"struct attribute"?

  and, of course, if one wanted to get the enclosing attribute
structure, there's always "container_of", but i'd just like to know if
there was a reason for the choice that was made.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

rationale for sysfs attr store/show accepting kobj_attribute?

[Jim Cromie]

On Sun, May 8, 2011 at 3:23 AM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
> ?more just a curiosity than anything else, but i'm perusing the
> kobject sample programs in the kernel source directory and in
> kobject-example.c, there's one thing about which i'm a bit puzzled.
>
> ?the kernel header file sysfs.h defines the basic attribute structure
> as:
>
> struct attribute {
> ? ? ? ?const char ? ? ? ? ? ? ?*name;
> ? ? ? ?mode_t ? ? ? ? ? ? ? ? ?mode;
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> ? ? ? ?struct lock_class_key ? *key;
> ? ? ? ?struct lock_class_key ? skey;
> #endif
> };
>
> in other words, simply the attribute name and mode. ?no problem.
>
> ?however, the header file kobject.h defines a more informative
> kobj_attribute structure thusly:
>
> struct kobj_attribute {
> ? ? ? ?struct attribute attr;
> ? ? ? ?ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,
> ? ? ? ? ? ? ? ? ? ? ? ?char *buf);
> ? ? ? ?ssize_t (*store)(struct kobject *kobj, struct kobj_attribute *attr,
> ? ? ? ? ? ? ? ? ? ? ? ? const char *buf, size_t count);
> };
>
> as you can see, it's just a regular sysfs attribute, bundled with the
> respective show and store routines.
>
> ?what puzzles me is why those show() and store() routines are defined
> as being passed the larger kobj_attribute structure, rather than the
> simpler sysfs attribute structure. ?it doesn't seem as if there's much
> value in having the show() and store() callbacks getting those two
> extra pieces of information.


well, theyre passing pointers, so "larger" doesnt matter.
the larger structure just has more info available.
Now whether pointers to show & store are particularly useful
from inside a show / store routine is debatable, as you noted.
A quick grep didnt show me anything, but my eye isnt
as discerning as Id like.

>
> ?in fact, in the sample program kobject-example.c, in the more
> general example, the callback routine needs to check the name of the
> attribute to know how to process it properly so it has to use
> "attr->attr.name" to access the field of the simpler "struct
> attribute" anyway.
>

thats to be expected.
looking up the name is far better than having many separate show/store
routines, each doing essentially the same thing, for
in0, in1, in2 ...

hwmon drivers used to do that (multiple fns, defined via macros),
the situation is much improved now, though I think a few still remain.

FWIW, there are SENSOR_ATTRs, which extend/subclass plain-old
struct attrs (modulo capitalization, approximations),
maybe that possibility is what kobj_attributes are about.

static struct sensor_device_attribute fan_min[] = {
	SENSOR_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 0),
	SENSOR_ATTR(fan2_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 1),
	SENSOR_ATTR(fan3_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 2),
};

hwmon-sysfs.h - hardware monitoring chip driver sysfs defines

struct sensor_device_attribute{
	struct device_attribute dev_attr;
	int index;
};
struct sensor_device_attribute_2 {
	struct device_attribute dev_attr;
	u8 index;
	u8 nr;
};

in device.h:

struct bus_attribute {
	struct attribute	attr;
	ssize_t (*show)(struct bus_type *bus, char *buf);
	ssize_t (*store)(struct bus_type *bus, const char *buf, size_t count);
};
struct driver_attribute {
	struct attribute attr;
	ssize_t (*show)(struct device_driver *driver, char *buf);
	ssize_t (*store)(struct device_driver *driver, const char *buf,
			 size_t count);
};
struct class_attribute {
	struct attribute attr;
	ssize_t (*show)(struct class *class, struct class_attribute *attr,
			char *buf);
	ssize_t (*store)(struct class *class, struct class_attribute *attr,
			const char *buf, size_t count);
};
struct device_attribute {
	struct attribute	attr;
	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
			char *buf);
	ssize_t (*store)(struct device *dev, struct device_attribute *attr,
			 const char *buf, size_t count);
};

I dont fully grok it, but theres clearly a pattern here.


> ?long question short -- even though it's clearly an arbitrary
> decision, was there some rationale for having those callback show()
> and store() routines be passed a pointer to the larger attribute
> structure containing the show() and store() routine addresses
> themselves (in this case, kobj_attribute)? ?or is everything those
> callback routines might be interested in available in the simpler
> "struct attribute"?
>
> ?and, of course, if one wanted to get the enclosing attribute
> structure, there's always "container_of", but i'd just like to know if
> there was a reason for the choice that was made.
>
> rday
>
> --

rationale for sysfs attr store/show accepting kobj_attribute?

[Robert P. J. Day]

On Tue, 10 May 2011, Jim Cromie wrote:

> On Sun, May 8, 2011 at 3:23 AM, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> >
> > ?more just a curiosity than anything else, but i'm perusing the
> > kobject sample programs in the kernel source directory and in
> > kobject-example.c, there's one thing about which i'm a bit puzzled.
> >
> > ?the kernel header file sysfs.h defines the basic attribute structure
> > as:
> >
> > struct attribute {
> > ? ? ? ?const char ? ? ? ? ? ? ?*name;
> > ? ? ? ?mode_t ? ? ? ? ? ? ? ? ?mode;
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > ? ? ? ?struct lock_class_key ? *key;
> > ? ? ? ?struct lock_class_key ? skey;
> > #endif
> > };
> >
> > in other words, simply the attribute name and mode. ?no problem.
> >
> > ?however, the header file kobject.h defines a more informative
> > kobj_attribute structure thusly:
> >
> > struct kobj_attribute {
> > ? ? ? ?struct attribute attr;
> > ? ? ? ?ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,
> > ? ? ? ? ? ? ? ? ? ? ? ?char *buf);
> > ? ? ? ?ssize_t (*store)(struct kobject *kobj, struct kobj_attribute *attr,
> > ? ? ? ? ? ? ? ? ? ? ? ? const char *buf, size_t count);
> > };
> >
> > as you can see, it's just a regular sysfs attribute, bundled with
> > the respective show and store routines.
> >
> > ?what puzzles me is why those show() and store() routines are
> > defined as being passed the larger kobj_attribute structure,
> > rather than the simpler sysfs attribute structure. ?it doesn't
> > seem as if there's much value in having the show() and store()
> > callbacks getting those two extra pieces of information.
>
> well, theyre passing pointers, so "larger" doesnt matter. the larger
> structure just has more info available. Now whether pointers to show
> & store are particularly useful from inside a show / store routine
> is debatable, as you noted. A quick grep didnt show me anything, but
> my eye isnt as discerning as Id like.

  and that was, of course, what i was curious about -- whether there
was any compelling reason for those show()/store() routines to be
passed the address of the larger, enclosing structure rather than the
more basic, common "struct attribute".  it's not a big deal, i'm just
curious as to the decision-making process that went into that.

> > ?in fact, in the sample program kobject-example.c, in the more
> > general example, the callback routine needs to check the name of the
> > attribute to know how to process it properly so it has to use
> > "attr->attr.name" to access the field of the simpler "struct
> > attribute" anyway.
>
> thats to be expected. looking up the name is far better than having
> many separate show/store routines, each doing essentially the same
> thing, for in0, in1, in2 ...

  yes, i know, but that wasn't my point.  my point was that, because
the larger structure address was passed, getting to the name required
following the struct attribute address.  in short, it just seemed
overkill to pass the address of the "larger" structure when, having
been passed it, the called routine simply dereferenced to get back to
the enclosed struct attribute structure.

> hwmon drivers used to do that (multiple fns, defined via macros),
> the situation is much improved now, though I think a few still remain.
>
> FWIW, there are SENSOR_ATTRs, which extend/subclass plain-old
> struct attrs (modulo capitalization, approximations),
> maybe that possibility is what kobj_attributes are about.
>
> static struct sensor_device_attribute fan_min[] = {
> 	SENSOR_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 0),
> 	SENSOR_ATTR(fan2_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 1),
> 	SENSOR_ATTR(fan3_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 2),
> };
>
> hwmon-sysfs.h - hardware monitoring chip driver sysfs defines
>
> struct sensor_device_attribute{
> 	struct device_attribute dev_attr;
> 	int index;
> };
> struct sensor_device_attribute_2 {
> 	struct device_attribute dev_attr;
> 	u8 index;
> 	u8 nr;
> };
>
> in device.h:
>
> struct bus_attribute {
> 	struct attribute	attr;
> 	ssize_t (*show)(struct bus_type *bus, char *buf);
> 	ssize_t (*store)(struct bus_type *bus, const char *buf, size_t count);
> };
> struct driver_attribute {
> 	struct attribute attr;
> 	ssize_t (*show)(struct device_driver *driver, char *buf);
> 	ssize_t (*store)(struct device_driver *driver, const char *buf,
> 			 size_t count);
> };
> struct class_attribute {
> 	struct attribute attr;
> 	ssize_t (*show)(struct class *class, struct class_attribute *attr,
> 			char *buf);
> 	ssize_t (*store)(struct class *class, struct class_attribute *attr,
> 			const char *buf, size_t count);
> };
> struct device_attribute {
> 	struct attribute	attr;
> 	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
> 			char *buf);
> 	ssize_t (*store)(struct device *dev, struct device_attribute *attr,
> 			 const char *buf, size_t count);
> };
>
> I dont fully grok it, but theres clearly a pattern here.

  ok, it's quite possibly a historical holdover and, as i said
earlier, it's not a critically important point to clarify, i was just
curious.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================