[PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[Yani Ioannou]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

Hi,

This patch updates all device attribute callbacks to match the
previously submitted sysfs dynamic callback device attribute patch
which added the void * to the callback function signatures. Although I
submitted a link to this patch in my last series of patches, it is
much easier for people to review (and probably apply) in-line and
split up.

The patch was split up automagically by a perl script I wrote which
can be used to split a large arbitrary patch first by a number of
directory levels (1 in this case) and second into patches of a maximum
size (40K in this case). You can find the perl script at:

http://osdn.dl.sourceforge.net/sourceforge/bmcsensors-26/lkmlize.pl

See below for a summary diffstat of the combined patch, each e-mail
will also have it's own summary diffstat.

Signed-off-by: Yani Ioannou <yani.ioannou@gmail.com>

Thanks,
Yani

---

[-- Attachment #2: patch-linux-2.6.12-rc4-sysfsdyncallback-deviceattr-nowarn.diff.diffstat.txt --]
[-- Type: text/plain, Size: 6300 bytes --]

 Documentation/filesystems/sysfs.txt      |    2 
 arch/arm/common/amba.c                   |    2 
 arch/arm/kernel/ecard.c                  |   12 +-
 arch/arm26/kernel/ecard.c                |   10 +-
 arch/ia64/sn/kernel/tiocx.c              |    4 
 arch/parisc/kernel/drivers.c             |    2 
 arch/ppc/kernel/pci.c                    |    2 
 arch/ppc/syslib/ocp.c                    |    2 
 arch/ppc/syslib/of_device.c              |    2 
 arch/ppc64/kernel/of_device.c            |    2 
 arch/ppc64/kernel/pci.c                  |    2 
 arch/ppc64/kernel/vio.c                  |    4 
 drivers/base/dmapool.c                   |    2 
 drivers/base/interface.c                 |    4 
 drivers/base/power/sysfs.c               |    4 
 drivers/block/ub.c                       |    2 
 drivers/char/hvcs.c                      |   14 +--
 drivers/char/mbcs.c                      |    4 
 drivers/char/mwave/mwavedd.c             |    2 
 drivers/char/tpm/tpm.c                   |    6 -
 drivers/dio/dio-sysfs.c                  |   10 +-
 drivers/eisa/eisa-bus.c                  |    4 
 drivers/i2c/chips/adm1021.c              |    6 -
 drivers/i2c/chips/adm1025.c              |   28 +++---
 drivers/i2c/chips/adm1026.c              |   98 ++++++++++++------------
 drivers/i2c/chips/adm1031.c              |   44 +++++-----
 drivers/i2c/chips/asb100.c               |   46 +++++------
 drivers/i2c/chips/ds1621.c               |    6 -
 drivers/i2c/chips/fscher.c               |    8 -
 drivers/i2c/chips/fscpos.c               |   16 +--
 drivers/i2c/chips/gl518sm.c              |   12 +-
 drivers/i2c/chips/gl520sm.c              |    8 -
 drivers/i2c/chips/it87.c                 |   50 ++++++------
 drivers/i2c/chips/lm63.c                 |   24 ++---
 drivers/i2c/chips/lm75.c                 |    4 
 drivers/i2c/chips/lm77.c                 |   14 +--
 drivers/i2c/chips/lm78.c                 |   36 ++++----
 drivers/i2c/chips/lm80.c                 |   20 ++--
 drivers/i2c/chips/lm83.c                 |    6 -
 drivers/i2c/chips/lm85.c                 |   72 ++++++++---------
 drivers/i2c/chips/lm87.c                 |   46 +++++------
 drivers/i2c/chips/lm90.c                 |   12 +-
 drivers/i2c/chips/lm92.c                 |   14 +--
 drivers/i2c/chips/max1619.c              |    6 -
 drivers/i2c/chips/pc87360.c              |   68 ++++++++--------
 drivers/i2c/chips/pcf8574.c              |    6 -
 drivers/i2c/chips/pcf8591.c              |   10 +-
 drivers/i2c/chips/sis5595.c              |   34 ++++----
 drivers/i2c/chips/smsc47b397.c           |    4 
 drivers/i2c/chips/smsc47m1.c             |   20 ++--
 drivers/i2c/chips/via686a.c              |   32 +++----
 drivers/i2c/chips/w83627hf.c             |   56 ++++++-------
 drivers/i2c/chips/w83781d.c              |   52 ++++++------
 drivers/i2c/chips/w83l785ts.c            |    4 
 drivers/i2c/i2c-core.c                   |    4 
 drivers/ieee1394/nodemgr.c               |   16 +--
 drivers/ieee1394/sbp2.c                  |    2 
 drivers/input/gameport/gameport.c        |    4 
 drivers/input/keyboard/atkbd.c           |    4 
 drivers/input/mouse/psmouse.h            |    4 
 drivers/input/serio/serio.c              |   16 +--
 drivers/macintosh/therm_adt746x.c        |    8 -
 drivers/macintosh/therm_pm72.c           |    4 
 drivers/macintosh/therm_windtunnel.c     |    4 
 drivers/mca/mca-bus.c                    |    4 
 drivers/message/fusion/mptscsih.c        |    2 
 drivers/mmc/mmc_sysfs.c                  |    2 
 drivers/pci/hotplug/cpqphp_sysfs.c       |    4 
 drivers/pci/hotplug/shpchp_sysfs.c       |    4 
 drivers/pci/pci-sysfs.c                  |    6 -
 drivers/pcmcia/ds.c                      |    4 
 drivers/pnp/card.c                       |    4 
 drivers/pnp/interface.c                  |    8 -
 drivers/s390/block/dasd_devmap.c         |   10 +-
 drivers/s390/block/dcssblk.c             |   24 ++---
 drivers/s390/char/raw3270.c              |    6 -
 drivers/s390/char/tape_core.c            |   10 +-
 drivers/s390/char/vmlogrdr.c             |   12 +-
 drivers/s390/cio/ccwgroup.c              |    6 -
 drivers/s390/cio/chsc.c                  |    6 -
 drivers/s390/cio/cmf.c                   |   12 +-
 drivers/s390/cio/device.c                |   14 +--
 drivers/s390/net/claw.c                  |   40 ++++-----
 drivers/s390/net/ctcmain.c               |   18 ++--
 drivers/s390/net/lcs.c                   |   10 +-
 drivers/s390/net/netiucv.c               |   44 +++++-----
 drivers/s390/net/qeth_sys.c              |  126 +++++++++++++++----------------
 drivers/s390/scsi/zfcp_scsi.c            |    2 
 drivers/s390/scsi/zfcp_sysfs_adapter.c   |   10 +-
 drivers/s390/scsi/zfcp_sysfs_port.c      |   10 +-
 drivers/s390/scsi/zfcp_sysfs_unit.c      |    6 -
 drivers/scsi/53c700.c                    |    2 
 drivers/scsi/arm/eesox.c                 |    4 
 drivers/scsi/arm/powertec.c              |    4 
 drivers/scsi/ipr.c                       |    2 
 drivers/scsi/megaraid/megaraid_mbox.c    |    4 
 drivers/scsi/scsi_sysfs.c                |   28 +++---
 drivers/sh/superhyway/superhyway-sysfs.c |    2 
 drivers/usb/core/sysfs.c                 |   24 ++---
 drivers/usb/gadget/dummy_hcd.c           |    4 
 drivers/usb/gadget/file_storage.c        |    8 -
 drivers/usb/gadget/net2280.c             |    6 -
 drivers/usb/gadget/pxa2xx_udc.c          |    2 
 drivers/usb/input/aiptek.c               |   78 +++++++++----------
 drivers/usb/misc/cytherm.c               |   20 ++--
 drivers/usb/misc/phidgetkit.c            |   14 +--
 drivers/usb/misc/phidgetservo.c          |    4 
 drivers/usb/misc/usbled.c                |    4 
 drivers/usb/serial/ftdi_sio.c            |    6 -
 drivers/usb/storage/scsiglue.c           |    4 
 drivers/video/gbefb.c                    |    4 
 drivers/video/w100fb.c                   |   12 +-
 drivers/w1/w1.c                          |   16 +--
 drivers/w1/w1_family.h                   |    4 
 drivers/w1/w1_smem.c                     |    8 -
 drivers/w1/w1_therm.c                    |    8 -
 drivers/zorro/zorro-sysfs.c              |    4 
 include/asm-ppc/ocp.h                    |    2 
 118 files changed, 854 insertions(+), 854 deletions(-)

Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[Russell King]

On Sat, May 14, 2005 at 05:23:56AM -0400, Yani Ioannou wrote:
> This patch updates all device attribute callbacks to match the
> previously submitted sysfs dynamic callback device attribute patch
> which added the void * to the callback function signatures.

I missed commenting on this first time round.  What is the purpose behind
this idea?

Currently, sysfs attributes are generally static structures which don't
require allocation, and are shared between all objects.  I'm unable to
see what advantage adding this void pointer is supposed to give us,
other than having to dynamically allocate these structures if we want
to use them.

What's more, I don't really see what adding this buys us - we already
have the pointer which is supposed to identify the object passed in.

There's another question - how is the lifetime of the object pointed
to by this void pointer managed?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[Yani Ioannou]

Hi Russell,

> I missed commenting on this first time round.  What is the purpose behind
> this idea?

My first post to LKML on the patch:
http://lkml.org/lkml/2005/5/7/60

The idea originated in the lm_sensors mailing list, so you might want
to take a look at the lm_sensors archive is you are interested, in
particular the following thread:

http://archives.andrew.net.au/lm-sensors/msg31162.html

> Currently, sysfs attributes are generally static structures which don't
> require allocation, and are shared between all objects.  I'm unable to
> see what advantage adding this void pointer is supposed to give us,
> other than having to dynamically allocate these structures if we want
> to use them.

This isn't changing, although there are cases where it is
necessary/preferable to dynamically create the attributes (again see
previous discussion). This patch helps both static and dynamically
created attributes. The adm1026 example I posted to the mailing list
earlier uses entirely static attributes still (and hence the need for
the new macros my latest patch adds), and I expect most attributes
will remain static.

> What's more, I don't really see what adding this buys us - we already
> have the pointer which is supposed to identify the object passed in.

Using device_attribute as an example (most other derived sysfs
attributes have near identical callbacks):

struct device_attribute {
        struct attribute        attr;
        ssize_t (*show)(struct device * dev, char * buf);
        ssize_t (*store)(struct device * dev, const char * buf, size_t count);
};

The only pointer passed in the present callbacks is the device
structure, this doesn't help at all identify what attribute the
callback is for when a device can have many attributes.

> There's another question - how is the lifetime of the object pointed
> to by this void pointer managed?

Think of the pointer like the the driver_data pointer in struct device
(http://lxr.linux.no/source/include/linux/device.h#L270), except
instead of driver specific data pointer, this is attribute specific
data pointer, and might not even be used (i.e. NULL). What the pointer
points to should managed by whatever created that entity... I kind of
like Greg's renaming of the void * to private for reasons along those
lines. Your question about lifetime seems to imply that that entity
must be dynamically allocated, but that is not necessarily so.

Thanks,
Yani

Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[Russell King]

On Sat, May 14, 2005 at 03:46:31PM -0400, Yani Ioannou wrote:
> My first post to LKML on the patch:
> http://lkml.org/lkml/2005/5/7/60
> 
> The idea originated in the lm_sensors mailing list, so you might want
> to take a look at the lm_sensors archive is you are interested, in
> particular the following thread:
> ...
> 
> This isn't changing, although there are cases where it is
> necessary/preferable to dynamically create the attributes (again see
> previous discussion). This patch helps both static and dynamically
> created attributes. The adm1026 example I posted to the mailing list
> earlier uses entirely static attributes still (and hence the need for
> the new macros my latest patch adds), and I expect most attributes
> will remain static.

Ok.  I do wonder if the better solution would be to encapsulate
"device_attribute" where this extra information is required, and
pass a pointer to device_attribute to its methods, in much the
same way as "sysfs_ops" works.

This means your attributes in adm1016 become:

struct adm1016_attr {
	struct device_attribute	dev_attr;
	int			nr;
};

#define ADM1016_ATTR(_name,_mode,_show,_store,_nr)		\
	struct adm1016_attr adm_attr_##_name = {		\
		.dev_attr = __ATTR(_name,_mode,_show,_store),	\
		.nr = _nr,					\
	}

static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr, char *buf)
{
	struct adm1016_attr *adm_attr = to_adm_attr(attr);
	struct adm1026_data *data = adm1026_update_device(dev);
	return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_max[adm_attr->nr]));
}

