[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Hi all,

I'm sending this driver sysfs callback patch to the lm-sensors list to
get more of the sensor driver maintainer's comments on the patch
(specifically the type to be associated with each sysfs entry) before
submitting to LKML. Jean and Greg have already given me invaluable
feedback and are supportive of the general idea of the patch (the
problem of which is epitomized by the kludge the 2.6 bmcsensors driver
is at the moment - see previous discussions in the list archive).

Included is a patch against drivers/base/core.c and
include/linux/driver.h that adds a void * to the device attribute
struct and passes it back to the two sysfs callbacks show/store. Also
included is a simple perl script we can run against the source tree to
update drivers to use the correct callback pointer types (otherwise
warnings are generated on compile for each). Of course 'real' fixes in
the spirit of the new callbacks would be to use the void */passed type
to implement a single/few dynamic callback instead of the messy static
callbacks at present, but I leave that up to the individual driver
maintainer.

The original patch I sent out to Jean/Greg for comment just passed the
sysfs name of the corresponding sysfs entry, and Jean wisely suggested
that this was not ideal and suggested using an int instead. Greg
instead suggested we use a void pointer (apparently like most other
such linux callbacks), and I am also favourable to that idea, although
both ideas are better than the present callbacks.

I do believe Greg's suggestion to pass back a void * is probably the
best balance of generalization and efficiency for kernel device
drivers, thinking just in terms of bmcsensors it is actually ideal. An
IPMI BMC (baseboard management controller) represents sensors by SDRs
(sensor data records) each of which is read by bmcsensors through the
i2c-ipmi (and in turn through the kernel IPMI interface) and into a
struct, and thereafter all are referenced in a fixed length array (as
in the original 2.4 driver).

With the new callbacks I could actually associate each of these SDRs
with the sysfs entries by passing a pointer to each struct, nothing
could be simpler to use. I can then use a linked-list (rather than the
fixed array) to store the pointers to aid in the de-allocation of the
SDR struct memory (or maybe even find a way to find all the pointers
created by the driver from the sysfs system and thus not even need the
list? I have to look into it).

Compared to passing back an int this has a distinct advantage - giving
each sdr an id and getting the id from the sysfs callback I would have
to search through the linked list of sdrs (using an array would imply
a fixed number/upper limit to the number of sensors), while given a
pointer to the sdr entry we don't need the id or to search for the
sdr.

Of course the best point about void * is that if all you want is an in
you can of course use a reference to an int...I don't think that the
allocation of memory for these ints is a problem given the amount of
memory we already allocate in creating the sysfs entry names, etc. I
really do think that this added complexity is trivial compared to the
added flexibility of a void * callback. We don't want to have to
change these sysfs callbacks ever again if we can help it (once will
be quite enough trouble! ;-) ) so I believe we should do things with
other driver's future needs in mind, and get it right the first time.

Thus at present my plan for re-writing bmcsensors goes something along
the lines of:

- get this (or a similar) patch accepted into the kernel.
- remove i2c-ipmi, roll IPMI interface back into bmcsensors/remove
what can be done directly with the kernel IPMI system.
- add bmcsensors to the new hwmon class of hardware sensors removing
any reference to the fake i2c code.
- remove fixed sdr list and replace with linked list.
- using the dynamic device callbacks this patch would introduce,
associate each sysfs entry with the corresponding sdr, replace the
kludge of the 100 (!) macro defined static sysfs callbacks with one
single dynamic callback that uses the void * pointer to the sdr to
decide what to return, thus allowing a virtually unlimited number of
sensors.
- cross fingers and submit to this list for testing and later
(hopefully) inclusion in kernel.

Mind you at the moment my IPMI BMC isn't even working in newer
(2.6.11.5) kernels, and I've got no response from the Linux IPMI list
on my problems so this might be a bit difficult for me to
develop/test.

Yani
-------------- next part --------------
diff -uprN -X dontdiff linux-2.6.12-rc3/drivers/base/core.c linux-2.6.12-rc3-devdyncallback/drivers/base/core.c
--- linux-2.6.12-rc3/drivers/base/core.c	2005-04-23 14:55:03.000000000 -0400
+++ linux-2.6.12-rc3-devdyncallback/drivers/base/core.c	2005-04-23 15:10:46.000000000 -0400
@@ -41,7 +41,7 @@ dev_attr_show(struct kobject * kobj, str
 	ssize_t ret = 0;
 
 	if (dev_attr->show)
-		ret = dev_attr->show(dev, buf);
+		ret = dev_attr->show(dev, buf, dev_attr->ptr);
 	return ret;
 }
 
@@ -54,7 +54,7 @@ dev_attr_store(struct kobject * kobj, st
 	ssize_t ret = 0;
 
 	if (dev_attr->store)
-		ret = dev_attr->store(dev, buf, count);
+		ret = dev_attr->store(dev, buf, count, dev_attr->ptr);
 	return ret;
 }
 
diff -uprN -X dontdiff linux-2.6.12-rc3/include/linux/device.h linux-2.6.12-rc3-devdyncallback/include/linux/device.h
--- linux-2.6.12-rc3/include/linux/device.h	2005-04-23 14:55:11.000000000 -0400
+++ linux-2.6.12-rc3-devdyncallback/include/linux/device.h	2005-04-23 15:10:05.000000000 -0400
@@ -335,8 +335,13 @@ extern void driver_attach(struct device_
 
 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);
+	ssize_t (*show)(struct device * dev, char * buf, void * ptr);
+	ssize_t (*store)(struct device * dev, const char * buf, size_t count, void * ptr);
+	/* 
+	 * void pointer can be used by driver to identify sysfs entry 
+	 * for 'dynamic' callbacks.
+	 */
+	void * ptr;
 };
 
 #define DEVICE_ATTR(_name,_mode,_show,_store) \




-------------- next part --------------
A non-text attachment was scrubbed...
Name: updatedyncallback.9249DEFANGED-pl
Type: application/octet-stream
Size: 257 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050425/68cb3ba7/updatedyncallback.obj

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Mon, Apr 25, 2005 at 01:36:56AM -0400, Yani Ioannou wrote:
> Hi all,
> 
> I'm sending this driver sysfs callback patch to the lm-sensors list to
> get more of the sensor driver maintainer's comments on the patch
> (specifically the type to be associated with each sysfs entry) before
> submitting to LKML. Jean and Greg have already given me invaluable
> feedback and are supportive of the general idea of the patch (the
> problem of which is epitomized by the kludge the 2.6 bmcsensors driver
> is at the moment - see previous discussions in the list archive).
> 
> Included is a patch against drivers/base/core.c and
> include/linux/driver.h that adds a void * to the device attribute
> struct and passes it back to the two sysfs callbacks show/store. Also
> included is a simple perl script we can run against the source tree to
> update drivers to use the correct callback pointer types (otherwise
> warnings are generated on compile for each). Of course 'real' fixes in
> the spirit of the new callbacks would be to use the void */passed type
> to implement a single/few dynamic callback instead of the messy static
> callbacks at present, but I leave that up to the individual driver
> maintainer.

Care to show how a driver would use this change?

And the void * shouldn't be called ptr, use what other structures call
their void pointers, "data", "private", etc.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Jean Delvare]

Hi Greg, Yani, all,

> Care to show how a driver would use this change?

I would like to see this too. I have thought about it, and just cannot
see how a pointer will be convenient to use.

Yani said earlier that a pointer would be fine for bmcsensors. However,
the device files are defined at driver level, not device level (at least
for all other hardware monitoring drivers). This means that the pointer
would need to be pointing at something driver-specific, which doesn't
make much sense as far as I can see. This is the reason why having the
additional parameter be an offset makes more sense to me. The callback
function already receives a device-specific pointer (dev * itself), and
the offset can be used to point to any part of the private data, at the
driver's option.

One possibility would be to start allocating the device files dynamically
instead of having them static as we do now. They would no more be shared
between devices. That way, the additional parameter could actually be
device-specific, and then I agree that a pointer can make sense
(although it still doesn't look optimum in the sensors case). But that
would be an even bigger change (and possibly not an enjoyable one
performance-wise).

> And the void * shouldn't be called ptr, use what other structures call
> their void pointers, "data", "private", etc.

What are these other structures, BTW? I'd like to take a look at them,
maybe it would enlighten my views.

Thanks,
--
Jean Delvare

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Jean Delvare]

Hi Dmitry,

> If void is added directly to the attribute structure that means that same
> attribute can not be shared between several instances of the same device
> -> unexpected. So far all attributes could be statically created.

My point exactly...

--
Jean Delvare

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Dmitry Torokhov]

Hi,

On Monday 25 April 2005 00:36, Yani Ioannou wrote:
> Hi all,
> 
> I'm sending this driver sysfs callback patch to the lm-sensors list to
> get more of the sensor driver maintainer's comments on the patch
> (specifically the type to be associated with each sysfs entry) before
> submitting to LKML. Jean and Greg have already given me invaluable
> feedback and are supportive of the general idea of the patch (the
> problem of which is epitomized by the kludge the 2.6 bmcsensors driver
> is at the moment - see previous discussions in the list archive).
> 
> Included is a patch against drivers/base/core.c and
> include/linux/driver.h that adds a void * to the device attribute
> struct and passes it back to the two sysfs callbacks show/store.

If void is added directly to the attribute structure that means that same
attribute can not be shared between several instances of the same device
-> unexpected. So far all attributes could be statically created.

-- 
Dmitry

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Tue, Apr 26, 2005 at 09:42:11AM +0200, Jean Delvare wrote:
> > And the void * shouldn't be called ptr, use what other structures call
> > their void pointers, "data", "private", etc.
> 
> What are these other structures, BTW? I'd like to take a look at them,
> maybe it would enlighten my views.

struct device
struct file
struct inode

And others.  No one needs to define a pointer as "ptr", that's just
redundant :)

But yes, it should be a "void *" that part is correct.  No matter what
we want to store in there, that will be sufficient.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Tue, Apr 26, 2005 at 02:45:06AM -0500, Dmitry Torokhov wrote:
> Hi,
> 
> On Monday 25 April 2005 00:36, Yani Ioannou wrote:
> > Hi all,
> > 
> > I'm sending this driver sysfs callback patch to the lm-sensors list to
> > get more of the sensor driver maintainer's comments on the patch
> > (specifically the type to be associated with each sysfs entry) before
> > submitting to LKML. Jean and Greg have already given me invaluable
> > feedback and are supportive of the general idea of the patch (the
> > problem of which is epitomized by the kludge the 2.6 bmcsensors driver
> > is at the moment - see previous discussions in the list archive).
> > 
> > Included is a patch against drivers/base/core.c and
> > include/linux/driver.h that adds a void * to the device attribute
> > struct and passes it back to the two sysfs callbacks show/store.
> 
> If void is added directly to the attribute structure that means that same
> attribute can not be shared between several instances of the same device
> -> unexpected. So far all attributes could be statically created.

One reason I will not accept such a patch, without seeing how it will be
used :)

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 4/26/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> If void is added directly to the attribute structure that means that same
> attribute can not be shared between several instances of the same device
> -> unexpected. So far all attributes could be statically created.

Dmitry: would you mind explaining this a bit more? Also is there
anywhere else you would suggest adding the callback specific void *
to? Perhaps creating some new structure(s) to track driver specific
data for each device?
 
On 4/26/05, Greg KH <greg@kroah.com> wrote:
> Care to show how a driver would use this change?
>
> And the void * shouldn't be called ptr, use what other structures call
> their void pointers, "data", "private", etc.

Attached is an updated patch (yes ptr is rather redundant...) and also
a quickly coded diff between my bmcsensors driver with and without
this patch. Note that a real bmcsensors update would also change the
sdrd[] array into a linked list and allow a potentially unlimited
number of sensors to be seen, since this patch also allows an
unlimited number of sysfs entries to be created at runtime, not
feasible with static callbacks.

This also significantly reduces the module's size, comparing both
modules running on my bmc with 17 sensors:

--------bmcsensors-devdyncallback lsmod -------
Module                  Size  Used by
bmcsensors             24996  0
-------------------bmcsensors lsmod--------------------
Module                  Size  Used by
bmcsensors             75940  0

In the hardware sensors driver class bmcsensors is rather an extreme -
none of the other hwmon drivers have to deal with an unlimited number
of sensors, but many do deal with more than a few and would no doubt
save some code complexity and module size if they took advantage of
this patch.

I hope my code speaks louder than my words ;-)

Thanks,
Yani
-------------- next part --------------
diff -uprN -X dontdiff linux-2.6.12-rc3/drivers/base/core.c linux-2.6.12-rc3-devdyncallback/drivers/base/core.c
--- linux-2.6.12-rc3/drivers/base/core.c	2005-04-26 22:03:23.000000000 -0400
+++ linux-2.6.12-rc3-devdyncallback/drivers/base/core.c	2005-04-27 01:01:21.000000000 -0400
@@ -41,7 +41,7 @@ dev_attr_show(struct kobject * kobj, str
 	ssize_t ret = 0;
 
 	if (dev_attr->show)
-		ret = dev_attr->show(dev, buf);
+		ret = dev_attr->show(dev, buf, dev_attr->data);
 	return ret;
 }
 
@@ -54,7 +54,7 @@ dev_attr_store(struct kobject * kobj, st
 	ssize_t ret = 0;
 
 	if (dev_attr->store)
-		ret = dev_attr->store(dev, buf, count);
+		ret = dev_attr->store(dev, buf, count, dev_attr->data);
 	return ret;
 }
 
diff -uprN -X dontdiff linux-2.6.12-rc3/include/linux/device.h linux-2.6.12-rc3-devdyncallback/include/linux/device.h
--- linux-2.6.12-rc3/include/linux/device.h	2005-04-26 22:03:32.000000000 -0400
+++ linux-2.6.12-rc3-devdyncallback/include/linux/device.h	2005-04-27 22:09:23.000000000 -0400
@@ -335,8 +335,13 @@ extern void driver_attach(struct device_
 
 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);
