[lm-sensors] [RFC-patch] pc87360 - unchecked

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim, Mark,
>
>   
hi guys
> Sorry for the late answer, I have a hard time catching up with e-mail
> since I returned from vacation.
>
>   
>>> - changing strategy  from completely-unchecked to 
>>> undo-everything-and-bailout
>>>    is a rather long step, and makes driver possibly unusable in some 
>>> corner cases
>>>    (none of which we know and can repeat)
>>>       
>> True enough, but I imagine that any other solution will be frowned upon
>> as "wallpapering over bugs".  Jean: any opinion on this?
>>     
>
> Yup, I don't much like Jim's approach, because it's meant to be
> temporary. Mark's approach seems better, especially since it is less
> difficult than I first thought. Now let's see how it scales to more
> complex drivers. Jim, would you like to try implementing something
> similar for the pc87360 driver (or any other complex driver of your
> choice, as long as you can test it) and see how it goes?
>
>   
Im completely impressed by how clean Mark's patch is.
In the end, the elegance seduced me, patch follows.

in the start, I cut-pasted my own remove-bunch/group.
its if-0'd now, to be yanked..

In contrast with Mark's declarative grouping, Im doing it at runtime:
     pc87360_detect()
            calls add_tothe_group() to add it to one_big_group[] for 
each sensor-attr wanted,
             then calls sysfs_create_group()

NB - the var and fn names are chosen to convey the implicit context,
and some sheepishness on my part for the obvious laziness ;-)
OTOH - all the implicit-ness is in the 1st 30 lines of the patch.


I thought about trying to generalize the fn a bit better :
    sysfs_addto_group( attr_group, new_attr);

but that seemed to imply promises about a dynamically allocated (and 
resized) group,
which probly should be done as a real LIST or something, but I wanted 
static allocation
(ENOUGH_ATTRS) since the driver has *only* statically declared attributes.

Its probly worth noting that pc87360 now does:    hwmon_register b4 
sysfs_create_group
whereas Mark's patch does the opposite.   Does this matter ??


Ive left a few other comments in the code...


diff -ruNp -X dontdiff -X exclude-diffs ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c sysdev/drivers/hwmon/pc87360.c
--- ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c	2006-06-17 19:49:35.000000000 -0600
+++ sysdev/drivers/hwmon/pc87360.c	2006-08-03 12:58:29.000000000 -0600
@@ -829,6 +829,83 @@ static int __init pc87360_find(int sioad
 	return 0;
 }
 
+/* Declare and use an attribute-group for simplicity.
+   This driver declares all attributes statically, so we count uses of 
+   SENSOR_ATTR, DEVICE_ATTR, and add 1 space for trailing NULL (+0 fudge)
+*/
+#define ENOUGH_ATTRS	89 + 4 + 1 + 0
+static struct attribute *one_big_group[ENOUGH_ATTRS];
+static int next_member;
+
+static struct attribute_group my_group = {
+	.attrs = one_big_group
+};
+
+static void add_tothe_group(struct device *dev,
+				struct device_attribute *devattr)
+{
+	/* add attr to the group for later sysfs_create_group */
+	if (next_member < ENOUGH_ATTRS)
+		one_big_group[next_member++] = &devattr->attr;
+	else {
+		BUG_ON("too many in group for static alloc!\n");
+		dev_err(dev, "too many in group for static alloc!\n");
+	}
+}
+
+#if 0
+static void remove_all_devattr_files(struct device *dev)
+{
+	int i;
+	/* Mark Hoffman used sysfs_remove_group here, it nicely tracks
+	   group membership, at cost of array of pointers.  For now,
+	   stick w non-group approach - cost is cut-paste in-elegance
+	   & maint if sensor set changes */
+
+	dev_info(dev, "created %d attr-files, w errs %d.  now destroying\n",
+		 next_member, devattr_file_create_errs);
+
+	/* tiny cheat - rely on size equivalence of {type}_{attr}[]
+	   arrays across all attrs for each type (declared that way)
+	*/
+	for (i = 0; i < ARRAY_SIZE(in_input); i++) {
+		device_remove_file(dev, &in_input[i].dev_attr);
+		device_remove_file(dev, &in_min[i].dev_attr);
+		device_remove_file(dev, &in_max[i].dev_attr);
+		device_remove_file(dev, &in_status[i].dev_attr);
+	}
+	device_remove_file(dev, &dev_attr_cpu0_vid);
+	device_remove_file(dev, &dev_attr_vrm);
+	device_remove_file(dev, &dev_attr_alarms_in);
+
+	for (i = 0; i < ARRAY_SIZE(temp_input); i++) {
+		device_remove_file(dev, &temp_input[i].dev_attr);
+		device_remove_file(dev, &temp_min[i].dev_attr);
+		device_remove_file(dev, &temp_max[i].dev_attr);
+		device_remove_file(dev, &temp_crit[i].dev_attr);
+		device_remove_file(dev, &temp_status[i].dev_attr);
+	}
+	device_remove_file(dev, &dev_attr_alarms_temp);
+
+	for (i = 0; i < ARRAY_SIZE(therm_input); i++) {
+		device_remove_file(dev, &therm_input[i].dev_attr);
+		device_remove_file(dev, &therm_min[i].dev_attr);
+		device_remove_file(dev, &therm_max[i].dev_attr);
+		device_remove_file(dev, &therm_crit[i].dev_attr);
+		device_remove_file(dev, &therm_status[i].dev_attr);
+	}
+	for (i = 0; i < ARRAY_SIZE(fan_input); i++) {
+		device_remove_file(dev, &fan_input[i].dev_attr);
+		device_remove_file(dev, &fan_min[i].dev_attr);
+		device_remove_file(dev, &fan_div[i].dev_attr);
+		device_remove_file(dev, &fan_status[i].dev_attr);
+		device_remove_file(dev, &pwm[i].dev_attr);
+	}
+
+	dev_info(dev, "remaining attr-files %d\n", next_member);
+}
+#endif
+
 static int pc87360_detect(struct i2c_adapter *adapter)
 {
 	int i;
@@ -944,50 +1021,61 @@ static int pc87360_detect(struct i2c_ada
 
 	if (data->innr) {
 		for (i = 0; i < 11; i++) {
-			device_create_file(dev, &in_input[i].dev_attr);
-			device_create_file(dev, &in_min[i].dev_attr);
-			device_create_file(dev, &in_max[i].dev_attr);
-			device_create_file(dev, &in_status[i].dev_attr);
-		}
-		device_create_file(dev, &dev_attr_cpu0_vid);
-		device_create_file(dev, &dev_attr_vrm);
-		device_create_file(dev, &dev_attr_alarms_in);
+			add_tothe_group(dev, &in_input[i].dev_attr);
+			add_tothe_group(dev, &in_min[i].dev_attr);
+			add_tothe_group(dev, &in_max[i].dev_attr);
+			add_tothe_group(dev, &in_status[i].dev_attr);
+		}
+		add_tothe_group(dev, &dev_attr_cpu0_vid);
+		add_tothe_group(dev, &dev_attr_vrm);
+		add_tothe_group(dev, &dev_attr_alarms_in);
 	}
 
 	if (data->tempnr) {
 		for (i = 0; i < data->tempnr; i++) {
-			device_create_file(dev, &temp_input[i].dev_attr);
-			device_create_file(dev, &temp_min[i].dev_attr);
-			device_create_file(dev, &temp_max[i].dev_attr);
-			device_create_file(dev, &temp_crit[i].dev_attr);
-			device_create_file(dev, &temp_status[i].dev_attr);
+			add_tothe_group(dev, &temp_input[i].dev_attr);
+			add_tothe_group(dev, &temp_min[i].dev_attr);
+			add_tothe_group(dev, &temp_max[i].dev_attr);
+			add_tothe_group(dev, &temp_crit[i].dev_attr);
+			add_tothe_group(dev, &temp_status[i].dev_attr);
 		}
-		device_create_file(dev, &dev_attr_alarms_temp);
+		add_tothe_group(dev, &dev_attr_alarms_temp);
 	}
 
 	if (data->innr = 14) {
 		for (i = 0; i < 3; i++) {
-			device_create_file(dev, &therm_input[i].dev_attr);
-			device_create_file(dev, &therm_min[i].dev_attr);
-			device_create_file(dev, &therm_max[i].dev_attr);
-			device_create_file(dev, &therm_crit[i].dev_attr);
-			device_create_file(dev, &therm_status[i].dev_attr);
+			add_tothe_group(dev, &therm_input[i].dev_attr);
+			add_tothe_group(dev, &therm_min[i].dev_attr);
+			add_tothe_group(dev, &therm_max[i].dev_attr);
+			add_tothe_group(dev, &therm_crit[i].dev_attr);
+			add_tothe_group(dev, &therm_status[i].dev_attr);
 		}
 	}
 
 	for (i = 0; i < data->fannr; i++) {
 		if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
-			device_create_file(dev, &fan_input[i].dev_attr);
-			device_create_file(dev, &fan_min[i].dev_attr);
-			device_create_file(dev, &fan_div[i].dev_attr);
-			device_create_file(dev, &fan_status[i].dev_attr);
+			add_tothe_group(dev, &fan_input[i].dev_attr);
+			add_tothe_group(dev, &fan_min[i].dev_attr);
+			add_tothe_group(dev, &fan_div[i].dev_attr);
+			add_tothe_group(dev, &fan_status[i].dev_attr);
 		}
 		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
-			device_create_file(dev, &pwm[i].dev_attr);
+			add_tothe_group(dev, &pwm[i].dev_attr);
 	}
 