#define temp_reg(offset)					\
...
static ADM1016_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR,	\
	show_temp_max, set_temp_max, offset)

There are two advantages to this way:

1. you're not having to impose the extra void * pointer in the
   attribute on everyone.
2. you allow people to add whatever data they please to the attribute
   in whatever format they wish - whether it be a void pointer, integer,
   or whatever.

This seems far more flexible to me, at least.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[Greg KH]

On Sat, May 14, 2005 at 10:18:38PM +0100, Russell King wrote:
> On Sat, May 14, 2005 at 03:46:31PM -0400, Yani Ioannou wrote:
> > My first post to LKML on the patch:
> > http://lkml.org/lkml/2005/5/7/60
> > 
> > The idea originated in the lm_sensors mailing list, so you might want
> > to take a look at the lm_sensors archive is you are interested, in
> > particular the following thread:
> > ...
> > 
> > This isn't changing, although there are cases where it is
> > necessary/preferable to dynamically create the attributes (again see
> > previous discussion). This patch helps both static and dynamically
> > created attributes. The adm1026 example I posted to the mailing list
> > earlier uses entirely static attributes still (and hence the need for
> > the new macros my latest patch adds), and I expect most attributes
> > will remain static.
> 
> Ok.  I do wonder if the better solution would be to encapsulate
> "device_attribute" where this extra information is required, and
> pass a pointer to device_attribute to its methods, in much the
> same way as "sysfs_ops" works.
> 
> This means your attributes in adm1016 become:
> 
> struct adm1016_attr {
> 	struct device_attribute	dev_attr;
> 	int			nr;
> };
> 
> #define ADM1016_ATTR(_name,_mode,_show,_store,_nr)		\
> 	struct adm1016_attr adm_attr_##_name = {		\
> 		.dev_attr = __ATTR(_name,_mode,_show,_store),	\
> 		.nr = _nr,					\
> 	}
> 
> static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr, char *buf)
> {
> 	struct adm1016_attr *adm_attr = to_adm_attr(attr);
> 	struct adm1026_data *data = adm1026_update_device(dev);
> 	return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_max[adm_attr->nr]));
> }
> 
> #define temp_reg(offset)					\
> ...
> static ADM1016_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR,	\
> 	show_temp_max, set_temp_max, offset)
> 
> There are two advantages to this way:
> 
> 1. you're not having to impose the extra void * pointer in the
>    attribute on everyone.
> 2. you allow people to add whatever data they please to the attribute
>    in whatever format they wish - whether it be a void pointer, integer,
>    or whatever.
> 
> This seems far more flexible to me, at least.