+	ssize_t (*show)(struct device * dev, char * buf, void * data);
+	ssize_t (*store)(struct device * dev, const char * buf, size_t count, void * data);
+	/* 
+	 * void pointer can be used by driver to identify sysfs entry 
+	 * for 'dynamic' callbacks.
+	 */
+	void * data;
 };
 
 #define DEVICE_ATTR(_name,_mode,_show,_store) \




-------------- next part --------------
--- linux-2.6.11.7-bmcsensors/drivers/i2c/chips/bmcsensors.c	2005-04-28 01:46:17.000000000 -0400
+++ linux-2.6.11.7-devdyncallback-bmcsensors/drivers/i2c/chips/bmcsensors.c	2005-04-28 01:25:18.000000000 -0400
@@ -328,1334 +328,67 @@ static void bmcsensors_select_thresholds
 
 /************* sysfs callback functions *********/
 
-static ssize_t show_sensor(struct device *dev, char *buf, int id)
+static ssize_t show_sensor(struct device *dev, char *buf, void *data)
 {
+	struct sdrdata *sdrd = ((struct sdrdata *)data);
 	bmcsensors_update_client(&bmc_client);
 	return snprintf(buf, 20, "%ld\n",
-			conv_val(sdrd[id].reading, &sdrd[id]));
+			conv_val(sdrd->reading, sdrd));
 }
 
-/* Yes this is a kludge, but with the device.h defined callbacks we don't seem
- * to have any choice at this time, see: 
- *
- * http://archives.andrew.net.au/lm-sensors/msg29942.html
- *
- * for a discussion of the problem. If you have a board with more than 100
- * sensors define more callbacks, add them to the array, and increase the
- * MAX_SDR_ENTRIES to the new limit.
- */
-/* macro to define a callback for sensor #id */
-#define show_sensor_id(id) \
-static ssize_t show_sensor_##id (struct device *dev, char *buf) \
-{ \
-	return show_sensor(dev,buf,id); \
-}
-
-/* define callbacks for 100 sensors */
-show_sensor_id(0);
-show_sensor_id(1);
-show_sensor_id(2);
-show_sensor_id(3);
-show_sensor_id(4);
-show_sensor_id(5);
-show_sensor_id(6);
-show_sensor_id(7);
-show_sensor_id(8);
-show_sensor_id(9);
-show_sensor_id(10);
-show_sensor_id(11);
-show_sensor_id(12);
-show_sensor_id(13);
-show_sensor_id(14);
-show_sensor_id(15);
-show_sensor_id(16);
-show_sensor_id(17);
-show_sensor_id(18);
-show_sensor_id(19);
-show_sensor_id(20);
-show_sensor_id(21);
-show_sensor_id(22);
-show_sensor_id(23);
-show_sensor_id(24);
-show_sensor_id(25);
-show_sensor_id(26);
-show_sensor_id(27);
-show_sensor_id(28);
-show_sensor_id(29);
-show_sensor_id(30);
-show_sensor_id(31);
-show_sensor_id(32);
-show_sensor_id(33);
-show_sensor_id(34);
-show_sensor_id(35);
-show_sensor_id(36);
-show_sensor_id(37);
-show_sensor_id(38);
-show_sensor_id(39);
-show_sensor_id(40);
-show_sensor_id(41);
-show_sensor_id(42);
-show_sensor_id(43);
-show_sensor_id(44);
-show_sensor_id(45);
-show_sensor_id(46);
-show_sensor_id(47);
-show_sensor_id(48);
-show_sensor_id(49);
-show_sensor_id(50);
-show_sensor_id(51);
-show_sensor_id(52);
-show_sensor_id(53);
-show_sensor_id(54);
-show_sensor_id(55);
-show_sensor_id(56);
-show_sensor_id(57);
-show_sensor_id(58);
-show_sensor_id(59);
-show_sensor_id(60);
-show_sensor_id(61);
-show_sensor_id(62);
-show_sensor_id(63);
-show_sensor_id(64);
-show_sensor_id(65);
-show_sensor_id(66);
-show_sensor_id(67);
-show_sensor_id(68);
-show_sensor_id(69);
-show_sensor_id(70);
-show_sensor_id(71);
-show_sensor_id(72);
-show_sensor_id(73);
-show_sensor_id(74);
-show_sensor_id(75);
-show_sensor_id(76);
-show_sensor_id(77);
-show_sensor_id(78);
-show_sensor_id(79);
-show_sensor_id(80);
-show_sensor_id(81);
-show_sensor_id(82);
-show_sensor_id(83);
-show_sensor_id(84);
-show_sensor_id(85);
-show_sensor_id(86);
-show_sensor_id(87);
-show_sensor_id(88);
-show_sensor_id(89);
-show_sensor_id(90);
-show_sensor_id(91);
-show_sensor_id(92);
-show_sensor_id(93);
-show_sensor_id(94);
-show_sensor_id(95);
-show_sensor_id(96);
-show_sensor_id(97);
-show_sensor_id(98);
-show_sensor_id(99);
-
-/* add the defined callbacks to a lookup table */
-static show_callback show_fcn[] = {
-	show_sensor_0,
-	show_sensor_1,
-	show_sensor_2,
-	show_sensor_3,
-	show_sensor_4,
-	show_sensor_5,
-	show_sensor_6,
-	show_sensor_7,
-	show_sensor_8,
-	show_sensor_9,
-	show_sensor_10,
-	show_sensor_11,
-	show_sensor_12,
-	show_sensor_13,
-	show_sensor_14,
-	show_sensor_15,
-	show_sensor_16,
-	show_sensor_17,
-	show_sensor_18,
-	show_sensor_19,
-	show_sensor_20,
-	show_sensor_21,
-	show_sensor_22,
-	show_sensor_23,
-	show_sensor_24,
-	show_sensor_25,
-	show_sensor_26,
-	show_sensor_27,
-	show_sensor_28,
-	show_sensor_29,
-	show_sensor_30,
-	show_sensor_31,
-	show_sensor_32,
-	show_sensor_33,
-	show_sensor_34,
-	show_sensor_35,
-	show_sensor_36,
-	show_sensor_37,
-	show_sensor_38,
-	show_sensor_39,
-	show_sensor_40,
-	show_sensor_41,
-	show_sensor_42,
-	show_sensor_43,
-	show_sensor_44,
-	show_sensor_45,
-	show_sensor_46,
-	show_sensor_47,
-	show_sensor_48,
-	show_sensor_49,
-	show_sensor_50,
-	show_sensor_51,
-	show_sensor_52,
-	show_sensor_53,
-	show_sensor_54,
-	show_sensor_55,
-	show_sensor_56,
-	show_sensor_57,
-	show_sensor_58,
-	show_sensor_59,
-	show_sensor_60,
-	show_sensor_61,
-	show_sensor_62,
-	show_sensor_63,
-	show_sensor_64,
-	show_sensor_65,
-	show_sensor_66,
-	show_sensor_67,
-	show_sensor_68,
-	show_sensor_69,
-	show_sensor_70,
-	show_sensor_71,
-	show_sensor_72,
-	show_sensor_73,
-	show_sensor_74,
-	show_sensor_75,
-	show_sensor_76,
-	show_sensor_77,
-	show_sensor_78,
-	show_sensor_79,
-	show_sensor_80,
-	show_sensor_81,
-	show_sensor_82,
-	show_sensor_83,
-	show_sensor_84,
-	show_sensor_85,
-	show_sensor_86,
-	show_sensor_87,
-	show_sensor_88,
-	show_sensor_89,
-	show_sensor_90,
-	show_sensor_91,
-	show_sensor_92,
-	show_sensor_93,
-	show_sensor_94,
-	show_sensor_95,
-	show_sensor_96,
-	show_sensor_97,
-	show_sensor_98,
-	show_sensor_99,
-};
-
-static ssize_t show_sensor_max(struct device *dev, char *buf, int id)
+static ssize_t show_sensor_max(struct device *dev, char *buf, void *data)
 {
 	long max = 0;
+	struct sdrdata *sdrd = ((struct sdrdata *)data);
 
-	if (sdrd[id].lim1 >= 0)
-		max = conv_val(sdrd[id].limits[sdrd[id].lim1], &sdrd[id]);
+	if (sdrd->lim1 >= 0)
+		max = conv_val(sdrd->limits[sdrd->lim1], sdrd);
 	return snprintf(buf, 20, "%ld\n", max);
 }
 
-#define show_sensor_max_id(id) \
-static ssize_t show_sensor_max_##id (struct device *dev, char *buf) \
-{\
-    return show_sensor_max(dev, buf, id); \
-}
-
-show_sensor_max_id(0);
-show_sensor_max_id(1);
-show_sensor_max_id(2);
-show_sensor_max_id(3);
-show_sensor_max_id(4);
-show_sensor_max_id(5);
-show_sensor_max_id(6);
-show_sensor_max_id(7);
-show_sensor_max_id(8);
-show_sensor_max_id(9);
-show_sensor_max_id(10);
-show_sensor_max_id(11);
-show_sensor_max_id(12);
-show_sensor_max_id(13);
-show_sensor_max_id(14);
-show_sensor_max_id(15);
-show_sensor_max_id(16);
-show_sensor_max_id(17);
-show_sensor_max_id(18);
-show_sensor_max_id(19);
-show_sensor_max_id(20);
-show_sensor_max_id(21);
-show_sensor_max_id(22);
-show_sensor_max_id(23);
-show_sensor_max_id(24);
-show_sensor_max_id(25);
-show_sensor_max_id(26);
-show_sensor_max_id(27);
-show_sensor_max_id(28);
-show_sensor_max_id(29);
-show_sensor_max_id(30);
-show_sensor_max_id(31);
-show_sensor_max_id(32);
-show_sensor_max_id(33);
-show_sensor_max_id(34);
-show_sensor_max_id(35);
-show_sensor_max_id(36);
-show_sensor_max_id(37);
-show_sensor_max_id(38);
-show_sensor_max_id(39);
-show_sensor_max_id(40);
-show_sensor_max_id(41);
-show_sensor_max_id(42);
-show_sensor_max_id(43);
-show_sensor_max_id(44);
-show_sensor_max_id(45);
-show_sensor_max_id(46);
-show_sensor_max_id(47);
-show_sensor_max_id(48);
-show_sensor_max_id(49);
-show_sensor_max_id(50);
-show_sensor_max_id(51);
-show_sensor_max_id(52);
-show_sensor_max_id(53);
-show_sensor_max_id(54);
-show_sensor_max_id(55);
-show_sensor_max_id(56);
-show_sensor_max_id(57);
-show_sensor_max_id(58);
-show_sensor_max_id(59);
-show_sensor_max_id(60);
-show_sensor_max_id(61);
-show_sensor_max_id(62);
-show_sensor_max_id(63);
-show_sensor_max_id(64);
-show_sensor_max_id(65);
-show_sensor_max_id(66);
-show_sensor_max_id(67);
-show_sensor_max_id(68);
-show_sensor_max_id(69);
-show_sensor_max_id(70);
-show_sensor_max_id(71);
-show_sensor_max_id(72);
-show_sensor_max_id(73);
-show_sensor_max_id(74);
-show_sensor_max_id(75);
-show_sensor_max_id(76);
-show_sensor_max_id(77);
-show_sensor_max_id(78);
-show_sensor_max_id(79);
-show_sensor_max_id(80);
-show_sensor_max_id(81);
-show_sensor_max_id(82);
-show_sensor_max_id(83);
-show_sensor_max_id(84);
-show_sensor_max_id(85);
-show_sensor_max_id(86);
-show_sensor_max_id(87);
-show_sensor_max_id(88);
-show_sensor_max_id(89);
-show_sensor_max_id(90);
-show_sensor_max_id(91);
-show_sensor_max_id(92);
-show_sensor_max_id(93);
-show_sensor_max_id(94);
-show_sensor_max_id(95);
-show_sensor_max_id(96);
-show_sensor_max_id(97);
-show_sensor_max_id(98);
-show_sensor_max_id(99);
-
-static show_callback show_max_fcn[] = {
-	show_sensor_max_0,
-	show_sensor_max_1,
-	show_sensor_max_2,
-	show_sensor_max_3,
-	show_sensor_max_4,
-	show_sensor_max_5,
-	show_sensor_max_6,
-	show_sensor_max_7,
-	show_sensor_max_8,
-	show_sensor_max_9,
-	show_sensor_max_10,
-	show_sensor_max_11,
-	show_sensor_max_12,
-	show_sensor_max_13,
-	show_sensor_max_14,
-	show_sensor_max_15,
-	show_sensor_max_16,
-	show_sensor_max_17,
-	show_sensor_max_18,
-	show_sensor_max_19,
-	show_sensor_max_20,
-	show_sensor_max_21,
-	show_sensor_max_22,
-	show_sensor_max_23,
-	show_sensor_max_24,
-	show_sensor_max_25,
-	show_sensor_max_26,
-	show_sensor_max_27,
-	show_sensor_max_28,
-	show_sensor_max_29,
-	show_sensor_max_30,
-	show_sensor_max_31,
-	show_sensor_max_32,
-	show_sensor_max_33,
-	show_sensor_max_34,
-	show_sensor_max_35,
-	show_sensor_max_36,
-	show_sensor_max_37,
-	show_sensor_max_38,
-	show_sensor_max_39,
-	show_sensor_max_40,
-	show_sensor_max_41,
-	show_sensor_max_42,
-	show_sensor_max_43,
-	show_sensor_max_44,
-	show_sensor_max_45,
-	show_sensor_max_46,
-	show_sensor_max_47,
-	show_sensor_max_48,
-	show_sensor_max_49,
-	show_sensor_max_50,
-	show_sensor_max_51,
-	show_sensor_max_52,
-	show_sensor_max_53,
-	show_sensor_max_54,
-	show_sensor_max_55,
-	show_sensor_max_56,
-	show_sensor_max_57,
-	show_sensor_max_58,
-	show_sensor_max_59,
-	show_sensor_max_60,
-	show_sensor_max_61,
-	show_sensor_max_62,
-	show_sensor_max_63,
-	show_sensor_max_64,
-	show_sensor_max_65,
-	show_sensor_max_66,
-	show_sensor_max_67,
-	show_sensor_max_68,
-	show_sensor_max_69,
-	show_sensor_max_70,
-	show_sensor_max_71,
-	show_sensor_max_72,
-	show_sensor_max_73,
-	show_sensor_max_74,
-	show_sensor_max_75,
-	show_sensor_max_76,
-	show_sensor_max_77,
-	show_sensor_max_78,
-	show_sensor_max_79,
-	show_sensor_max_80,
-	show_sensor_max_81,
-	show_sensor_max_82,
-	show_sensor_max_83,
-	show_sensor_max_84,
-	show_sensor_max_85,
-	show_sensor_max_86,
-	show_sensor_max_87,
-	show_sensor_max_88,
-	show_sensor_max_89,
-	show_sensor_max_90,
-	show_sensor_max_91,
-	show_sensor_max_92,
-	show_sensor_max_93,
-	show_sensor_max_94,
-	show_sensor_max_95,
-	show_sensor_max_96,
-	show_sensor_max_97,
-	show_sensor_max_98,
-	show_sensor_max_99,
-};
-
-static ssize_t show_sensor_min(struct device *dev, char *buf, int id)
+static ssize_t show_sensor_min(struct device *dev, char *buf, void *data)
 {
 	long min = 0;
+	struct sdrdata *sdrd = ((struct sdrdata *)data);
 
-	if (sdrd[id].lim2 >= 0)
-		min = conv_val(sdrd[id].limits[sdrd[id].lim2], &sdrd[id]);
+	if (sdrd->lim2 >= 0)
+		min = conv_val(sdrd->limits[sdrd->lim2], sdrd);
 	return snprintf(buf, 20, "%ld\n", min);
 };
 