+	/* Register sysfs hooks for the group */
+	err = sysfs_create_group(&client->dev.kobj, &my_group);
+	if (err) {
+		dev_err(&client->dev, "group-create failed: %d, %d members\n",
+			err, next_member);
+		goto ERROR3;
+	}
+	
 	return 0;
 
+	/* ERROR4:
+	   sysfs_remove_group(&client->dev.kobj, &my_group);
+	*/
 ERROR3:
 	i2c_detach_client(client);
 ERROR2:
@@ -1006,6 +1094,7 @@ static int pc87360_detach_client(struct 
 	struct pc87360_data *data = i2c_get_clientdata(client);
 	int i;
 
+	sysfs_remove_group(&client->dev.kobj, &my_group);
 	hwmon_device_unregister(data->class_dev);
 
 	if ((i = i2c_detach_client(client)))

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Mark M. Hoffman]

Hi Jim:

> Jean Delvare wrote:
> > Yup, I don't much like Jim's approach, because it's meant to be
> > temporary. Mark's approach seems better, especially since it is less
> > difficult than I first thought. Now let's see how it scales to more
> > complex drivers. Jim, would you like to try implementing something
> > similar for the pc87360 driver (or any other complex driver of your
> > choice, as long as you can test it) and see how it goes?

* Jim Cromie <jim.cromie at gmail.com> [2006-08-03 13:40:59 -0600]:
> Im completely impressed by how clean Mark's patch is.
> In the end, the elegance seduced me, patch follows.
> 
> in the start, I cut-pasted my own remove-bunch/group.
> its if-0'd now, to be yanked..
> 
> In contrast with Mark's declarative grouping, Im doing it at runtime:
>      pc87360_detect()
>             calls add_tothe_group() to add it to one_big_group[] for 
> each sensor-attr wanted,
>              then calls sysfs_create_group()

What is the point of generating the group at runtime?

> NB - the var and fn names are chosen to convey the implicit context,
> and some sheepishness on my part for the obvious laziness ;-)
> OTOH - all the implicit-ness is in the 1st 30 lines of the patch.
> 
> 
> I thought about trying to generalize the fn a bit better :
>     sysfs_addto_group( attr_group, new_attr);
> 
> but that seemed to imply promises about a dynamically allocated (and 
> resized) group,
> which probly should be done as a real LIST or something, but I wanted 
> static allocation
> (ENOUGH_ATTRS) since the driver has *only* statically declared attributes.
> 
> Its probly worth noting that pc87360 now does:    hwmon_register b4 
> sysfs_create_group
> whereas Mark's patch does the opposite.   Does this matter ??

I changed the ordering in asb100 on purpose... it closes the small window
of time between when the hwmon class is registered and the device attributes
are populated.  It's something of a race condition... sensors(1) could run
in the middle of that.

> Ive left a few other comments in the code...
> 
> 
> diff -ruNp -X dontdiff -X exclude-diffs ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c sysdev/drivers/hwmon/pc87360.c
> --- ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c	2006-06-17 19:49:35.000000000 -0600
> +++ sysdev/drivers/hwmon/pc87360.c	2006-08-03 12:58:29.000000000 -0600
> @@ -829,6 +829,83 @@ static int __init pc87360_find(int sioad
>  	return 0;
>  }
>  
> +/* Declare and use an attribute-group for simplicity.
> +   This driver declares all attributes statically, so we count uses of 
> +   SENSOR_ATTR, DEVICE_ATTR, and add 1 space for trailing NULL (+0 fudge)
> +*/
> +#define ENOUGH_ATTRS	89 + 4 + 1 + 0
> +static struct attribute *one_big_group[ENOUGH_ATTRS];
> +static int next_member;
> +
> +static struct attribute_group my_group = {
> +	.attrs = one_big_group
> +};
> +
> +static void add_tothe_group(struct device *dev,
> +				struct device_attribute *devattr)
> +{
> +	/* add attr to the group for later sysfs_create_group */
> +	if (next_member < ENOUGH_ATTRS)
> +		one_big_group[next_member++] = &devattr->attr;
> +	else {
> +		BUG_ON("too many in group for static alloc!\n");
> +		dev_err(dev, "too many in group for static alloc!\n");
> +	}
> +}
> +
> +#if 0
> +static void remove_all_devattr_files(struct device *dev)
> +{
> +	int i;
> +	/* Mark Hoffman used sysfs_remove_group here, it nicely tracks
> +	   group membership, at cost of array of pointers.  For now,
> +	   stick w non-group approach - cost is cut-paste in-elegance
> +	   & maint if sensor set changes */
> +
> +	dev_info(dev, "created %d attr-files, w errs %d.  now destroying\n",
> +		 next_member, devattr_file_create_errs);
> +
> +	/* tiny cheat - rely on size equivalence of {type}_{attr}[]
> +	   arrays across all attrs for each type (declared that way)
> +	*/
> +	for (i = 0; i < ARRAY_SIZE(in_input); i++) {
> +		device_remove_file(dev, &in_input[i].dev_attr);
> +		device_remove_file(dev, &in_min[i].dev_attr);
> +		device_remove_file(dev, &in_max[i].dev_attr);
> +		device_remove_file(dev, &in_status[i].dev_attr);
> +	}
> +	device_remove_file(dev, &dev_attr_cpu0_vid);
> +	device_remove_file(dev, &dev_attr_vrm);
> +	device_remove_file(dev, &dev_attr_alarms_in);
> +
> +	for (i = 0; i < ARRAY_SIZE(temp_input); i++) {
> +		device_remove_file(dev, &temp_input[i].dev_attr);
> +		device_remove_file(dev, &temp_min[i].dev_attr);
> +		device_remove_file(dev, &temp_max[i].dev_attr);
> +		device_remove_file(dev, &temp_crit[i].dev_attr);
> +		device_remove_file(dev, &temp_status[i].dev_attr);
> +	}
> +	device_remove_file(dev, &dev_attr_alarms_temp);
> +
> +	for (i = 0; i < ARRAY_SIZE(therm_input); i++) {
> +		device_remove_file(dev, &therm_input[i].dev_attr);
> +		device_remove_file(dev, &therm_min[i].dev_attr);
> +		device_remove_file(dev, &therm_max[i].dev_attr);
> +		device_remove_file(dev, &therm_crit[i].dev_attr);
> +		device_remove_file(dev, &therm_status[i].dev_attr);
> +	}
> +	for (i = 0; i < ARRAY_SIZE(fan_input); i++) {
> +		device_remove_file(dev, &fan_input[i].dev_attr);
> +		device_remove_file(dev, &fan_min[i].dev_attr);
> +		device_remove_file(dev, &fan_div[i].dev_attr);
> +		device_remove_file(dev, &fan_status[i].dev_attr);
> +		device_remove_file(dev, &pwm[i].dev_attr);
> +	}
> +
> +	dev_info(dev, "remaining attr-files %d\n", next_member);
> +}
> +#endif
> +
>  static int pc87360_detect(struct i2c_adapter *adapter)

