dynamic driver sysfs callbacks and RFC on

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
@@ -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
@@ -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

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

Hi,

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

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

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

thanks,

greg k-h

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

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

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

Yes, that is what I was looking for.

thanks,

greg k-h

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?

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
@@ -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
@@ -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 --------------

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

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

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

Hi Dmitry,

My point exactly...

--
Jean Delvare

Hi Greg, Yani, all,

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).

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

Thanks,
--
Jean Delvare

Hi Yani,

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&m1138239422686&q=p3

attr_group_array.patch:
http://marc.theaimsgroup.com/?l=linux-kernel&m1138239422686&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

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

Hi Greg,

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

--Grant.

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

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

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.)

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

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

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

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

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

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, 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

Hi Yani,

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

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

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?

Jean:

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? ;-)

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).

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

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

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.

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

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

Hi All,

Yes, I want to know too :)

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.

Hi Greg, Yani,

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

Hi Yani,

Okay, I see that. also did a 'cat <eeprom>' oops, pipe through xxd 'cos it returned binary data :)

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.

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

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

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).

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

Jean Delvare

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

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

Hi 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)

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

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

Hi Jean,

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

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).

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

Hi Jean,

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..

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

Yani

Hi Yani,

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