Ah, nice, I hadn't thought about that.  But yes, it would be much
smaller and simpler to do this, very good idea.

And if enough i2c drivers want to do this, just make a i2c driver
attribute that they all use to achieve this.

Yani, what do you think?

thanks,

greg k-h

Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks

[Yani Ioannou]

On 5/14/05, Greg KH <greg@kroah.com> wrote:
> On Sat, May 14, 2005 at 10:18:38PM +0100, Russell King wrote:
> > There are two advantages to this way:
> >
> > 1. you're not having to impose the extra void * pointer in the
> >    attribute on everyone.

Well this is less of an advantage when you consider that most sysfs
attributes in the kernel should be taking advantage of it. A quick
look through the various sysfs attributes (e.g. class_attribute,
device_attribute, etc) in the kernel and how they are used quickly
convinces you that the idea doesn't just address a problem in one or
two drivers, or even one particular type of attribute, but most of the
kernel sysfs attributes. Of course that problem is more obvious in
some sections of the kernel than others.

> > 2. you allow people to add whatever data they please to the attribute
> >    in whatever format they wish - whether it be a void pointer, integer,
> >    or whatever.

You can do just as much with a void * , e.g. define a struct with
whatever in it and point to it is functionally the same thing, and my
net-sysfs.c patch did this (see lm_sensors archive again), but this is
much more obvious, transparent and less error prone (in my opinion).
However I never liked passing an int via a pointer for adm1026,
despite how 'natural' Greg claims it is ;-).