You can't fill your static array from here.  What if you have two
pc87360s?  If you're going to do it at runtime, it has to be during
module init.  But again, why do it at runtime in the first place?

>  {
>  	int i;
> @@ -944,50 +1021,61 @@ static int pc87360_detect(struct i2c_ada
>  
>  	if (data->innr) {
>  		for (i = 0; i < 11; i++) {
> -			device_create_file(dev, &in_input[i].dev_attr);
> -			device_create_file(dev, &in_min[i].dev_attr);
> -			device_create_file(dev, &in_max[i].dev_attr);
> -			device_create_file(dev, &in_status[i].dev_attr);
> -		}
> -		device_create_file(dev, &dev_attr_cpu0_vid);
> -		device_create_file(dev, &dev_attr_vrm);
> -		device_create_file(dev, &dev_attr_alarms_in);
> +			add_tothe_group(dev, &in_input[i].dev_attr);
> +			add_tothe_group(dev, &in_min[i].dev_attr);
> +			add_tothe_group(dev, &in_max[i].dev_attr);
> +			add_tothe_group(dev, &in_status[i].dev_attr);
> +		}
> +		add_tothe_group(dev, &dev_attr_cpu0_vid);
> +		add_tothe_group(dev, &dev_attr_vrm);
> +		add_tothe_group(dev, &dev_attr_alarms_in);
>  	}
>  
>  	if (data->tempnr) {
>  		for (i = 0; i < data->tempnr; i++) {
> -			device_create_file(dev, &temp_input[i].dev_attr);
> -			device_create_file(dev, &temp_min[i].dev_attr);
> -			device_create_file(dev, &temp_max[i].dev_attr);
> -			device_create_file(dev, &temp_crit[i].dev_attr);
> -			device_create_file(dev, &temp_status[i].dev_attr);
> +			add_tothe_group(dev, &temp_input[i].dev_attr);
> +			add_tothe_group(dev, &temp_min[i].dev_attr);
> +			add_tothe_group(dev, &temp_max[i].dev_attr);
> +			add_tothe_group(dev, &temp_crit[i].dev_attr);
> +			add_tothe_group(dev, &temp_status[i].dev_attr);
>  		}
> -		device_create_file(dev, &dev_attr_alarms_temp);
> +		add_tothe_group(dev, &dev_attr_alarms_temp);
>  	}
>  
>  	if (data->innr = 14) {
>  		for (i = 0; i < 3; i++) {
> -			device_create_file(dev, &therm_input[i].dev_attr);
> -			device_create_file(dev, &therm_min[i].dev_attr);
> -			device_create_file(dev, &therm_max[i].dev_attr);
> -			device_create_file(dev, &therm_crit[i].dev_attr);
> -			device_create_file(dev, &therm_status[i].dev_attr);
> +			add_tothe_group(dev, &therm_input[i].dev_attr);
> +			add_tothe_group(dev, &therm_min[i].dev_attr);
> +			add_tothe_group(dev, &therm_max[i].dev_attr);
> +			add_tothe_group(dev, &therm_crit[i].dev_attr);
> +			add_tothe_group(dev, &therm_status[i].dev_attr);
>  		}
>  	}
>  
>  	for (i = 0; i < data->fannr; i++) {
>  		if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
> -			device_create_file(dev, &fan_input[i].dev_attr);
> -			device_create_file(dev, &fan_min[i].dev_attr);
> -			device_create_file(dev, &fan_div[i].dev_attr);
> -			device_create_file(dev, &fan_status[i].dev_attr);
> +			add_tothe_group(dev, &fan_input[i].dev_attr);
> +			add_tothe_group(dev, &fan_min[i].dev_attr);
> +			add_tothe_group(dev, &fan_div[i].dev_attr);
> +			add_tothe_group(dev, &fan_status[i].dev_attr);
>  		}
>  		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
> -			device_create_file(dev, &pwm[i].dev_attr);
> +			add_tothe_group(dev, &pwm[i].dev_attr);
>  	}
>  
> +	/* Register sysfs hooks for the group */
> +	err = sysfs_create_group(&client->dev.kobj, &my_group);
> +	if (err) {
> +		dev_err(&client->dev, "group-create failed: %d, %d members\n",
> +			err, next_member);
> +		goto ERROR3;
> +	}
> +	
>  	return 0;
>  
> +	/* ERROR4:
> +	   sysfs_remove_group(&client->dev.kobj, &my_group);
> +	*/
>  ERROR3:
>  	i2c_detach_client(client);
>  ERROR2:
> @@ -1006,6 +1094,7 @@ static int pc87360_detach_client(struct 
>  	struct pc87360_data *data = i2c_get_clientdata(client);
>  	int i;
>  
> +	sysfs_remove_group(&client->dev.kobj, &my_group);
>  	hwmon_device_unregister(data->class_dev);
>  
>  	if ((i = i2c_detach_client(client)))

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jean Delvare]

Hi Mark, Jim,

> * Jim Cromie <jim.cromie at gmail.com> [2006-08-03 13:40:59 -0600]:
> > Im completely impressed by how clean Mark's patch is.
> > In the end, the elegance seduced me, patch follows.
> > 
> > in the start, I cut-pasted my own remove-bunch/group.
> > its if-0'd now, to be yanked..
> > 
> > In contrast with Mark's declarative grouping, Im doing it at runtime:
> >      pc87360_detect()
> >             calls add_tothe_group() to add it to one_big_group[] for 
> > each sensor-attr wanted,
> >              then calls sysfs_create_group()
> 
> What is the point of generating the group at runtime?

Having a single file group to create and delete. I made something
similar for the pcf8591 driver (although I didn't publish that patch
yet.)

> > Its probly worth noting that pc87360 now does:    hwmon_register b4 
> > sysfs_create_group
> > whereas Mark's patch does the opposite.   Does this matter ??
> 
> I changed the ordering in asb100 on purpose... it closes the small window
> of time between when the hwmon class is registered and the device attributes
> are populated.  It's something of a race condition... sensors(1) could run
> in the middle of that.

And I second that change. We should fix all drivers in a similar way,
before we add feature detection capability to libsensors.

> > --- ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c	2006-06-17 19:49:35.000000000 -0600
> > +++ sysdev/drivers/hwmon/pc87360.c	2006-08-03 12:58:29.000000000 -0600
> > @@ -829,6 +829,83 @@ static int __init pc87360_find(int sioad
> >  	return 0;
> >  }
> >  
> > +/* Declare and use an attribute-group for simplicity.
> > +   This driver declares all attributes statically, so we count uses of 
> > +   SENSOR_ATTR, DEVICE_ATTR, and add 1 space for trailing NULL (+0 fudge)
> > +*/
> > +#define ENOUGH_ATTRS	89 + 4 + 1 + 0