-#define show_sensor_min_id(id) \
-static ssize_t show_sensor_min_##id (struct device *dev, char *buf) \
-{ \
-	return show_sensor_min(dev,buf,id); \
-};
-
-show_sensor_min_id(0);
-show_sensor_min_id(1);
-show_sensor_min_id(2);
-show_sensor_min_id(3);
-show_sensor_min_id(4);
-show_sensor_min_id(5);
-show_sensor_min_id(6);
-show_sensor_min_id(7);
-show_sensor_min_id(8);
-show_sensor_min_id(9);
-show_sensor_min_id(10);
-show_sensor_min_id(11);
-show_sensor_min_id(12);
-show_sensor_min_id(13);
-show_sensor_min_id(14);
-show_sensor_min_id(15);
-show_sensor_min_id(16);
-show_sensor_min_id(17);
-show_sensor_min_id(18);
-show_sensor_min_id(19);
-show_sensor_min_id(20);
-show_sensor_min_id(21);
-show_sensor_min_id(22);
-show_sensor_min_id(23);
-show_sensor_min_id(24);
-show_sensor_min_id(25);
-show_sensor_min_id(26);
-show_sensor_min_id(27);
-show_sensor_min_id(28);
-show_sensor_min_id(29);
-show_sensor_min_id(30);
-show_sensor_min_id(31);
-show_sensor_min_id(32);
-show_sensor_min_id(33);
-show_sensor_min_id(34);
-show_sensor_min_id(35);
-show_sensor_min_id(36);
-show_sensor_min_id(37);
-show_sensor_min_id(38);
-show_sensor_min_id(39);
-show_sensor_min_id(40);
-show_sensor_min_id(41);
-show_sensor_min_id(42);
-show_sensor_min_id(43);
-show_sensor_min_id(44);
-show_sensor_min_id(45);
-show_sensor_min_id(46);
-show_sensor_min_id(47);
-show_sensor_min_id(48);
-show_sensor_min_id(49);
-show_sensor_min_id(50);
-show_sensor_min_id(51);
-show_sensor_min_id(52);
-show_sensor_min_id(53);
-show_sensor_min_id(54);
-show_sensor_min_id(55);
-show_sensor_min_id(56);
-show_sensor_min_id(57);
-show_sensor_min_id(58);
-show_sensor_min_id(59);
-show_sensor_min_id(60);
-show_sensor_min_id(61);
-show_sensor_min_id(62);
-show_sensor_min_id(63);
-show_sensor_min_id(64);
-show_sensor_min_id(65);
-show_sensor_min_id(66);
-show_sensor_min_id(67);
-show_sensor_min_id(68);
-show_sensor_min_id(69);
-show_sensor_min_id(70);
-show_sensor_min_id(71);
-show_sensor_min_id(72);
-show_sensor_min_id(73);
-show_sensor_min_id(74);
-show_sensor_min_id(75);
-show_sensor_min_id(76);
-show_sensor_min_id(77);
-show_sensor_min_id(78);
-show_sensor_min_id(79);
-show_sensor_min_id(80);
-show_sensor_min_id(81);
-show_sensor_min_id(82);
-show_sensor_min_id(83);
-show_sensor_min_id(84);
-show_sensor_min_id(85);
-show_sensor_min_id(86);
-show_sensor_min_id(87);
-show_sensor_min_id(88);
-show_sensor_min_id(89);
-show_sensor_min_id(90);
-show_sensor_min_id(91);
-show_sensor_min_id(92);
-show_sensor_min_id(93);
-show_sensor_min_id(94);
-show_sensor_min_id(95);
-show_sensor_min_id(96);
-show_sensor_min_id(97);
-show_sensor_min_id(98);
-show_sensor_min_id(99);
-
-static show_callback show_min_fcn[] = {
-	show_sensor_min_0,
-	show_sensor_min_1,
-	show_sensor_min_2,
-	show_sensor_min_3,
-	show_sensor_min_4,
-	show_sensor_min_5,
-	show_sensor_min_6,
-	show_sensor_min_7,
-	show_sensor_min_8,
-	show_sensor_min_9,
-	show_sensor_min_10,
-	show_sensor_min_11,
-	show_sensor_min_12,
-	show_sensor_min_13,
-	show_sensor_min_14,
-	show_sensor_min_15,
-	show_sensor_min_16,
-	show_sensor_min_17,
-	show_sensor_min_18,
-	show_sensor_min_19,
-	show_sensor_min_20,
-	show_sensor_min_21,
-	show_sensor_min_22,
-	show_sensor_min_23,
-	show_sensor_min_24,
-	show_sensor_min_25,
-	show_sensor_min_26,
-	show_sensor_min_27,
-	show_sensor_min_28,
-	show_sensor_min_29,
-	show_sensor_min_30,
-	show_sensor_min_31,
-	show_sensor_min_32,
-	show_sensor_min_33,
-	show_sensor_min_34,
-	show_sensor_min_35,
-	show_sensor_min_36,
-	show_sensor_min_37,
-	show_sensor_min_38,
-	show_sensor_min_39,
-	show_sensor_min_40,
-	show_sensor_min_41,
-	show_sensor_min_42,
-	show_sensor_min_43,
-	show_sensor_min_44,
-	show_sensor_min_45,
-	show_sensor_min_46,
-	show_sensor_min_47,
-	show_sensor_min_48,
-	show_sensor_min_49,
-	show_sensor_min_50,
-	show_sensor_min_51,
-	show_sensor_min_52,
-	show_sensor_min_53,
-	show_sensor_min_54,
-	show_sensor_min_55,
-	show_sensor_min_56,
-	show_sensor_min_57,
-	show_sensor_min_58,
-	show_sensor_min_59,
-	show_sensor_min_60,
-	show_sensor_min_61,
-	show_sensor_min_62,
-	show_sensor_min_63,
-	show_sensor_min_64,
-	show_sensor_min_65,
-	show_sensor_min_66,
-	show_sensor_min_67,
-	show_sensor_min_68,
-	show_sensor_min_69,
-	show_sensor_min_70,
-	show_sensor_min_71,
-	show_sensor_min_72,
-	show_sensor_min_73,
-	show_sensor_min_74,
-	show_sensor_min_75,
-	show_sensor_min_76,
-	show_sensor_min_77,
-	show_sensor_min_78,
-	show_sensor_min_79,
-	show_sensor_min_80,
-	show_sensor_min_81,
-	show_sensor_min_82,
-	show_sensor_min_83,
-	show_sensor_min_84,
-	show_sensor_min_85,
-	show_sensor_min_86,
-	show_sensor_min_87,
-	show_sensor_min_88,
-	show_sensor_min_89,
-	show_sensor_min_90,
-	show_sensor_min_91,
-	show_sensor_min_92,
-	show_sensor_min_93,
-	show_sensor_min_94,
-	show_sensor_min_95,
-	show_sensor_min_96,
-	show_sensor_min_97,
-	show_sensor_min_98,
-	show_sensor_min_99,
-};
-
-
-static ssize_t show_sensor_label(struct device *dev, char *buf, int i)
+static ssize_t show_sensor_label(struct device *dev, char *buf, void *data)
 {
+	struct sdrdata *sdrd = ((struct sdrdata *)data);
 	u8 label[SDR_MAX_UNPACKED_ID_LENGTH];
-	ipmi_sprintf(label, sdrd[i].id, sdrd[i].string_type, sdrd[i].id_length);
+	ipmi_sprintf(label, sdrd->id, sdrd->string_type, sdrd->id_length);
 	return snprintf(buf, 20, "%s\n", label);
 };
 
-#define show_sensor_label_id(i) \
-static ssize_t show_sensor_label_##i (struct device *dev, char *buf) \
-{ \
-    return show_sensor_label(dev,buf,i);\
-};
-
-show_sensor_label_id(0);
-show_sensor_label_id(1);
-show_sensor_label_id(2);
-show_sensor_label_id(3);
-show_sensor_label_id(4);
-show_sensor_label_id(5);
-show_sensor_label_id(6);
-show_sensor_label_id(7);
-show_sensor_label_id(8);
-show_sensor_label_id(9);
-show_sensor_label_id(10);
-show_sensor_label_id(11);
-show_sensor_label_id(12);
-show_sensor_label_id(13);
-show_sensor_label_id(14);
-show_sensor_label_id(15);
-show_sensor_label_id(16);
-show_sensor_label_id(17);
-show_sensor_label_id(18);
-show_sensor_label_id(19);
-show_sensor_label_id(20);
-show_sensor_label_id(21);
-show_sensor_label_id(22);
-show_sensor_label_id(23);
-show_sensor_label_id(24);
-show_sensor_label_id(25);
-show_sensor_label_id(26);
-show_sensor_label_id(27);
-show_sensor_label_id(28);
-show_sensor_label_id(29);
-show_sensor_label_id(30);
-show_sensor_label_id(31);
-show_sensor_label_id(32);
-show_sensor_label_id(33);
-show_sensor_label_id(34);
-show_sensor_label_id(35);
-show_sensor_label_id(36);
-show_sensor_label_id(37);
-show_sensor_label_id(38);
-show_sensor_label_id(39);
-show_sensor_label_id(40);
-show_sensor_label_id(41);
-show_sensor_label_id(42);
-show_sensor_label_id(43);
-show_sensor_label_id(44);
-show_sensor_label_id(45);
-show_sensor_label_id(46);
-show_sensor_label_id(47);
-show_sensor_label_id(48);
-show_sensor_label_id(49);
-show_sensor_label_id(50);
-show_sensor_label_id(51);
-show_sensor_label_id(52);
-show_sensor_label_id(53);
-show_sensor_label_id(54);
-show_sensor_label_id(55);
-show_sensor_label_id(56);
-show_sensor_label_id(57);
-show_sensor_label_id(58);
-show_sensor_label_id(59);
-show_sensor_label_id(60);
-show_sensor_label_id(61);
-show_sensor_label_id(62);
-show_sensor_label_id(63);
-show_sensor_label_id(64);
-show_sensor_label_id(65);
-show_sensor_label_id(66);
-show_sensor_label_id(67);
-show_sensor_label_id(68);
-show_sensor_label_id(69);
-show_sensor_label_id(70);
-show_sensor_label_id(71);
-show_sensor_label_id(72);
-show_sensor_label_id(73);
-show_sensor_label_id(74);
-show_sensor_label_id(75);
-show_sensor_label_id(76);
-show_sensor_label_id(77);
-show_sensor_label_id(78);
-show_sensor_label_id(79);
-show_sensor_label_id(80);
-show_sensor_label_id(81);
-show_sensor_label_id(82);
-show_sensor_label_id(83);
-show_sensor_label_id(84);
-show_sensor_label_id(85);
-show_sensor_label_id(86);
-show_sensor_label_id(87);
-show_sensor_label_id(88);
-show_sensor_label_id(89);
-show_sensor_label_id(90);
-show_sensor_label_id(91);
-show_sensor_label_id(92);
-show_sensor_label_id(93);
-show_sensor_label_id(94);
-show_sensor_label_id(95);
-show_sensor_label_id(96);
-show_sensor_label_id(97);
-show_sensor_label_id(98);
-show_sensor_label_id(99);
-
-static show_callback show_label_fcn[] = {
-	show_sensor_label_0,
-	show_sensor_label_1,
-	show_sensor_label_2,
-	show_sensor_label_3,
-	show_sensor_label_4,
-	show_sensor_label_5,
-	show_sensor_label_6,
-	show_sensor_label_7,
-	show_sensor_label_8,
-	show_sensor_label_9,
-	show_sensor_label_10,
-	show_sensor_label_11,
-	show_sensor_label_12,
-	show_sensor_label_13,
-	show_sensor_label_14,
-	show_sensor_label_15,
-	show_sensor_label_16,
-	show_sensor_label_17,
-	show_sensor_label_18,
-	show_sensor_label_19,
-	show_sensor_label_20,
-	show_sensor_label_21,
-	show_sensor_label_22,
-	show_sensor_label_23,
-	show_sensor_label_24,
-	show_sensor_label_25,
-	show_sensor_label_26,
-	show_sensor_label_27,
-	show_sensor_label_28,
-	show_sensor_label_29,
-	show_sensor_label_30,
-	show_sensor_label_31,
-	show_sensor_label_32,
-	show_sensor_label_33,
-	show_sensor_label_34,
-	show_sensor_label_35,
-	show_sensor_label_36,
-	show_sensor_label_37,
-	show_sensor_label_38,
-	show_sensor_label_39,
-	show_sensor_label_40,
-	show_sensor_label_41,
-	show_sensor_label_42,
-	show_sensor_label_43,
-	show_sensor_label_44,
-	show_sensor_label_45,
-	show_sensor_label_46,
-	show_sensor_label_47,
-	show_sensor_label_48,
-	show_sensor_label_49,
-	show_sensor_label_50,
-	show_sensor_label_51,
-	show_sensor_label_52,
-	show_sensor_label_53,
-	show_sensor_label_54,
-	show_sensor_label_55,
-	show_sensor_label_56,
-	show_sensor_label_57,
-	show_sensor_label_58,
-	show_sensor_label_59,
-	show_sensor_label_60,
-	show_sensor_label_61,
-	show_sensor_label_62,
-	show_sensor_label_63,
-	show_sensor_label_64,
-	show_sensor_label_65,
-	show_sensor_label_66,
-	show_sensor_label_67,
-	show_sensor_label_68,
-	show_sensor_label_69,
-	show_sensor_label_70,
-	show_sensor_label_71,
-	show_sensor_label_72,
-	show_sensor_label_73,
-	show_sensor_label_74,
-	show_sensor_label_75,
-	show_sensor_label_76,
-	show_sensor_label_77,
-	show_sensor_label_78,
-	show_sensor_label_79,
-	show_sensor_label_80,
-	show_sensor_label_81,
-	show_sensor_label_82,
-	show_sensor_label_83,
-	show_sensor_label_84,
-	show_sensor_label_85,
-	show_sensor_label_86,
-	show_sensor_label_87,
-	show_sensor_label_88,
-	show_sensor_label_89,
-	show_sensor_label_90,
-	show_sensor_label_91,
-	show_sensor_label_92,
-	show_sensor_label_93,
-	show_sensor_label_94,
-	show_sensor_label_95,
-	show_sensor_label_96,
-	show_sensor_label_97,
-	show_sensor_label_98,
-	show_sensor_label_99,
-};
-
-static ssize_t store_max(struct device *dev, const char *buf, size_t count,
-			 int id)
+static ssize_t store_sensor_max(struct device *dev, const char *buf, size_t count,
+			 void *data)
 {
+	struct sdrdata *sdrd = ((struct sdrdata *)data);
 	long val = simple_strtoul(buf, NULL, 10);
 #ifdef DEBUG
-	printk(KERN_INFO "bmcsensors.o: set max on sensor #%d to %ld", id, val);
+	printk(KERN_INFO "bmcsensors.o: set max on sensor #%d to %ld", sdrd->id, val);
 #endif
-	bmcsensors_set_sensor_threshold(sdrd[id].number, val, sdrd[id].lim1);
+	bmcsensors_set_sensor_threshold(sdrd->number, val, sdrd->lim1);
 	return count;
 };
 