> Ah, nice, I hadn't thought about that.  But yes, it would be much
> smaller and simpler to do this, very good idea.
>
> Yani, what do you think?

I like it and think its a good idea, just not for the advantages
Russell listed :-). Functionally it does the same thing, but in a much
more semantically appropriate way:

- It is much more obvious by passing the attribute pointer to the
callbacks what that extra parameter is used for (identifying which
attribute the callback is for).
- No awkward and error-prone casting when all you want to do is pass an int.
- It will be much more transparent to an outsider reading through the
sysfs attribute code that adm1026_attribute represents an adm1026
sensors attribute, and how, etc.

> And if enough i2c drivers want to do this, just make a i2c driver
> attribute that they all use to achieve this.

This sounds like a good idea for the majority of i2c chip drivers,
however in special cases like bmcsensors it might have to use it's own
attribute type (which in bmcsensor's case would contain the sdr_data
struct).

My only present worry is that this makes the creation of a standard
method to dynamically allocate and create derived sysfs attributes a
lot harder (impossible?)...

Thanks,
Yani

[PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device

[Yani Ioannou]

On 5/14/05, Greg KH <greg@kroah.com> wrote:
> On Sat, May 14, 2005 at 10:18:38PM +0100, Russell King wrote:
> > There are two advantages to this way:
> >
> > 1. you're not having to impose the extra void * pointer in the
> >    attribute on everyone.

Well this is less of an advantage when you consider that most sysfs
attributes in the kernel should be taking advantage of it. A quick
look through the various sysfs attributes (e.g. class_attribute,
device_attribute, etc) in the kernel and how they are used quickly
convinces you that the idea doesn't just address a problem in one or
two drivers, or even one particular type of attribute, but most of the
kernel sysfs attributes. Of course that problem is more obvious in
some sections of the kernel than others.

> > 2. you allow people to add whatever data they please to the attribute
> >    in whatever format they wish - whether it be a void pointer, integer,
> >    or whatever.

You can do just as much with a void * , e.g. define a struct with
whatever in it and point to it is functionally the same thing, and my
net-sysfs.c patch did this (see lm_sensors archive again), but this is
much more obvious, transparent and less error prone (in my opinion).
However I never liked passing an int via a pointer for adm1026,
despite how 'natural' Greg claims it is ;-).

> Ah, nice, I hadn't thought about that.  But yes, it would be much
> smaller and simpler to do this, very good idea.
>
> Yani, what do you think?

I like it and think its a good idea, just not for the advantages
Russell listed :-). Functionally it does the same thing, but in a much
more semantically appropriate way:

- It is much more obvious by passing the attribute pointer to the
callbacks what that extra parameter is used for (identifying which
attribute the callback is for).
- No awkward and error-prone casting when all you want to do is pass an int.
- It will be much more transparent to an outsider reading through the
sysfs attribute code that adm1026_attribute represents an adm1026
sensors attribute, and how, etc.

> And if enough i2c drivers want to do this, just make a i2c driver
> attribute that they all use to achieve this.

This sounds like a good idea for the majority of i2c chip drivers,
however in special cases like bmcsensors it might have to use it's own
attribute type (which in bmcsensor's case would contain the sdr_data
struct).

My only present worry is that this makes the creation of a standard
method to dynamically allocate and create derived sysfs attributes a
lot harder (impossible?)...

Thanks,
Yani