My experience is that this kind of construct is likely to break as soon
as the driver evolves :( I see you check carefully so no overflow can
happen, good, but still...

> > +static struct attribute *one_big_group[ENOUGH_ATTRS];
> > +static int next_member;
> > +
> > +static struct attribute_group my_group = {
> > +	.attrs = one_big_group
> > +};
> > +
> > +static void add_tothe_group(struct device *dev,
> > +				struct device_attribute *devattr)
> > +{
> > +	/* add attr to the group for later sysfs_create_group */
> > +	if (next_member < ENOUGH_ATTRS)
> > +		one_big_group[next_member++] = &devattr->attr;
> > +	else {
> > +		BUG_ON("too many in group for static alloc!\n");
> > +		dev_err(dev, "too many in group for static alloc!\n");
> > +	}
> > +}
> > +
> > +#if 0
> > +static void remove_all_devattr_files(struct device *dev)
> > +{
> > +	int i;
> > +	/* Mark Hoffman used sysfs_remove_group here, it nicely tracks
> > +	   group membership, at cost of array of pointers.  For now,
> > +	   stick w non-group approach - cost is cut-paste in-elegance
> > +	   & maint if sensor set changes */
> > +
> > +	dev_info(dev, "created %d attr-files, w errs %d.  now destroying\n",
> > +		 next_member, devattr_file_create_errs);
> > +
> > +	/* tiny cheat - rely on size equivalence of {type}_{attr}[]
> > +	   arrays across all attrs for each type (declared that way)
> > +	*/
> > +	for (i = 0; i < ARRAY_SIZE(in_input); i++) {
> > +		device_remove_file(dev, &in_input[i].dev_attr);
> > +		device_remove_file(dev, &in_min[i].dev_attr);
> > +		device_remove_file(dev, &in_max[i].dev_attr);
> > +		device_remove_file(dev, &in_status[i].dev_attr);
> > +	}
> > +	device_remove_file(dev, &dev_attr_cpu0_vid);
> > +	device_remove_file(dev, &dev_attr_vrm);
> > +	device_remove_file(dev, &dev_attr_alarms_in);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(temp_input); i++) {
> > +		device_remove_file(dev, &temp_input[i].dev_attr);
> > +		device_remove_file(dev, &temp_min[i].dev_attr);
> > +		device_remove_file(dev, &temp_max[i].dev_attr);
> > +		device_remove_file(dev, &temp_crit[i].dev_attr);
> > +		device_remove_file(dev, &temp_status[i].dev_attr);
> > +	}
> > +	device_remove_file(dev, &dev_attr_alarms_temp);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(therm_input); i++) {
> > +		device_remove_file(dev, &therm_input[i].dev_attr);
> > +		device_remove_file(dev, &therm_min[i].dev_attr);
> > +		device_remove_file(dev, &therm_max[i].dev_attr);
> > +		device_remove_file(dev, &therm_crit[i].dev_attr);
> > +		device_remove_file(dev, &therm_status[i].dev_attr);
> > +	}
> > +	for (i = 0; i < ARRAY_SIZE(fan_input); i++) {
> > +		device_remove_file(dev, &fan_input[i].dev_attr);
> > +		device_remove_file(dev, &fan_min[i].dev_attr);
> > +		device_remove_file(dev, &fan_div[i].dev_attr);
> > +		device_remove_file(dev, &fan_status[i].dev_attr);
> > +		device_remove_file(dev, &pwm[i].dev_attr);
> > +	}
> > +
> > +	dev_info(dev, "remaining attr-files %d\n", next_member);
> > +}
> > +#endif
> > +
> >  static int pc87360_detect(struct i2c_adapter *adapter)
> 
> You can't fill your static array from here.  What if you have two
> pc87360s?  If you're going to do it at runtime, it has to be during
> module init.  But again, why do it at runtime in the first place?

There will never be two PC87360s in a machine, that wouldn't make
sense. But this raises a concern about Jim's approach: it only works
for drivers which can handle a single chip, or at least where all chips
are guaranteed to share the exact same configuration. I took that
shortcut for the PCF8591 as well, but on second though it's plain
wrong. This doesn't fit in the device driver model at all. Given that
we want to convert all i2c-isa drivers to platform drivers at the end
of this year, and the i2c subsystem to the driver model hopefully even
before that, we are shooting ourselves in the feet if we go Jim's (and
my) way.

Mark's approach is magnitudes better.

Jim, sorry about that, nothing personal ;) Thanks for your contribution
anyway, I'm certain it helps Mark refine his own excellent ideas :)

-- 
Jean Delvare

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jim Cromie]

hi folks,
(trimming aggressively)

>> What is the point of generating the group at runtime?
>>     
>
> Having a single file group to create and delete. I made something
> similar for the pcf8591 driver (although I didn't publish that patch
> yet.)
>
>   
and further, that the number of sensors changes by chip model in some cases.


>> I changed the ordering in asb100 on purpose... it closes the small window
>> of time between when the hwmon class is registered and the device attributes
>> are populated.  It's something of a race condition... sensors(1) could run
>> in the middle of that.
>>     
>
> And I second that change. We should fix all drivers in a similar way,
> before we add feature detection capability to libsensors.
>
>   
Cool.  Observation becomes Knowledge   :-D

>>> --- ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c	2006-06-17 19:49:35.000000000 -0600
>>> +++ sysdev/drivers/hwmon/pc87360.c	2006-08-03 12:58:29.000000000 -0600
>>> @@ -829,6 +829,83 @@ static int __init pc87360_find(int sioad
>>>  	return 0;
>>>  }
>>>  
>>> +/* Declare and use an attribute-group for simplicity.
>>> +   This driver declares all attributes statically, so we count uses of 
>>> +   SENSOR_ATTR, DEVICE_ATTR, and add 1 space for trailing NULL (+0 fudge)
>>> +*/
>>> +#define ENOUGH_ATTRS	89 + 4 + 1 + 0
>>>       
>
> My experience is that this kind of construct is likely to break as soon
> as the driver evolves :( I see you check carefully so no overflow can
> happen, good, but still...
>
>   
I was never comfortable with that, hence the goofy symbol names.
>>
>> You can't fill your static array from here.  What if you have two
>> pc87360s?  If you're going to do it at runtime, it has to be during
>> module init.  But again, why do it at runtime in the first place?
>>     
>
> There will never be two PC87360s in a machine, that wouldn't make
> sense. But this raises a concern about Jim's approach: it only works
> for drivers which can handle a single chip, or at least where all chips
> are guaranteed to share the exact same configuration. I took that
> shortcut for the PCF8591 as well, but on second though it's plain
> wrong. This doesn't fit in the device driver model at all. Given that
> we want to convert all i2c-isa drivers to platform drivers at the end
> of this year, and the i2c subsystem to the driver model hopefully even
> before that, we are shooting ourselves in the feet if we go Jim's (and
> my) way.
>
> Mark's approach is magnitudes better.
>
> Jim, sorry about that, nothing personal ;) Thanks for your contribution
> anyway, I'm certain it helps Mark refine his own excellent ideas :)
>
>   

Im not sure which parts bother you most, so I'll break it down as I see it.

1.  static decls of ATTRs  intrinsically limits driver to 1 instance, I 
agree.

As you note, the chip wouldnt be used 2x on a board, so designing for that
wasnt valuable (Im guessing).  I dont see the value now in re-doing the 
driver
to change this, unless theres something about the migration plan ( that
has effects, requirements that I dont appreciate yet)

2.  Previous patch built groups at runtime - thats now gone.

This patch version declares a separate sub-group for each sensor-type,
which allows the same runtime-choices, but now creating attr-groups,
instead of looping thru and creating single attrs.
As I see it, this copies Marks approach closely.

patch includes a code-move to bring 3 volts-related attrs and callbacks up
with the code for VIN, so theyre in-scope for the vin-attr-grp declaration.

Its had cursory testing, will test over next few days.

--------