-#define store_max_id(i) \
-static ssize_t store_max_##i (struct device *dev, const char *buf, size_t count) \
-{ \
-    return store_max(dev,buf,count, i);\
-};
-
-store_max_id(0);
-store_max_id(1);
-store_max_id(2);
-store_max_id(3);
-store_max_id(4);
-store_max_id(5);
-store_max_id(6);
-store_max_id(7);
-store_max_id(8);
-store_max_id(9);
-store_max_id(10);
-store_max_id(11);
-store_max_id(12);
-store_max_id(13);
-store_max_id(14);
-store_max_id(15);
-store_max_id(16);
-store_max_id(17);
-store_max_id(18);
-store_max_id(19);
-store_max_id(20);
-store_max_id(21);
-store_max_id(22);
-store_max_id(23);
-store_max_id(24);
-store_max_id(25);
-store_max_id(26);
-store_max_id(27);
-store_max_id(28);
-store_max_id(29);
-store_max_id(30);
-store_max_id(31);
-store_max_id(32);
-store_max_id(33);
-store_max_id(34);
-store_max_id(35);
-store_max_id(36);
-store_max_id(37);
-store_max_id(38);
-store_max_id(39);
-store_max_id(40);
-store_max_id(41);
-store_max_id(42);
-store_max_id(43);
-store_max_id(44);
-store_max_id(45);
-store_max_id(46);
-store_max_id(47);
-store_max_id(48);
-store_max_id(49);
-store_max_id(50);
-store_max_id(51);
-store_max_id(52);
-store_max_id(53);
-store_max_id(54);
-store_max_id(55);
-store_max_id(56);
-store_max_id(57);
-store_max_id(58);
-store_max_id(59);
-store_max_id(60);
-store_max_id(61);
-store_max_id(62);
-store_max_id(63);
-store_max_id(64);
-store_max_id(65);
-store_max_id(66);
-store_max_id(67);
-store_max_id(68);
-store_max_id(69);
-store_max_id(70);
-store_max_id(71);
-store_max_id(72);
-store_max_id(73);
-store_max_id(74);
-store_max_id(75);
-store_max_id(76);
-store_max_id(77);
-store_max_id(78);
-store_max_id(79);
-store_max_id(80);
-store_max_id(81);
-store_max_id(82);
-store_max_id(83);
-store_max_id(84);
-store_max_id(85);
-store_max_id(86);
-store_max_id(87);
-store_max_id(88);
-store_max_id(89);
-store_max_id(90);
-store_max_id(91);
-store_max_id(92);
-store_max_id(93);
-store_max_id(94);
-store_max_id(95);
-store_max_id(96);
-store_max_id(97);
-store_max_id(98);
-store_max_id(99);
-
-static store_callback store_max_fcn[] = {
-	store_max_0,
-	store_max_1,
-	store_max_2,
-	store_max_3,
-	store_max_4,
-	store_max_5,
-	store_max_6,
-	store_max_7,
-	store_max_8,
-	store_max_9,
-	store_max_10,
-	store_max_11,
-	store_max_12,
-	store_max_13,
-	store_max_14,
-	store_max_15,
-	store_max_16,
-	store_max_17,
-	store_max_18,
-	store_max_19,
-	store_max_20,
-	store_max_21,
-	store_max_22,
-	store_max_23,
-	store_max_24,
-	store_max_25,
-	store_max_26,
-	store_max_27,
-	store_max_28,
-	store_max_29,
-	store_max_30,
-	store_max_31,
-	store_max_32,
-	store_max_33,
-	store_max_34,
-	store_max_35,
-	store_max_36,
-	store_max_37,
-	store_max_38,
-	store_max_39,
-	store_max_40,
-	store_max_41,
-	store_max_42,
-	store_max_43,
-	store_max_44,
-	store_max_45,
-	store_max_46,
-	store_max_47,
-	store_max_48,
-	store_max_49,
-	store_max_50,
-	store_max_51,
-	store_max_52,
-	store_max_53,
-	store_max_54,
-	store_max_55,
-	store_max_56,
-	store_max_57,
-	store_max_58,
-	store_max_59,
-	store_max_60,
-	store_max_61,
-	store_max_62,
-	store_max_63,
-	store_max_64,
-	store_max_65,
-	store_max_66,
-	store_max_67,
-	store_max_68,
-	store_max_69,
-	store_max_70,
-	store_max_71,
-	store_max_72,
-	store_max_73,
-	store_max_74,
-	store_max_75,
-	store_max_76,
-	store_max_77,
-	store_max_78,
-	store_max_79,
-	store_max_80,
-	store_max_81,
-	store_max_82,
-	store_max_83,
-	store_max_84,
-	store_max_85,
-	store_max_86,
-	store_max_87,
-	store_max_88,
-	store_max_89,
-	store_max_90,
-	store_max_91,
-	store_max_92,
-	store_max_93,
-	store_max_94,
-	store_max_95,
-	store_max_96,
-	store_max_97,
-	store_max_98,
-	store_max_99,
-};
-
-static ssize_t store_min(struct device *dev, const char *buf, size_t count,
-			 int id)
+static ssize_t store_sensor_min(struct device *dev, const char *buf, size_t count,
+			 void *data)
 {
+	struct sdrdata *sdrd = ((struct sdrdata *)data);
 	long val = simple_strtoul(buf, NULL, 10);
 #ifdef DEBUG
-	printk(KERN_INFO "bmcsensors.o: set min on sensor #%d to %ld", id, val);
+	printk(KERN_INFO "bmcsensors.o: set min on sensor #%d to %ld", sdrd->id, val);
 #endif
-	bmcsensors_set_sensor_threshold(sdrd[id].number, val, sdrd[id].lim2);
+	bmcsensors_set_sensor_threshold(sdrd->number, val, sdrd->lim2);
 	return count;
 };
 
