* [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks @ 2005-05-14 9:23 Yani Ioannou 2005-05-14 10:22 ` Russell King 0 siblings, 1 reply; 6+ messages in thread From: Yani Ioannou @ 2005-05-14 9:23 UTC (permalink / raw) To: Greg KH, linux-kernel [-- 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(-) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks 2005-05-14 9:23 [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks Yani Ioannou @ 2005-05-14 10:22 ` Russell King 2005-05-14 19:46 ` Yani Ioannou 0 siblings, 1 reply; 6+ messages in thread From: Russell King @ 2005-05-14 10:22 UTC (permalink / raw) To: Yani Ioannou; +Cc: Greg KH, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks 2005-05-14 10:22 ` Russell King @ 2005-05-14 19:46 ` Yani Ioannou 2005-05-14 21:18 ` Russell King 0 siblings, 1 reply; 6+ messages in thread From: Yani Ioannou @ 2005-05-14 19:46 UTC (permalink / raw) To: Yani Ioannou, Greg KH, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks 2005-05-14 19:46 ` Yani Ioannou @ 2005-05-14 21:18 ` Russell King 2005-05-14 21:34 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Russell King @ 2005-05-14 21:18 UTC (permalink / raw) To: Yani Ioannou; +Cc: Greg KH, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks 2005-05-14 21:18 ` Russell King @ 2005-05-14 21:34 ` Greg KH 2005-05-14 23:31 ` Yani Ioannou 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2005-05-14 21:34 UTC (permalink / raw) To: Yani Ioannou, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks 2005-05-14 21:34 ` Greg KH @ 2005-05-14 23:31 ` Yani Ioannou 0 siblings, 0 replies; 6+ messages in thread From: Yani Ioannou @ 2005-05-14 23:31 UTC (permalink / raw) To: Greg KH; +Cc: linux-kernel, LM Sensors 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-05-14 23:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-05-14 9:23 [PATCH 2.6.12-rc4 1/12] (dynamic sysfs callbacks) update device attribute callbacks Yani Ioannou 2005-05-14 10:22 ` Russell King 2005-05-14 19:46 ` Yani Ioannou 2005-05-14 21:18 ` Russell King 2005-05-14 21:34 ` Greg KH 2005-05-14 23:31 ` Yani Ioannou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).