Convert loops of device_create_file() calls, with unchecked return codes,
into declarations of attribute groups per sensor-type, and a call
to sysfs-create-group() for each.

Signed-off-by:  Jim Cromie  jim.cromie at gmail.com

[jimc at harpo hwmon-stuff]$ diffstat diff.sys-grp.20060815.223612
 pc87360.c |  216 
++++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 142 insertions(+), 74 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ../linux-2.6.18-rc4-mm1-sk/drivers/hwmon/pc87360.c sys-grp/drivers/hwmon/pc87360.c
--- ../linux-2.6.18-rc4-mm1-sk/drivers/hwmon/pc87360.c	2006-08-14 21:19:24.000000000 -0600
+++ sys-grp/drivers/hwmon/pc87360.c	2006-08-15 22:46:16.000000000 -0600
@@ -327,6 +327,12 @@ static struct sensor_device_attribute fa
 	SENSOR_ATTR(fan3_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 2),
 };
 
+#define FAN_UNIT_ATTRS(X)	\
+	&fan_input[X].dev_attr.attr,	\
+	&fan_status[X].dev_attr.attr,	\
+	&fan_div[X].dev_attr.attr,	\
+	&fan_min[X].dev_attr.attr
+
 static ssize_t show_pwm(struct device *dev, struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -359,6 +365,19 @@ static struct sensor_device_attribute pw
 	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 2),
 };
 
+static struct attribute * pc8736x_fan_attr_array[] = {
+	FAN_UNIT_ATTRS(0),
+	FAN_UNIT_ATTRS(1),
+	FAN_UNIT_ATTRS(2),
+	&pwm[0].dev_attr.attr,
+	&pwm[1].dev_attr.attr,
+	&pwm[2].dev_attr.attr,
+	NULL
+};
+static const struct attribute_group pc8736x_fan_group = {
+	.attrs = pc8736x_fan_attr_array,
+};
+
 static ssize_t show_in_input(struct device *dev, struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -471,6 +490,61 @@ static struct sensor_device_attribute in
 	SENSOR_ATTR(in10_max, S_IWUSR | S_IRUGO, show_in_max, set_in_max, 10),
 };
 
+#define VIN_UNIT_ATTRS(X) \
+	&in_input[X].dev_attr.attr,	\
+	&in_status[X].dev_attr.attr,	\
+	&in_min[X].dev_attr.attr,	\
+	&in_max[X].dev_attr.attr
+
+static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", vid_from_reg(data->vid, data->vrm));
+}
+static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
+
+static ssize_t show_vrm(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", data->vrm);
+}
+static ssize_t set_vrm(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	data->vrm = simple_strtoul(buf, NULL, 10);
+	return count;
+}
+static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
+
+static ssize_t show_in_alarms(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", data->in_alarms);
+}
+static DEVICE_ATTR(alarms_in, S_IRUGO, show_in_alarms, NULL);
+
+static struct attribute *pc8736x_vin_attr_array[] = {
+	VIN_UNIT_ATTRS(0),
+	VIN_UNIT_ATTRS(1),
+	VIN_UNIT_ATTRS(2),
+	VIN_UNIT_ATTRS(3),
+	VIN_UNIT_ATTRS(4),
+	VIN_UNIT_ATTRS(5),
+	VIN_UNIT_ATTRS(6),
+	VIN_UNIT_ATTRS(7),
+	VIN_UNIT_ATTRS(8),
+	VIN_UNIT_ATTRS(9),
+	VIN_UNIT_ATTRS(10),
+	&dev_attr_cpu0_vid.attr,
+	&dev_attr_vrm.attr,
+	&dev_attr_alarms_in.attr,
+	NULL
+};
+static const struct attribute_group pc8736x_vin_group = {
+	.attrs = pc8736x_vin_attr_array,
+};
+
 static ssize_t show_therm_input(struct device *dev, struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -589,33 +663,22 @@ static struct sensor_device_attribute th
 		    show_therm_crit, set_therm_crit, 2+11),
 };
 
-static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct pc87360_data *data = pc87360_update_device(dev);
-	return sprintf(buf, "%u\n", vid_from_reg(data->vid, data->vrm));
-}
-static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
-
-static ssize_t show_vrm(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct pc87360_data *data = pc87360_update_device(dev);
-	return sprintf(buf, "%u\n", data->vrm);
-}
-static ssize_t set_vrm(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct pc87360_data *data = i2c_get_clientdata(client);
-	data->vrm = simple_strtoul(buf, NULL, 10);
-	return count;
-}
-static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
-
-static ssize_t show_in_alarms(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct pc87360_data *data = pc87360_update_device(dev);
-	return sprintf(buf, "%u\n", data->in_alarms);
-}
-static DEVICE_ATTR(alarms_in, S_IRUGO, show_in_alarms, NULL);
+#define THERM_UNIT_ATTRS(X) \
+	&therm_input[X].dev_attr.attr,	\
+	&therm_status[X].dev_attr.attr,	\
+	&therm_min[X].dev_attr.attr,	\
+	&therm_max[X].dev_attr.attr,	\
+	&therm_crit[X].dev_attr.attr
+
+static struct attribute * pc8736x_therm_attr_array[] = {
+	THERM_UNIT_ATTRS(0),
+	THERM_UNIT_ATTRS(1),
+	THERM_UNIT_ATTRS(2),
+	NULL
+};
+static const struct attribute_group pc8736x_therm_group = {
+	.attrs = pc8736x_therm_attr_array,
+};
 
 static ssize_t show_temp_input(struct device *dev, struct device_attribute *devattr, char *buf)
 {
@@ -735,6 +798,25 @@ static ssize_t show_temp_alarms(struct d
 }
 static DEVICE_ATTR(alarms_temp, S_IRUGO, show_temp_alarms, NULL);
 
+#define TEMP_UNIT_ATTRS(X) \
+	&temp_input[X].dev_attr.attr,	\
+	&temp_status[X].dev_attr.attr,	\
+	&temp_min[X].dev_attr.attr,	\
+	&temp_max[X].dev_attr.attr,	\
+	&temp_crit[X].dev_attr.attr
+
+static struct attribute * pc8736x_temp_attr_array[] = {
+	TEMP_UNIT_ATTRS(0),
+	TEMP_UNIT_ATTRS(1),
+	TEMP_UNIT_ATTRS(2),
+	/* include the few miscellaneous atts here */
+	&dev_attr_alarms_temp.attr,
+	NULL
+};
+static const struct attribute_group pc8736x_temp_group = {
+	.attrs = pc8736x_temp_attr_array,
+};
+
 /*
  * Device detection, registration and update
  */
@@ -935,67 +1017,46 @@ static int pc87360_detect(struct i2c_ada
 		pc87360_init_client(client, use_thermistors);
 	}
 
-	/* Register sysfs hooks */
-	data->class_dev = hwmon_device_register(&client->dev);
-	if (IS_ERR(data->class_dev)) {
-		err = PTR_ERR(data->class_dev);
+	/* Register sysfs groups */
+
+	if (data->innr && 
+	    sysfs_create_group(&client->dev.kobj,
+			       &pc8736x_vin_group))
 		goto ERROR3;
-	}
 
-	if (data->innr) {
-		for (i = 0; i < 11; i++) {
-			device_create_file(dev, &in_input[i].dev_attr);
-			device_create_file(dev, &in_min[i].dev_attr);
-			device_create_file(dev, &in_max[i].dev_attr);
-			device_create_file(dev, &in_status[i].dev_attr);
-		}
-		device_create_file(dev, &dev_attr_cpu0_vid);
-		device_create_file(dev, &dev_attr_vrm);
-		device_create_file(dev, &dev_attr_alarms_in);
-	}
+	if (data->tempnr &&
+	    sysfs_create_group(&client->dev.kobj, 
+			       &pc8736x_temp_group))
+		goto ERROR3;
 
-	if (data->tempnr) {
-		for (i = 0; i < data->tempnr; i++) {
-			device_create_file(dev, &temp_input[i].dev_attr);
-			device_create_file(dev, &temp_min[i].dev_attr);
-			device_create_file(dev, &temp_max[i].dev_attr);
-			device_create_file(dev, &temp_crit[i].dev_attr);
-			device_create_file(dev, &temp_status[i].dev_attr);
-		}
-		device_create_file(dev, &dev_attr_alarms_temp);
-	}
+	if (data->innr = 14 &&
+	    sysfs_create_group(&client->dev.kobj, 
+			       &pc8736x_therm_group))
+		goto ERROR3;
 
-	if (data->innr = 14) {
-		for (i = 0; i < 3; i++) {
-			device_create_file(dev, &therm_input[i].dev_attr);
-			device_create_file(dev, &therm_min[i].dev_attr);
-			device_create_file(dev, &therm_max[i].dev_attr);
-			device_create_file(dev, &therm_crit[i].dev_attr);
-			device_create_file(dev, &therm_status[i].dev_attr);
-		}
-	}
+	if (data->fannr &&
+	    sysfs_create_group(&client->dev.kobj,
+			       &pc8736x_fan_group))
+		goto ERROR3;
 
-	for (i = 0; i < data->fannr; i++) {
-		if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
-			device_create_file(dev, &fan_input[i].dev_attr);
-			device_create_file(dev, &fan_min[i].dev_attr);
-			device_create_file(dev, &fan_div[i].dev_attr);
-			device_create_file(dev, &fan_status[i].dev_attr);
-		}
-		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
-			device_create_file(dev, &pwm[i].dev_attr);
+	data->class_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->class_dev)) {
+		err = PTR_ERR(data->class_dev);
+		goto ERROR3;
 	}