-#define store_min_id(i) \
-static ssize_t store_min_##i (struct device *dev, const char *buf, size_t count) \
-{ \
-    return store_min(dev,buf,count, i);\
-};
-
-store_min_id(0);
-store_min_id(1);
-store_min_id(2);
-store_min_id(3);
-store_min_id(4);
-store_min_id(5);
-store_min_id(6);
-store_min_id(7);
-store_min_id(8);
-store_min_id(9);
-store_min_id(10);
-store_min_id(11);
-store_min_id(12);
-store_min_id(13);
-store_min_id(14);
-store_min_id(15);
-store_min_id(16);
-store_min_id(17);
-store_min_id(18);
-store_min_id(19);
-store_min_id(20);
-store_min_id(21);
-store_min_id(22);
-store_min_id(23);
-store_min_id(24);
-store_min_id(25);
-store_min_id(26);
-store_min_id(27);
-store_min_id(28);
-store_min_id(29);
-store_min_id(30);
-store_min_id(31);
-store_min_id(32);
-store_min_id(33);
-store_min_id(34);
-store_min_id(35);
-store_min_id(36);
-store_min_id(37);
-store_min_id(38);
-store_min_id(39);
-store_min_id(40);
-store_min_id(41);
-store_min_id(42);
-store_min_id(43);
-store_min_id(44);
-store_min_id(45);
-store_min_id(46);
-store_min_id(47);
-store_min_id(48);
-store_min_id(49);
-store_min_id(50);
-store_min_id(51);
-store_min_id(52);
-store_min_id(53);
-store_min_id(54);
-store_min_id(55);
-store_min_id(56);
-store_min_id(57);
-store_min_id(58);
-store_min_id(59);
-store_min_id(60);
-store_min_id(61);
-store_min_id(62);
-store_min_id(63);
-store_min_id(64);
-store_min_id(65);
-store_min_id(66);
-store_min_id(67);
-store_min_id(68);
-store_min_id(69);
-store_min_id(70);
-store_min_id(71);
-store_min_id(72);
-store_min_id(73);
-store_min_id(74);
-store_min_id(75);
-store_min_id(76);
-store_min_id(77);
-store_min_id(78);
-store_min_id(79);
-store_min_id(80);
-store_min_id(81);
-store_min_id(82);
-store_min_id(83);
-store_min_id(84);
-store_min_id(85);
-store_min_id(86);
-store_min_id(87);
-store_min_id(88);
-store_min_id(89);
-store_min_id(90);
-store_min_id(91);
-store_min_id(92);
-store_min_id(93);
-store_min_id(94);
-store_min_id(95);
-store_min_id(96);
-store_min_id(97);
-store_min_id(98);
-store_min_id(99);
-
-static store_callback store_min_fcn[] = {
-	store_min_0,
-	store_min_1,
-	store_min_2,
-	store_min_3,
-	store_min_4,
-	store_min_5,
-	store_min_6,
-	store_min_7,
-	store_min_8,
-	store_min_9,
-	store_min_10,
-	store_min_11,
-	store_min_12,
-	store_min_13,
-	store_min_14,
-	store_min_15,
-	store_min_16,
-	store_min_17,
-	store_min_18,
-	store_min_19,
-	store_min_20,
-	store_min_21,
-	store_min_22,
-	store_min_23,
-	store_min_24,
-	store_min_25,
-	store_min_26,
-	store_min_27,
-	store_min_28,
-	store_min_29,
-	store_min_30,
-	store_min_31,
-	store_min_32,
-	store_min_33,
-	store_min_34,
-	store_min_35,
-	store_min_36,
-	store_min_37,
-	store_min_38,
-	store_min_39,
-	store_min_40,
-	store_min_41,
-	store_min_42,
-	store_min_43,
-	store_min_44,
-	store_min_45,
-	store_min_46,
-	store_min_47,
-	store_min_48,
-	store_min_49,
-	store_min_50,
-	store_min_51,
-	store_min_52,
-	store_min_53,
-	store_min_54,
-	store_min_55,
-	store_min_56,
-	store_min_57,
-	store_min_58,
-	store_min_59,
-	store_min_60,
-	store_min_61,
-	store_min_62,
-	store_min_63,
-	store_min_64,
-	store_min_65,
-	store_min_66,
-	store_min_67,
-	store_min_68,
-	store_min_69,
-	store_min_70,
-	store_min_71,
-	store_min_72,
-	store_min_73,
-	store_min_74,
-	store_min_75,
-	store_min_76,
-	store_min_77,
-	store_min_78,
-	store_min_79,
-	store_min_80,
-	store_min_81,
-	store_min_82,
-	store_min_83,
-	store_min_84,
-	store_min_85,
-	store_min_86,
-	store_min_87,
-	store_min_88,
-	store_min_89,
-	store_min_90,
-	store_min_91,
-	store_min_92,
-	store_min_93,
-	store_min_94,
-	store_min_95,
-	store_min_96,
-	store_min_97,
-	store_min_98,
-	store_min_99,
-};
-
-static ssize_t show_alarms(struct device *dev, char *buf)
+static ssize_t show_alarms(struct device *dev, char *buf, void *data)
 {
 	bmcsensors_update_client(&bmc_client);
 	return snprintf(buf, 20, "%d\n", bmc_data.alarms);
@@ -1703,6 +436,8 @@ static void bmcsensors_build_sysfs()
 			    (struct device_attribute *)
 			    kmalloc(sizeof(struct device_attribute),
 				    GFP_KERNEL);
+			sysfs_attribute_label->data = &sdrd[i];
+			
 			if (sysfs_attribute_label = NULL
 			    || sensor_label = NULL) {
 				printk(KERN_INFO
@@ -1777,15 +512,17 @@ static void bmcsensors_build_sysfs()
 		sysfs_attribute->attr.name = sensor_name;
 		sysfs_attribute->attr.mode = S_IRUGO;
 		sysfs_attribute->attr.owner = THIS_MODULE;
-		sysfs_attribute->show = show_fcn[i];
+		sysfs_attribute->show = show_sensor;
 		sysfs_attribute->store = NULL;
-
+		sysfs_attribute->data = &sdrd[i];
+		
 		sysfs_attribute_min->attr.name = sensor_minname;
 		sysfs_attribute_min->attr.owner = THIS_MODULE;
-		sysfs_attribute_min->show = show_min_fcn[i];
+		sysfs_attribute_min->show = show_sensor_min;
+		sysfs_attribute_min->data = &sdrd[i];
 
 		if (sdrd[i].lim2_write) {
-			sysfs_attribute_min->store = store_min_fcn[i];
+			sysfs_attribute_min->store = store_sensor_min;
 			sysfs_attribute_min->attr.mode = S_IWUSR | S_IRUGO;
 		} else {
 			sysfs_attribute_min->store = NULL;
@@ -1793,12 +530,12 @@ static void bmcsensors_build_sysfs()
 		}
 
 		sysfs_attribute_max->attr.name = sensor_maxname;
-
 		sysfs_attribute_max->attr.owner = THIS_MODULE;
-		sysfs_attribute_max->show = show_max_fcn[i];
+		sysfs_attribute_max->show = show_sensor_max;
+		sysfs_attribute_max->data = &sdrd[i];
 
 		if (sdrd[i].lim1_write) {
-			sysfs_attribute_max->store = store_max_fcn[i];
+			sysfs_attribute_max->store = store_sensor_max;
 			sysfs_attribute_max->attr.mode = S_IWUSR | S_IRUGO;
 		} else {
 			sysfs_attribute_max->store = NULL;
@@ -1809,7 +546,7 @@ static void bmcsensors_build_sysfs()
 			sysfs_attribute_label->attr.name = sensor_label;
 			sysfs_attribute_label->attr.mode = S_IRUGO;
 			sysfs_attribute_label->attr.owner = THIS_MODULE;
-			sysfs_attribute_label->show = show_label_fcn[i];
+			sysfs_attribute_label->show = show_sensor_label;
 			sysfs_attribute_label->store = NULL;
 		}
 

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Thu, Apr 28, 2005 at 02:44:36AM -0400, Yani Ioannou wrote:
> On 4/26/05, Greg KH <greg@kroah.com> wrote:
> > Care to show how a driver would use this change?
> >
> > And the void * shouldn't be called ptr, use what other structures call
> > their void pointers, "data", "private", etc.
> 
> Attached is an updated patch (yes ptr is rather redundant...) and also
> a quickly coded diff between my bmcsensors driver with and without
> this patch. Note that a real bmcsensors update would also change the
> sdrd[] array into a linked list and allow a potentially unlimited
> number of sensors to be seen, since this patch also allows an
> unlimited number of sysfs entries to be created at runtime, not
> feasible with static callbacks.

Ok, you are showing how this works with raw sysfs attributes, that's
simple.  But you should be using device attributes, how would this patch
work with them?

And how would it work with the 99% of the kernel that doesn't create
attributes dynamically on the fly?

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Grant Coady]

Hi Greg,
On Wed, 27 Apr 2005 23:57:41 -0700, Greg KH <greg@kroah.com> wrote:

>
>And how would it work with the 99% of the kernel that doesn't create
>attributes dynamically on the fly?

Create an index attribute: write index value, read/write attribute 
data?  Nothing else need know, just the driver needing unbounded 
indexed attributes.

--Grant.

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Actually I was running a kernel with only bmcsensors changed to use
the void * - it should work transparently for all the other drivers
(except for a compilation warning about invalid show/store pointer
types being passed - and I have a perl script we can run against the
source tree to update drivers and get rid of those warnings).

The void * is completely optional of course, its up to the driver to
use it or not (unless I'm missing your question here?).

Yani

On 4/28/05, Grant Coady <grant_lkml@dodo.com.au> wrote:
> Hi Greg,
> On Wed, 27 Apr 2005 23:57:41 -0700, Greg KH <greg@kroah.com> wrote:
> 
> >
> >And how would it work with the 99% of the kernel that doesn't create
> >attributes dynamically on the fly?
> 
> Create an index attribute: write index value, read/write attribute
> data?  Nothing else need know, just the driver needing unbounded
> indexed attributes.
> 
> --Grant.
> 
>

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Dmitry Torokhov]

Hi Yani,

On 4/28/05, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> On 4/26/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > If void is added directly to the attribute structure that means that same
> > attribute can not be shared between several instances of the same device
> > -> unexpected. So far all attributes could be statically created.
> 
> Dmitry: would you mind explaining this a bit more? Also is there
> anywhere else you would suggest adding the callback specific void *
> to? Perhaps creating some new structure(s) to track driver specific
> data for each device?

Mosy of the code does something like that:

static DEVICE_ATTR(blah, 0644, blah_show, blah_store);
and then
         sysfs_create_file(mydev, &blah_device_attr);
or qute often install it as a default driver/bus/device attribute.

In this case one structure is shared between several instances of
driver model objects. With your changes people have to be aware that
they have to allocate attributes dynamically and that they can't use
bus/driver default attribute mechanism nor attribute_groups.

Actually I have toyed with attribute_array and attribute_array_group
for some time. Greg did not like it mostly because there were no
possible users. But it looks like bmcsensort and infiniband could use
attribute arrays, so maybe he'll change his mind. With my patches you
can make initial structure static, all synamic allocations go behind
the scene.

attr_array.patch:
http://marc.theaimsgroup.com/?l=linux-kernel&m\x111138239422686&q=p3

attr_group_array.patch:
http://marc.theaimsgroup.com/?l=linux-kernel&m\x111138239422686&q=p4

One can use them like this (i8k laptop has 2 fans, this is a legacy SMM driver):

static ssize_t i8k_sysfs_fan_speed_show(struct kobject *kobj, unsigned
int idx, char *buf)
 {
	int speed = i8k_get_fan_speed(idx);
	return speed < 0 ? -EIO : sprintf(buf, "%d\n", speed);
}

static struct enumerated_attr i8k_fan_attrs[] = {
	__ATTR(state, 0644, i8k_sysfs_fan_show, i8k_sysfs_fan_set),
	__ATTR(speed, 0444, i8k_sysfs_fan_speed_show, NULL),
	__ATTR_NULL
};

static struct attribute_group_array i8k_fan_attr_array = {
	.name = "fan",
	.attrs = i8k_fan_attrs,
};
..
err = sysfs_create_group_array(&i8k_device->dev.kobj, &i8k_fan_attr_array, num);

-- 
Dmitry

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Thu, Apr 28, 2005 at 05:53:51PM +1000, Grant Coady wrote:
> Hi Greg,
> On Wed, 27 Apr 2005 23:57:41 -0700, Greg KH <greg@kroah.com> wrote:
> 
> >
> >And how would it work with the 99% of the kernel that doesn't create
> >attributes dynamically on the fly?
> 
> Create an index attribute: write index value, read/write attribute 
> data?  Nothing else need know, just the driver needing unbounded 
> indexed attributes.

Sorry, I don't see the patch you must have forgotten to apply to the
email, showing how you would do this...

:)

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Thu, Apr 28, 2005 at 09:45:38AM -0400, Yani Ioannou wrote:
> Actually I was running a kernel with only bmcsensors changed to use
> the void * - it should work transparently for all the other drivers
> (except for a compilation warning about invalid show/store pointer
> types being passed - and I have a perl script we can run against the
> source tree to update drivers and get rid of those warnings).
> 
> The void * is completely optional of course, its up to the driver to
> use it or not (unless I'm missing your question here?).

My question is, please show me how you propagate this out to the driver
core code, so that the drivers that use the DEVICE_ATTRIBUTE() stuff can
take advantage of this.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 4/28/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Yani,
> 
> On 4/28/05, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> > On 4/26/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > If void is added directly to the attribute structure that means that same
> > > attribute can not be shared between several instances of the same device
> > > -> unexpected. So far all attributes could be statically created.
> >
> > Dmitry: would you mind explaining this a bit more? Also is there
> > anywhere else you would suggest adding the callback specific void *
> > to? Perhaps creating some new structure(s) to track driver specific
> > data for each device?
> 
> Mosy of the code does something like that:
> 
> static DEVICE_ATTR(blah, 0644, blah_show, blah_store);
> and then
>          sysfs_create_file(mydev, &blah_device_attr);
> or qute often install it as a default driver/bus/device attribute.
> 
> In this case one structure is shared between several instances of
> driver model objects. With your changes people have to be aware that
> they have to allocate attributes dynamically and that they can't use
> bus/driver default attribute mechanism nor attribute_groups.
> 
> Actually I have toyed with attribute_array and attribute_array_group
> for some time. Greg did not like it mostly because there were no
> possible users. But it looks like bmcsensort and infiniband could use
> attribute arrays, so maybe he'll change his mind. With my patches you
> can make initial structure static, all synamic allocations go behind
> the scene.
> 
> attr_array.patch:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x111138239422686&q=p3
> 
> attr_group_array.patch:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x111138239422686&q=p4
> 
> One can use them like this (i8k laptop has 2 fans, this is a legacy SMM driver):
> 
> static ssize_t i8k_sysfs_fan_speed_show(struct kobject *kobj, unsigned
> int idx, char *buf)
>  {
>         int speed = i8k_get_fan_speed(idx);
>         return speed < 0 ? -EIO : sprintf(buf, "%d\n", speed);
> }
> 
> static struct enumerated_attr i8k_fan_attrs[] = {
>         __ATTR(state, 0644, i8k_sysfs_fan_show, i8k_sysfs_fan_set),
>         __ATTR(speed, 0444, i8k_sysfs_fan_speed_show, NULL),
>         __ATTR_NULL
> };
> 
> static struct attribute_group_array i8k_fan_attr_array = {
>         .name = "fan",
>         .attrs = i8k_fan_attrs,
> };
> ...
> err = sysfs_create_group_array(&i8k_device->dev.kobj, &i8k_fan_attr_array, num);
> 
> --
> Dmitry
> 
Hi Dmitry,

Yes attribute groups would certainly aid bmcsensors in terms of
getting rid of the static callbacks and replacing them with a few,
however in using an array it still has the limitation that bmcsensors
would have to set a limit on the number of sensors that it can see,
and in doing so allocate space that likely will never be used (most
ipmi bmcs have access to 10-40 sensors, but the odd few on high end
servers from people I've been in contact with have at least 60, and so
the current limit is 100).

Sorry my example bmcsensors diff was misleading in that it still used
an array, but a proper patch would change the array to a linked list.
The void * callback is a bit more flexible (and simpler) in that it
allows us to create an unlimited and non-predetermined number of sysfs
entries.

Thanks,
Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Dmitry Torokhov]

On 4/28/05, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> On 4/28/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > ...
> > err = sysfs_create_group_array(&i8k_device->dev.kobj, &i8k_fan_attr_array, num);
> >
> 
> Yes attribute groups would certainly aid bmcsensors in terms of
> getting rid of the static callbacks and replacing them with a few,
> however in using an array it still has the limitation that bmcsensors
> would have to set a limit on the number of sensors that it can see,
> and in doing so allocate space that likely will never be used (most
> ipmi bmcs have access to 10-40 sensors, but the odd few on high end
> servers from people I've been in contact with have at least 60, and so
> the current limit is 100).

Number of elements in an array is specified at creation time (3rd
argument to sysfs_create_group_array) so you do not have to set an
arbitrary limit. Interrogate your device during ->probe() and adjust
the limit accordingly.

Or maybe I still miss something?

-- 
Dmitry

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

> My question is, please show me how you propagate this out to the driver
> core code, so that the drivers that use the DEVICE_ATTRIBUTE() stuff can
> take advantage of this.
> 
> thanks,
> 
> greg k-h
> 

Well bmcsensors (before this patch) was creating device attributes
like the other chip drivers, just not explicitly using the
DEVICE_ATTRIBUTE macro, however I can easily modify one of the other
sensor chip drivers to show you how they might take advantage of this
if that what you are looking for? I'll probably have to do it tonight
after I get home though :-).

