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 at 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
========================================================================