-
 	return 0;
 
 ERROR3:
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_temp_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_fan_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_therm_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_vin_group);
+
 	i2c_detach_client(client);
 ERROR2:
-	for (i = 0; i < 3; i++) {
-		if (data->address[i]) {
+	for (i = 0; i < 3; i++)
+		if (data->address[i])
 			release_region(data->address[i], PC87360_EXTENT);
-		}
-	}
 ERROR1:
 	kfree(data);
 	return err;
@@ -1008,16 +1069,19 @@ static int pc87360_detach_client(struct 
 
 	hwmon_device_unregister(data->class_dev);
 
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_temp_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_fan_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_therm_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_vin_group);
+
 	if ((i = i2c_detach_client(client)))
 		return i;
 
-	for (i = 0; i < 3; i++) {
-		if (data->address[i]) {
+	for (i = 0; i < 3; i++)
+		if (data->address[i])
 			release_region(data->address[i], PC87360_EXTENT);
-		}
-	}
-	kfree(data);
 
+	kfree(data);
 	return 0;
 }
 

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jean Delvare]

Hi Jim,

> Im not sure which parts bother you most, so I'll break it down as I see it.
> 
> 1.  static decls of ATTRs  intrinsically limits driver to 1 instance, I 
> agree.
> 
> As you note, the chip wouldnt be used 2x on a board, so designing for that
> wasnt valuable (Im guessing).  I dont see the value now in re-doing the 
> driver
> to change this, unless theres something about the migration plan ( that
> has effects, requirements that I dont appreciate yet)

All drivers for Super-I/O chips based on i2c-isa suffer from that
problem. i2c-isa doesn't provide a way to transfer Super-I/O
configuration data up to the device registration function, other than
globals. But once we convert them to platform drivers, things will get
much better and cleaner.

Now I agree that, even then, we probably will never see two Super-I/O
chip on the same board, so that's not really an issue.

> 2.  Previous patch built groups at runtime - thats now gone.
> 
> This patch version declares a separate sub-group for each sensor-type,
> which allows the same runtime-choices, but now creating attr-groups,
> instead of looping thru and creating single attrs.
> As I see it, this copies Marks approach closely.
> 
> patch includes a code-move to bring 3 volts-related attrs and callbacks up
> with the code for VIN, so theyre in-scope for the vin-attr-grp declaration.
> 
> Its had cursory testing, will test over next few days.

Yeah, I like this one much better, however it does change the code,
which isn't OK. Not all chips have 3 fans, and not all chips have 3
temperature sensors. The original code did only create the files for
existing inputs, and I'd like the new code to do the same. It shouldn't
be too difficult, see how Mark did for the w83627hf driver, it looks
nice enough to me. For these entries you'll have to keep registering
them manually, even though you can delete them as a group.

You can probably avoid moving the code around by adding the arrays of
pointers at the end of the attribute declarations.

Thanks,
-- 
Jean Delvare

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
hi !

BTW - are you considering these as 18 bugfix material, or are they
"long standing sub-optimalities" for 19 when it opens ?  (ie when 18 is out)

Obviously (from the experimentalism in my patches), Ive been treating it 
as 19 stuff ;-)
Apologies for making this more *in-need-of-feedback* than it has to be,
but I guess I cant quite resist..

>
> Now I agree that, even then, we probably will never see two 
of the same kind of  (not that the distinction matters here..)
> Super-I/O
> chip on the same board, so that's not really an issue.
>
>   

Um.. I just looked at asb100.c, and Im seeing static decls like:
        static DEVICE_ATTR(..)