I was thinking of modifying the DEVICE_ATTRIBUTE macro to set the void
* pointer too, but decided against it for the moment...

Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Thu, Apr 28, 2005 at 01:25:29PM -0400, Yani Ioannou wrote:
> > My question is, please show me how you propagate this out to the driver
> > core code, so that the drivers that use the DEVICE_ATTRIBUTE() stuff can
> > take advantage of this.
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> Well bmcsensors (before this patch) was creating device attributes
> like the other chip drivers, just not explicitly using the
> DEVICE_ATTRIBUTE macro, however I can easily modify one of the other
> sensor chip drivers to show you how they might take advantage of this
> if that what you are looking for? I'll probably have to do it tonight
> after I get home though :-).

Yes, that is what I was looking for.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 4/28/05, Greg KH <greg@kroah.com> wrote:
> On Thu, Apr 28, 2005 at 01:25:29PM -0400, Yani Ioannou wrote:
> > > My question is, please show me how you propagate this out to the driver
> > > core code, so that the drivers that use the DEVICE_ATTRIBUTE() stuff can
> > > take advantage of this.
> > >
> > > thanks,
> > >
> > > greg k-h
> > >
> >
> > Well bmcsensors (before this patch) was creating device attributes
> > like the other chip drivers, just not explicitly using the
> > DEVICE_ATTRIBUTE macro, however I can easily modify one of the other
> > sensor chip drivers to show you how they might take advantage of this
> > if that what you are looking for? I'll probably have to do it tonight
> > after I get home though :-).
> 
> Yes, that is what I was looking for.
> 
> thanks,
> 
> greg k-h
> 

Hi, sorry for the delay. 

Obviously the drivers with more device attributes will benefit the
most from the patch (and in the extreme case of unlimited, like
bmcsensors), so I chose to demonstrate a patch for adm1026.c which
seems to have one of the largest number of attributes for the i2c
chips.

I've included a modified version of the previous dynamic callback
patch where I defined a DEVICE_ATTR_DATA macro which becomes useful in
these cases, likely in any real patch this would be rolled into the
DEVICE_ATTR macro but for simplicity (i.e. not breaking everything
else that uses DEVICE_ATTR out there) I'm using a separate one for
now.

The second attached patch simply modifies the driver to avoid compiler
warnings from using invalid show/store function pointers - this is the
bare minimum change a driver would need to make (although leaving the
warnings doesn't affect anything functionally I for one hate seeing
warnings on a kernel compile).

The third patch shows how we could get rid of the macro defined hoard
of static sysfs callbacks in the easiest and probably the best way -
define ints for indexes and pass pointers to them as void *data. This
compiles and should work fine (sorry can't test this since I don't
have this chip). Loading the module though the size difference is
again apparent:

-------------------2.6.11.7--------------------
Module                  Size  Used by
adm1026                44692  0
--------2.6.12-rc3-devdyncallback-----
Module                  Size  Used by
adm1026                33172  0


You can see in this example why Jean is not convinced that void * is
better than passing an int, in the case of the i2c chip drivers the
majority seem to do something along these lines, and really an int
suffices. In the grand scheme of things though I think the void * is
much more flexible.

Taking things even further you might restructure the the adm1026_data
structure and group together the related sensor attributes so that we
can pass a pointer to a data structure instead of the int index,
something like:

struct in_data {
	int id;
	u8 value;            /* Register value */
	u8 max;              /* Register value */
	u8 min;              /* Register value */
};

struct fan_data {
	int id;
	u8 value;           /* Register value */
	u8 min;             /* Register value */
	u8 div;             /* Decoded value */
};


struct temp_data {
	int id;
	s8 value;          /* Register value */
	s8 min;            /* Register value */
	s8 max;            /* Register value */
	s8 tmin;           /* Register value */
	s8 crit;           /* Register value */
	s8 offset;         /* Register value */	
};

struct adm1026_data {
	...
	struct in_data in[17];     /* Volt Sensor Data */
	struct fan_data fanp[8];     /* Fan Sensor Data */
	struct temp_data temp[3];    /* Temp Sensor Data */
	struct pwm_data pwm1;   /* Pwm control values */

	/* Voltage sensor device attributes */
	struct device_attribute in_reg_attr[17];
	struct device_attribute in_min_attr[17];
	struct device_attribute in_max_attr[17];
	
	/* Fan sensor device attributes */
	struct device_attribute fan_input_attr[8];
	struct device_attribute fan_min_attr[8];
	...
}

We would then be able to just use the passed data structure (e.g.
temp_data[offset] for a temp attribute callback) in the sysfs callback
as in the bmcsensors patch.

This won't work with static device_attributes like adm1026 uses
because we encounter the problem Dmitry described - adm1026 allows
multiple i2c_clients each with it's own adm1026_data structure, but
all of them would share the same static device_attribute. The
integer/index passed data above works because the adm1026 sensors
always have the same index for any i2c client. To pass something
client specific therefore each client would have to have it's own
device attribute created in the adm1026_init_client() rather than a
shared static one. This isn't worth the trouble or extra space for
something like adm1026 though, and is really only useful for something
like bmcsensors where we have to dynamically allocate the device
attributes.

Thanks,
Yani
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-linux-2.6.12-rc3-devdyncallback-driversys.diff
Type: text/x-patch
Size: 2169 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050503/7163a9b5/patch-linux-2.6.12-rc3-devdyncallback-driversys.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: adm1026-devdyncallback.nowarn.diff
Type: text/x-patch
Size: 19384 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050503/7163a9b5/adm1026-devdyncallback.nowarn.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: adm1026-devdyncallback.sensor_nr.diff
Type: text/x-patch
Size: 29367 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050503/7163a9b5/adm1026-devdyncallback.sensor_nr.bin

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Tue, May 03, 2005 at 11:57:33AM -0400, Yani Ioannou wrote:
> Obviously the drivers with more device attributes will benefit the
> most from the patch (and in the extreme case of unlimited, like
> bmcsensors), so I chose to demonstrate a patch for adm1026.c which
> seems to have one of the largest number of attributes for the i2c
> chips.
> 
> I've included a modified version of the previous dynamic callback
> patch where I defined a DEVICE_ATTR_DATA macro which becomes useful in
> these cases, likely in any real patch this would be rolled into the
> DEVICE_ATTR macro but for simplicity (i.e. not breaking everything
> else that uses DEVICE_ATTR out there) I'm using a separate one for
> now.

Try modifying __ATTR() to have the data pointer and then make
DEVICE_ATTR pass a NULL for the data field.

> The second attached patch simply modifies the driver to avoid compiler
> warnings from using invalid show/store function pointers - this is the
> bare minimum change a driver would need to make (although leaving the
> warnings doesn't affect anything functionally I for one hate seeing
> warnings on a kernel compile).

Those warnings must be fixed up, yeah we could get away with it
possibly, but I would not guarantee it...

> The third patch shows how we could get rid of the macro defined hoard
> of static sysfs callbacks in the easiest and probably the best way -
> define ints for indexes and pass pointers to them as void *data. This
> compiles and should work fine (sorry can't test this since I don't
> have this chip). Loading the module though the size difference is
> again apparent:
> 
> -------------------2.6.11.7--------------------
> Module                  Size  Used by
> adm1026                44692  0
> --------2.6.12-rc3-devdyncallback-----
> Module                  Size  Used by
> adm1026                33172  0
> 
> 
> You can see in this example why Jean is not convinced that void * is
> better than passing an int, in the case of the i2c chip drivers the
> majority seem to do something along these lines, and really an int
> suffices. In the grand scheme of things though I think the void * is
> much more flexible.

Just use the void * as an int.  No need to point it to an integer
variable, right?  Would that make this code a bit more readable (it's
pretty messy as is.)

> Taking things even further you might restructure the the adm1026_data
> structure and group together the related sensor attributes so that we
> can pass a pointer to a data structure instead of the int index,
> something like:
> 
> struct in_data {
> 	int id;
> 	u8 value;            /* Register value */
> 	u8 max;              /* Register value */
> 	u8 min;              /* Register value */
> };
> 
> struct fan_data {
> 	int id;
> 	u8 value;           /* Register value */
> 	u8 min;             /* Register value */
> 	u8 div;             /* Decoded value */
> };
> 
> 
> struct temp_data {
> 	int id;
> 	s8 value;          /* Register value */
> 	s8 min;            /* Register value */
> 	s8 max;            /* Register value */
> 	s8 tmin;           /* Register value */
> 	s8 crit;           /* Register value */
> 	s8 offset;         /* Register value */	
> };
> 
> struct adm1026_data {
> 	...
> 	struct in_data in[17];     /* Volt Sensor Data */
> 	struct fan_data fanp[8];     /* Fan Sensor Data */
> 	struct temp_data temp[3];    /* Temp Sensor Data */
> 	struct pwm_data pwm1;   /* Pwm control values */
> 
> 	/* Voltage sensor device attributes */
> 	struct device_attribute in_reg_attr[17];
> 	struct device_attribute in_min_attr[17];
> 	struct device_attribute in_max_attr[17];
> 	
> 	/* Fan sensor device attributes */
> 	struct device_attribute fan_input_attr[8];
> 	struct device_attribute fan_min_attr[8];
> 	...
> }
> 
> We would then be able to just use the passed data structure (e.g.
> temp_data[offset] for a temp attribute callback) in the sysfs callback
> as in the bmcsensors patch.
> 
> This won't work with static device_attributes like adm1026 uses
> because we encounter the problem Dmitry described - adm1026 allows
> multiple i2c_clients each with it's own adm1026_data structure, but
> all of them would share the same static device_attribute.

Yes, dynamic attributes are the best way to help solve this issue.

> The
> integer/index passed data above works because the adm1026 sensors
> always have the same index for any i2c client. To pass something
> client specific therefore each client would have to have it's own
> device attribute created in the adm1026_init_client() rather than a
> shared static one. This isn't worth the trouble or extra space for
> something like adm1026 though, and is really only useful for something
> like bmcsensors where we have to dynamically allocate the device
> attributes.

Hm, as we want to move toward dynamic attributes, would a helper
function to create one be a good idea to have?

And thanks for doing this, it makes a bit more sense now, and seems
almost reasonable.  I'd like a few more cleanups before adding it to my
trees :)

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Jean Delvare]

Hi Greg, Yani,

> Just use the void * as an int.  No need to point it to an integer
> variable, right?  Would that make this code a bit more readable (it's
> pretty messy as is.)

Irk. What about the kittens? :(

As a side note I would like to apologize for not helping more on this.
This is simply above my strength and capabilities for the time being.
Let's just hope it's temporary.

--
Jean Delvare

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

> > I've included a modified version of the previous dynamic callback
> > patch where I defined a DEVICE_ATTR_DATA macro which becomes useful in
> > these cases, likely in any real patch this would be rolled into the
> > DEVICE_ATTR macro but for simplicity (i.e. not breaking everything
> > else that uses DEVICE_ATTR out there) I'm using a separate one for
> > now.
> 
> Try modifying __ATTR() to have the data pointer and then make
> DEVICE_ATTR pass a NULL for the data field.
> 

This would work except __ATTR is used by all the attribute macros not
just DEVICE_ATTRIBUTE. Would a void * field in those attributes be
useful too?

> Just use the void * as an int.  No need to point it to an integer
> variable, right?  Would that make this code a bit more readable (it's
> pretty messy as is.)
Jean:
On 5/4/05, Jean Delvare <khali@linux-fr.org> wrote:
>Irk. What about the kittens? :(

I was tempted to do this but it set off alarms in my better coding
judgement (abusing a pointer to store an int). It is certainly
'cleaner' than defining the awkward static ints, but aside from the
language abuse would doing something like this cause any portability
issues? And yes, what about those poor kittens? ;-)

> Hm, as we want to move toward dynamic attributes, would a helper
> function to create one be a good idea to have?
> 
It would be useful for most of the cases (like moving to dynamic
attributes in adm1026), but when creating completely dynamic
attributes (as in you don't know what you will be creating at compile
time) you have to create and allocate the sysfs name strings too (see
bmcsensors).

> And thanks for doing this, it makes a bit more sense now, and seems
> almost reasonable.  I'd like a few more cleanups before adding it to my
> trees :)
>

Well I'm an almost reasonable person ;-). Yes this code was mainly for
comment so it definitely needs cleaning up, although any pointers on
how/where are welcome.

Thanks,
Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 5/4/05, Jean Delvare <khali@linux-fr.org> wrote:

> As a side note I would like to apologize for not helping more on this.
> This is simply above my strength and capabilities for the time being.
> Let's just hope it's temporary.

Are you unwell Jean? I hope you feel better soon if so. Your comments
and encouragement have been a great deal of help to me alone.

Thanks,
Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Grant Coady]

Hi All,
On Wed, 4 May 2005 15:16:11 -0400, Yani Ioannou <yani.ioannou@gmail.com> wrote:

>I was tempted to do this but it set off alarms in my better coding
>judgement (abusing a pointer to store an int). It is certainly
>'cleaner' than defining the awkward static ints, but aside from the
>language abuse would doing something like this cause any portability
>issues? And yes, what about those poor kittens? ;-)
Yes, I want to know too :)

>Well I'm an almost reasonable person ;-). Yes this code was mainly for
>comment so it definitely needs cleaning up, although any pointers on
>how/where are welcome.

I have queries, is there a reason why sysfs doesn't return multiple 
values?  So instead of asking for a filename with encoded array index, 
one gets a list of values?  One line per value, it works, easy to use 
as 'flat' datafiles.  And it removes index from filename.  the many 
to many (eg. temp channel -> fan speed control channel) can be resolved 
same as database normalisation.

Secondly is there a design document I've missed for this?  What does 
dynamic sysfs look like to user?  sub-directory per device/block or 
in the same directory level namespace?  I damaged the patches somewhere, 
one didn't apply so couldn't 'see' it yet.

--Grant.

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 5/4/05, Grant Coady <grant_lkml@dodo.com.au> wrote:
> I have queries, is there a reason why sysfs doesn't return multiple
> values?  So instead of asking for a filename with encoded array index,
> one gets a list of values?  One line per value, it works, easy to use
> as 'flat' datafiles.  And it removes index from filename.  the many
> to many (eg. temp channel -> fan speed control channel) can be resolved
> same as database normalisation.

As I understand it this was a deliberate decision to ease the problems
experienced with 2.4 proc entries where userspace programs would have
to parse the contents to extract specific information, and every time
the format of the contents (e.g. number/order of columns) changed all
these userspace apps would be broken. It was decided presenting the
specific information per file was a much simpler and more sensible
interface. I tend to agree.

> 
> Secondly is there a design document I've missed for this?  What does
> dynamic sysfs look like to user?  sub-directory per device/block or
> in the same directory level namespace?  I damaged the patches somewhere,
> one didn't apply so couldn't 'see' it yet.

This patch just changes the way device attributes are created, it does
not change the external sysfs interface at all.

At the moment every driver in the kernel (as far as I can see) defines
a set of static device attribute structs which are used by the driver
when calling the sysfs_create_file function. Greg was referring to
creating non-static attributes or creating device attributes at
runtime (as bmcsensors does) as dynamic.

Due to the nature of the sysfs device callbacks, each device attribute
in general requires a separate sysfs show/store callback function.
This has led to the kludge of a large number of macro defined
callbacks that are virtually identical. This leads to two problems,
first we can only define a limited and pre-determined number of sysfs
callbacks and hence device_attributes (in the case of bmcsensors it
can have an unlimited number of sensors), secondly a whole load of
redundant code is generated by the macros and is a large waste of
space. By adding a void * attribute member to the device_attribute
structure and passing these to callback functions we allow drivers to
greatly reduce the number of callback functions and also allow for a
potentially unlimited number of sysfs entries to be created. I refer
to these callbacks as dynamic because they change their function
depending on what is passed to them.

Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Grant Coady]

Hi Yani,
On Wed, 4 May 2005 23:29:47 -0400, Yani Ioannou <yani.ioannou@gmail.com> wrote:

>                                      It was decided presenting the
>specific information per file was a much simpler and more sensible
>interface. I tend to agree.
Okay, I see that.  also did a 'cat <eeprom>' oops, pipe through xxd 
'cos it returned binary data :)  

>> Secondly is there a design document I've missed for this?  What does
>> dynamic sysfs look like to user?  sub-directory per device/block or
>> in the same directory level namespace?  I damaged the patches somewhere,
>> one didn't apply so couldn't 'see' it yet.
>
>This patch just changes the way device attributes are created, it does
>not change the external sysfs interface at all.
Okay

<snip>
Okay, I'm an interested observer then, 'cos I make cross-reference 
script to see which driver uses what sysfs name, still learning.

Thanks,
--Grant.

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Thu, May 05, 2005 at 09:00:56AM +1000, Grant Coady wrote:
> Hi All,
> On Wed, 4 May 2005 15:16:11 -0400, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> 
> >I was tempted to do this but it set off alarms in my better coding
> >judgement (abusing a pointer to store an int). It is certainly
> >'cleaner' than defining the awkward static ints, but aside from the
> >language abuse would doing something like this cause any portability
> >issues? And yes, what about those poor kittens? ;-)
> Yes, I want to know too :)

I don't care about the kittens, abuse that void *, that's what it is
there for.

> >Well I'm an almost reasonable person ;-). Yes this code was mainly for
> >comment so it definitely needs cleaning up, although any pointers on
> >how/where are welcome.
> 
> I have queries, is there a reason why sysfs doesn't return multiple 
> values?

Yes, that is the way it is designed.  One value per file is the RULE in
sysfs files.  End of story.

> Secondly is there a design document I've missed for this?  What does 
> dynamic sysfs look like to user?  sub-directory per device/block or 
> in the same directory level namespace?  I damaged the patches somewhere, 
> one didn't apply so couldn't 'see' it yet.

These patches don't change anything that the user sees, becides a
smaller kernel memory footprint.  The sysfs tree and files stay the
same.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Wed, May 04, 2005 at 03:16:11PM -0400, Yani Ioannou wrote:
> > > I've included a modified version of the previous dynamic callback
> > > patch where I defined a DEVICE_ATTR_DATA macro which becomes useful in
> > > these cases, likely in any real patch this would be rolled into the
> > > DEVICE_ATTR macro but for simplicity (i.e. not breaking everything
> > > else that uses DEVICE_ATTR out there) I'm using a separate one for
> > > now.
> > 
> > Try modifying __ATTR() to have the data pointer and then make
> > DEVICE_ATTR pass a NULL for the data field.
> > 
> 
> This would work except __ATTR is used by all the attribute macros not
> just DEVICE_ATTRIBUTE. Would a void * field in those attributes be
> useful too?

Wait, are you only changing the device attribute?  What about the
general sysfs attributes?  That is where this should be changed.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

> Wait, are you only changing the device attribute?  What about the
> general sysfs attributes?  That is where this should be changed.
> 
> thanks,
> 
> greg k-h
> 

Yes my patch changes the device attributes simply because that's all I
ever worked with, but I can see this benefiting all sysfs attributes,
I'll make the change and change __ATTR.

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Thu, May 05, 2005 at 02:03:06PM +1000, Grant Coady wrote:
> Hi Yani,
> On Wed, 4 May 2005 23:29:47 -0400, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> 
> >                                      It was decided presenting the
> >specific information per file was a much simpler and more sensible
> >interface. I tend to agree.
> Okay, I see that.  also did a 'cat <eeprom>' oops, pipe through xxd 
> 'cos it returned binary data :)  

Yes, some files in sysfs are in binary form.  They are ones that are
only "pass through" type files where the kernel does not modify the data
at all.  Examples of this is the eeprom files, and the pci config space
files.

thanks,

greg k-h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 5/5/05, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> > Wait, are you only changing the device attribute?  What about the
> > general sysfs attributes?  That is where this should be changed.
> >
> > thanks,
> >
> > greg k-h
> >
> 
> Yes my patch changes the device attributes simply because that's all I
> ever worked with, but I can see this benefiting all sysfs attributes,
> I'll make the change and change __ATTR.

Here is a much more ambitious patch that inserts the void * into the
general sysfs attribute, updates the __ATTR and ATTR_RO macros to
automatically set it to NULL and updates all the derived attributes
(that I can identify anyway) to pass it to their sysfs callbacks. If
nobody has any objections/improvements to this patch I will submit a
(rather large) patch mainly generated by my updated perl script to
remove the warnings this causes on any code that defines sysfs
callbacks.

We would probably want to define another DEVICE_ATTRIBUTE like macro
or add a void * parameter to the DEVICE_ATTRIBUTE macro. Any opinions
on which way to go? This might be done for the other *_ATTRIBUTE
macros too.

I noticed as I was going through the kernel source that implements
sysfs attributes that a lot of non-i2c drivers and even a lot of
non-driver code could benefit from this patch, it would be interesting
to see how much this patch could reduce the kernel size. A lot of work
would have to be done before that is realized though in updating code
to take advantage of it.

Thanks,
Yani
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-linux-2.6.12-rc3-sysfsdyncallback-core.diff
Type: text/x-patch
Size: 17264 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050506/344c3d90/patch-linux-2.6.12-rc3-sysfsdyncallback-core.bin

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Dmitry Torokhov]

Hi Yani,

On Friday 06 May 2005 01:40, Yani Ioannou wrote:
> We would probably want to define another DEVICE_ATTRIBUTE like macro
> or add a void * parameter to the DEVICE_ATTRIBUTE macro. Any opinions
> on which way to go? This might be done for the other *_ATTRIBUTE
> macros too.
> 

No, I don't think so. Again, macros are normally used to initialize
statically allocated attributes, exposing void * pointer does not do
them any good. It can only lead to bugs when driver writer misses the
fact that pointer is shared between all instances of attribute.

-- 
Dmitry

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Hi Dmitry,

Even for static attributes the void * can be very useful, there are
many cases, maybe even most cases (I'm looking through all the sysfs
attribute code looking to update it to take advantage of this patch
now) where static attributes using the void * are fine and the
attribute is not shared amongst different clients/instances of code.

For example, adm1026 we could use the index for that static attributes
as my previous patch shows and we really need a macro in that case. Or
look at net/core/net-sysfs.c, the macros NETDEVICE_SHOW,
NETDEVICE_ATTR, etc are best replaced by static attributes using a
single callback function and using the void *, for which we really
need a macro to set the void *.

Perhaps declaring seperate macros for this (e.g.
DEVICE_ATTRIBUTE_DATA, such as I defined in the adm1026 example patch)
would be more clear?

Thanks,
Yani


On 5/6/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> Hi Yani,
> 
> On Friday 06 May 2005 01:40, Yani Ioannou wrote:
> > We would probably want to define another DEVICE_ATTRIBUTE like macro
> > or add a void * parameter to the DEVICE_ATTRIBUTE macro. Any opinions
> > on which way to go? This might be done for the other *_ATTRIBUTE
> > macros too.
> >
> 
> No, I don't think so. Again, macros are normally used to initialize
> statically allocated attributes, exposing void * pointer does not do
> them any good. It can only lead to bugs when driver writer misses the
> fact that pointer is shared between all instances of attribute.
> 
> --
> Dmitry
>

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Dmitry Torokhov]

Hi Yani,

On Sunday 08 May 2005 21:10, Yani Ioannou wrote:
> Hi Dmitry,
> 
> Even for static attributes the void * can be very useful, there are
> many cases, maybe even most cases (I'm looking through all the sysfs
> attribute code looking to update it to take advantage of this patch
> now) where static attributes using the void * are fine and the
> attribute is not shared amongst different clients/instances of code.
> 
> For example, adm1026 we could use the index for that static attributes
> as my previous patch shows and we really need a macro in that case. Or
> look at net/core/net-sysfs.c, the macros NETDEVICE_SHOW,
> NETDEVICE_ATTR, etc are best replaced by static attributes using a
> single callback function and using the void *, for which we really
> need a macro to set the void *.
> 
> Perhaps declaring seperate macros for this (e.g.
> DEVICE_ATTRIBUTE_DATA, such as I defined in the adm1026 example patch)
> would be more clear?
>

Yes, I see what you mean. But I think what we might need is actually 2
void * pointers, something like "attribute_data" and "instance_data".
Macros would initialize "attribute_data" but not "instance_data". This
way their usage is clearly defined and there hopefully less confusion.

-- 
Dmitry

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Hi Dmitry,

On 5/8/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> Hi Yani,
> 
> Yes, I see what you mean. But I think what we might need is actually 2
> void * pointers, something like "attribute_data" and "instance_data".
> Macros would initialize "attribute_data" but not "instance_data". This
> way their usage is clearly defined and there hopefully less confusion.
> 
> --
> Dmitry
> 

The naming might make the distinction in use, but nothing is really
stopping anyone from using one or the other and it might even confuse
further (i.e. not understanding the difference, using both, using the
wrong one). Since the two would be mutually exclusive I also can't
help feel it would be a waste of space. Maybe naming the macro that
sets the data member to something appropriate would help?

I think a brief comment on the matter in device.h and further
explanation in the sysfs documentation might be as far as we could go,
I don't see a nice way to enforce something like this. Its a difficult
decision, but sometimes we just have to trust people read the
documentation and know what they are doing (and peer review).

Yani

(P.S. net/core/net-sysfs.c wasn't the best example in my previous
message - its not as easy to change as it first looks)

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Dmitry Torokhov]

On Monday 09 May 2005 00:01, Yani Ioannou wrote:
> Hi Dmitry,
> 
> On 5/8/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > Hi Yani,
> > 
> > Yes, I see what you mean. But I think what we might need is actually 2
> > void * pointers, something like "attribute_data" and "instance_data".
> > Macros would initialize "attribute_data" but not "instance_data". This
> > way their usage is clearly defined and there hopefully less confusion.
> > 
> > --
> > Dmitry
> > 
> 
> The naming might make the distinction in use, but nothing is really
> stopping anyone from using one or the other and it might even confuse
> further (i.e. not understanding the difference, using both, using the
> wrong one). Since the two would be mutually exclusive

Why would they? Consider something like generic show function you give
it a pointer to data you want to be printed in instance_data and a pointer
to format string in attribute_data and that's it.

-- 
Dmitry

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

On 5/9/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> On Monday 09 May 2005 00:01, Yani Ioannou wrote:
> > Hi Dmitry,
> >
> > On 5/8/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > Hi Yani,
> > >
> > > Yes, I see what you mean. But I think what we might need is actually 2
> > > void * pointers, something like "attribute_data" and "instance_data".
> > > Macros would initialize "attribute_data" but not "instance_data". This
> > > way their usage is clearly defined and there hopefully less confusion.
> > >
> > > --
> > > Dmitry
> > >
> >
> > The naming might make the distinction in use, but nothing is really
> > stopping anyone from using one or the other and it might even confuse
> > further (i.e. not understanding the difference, using both, using the
> > wrong one). Since the two would be mutually exclusive
> 
> Why would they? Consider something like generic show function you give
> it a pointer to data you want to be printed in instance_data and a pointer
> to format string in attribute_data and that's it.