Unless Im misunderstanding something, this is sufficient to preclude 
supporting a 2nd device.
IOW, to support multiple devices, drivers would need to create 
attributes, groups, etc out of
kalloc'd memory, sacrificing the (heavy) use of static initialization in 
hwmon/*.c

[jimc at harpo hwmon-stuff]$ grep -rn ATTR grp2/drivers/hwmon/* |wc
   1097    6301  102150


>> 2.  Previous patch built groups at runtime - thats now gone.
>>
>> This patch version declares a separate sub-group for each sensor-type,
>> which allows the same runtime-choices, but now creating attr-groups,
>> instead of looping thru and creating single attrs.
>> As I see it, this copies Marks approach closely.
>>
>> patch includes a code-move to bring 3 volts-related attrs and callbacks up
>> with the code for VIN, so theyre in-scope for the vin-attr-grp declaration.
>>
>> Its had cursory testing, will test over next few days.
>>     
>
> Yeah, I like this one much better, however it does change the code,
> which isn't OK. Not all chips have 3 fans, and not all chips have 3
> temperature sensors. 
Oops yes.
  
I noted the fixed-length loop in these:
        if (data->innr) {
                for (i = 0; i < 11; i++) {
        if (data->innr = 14) {
                for (i = 0; i < 3; i++) {

but overlooked the variant length in these:

        if (data->tempnr) {
                for (i = 0; i < data->tempnr; i++) {..}
        for (i = 0; i < data->fannr; i++) {..}

> The original code did only create the files for
> existing inputs, and I'd like the new code to do the same. It shouldn't
> be too difficult, see how Mark did for the w83627hf driver, it looks
> nice enough to me. For these entries you'll have to keep registering
> them manually, even though you can delete them as a group.
>
>   

Yeah, I guess so.   The only alternative is  *un-conservative*

1 - teach sysfs_create_group(or callees) to selectively create files for 
its members, based upon:
          mode - no read permissions
          or show-handler set to NULL (currently checked, near EACCESS iirc)
      cc'd gregkh for quick execution-with-prejudice, if guilty :-O

2. prior to calling sysfs_create_group() for a group of optional 'files'
    must disable the un-needed files, by zeroing attr->mode.
    fwiw - the disable is slightly weird, but is due to 
initialize-as-enabled design

SO - One more experimental patch - following outline above.
Patch is incremental upon the last one (for easy review ;)
and for its good isolation of the possibly controversial usage of the 
mode field.

Just to reiterate:

I'm resetting attr.mode = 0 for some members, and altering 
sysfs_create_group() to
not create files for disabled/reset members.  It gives us an easy way to
tailor runtime group population while preserving compile-time group 
composition.

This is not in conflict with any static uses Im aware of (FWIW),
the patch only affects groups, not individual device_create_file() uses.
I havent tried repeatedly creating and removing groups, but
I dont see that having any chance of exposing other bugs.
(caveats apply ;-)

> You can probably avoid moving the code around by adding the arrays of
> pointers at the end of the attribute declarations.
>
>   
I thought the move had its own merit, since the moved stuff pertains to 
the voltage 'group'
hence the code belongs together.  ATM, the 'connection' between them is 
evident only
at the end of the (previously mentioned) code-block doing 
device_create_file()s:
       if (data->innr) {... }

IOW, moving it improves existing code organization, ie fit to this model:
for 4 sensor types, code has:
- callbacks,
- static-decls of ATTRs, with initializations referencing those callbacks.
- (now) a group of those ATTRs, tucked in with related code.

(acceptable as separate pre-patch ? ;)



> Thanks,
>   

thanks Jean,
-jimc



diff -ruNp -X dontdiff -X exclude-diffs sys-grp/drivers/hwmon/pc87360.c grp2/drivers/hwmon/pc87360.c
--- sys-grp/drivers/hwmon/pc87360.c	2006-08-16 07:53:00.000000000 -0600
+++ grp2/drivers/hwmon/pc87360.c	2006-08-17 11:54:59.000000000 -0600
@@ -1020,7 +1020,32 @@ static int pc87360_detect(struct i2c_ada
 		pc87360_init_client(client, use_thermistors);
 	}
 
-	/* Register sysfs groups */
+	/* disable group members which arent supported by the
+	   available hardware.  For efficiency, we handle all-or-none
+	   groups separately below, and more variant groups here
+	 */
+
+#	define disable_attr(X) 	(X).dev_attr.attr.mode = 0
+
+	for (i = 0; i < ARRAY_SIZE(fan_input); i++) {
+		if (!FAN_CONFIG_MONITOR(data->fan_conf, i)) {
+			disable_attr(fan_input[i]);
+			disable_attr(fan_min[i]);
+			disable_attr(fan_div[i]);
+			disable_attr(fan_status[i]);
+		}
+		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
+			disable_attr(pwm[i]);
+	}
+	for (i = data->tempnr; i && i < ARRAY_SIZE(temp_input); i++) {
+		disable_attr(temp_input[i]);
+		disable_attr(temp_min[i]);
+		disable_attr(temp_max[i]);
+		disable_attr(temp_crit[i]);
+		disable_attr(temp_status[i]);
+	}
+
+	/* Register sysfs groups for available hardware */
 
 	if (data->innr && 
 	    sysfs_create_group(&client->dev.kobj,
diff -ruNp -X dontdiff -X exclude-diffs sys-grp/fs/sysfs/group.c grp2/fs/sysfs/group.c
--- sys-grp/fs/sysfs/group.c	2006-06-17 19:49:35.000000000 -0600
+++ grp2/fs/sysfs/group.c	2006-08-17 11:14:57.000000000 -0600
@@ -32,7 +32,8 @@ static int create_files(struct dentry * 
 	int error = 0;
 
 	for (attr = grp->attrs; *attr && !error; attr++) {
-		error = sysfs_add_file(dir, *attr, SYSFS_KOBJ_ATTR);
+		if ((*attr)->mode)
+			error = sysfs_add_file(dir, *attr, SYSFS_KOBJ_ATTR);
 	}
 	if (error)
 		remove_files(dir,grp);

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Greg KH]

On Thu, Aug 17, 2006 at 01:35:30PM -0600, Jim Cromie wrote:
> 
> Jean Delvare wrote:
> >Hi Jim,
> >
> >  
> hi !
> 
> BTW - are you considering these as 18 bugfix material, or are they
> "long standing sub-optimalities" for 19 when it opens ?  (ie when 18 is out)
> 
> Obviously (from the experimentalism in my patches), Ive been treating it 
> as 19 stuff ;-)
> Apologies for making this more *in-need-of-feedback* than it has to be,
> but I guess I cant quite resist..
> 
> >
> >Now I agree that, even then, we probably will never see two 
> of the same kind of  (not that the distinction matters here..)
> >Super-I/O
> >chip on the same board, so that's not really an issue.
> >
> >  
> 
> Um.. I just looked at asb100.c, and Im seeing static decls like:
>        static DEVICE_ATTR(..)
> 
> Unless Im misunderstanding something, this is sufficient to preclude 
> supporting a 2nd device.
> IOW, to support multiple devices, drivers would need to create 
> attributes, groups, etc out of
> kalloc'd memory, sacrificing the (heavy) use of static initialization in 
> hwmon/*.c

No, you are incorrect, it will work just fine.  The dynamic thing is the
struct device, not the functions that make up the file callbacks.

thanks,

greg k-h

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jim Cromie]

Greg KH wrote:
> On Thu, Aug 17, 2006 at 01:35:30PM -0600, Jim Cromie wrote:
>   
>> Jean Delvare wrote:
>>     
>>> Hi Jim,
>>>
>>>  
>>>       
>> hi !
>>
>> BTW - are you considering these as 18 bugfix material, or are they
>> "long standing sub-optimalities" for 19 when it opens ?  (ie when 18 is out)
>>
>> Obviously (from the experimentalism in my patches), Ive been treating it 
>> as 19 stuff ;-)
>> Apologies for making this more *in-need-of-feedback* than it has to be,
>> but I guess I cant quite resist..
>>
>>     
>>> Now I agree that, even then, we probably will never see two 
>>>       
>> of the same kind of  (not that the distinction matters here..)
>>     
>>> Super-I/O
>>> chip on the same board, so that's not really an issue.
>>>
>>>  
>>>       
>> Um.. I just looked at asb100.c, and Im seeing static decls like:
>>        static DEVICE_ATTR(..)
>>
>> Unless Im misunderstanding something, this is sufficient to preclude 
>> supporting a 2nd device.
>> IOW, to support multiple devices, drivers would need to create 
>> attributes, groups, etc out of
>> kalloc'd memory, sacrificing the (heavy) use of static initialization in 
>> hwmon/*.c
>>     
>
> No, you are incorrect, it will work just fine.  The dynamic thing is the
> struct device, not the functions that make up the file callbacks.
>
>   

Im not speaking of the callbacks themselves, rather the 
device_attributes that
keep references to them.  Assuming 2 'static DEVICE_ATTR()'s,
that are init'd with names "in0_max", "in1_max". theyre separate 
attributes,
reusing the same show,store functions, thru separate refs.  Correct ?

Are you saying that I could reuse a statically defined group (ie the 
device_attrs within it)
to create another set of attr-files ?

IOW - pc87360 creates this:

soekris:/sys/devices/platform/i2c-9191/9191-6620# ls
alarms_in      in1_input   in4_min     in8_input     temp2_crit    
temp4_status
alarms_temp    in1_max     in4_status  in8_max       temp2_input   
temp5_crit
bus@           in1_min     in5_input   in8_min       temp2_max     
temp5_input
cpu0_vid       in1_status  in5_max     in8_status    temp2_min     temp5_max
...

Could same static structures also support a 2nd set  ?   (assuming other 
limits are lifted)

soekris:/sys/devices/platform/i2c-(another-number)/(another-number)-6620# ls
alarms_in      in1_input   in4_min     in8_input     temp2_crit    
temp4_status
alarms_temp    in1_max     in4_status  in8_max       temp2_input   
temp5_crit

or would they need to be separate ? ( and hence better to be kallocd, 
then initialized )

> thanks,
>
> greg k-h
>
>   


Also, Id rather hoped you'd respond to the hack in fs/sysfs/group.c,
were you just being polite ? :-}


thanks
jimc

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Mark M. Hoffman]

Hi Jim:

> > On Thu, Aug 17, 2006 at 01:35:30PM -0600, Jim Cromie wrote:
> >> Um.. I just looked at asb100.c, and Im seeing static decls like:
> >>        static DEVICE_ATTR(..)
> >>
> >> Unless Im misunderstanding something, this is sufficient to preclude 
> >> supporting a 2nd device.
> >> IOW, to support multiple devices, drivers would need to create 
> >> attributes, groups, etc out of
> >> kalloc'd memory, sacrificing the (heavy) use of static initialization in 
> >> hwmon/*.c

> Greg KH wrote:
> > No, you are incorrect, it will work just fine.  The dynamic thing is the
> > struct device, not the functions that make up the file callbacks.

... nor the accompanying struct attributes, nor the struct attribute_group.

* Jim Cromie <jim.cromie at gmail.com> [2006-08-17 15:45:06 -0600]:
> Im not speaking of the callbacks themselves, rather the 
> device_attributes that
> keep references to them.  Assuming 2 'static DEVICE_ATTR()'s,
> that are init'd with names "in0_max", "in1_max". theyre separate 
> attributes,
> reusing the same show,store functions, thru separate refs.  Correct ?
> 
> Are you saying that I could reuse a statically defined group (ie the 
> device_attrs within it)
> to create another set of attr-files ?

I won't presume to speak for Greg, but yes I think that works fine too.

It's a reasonable question though.  I wondered about it for a little bit
when I saw that struct attribute_group seems to be missing two const
modifiers, i.e.:

--- linux-2.6.18-rc4-mm1.orig/include/linux/sysfs.h
+++ linux-2.6.18-rc4-mm1/include/linux/sysfs.h
@@ -24,7 +24,7 @@ struct attribute {
 
 struct attribute_group {
 	const char		* name;
-	struct attribute	** attrs;
+	const struct attribute	* const * attrs;
 };
 
But actually compiling a kernel with the above introduces a ton of warnings.
All those I examined could be fixed by adding const to struct attribute
declarations (AFAICT nobody is actually modifying any data through those
pointers), but I haven't looked at them all, and it's late here, blah...

Anyway, Jim if you look at the actual implementation of sysfs_add_group()
it should become apparent that the attribute structures can be shared.

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Greg KH]

On Thu, Aug 17, 2006 at 03:45:06PM -0600, Jim Cromie wrote:
> Greg KH wrote:
> >On Thu, Aug 17, 2006 at 01:35:30PM -0600, Jim Cromie wrote:
> >  
> >>Jean Delvare wrote:
> >>    
> >>>Hi Jim,
> >>>
> >>> 
> >>>      
> >>hi !
> >>
> >>BTW - are you considering these as 18 bugfix material, or are they
> >>"long standing sub-optimalities" for 19 when it opens ?  (ie when 18 is 
> >>out)
> >>
> >>Obviously (from the experimentalism in my patches), Ive been treating it 
> >>as 19 stuff ;-)
> >>Apologies for making this more *in-need-of-feedback* than it has to be,
> >>but I guess I cant quite resist..
> >>
> >>    
> >>>Now I agree that, even then, we probably will never see two 
> >>>      
> >>of the same kind of  (not that the distinction matters here..)
> >>    
> >>>Super-I/O
> >>>chip on the same board, so that's not really an issue.
> >>>
> >>> 
> >>>      
> >>Um.. I just looked at asb100.c, and Im seeing static decls like:
> >>       static DEVICE_ATTR(..)
> >>
> >>Unless Im misunderstanding something, this is sufficient to preclude 
> >>supporting a 2nd device.
> >>IOW, to support multiple devices, drivers would need to create 
> >>attributes, groups, etc out of
> >>kalloc'd memory, sacrificing the (heavy) use of static initialization in 
> >>hwmon/*.c
> >>    
> >
> >No, you are incorrect, it will work just fine.  The dynamic thing is the
> >struct device, not the functions that make up the file callbacks.
> >
> >  
> 
> Im not speaking of the callbacks themselves, rather the 
> device_attributes that
> keep references to them.  Assuming 2 'static DEVICE_ATTR()'s,
> that are init'd with names "in0_max", "in1_max". theyre separate 
> attributes, reusing the same show,store functions, thru separate refs.
> Correct ?

Yes.

> Are you saying that I could reuse a statically defined group (ie the 
> device_attrs within it) to create another set of attr-files ?

Yes.

> IOW - pc87360 creates this:
> 
> soekris:/sys/devices/platform/i2c-9191/9191-6620# ls
> alarms_in      in1_input   in4_min     in8_input     temp2_crit    
> temp4_status
> alarms_temp    in1_max     in4_status  in8_max       temp2_input   
> temp5_crit
> bus@           in1_min     in5_input   in8_min       temp2_max     
> temp5_input
> cpu0_vid       in1_status  in5_max     in8_status    temp2_min     temp5_max
> ...
> 
> Could same static structures also support a 2nd set  ?   (assuming other 
> limits are lifted)
> 
> soekris:/sys/devices/platform/i2c-(another-number)/(another-number)-6620# ls
> alarms_in      in1_input   in4_min     in8_input     temp2_crit    
> temp4_status
> alarms_temp    in1_max     in4_status  in8_max       temp2_input   
> temp5_crit

Yes.

> or would they need to be separate ? ( and hence better to be kallocd, 
> then initialized )

No.  Please see the driver core chapter in the Linux Device Drivers book
(free online) for more details here.

> Also, Id rather hoped you'd respond to the hack in fs/sysfs/group.c,
> were you just being polite ? :-}

Hm, I vaguely remember that, and I think I was probably just being
polite :)

But if you want me to take it seriously, please, resend it.  Conference
season is now over so I will have more time to focus back on kernel
issues.

thanks,

greg k-h

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Jean Delvare]

Jim,

> BTW - are you considering these as 18 bugfix material, or are they
> "long standing sub-optimalities" for 19 when it opens ?  (ie when 18 is out)
> 
> Obviously (from the experimentalism in my patches), Ive been treating it 
> as 19 stuff ;-)

Definitely not .18, as they don't fix any bug. So it's .19 material,
for what we can have ready before the .19 merge window. All drivers
need to be converted, this is a significant amount of work. Also the
change is likely to break other patches, so I don't want to wait for
all drivers to be converted to push all the patches - this would block
too many other patches. We can make several batches, one for .19 and
one or more after that.

A summary would be: ".19 at earliest, then as soon as possible, as time
permits". This is a background job, we have many other, more important
things in the works.

> Apologies for making this more *in-need-of-feedback* than it has to be,
> but I guess I cant quite resist..

No worry, getting feedback is the point of posting to a mailing list :)

-- 
Jean Delvare

[lm-sensors] [RFC-patch] pc87360 - unchecked

[Greg KH]

On Thu, Aug 17, 2006 at 10:45:03PM -0400, Mark M. Hoffman wrote:
> It's a reasonable question though.  I wondered about it for a little bit
> when I saw that struct attribute_group seems to be missing two const
> modifiers, i.e.:
> 
> --- linux-2.6.18-rc4-mm1.orig/include/linux/sysfs.h
> +++ linux-2.6.18-rc4-mm1/include/linux/sysfs.h
> @@ -24,7 +24,7 @@ struct attribute {
>  
>  struct attribute_group {
>  	const char		* name;
> -	struct attribute	** attrs;
> +	const struct attribute	* const * attrs;
>  };
>  
> But actually compiling a kernel with the above introduces a ton of warnings.

It shouldn't make that many warnings, but yeah, that would be the
correct fix.

> All those I examined could be fixed by adding const to struct attribute
> declarations (AFAICT nobody is actually modifying any data through those
> pointers), but I haven't looked at them all, and it's late here, blah...

Care to send a patch for this?  We are trying to fix up things like this
in the kernel to make it harder for people to do things wrong.

thanks,

greg k-h