Each `instance' would have to have it's own sysfs attribute (i.e.
non-static/shared) to be able to set an instance specific pointer
anyway, and so setting attribute_data for bother of those really
doesn't make any sense to me. If you are looking to pass two (or more)
pointers as in your example, just create a single struct containing
them and point to it (although I'm trying not to do something like
that in net-sysfs.c).

Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Two patches for comment, first defining a set of macros as I
previously described would be useful, and second an initial patch to
adapt net/core/net-sysfs.c to take advantage of dynamic callbacks
(although this needs testing) using said macros.

Before: 15K linux-2.6.12-rc4/usr/src/linux/net/core/net-sysfs.o
After: 12K  linux-2.6.12-rc4-sysfsdyncallback-net-sysfs/net/core/net-sysfs.o

Obviously not as big a difference as with adm1026, but that's to be
expected, it doesn't define near as many attributes. I think the patch
cleans up the code aesthetically quite a bit too. If someone could
work out how to get rid of the macros for sprintfs (FIELD_FORMAT, etc)
that would be even better.

Night...errr morning :-),
Yani
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-linux-2.6.12-rc4-sysfsdyncallback-datamacros.diff
Type: text/x-patch
Size: 3218 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050509/d3771d4c/patch-linux-2.6.12-rc4-sysfsdyncallback-datamacros.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-linux-2.6.12-rc4-sysfsdyncallback-net-sysfs.diff
Type: text/x-patch
Size: 7291 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050509/d3771d4c/patch-linux-2.6.12-rc4-sysfsdyncallback-net-sysfs.bin

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Jean Delvare]

Hi Yani,

> The naming might make the distinction in use, but nothing is really
> stopping anyone from using one or the other and it might even confuse
> further (i.e. not understanding the difference, using both, using the
> wrong one). Since the two would be mutually exclusive I also can't
> help feel it would be a waste of space. Maybe naming the macro that
> sets the data member to something appropriate would help?

If both are actually mutually exclusive, you could use a union in the
struct definition.

Also, if we go for two different attributes, wouldn't it make sense to
have the static one an int and the per-instance one a void*? I keep
thinking that a pointer for the static case won't be of any help and is
actually error-prone.

That being said, these are only my random thoughts on the topic. I am not
going to make a decision or even emit an authoritative opinion here
(especially not when it seems that Greg has different views). Hopefully
I'll be back "soon", the worst seems to be behind me now.

Thanks,
--
Jean Delvare

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Hi Jean,

On 5/9/05, Jean Delvare <khali@linux-fr.org> wrote:
> 
> Hi Yani,
>
> If both are actually mutually exclusive, you could use a union in the
> struct definition.

That's an interesting suggestion, I've never worked with unions to
tell you the truth, but they look useful...

> Also, if we go for two different attributes, wouldn't it make sense to
> have the static one an int and the per-instance one a void*? I keep
> thinking that a pointer for the static case won't be of any help and is
> actually error-prone.

With respect to the lm_sensors chip drivers an int alone might make
sense (although for bmcsensors the void* is better), but when you
start looking at the rest of the kernel code that uses attributes the
void * looks like a very good decision.

If you have a look at my patch for net-sysfs.c for example, its hard
(impossible?) to imagine a way to do a similar thing with just an int
being passed to the callbacks, and this is using static attributes.
There are better examples out there I'm sure but I'm working down my
list slowly and net-sysfs.c happens to be at the top :-) (see the end
of this e-mail for a list of files that could potentially be updated
to use the dynamic callbacks).

> That being said, these are only my random thoughts on the topic. I am not
> going to make a decision or even emit an authoritative opinion here
> (especially not when it seems that Greg has different views). Hopefully
> I'll be back "soon", the worst seems to be behind me now.

Yes, it is starting to look like the cold hand of a dictator might be
needed ... where is Greg anyway ;-)?

Yani

Files implementing sysfs attributes:
net/core/net-sysfs.c
net/bridge/br_sysfs_br.c
net/bluetooth/hci_sysfs.c
arch/sh/drivers/dma/dma-sysfs.c
arch/arm/common/amba.c
arch/arm/kernel/ecard.c
arch/arm/kernel/time.c
arch/ppc/kernel/pci.c
arch/ppc/syslib/of_device.c
arch/ppc/syslib/ocp.c
arch/ia64/sn/kernel/tiocx.c
arch/arm26/kernel/ecard.c
arch/ppc64/kernel/of_device.c
arch/ppc64/kernel/pci.c
arch/ppc64/kernel/vio.c
arch/ppc64/kernel/sysfs.c
arch/parisc/kernel/drivers.c
arch/x86_64/kernel/mce.c
drivers/w1/w1.c
drivers/w1/w1_smem.c
drivers/w1/w1_therm.c
drivers/sh/superhyway/superhyway-sysfs.c
drivers/i2c/i2c-core.c
drivers/i2c/chips/w83l785ts.c
drivers/i2c/chips/max1619.c
drivers/i2c/chips/smsc47m1.c
drivers/i2c/chips/w83781d.c
drivers/i2c/chips/smsc47b397.c
drivers/i2c/chips/it87.c
drivers/i2c/chips/lm63.c
drivers/i2c/chips/lm75.c
drivers/i2c/chips/lm77.c
drivers/i2c/chips/lm78.c
drivers/i2c/chips/lm80.c
drivers/i2c/chips/lm83.c
drivers/i2c/chips/lm85.c
drivers/i2c/chips/lm87.c
drivers/i2c/chips/lm90.c
drivers/i2c/chips/lm92.c
drivers/i2c/chips/adm1021.c
drivers/i2c/chips/adm1025.c
drivers/i2c/chips/adm1026.c
drivers/i2c/chips/adm1031.c
drivers/i2c/chips/w83627hf.c
drivers/i2c/chips/gl518sm.c
drivers/i2c/chips/asb100.c
drivers/i2c/chips/gl520sm.c
drivers/i2c/chips/pcf8574.c
drivers/i2c/chips/pcf8591.c
drivers/i2c/chips/fscher.c
drivers/i2c/chips/fscpos.c
drivers/i2c/chips/ds1621.c
drivers/i2c/chips/sis5595.c
drivers/i2c/chips/via686a.c
drivers/i2c/chips/pc87360.c
drivers/i2c/i2c-dev.c
drivers/dio/dio-sysfs.c
drivers/mca/mca-bus.c
drivers/mmc/mmc_sysfs.c
drivers/pci/pci-sysfs.c
drivers/pci/hotplug/cpqphp_sysfs.c
drivers/pci/hotplug/shpchp_sysfs.c
drivers/pci/probe.c
drivers/pnp/interface.c
drivers/pnp/card.c
drivers/usb/core/sysfs.c
drivers/usb/host/ehci-dbg.c
drivers/usb/host/ohci-dbg.c
drivers/usb/misc/phidgetkit.c
drivers/usb/misc/cytherm.c
drivers/usb/misc/phidgetservo.c
drivers/usb/misc/usbled.c
drivers/usb/input/aiptek.c
drivers/usb/media/sn9c102_core.c
drivers/usb/media/ov511.c
drivers/usb/media/stv680.c
drivers/usb/storage/scsiglue.c
drivers/usb/gadget/file_storage.c
drivers/usb/gadget/net2280.c
drivers/usb/gadget/dummy_hcd.c
drivers/usb/gadget/pxa2xx_udc.c
drivers/usb/serial/ftdi_sio.c
drivers/base/interface.c
drivers/base/cpu.c
drivers/base/power/sysfs.c
drivers/base/firmware_class.c
drivers/base/node.c
drivers/base/dmapool.c
drivers/char/drm/drm_sysfs.c
drivers/char/tpm/tpm.c
drivers/char/mwave/mwavedd.c
drivers/char/hvcs.c
drivers/char/mbcs.c
drivers/eisa/eisa-bus.c
drivers/s390/cio/cmf.c
drivers/s390/cio/ccwgroup.c
drivers/s390/cio/chsc.c
drivers/s390/cio/device.c
drivers/s390/net/lcs.c
drivers/s390/net/qeth_sys.c
drivers/s390/net/netiucv.c
drivers/s390/net/claw.c
drivers/s390/net/cu3088.c
drivers/s390/net/ctcmain.c
drivers/s390/char/raw3270.c
drivers/s390/char/vmlogrdr.c
drivers/s390/char/tape_core.c
drivers/s390/scsi/zfcp_sysfs_driver.c
drivers/s390/scsi/zfcp_scsi.c
drivers/s390/scsi/zfcp_sysfs_port.c
drivers/s390/scsi/zfcp_sysfs_unit.c
drivers/s390/scsi/zfcp_sysfs_adapter.c
drivers/s390/block/dasd_devmap.c
drivers/s390/block/dcssblk.c
drivers/scsi/arm/powertec.c
drivers/scsi/arm/eesox.c
drivers/scsi/lpfc/lpfc_attr.c
drivers/scsi/st.c
drivers/scsi/scsi_debug.c
drivers/scsi/scsi_transport_fc.c
drivers/scsi/ipr.c
drivers/scsi/ibmvscsi/ibmvscsi.c
drivers/scsi/scsi_transport_spi.c
drivers/scsi/3w-xxxx.c
drivers/scsi/3w-9xxx.c
drivers/scsi/scsi_sysfs.c
drivers/scsi/ncr53c8xx.c
drivers/scsi/pcmcia/sym53c500_cs.c
drivers/scsi/osst.c
drivers/scsi/53c700.c
drivers/scsi/scsi_transport_iscsi.c
drivers/scsi/megaraid/megaraid_mbox.c
drivers/cpufreq/cpufreq_ondemand.c
drivers/cpufreq/freq_table.c
drivers/cpufreq/cpufreq_userspace.c
drivers/cpufreq/cpufreq_stats.c
drivers/cpufreq/cpufreq.c
drivers/infiniband/hw/mthca/mthca_provider.c
drivers/infiniband/ulp/ipoib/ipoib_main.c
drivers/infiniband/ulp/ipoib/ipoib_vlan.c
drivers/infiniband/core/sysfs.c
drivers/infiniband/core/user_mad.c
drivers/block/aoe/aoeblk.c
drivers/block/ub.c
drivers/block/genhd.c
drivers/block/viodasd.c
drivers/block/genhd.c.orig
drivers/input/mouse/psmouse.h
drivers/input/serio/serio.c
drivers/input/gameport/gameport.c
drivers/input/keyboard/atkbd.c
drivers/media/video/bttv-driver.c
drivers/media/video/videodev.c
drivers/video/fbsysfs.c
drivers/video/gbefb.c
drivers/video/w100fb.c
drivers/video/backlight/lcd.c
drivers/video/backlight/backlight.c
drivers/zorro/zorro-sysfs.c
drivers/message/i2o/device.c
drivers/message/fusion/mptscsih.c
drivers/firmware/efivars.c
drivers/ieee1394/nodemgr.c
drivers/ieee1394/sbp2.c
drivers/parisc/pdc_stable.c
drivers/pcmcia/ds.c
drivers/pcmcia/m32r_cfc.c
drivers/pcmcia/m32r_pcc.c
drivers/pcmcia/i82365.c
drivers/pcmcia/soc_common.c
drivers/pcmcia/rsrc_nonstatic.c
drivers/pcmcia/socket_sysfs.c
drivers/macintosh/therm_pm72.c
drivers/macintosh/therm_windtunnel.c
drivers/macintosh/therm_adt746x.c
kernel/ksysfs.c
kernel/power/disk.c
kernel/power/main.c
include/asm-ppc/ocp.h

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC

[Jean Delvare]

Hi Yani,

> With respect to the lm_sensors chip drivers an int alone might make
> sense (although for bmcsensors the void* is better), but when you
> start looking at the rest of the kernel code that uses attributes the
> void * looks like a very good decision.
> 
> If you have a look at my patch for net-sysfs.c for example, its hard
> (impossible?) to imagine a way to do a similar thing with just an int
> being passed to the callbacks, and this is using static attributes.

You could always define a static array of pointers, and use the passed
int as an index to that array, thus converting the int into a pointer.
It's not very different from defining a static array of ints and passing
the int addresses as the void*, but for some reason I tend to prefer the
int -> pointer conversion (most certainly because my view if awfully
biased by my misknowledge of the kernel except for the i2c and hardware
monitoring code).

> Yes, it is starting to look like the cold hand of a dictator might be
> needed ... where is Greg anyway ;-)?

Don't blame him, I am wasting his time over IRC, this is why... ;)

-- 
Jean Delvare

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Yani Ioannou]

Hi Jean,

On 5/9/05, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Yani,
> 
> You could always define a static array of pointers, and use the passed
> int as an index to that array, thus converting the int into a pointer.
> It's not very different from defining a static array of ints and passing
> the int addresses as the void*, but for some reason I tend to prefer the
> int -> pointer conversion (most certainly because my view if awfully
> biased by my misknowledge of the kernel except for the i2c and hardware
> monitoring code).

Yes, so it is possible just very ugly :-). I'm not sure what I think
of the int->pointer conversion, I guess its a necessary evil. I can't
help feel like I'm doing something wrong when I do it though..

> 
> > Yes, it is starting to look like the cold hand of a dictator might be
> > needed ... where is Greg anyway ;-)?
> 
> Don't blame him, I am wasting his time over IRC, this is why... ;)

Ahhh, that explains it .. poor Greg ;).

Yani

[RFC PATCH 2.6.12-rc3] dynamic driver sysfs callbacks and RFC on

[Greg KH]

On Mon, May 09, 2005 at 06:34:24PM -0400, Yani Ioannou wrote:
> Hi Jean,
> 
> On 5/9/05, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Yani,
> > 
> > You could always define a static array of pointers, and use the passed
> > int as an index to that array, thus converting the int into a pointer.
> > It's not very different from defining a static array of ints and passing
> > the int addresses as the void*, but for some reason I tend to prefer the
> > int -> pointer conversion (most certainly because my view if awfully
> > biased by my misknowledge of the kernel except for the i2c and hardware
> > monitoring code).
> 
> Yes, so it is possible just very ugly :-). I'm not sure what I think
> of the int->pointer conversion, I guess its a necessary evil. I can't
> help feel like I'm doing something wrong when I do it though..

Storing an int or a long in a pointer is just fine, don't feel dirty,
it's a natural thing :)

More comments on your main patch in a bit...

thanks,

greg k-h