[lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes

Ken Milmore ken at kenm.demon.co.uk
Mon May 3 14:47:30 CEST 2010


Hi Jean, thanks for the feedback.

Regarding the SENSORS_LIMIT(reqval, 0, 0xffff) in store_in8() which you 
have tagged as spurious, I believe it is necessary to prevent an 
overflow situation if an input value were provided such that reqval * 
0xc0 > LONG_MAX.  I should probably have added a comment to this effect. 
  The second SENSORS_LIMIT() then limits the result of the scaling 
calculation to the register range 0 - 0xff.  To get away with a single 
SENSORS_LIMIT(), it would be necessary guard to against the full-scale 
value mapping to 256, which won't fit in the register.  This could be 
accomplished like this:

SENSORS_LIMIT(reqval, 0, ((asc7621_in_scaling[nr] * 0x100) / 0xc0) - 1);

- but that just looks far too obscure to me, and not worth the bother. 
Please let me know if you think otherwise.

In general I've been very cautious in preparing the patch to only change 
lines of code that were demonstrably broken, rather than fixing up 
stylistic issues.  For example, the temperature scaling calculation is 
done in what is IMHO a rather bizarre fashion, but it doesn't actually 
get the wrong answers so I have left well alone.  The same goes for use 
of signed and unsigned types: personally, I'd strongly prefer to see 
long, simple_strtol() and sprintf("%ld") used throughout the driver for 
the scaled values, as I think consistency makes for more maintainable 
code.  But I'm happy to go with your suggested changes all the same.

Lastly, regarding the PWM control settings, I will take a look at what 
can be done to make these comply better with the sysfs-interface.  IMHO 
there is no completely clean solution here; several of the 
sysfs-interface settings map onto the same bits of the same registers in 
the device, so they cannot be completely orthogonalised.

On a note of more general discussion:  I don't want to sound negative, 
but maybe the PWM control interface has been a step too far for the 
hwmon drivers.  For a device like the asc7621, the programmer has to 
bend over backwards to make it fit the published interface, simply 
because the settings must be poked into the device one at a time as they 
are changed.  For this kind of device, it would be better to build up a 
complete register profile and then stuff the whole thing into the device 
in one go.  This could be better handled by a userland library than a 
kernel driver...  Opinionated rant over. ;-)

Best wishes,

-Ken.


On 03/05/10 11:58, Jean Delvare wrote:
> Hi Ken,
>
> On Sat, 01 May 2010 14:08:20 +0100, Ken Milmore wrote:
>> Find patch attached; this should apply against Linus' 2.6 tree. The
>> issues fixed here have all been discussed before on the lm_sensors list
>> (see 07/2008 and 04/2010), so hopefully they are uncontroversial.
>
> Thanks for the patch. Review below.
>
>> Having taken a brief look at the PWM control inteface in the driver, I
>> can see there are a few rough edges there but it is probably too late to
>> address them now.  In particular the pwm[1-3]_enable and
>> pwm[1-3]_auto_channels settings tend to interpret "Fan control disabled"
>> as "Fan off", which doesn't accord with the principle of least surprise,
>> or with the sysfs-interface documentation.  IMHO "Fan full on" would
>> have been a much safer default for these settings...
>
> Indeed, and this is serious enough that I would take a patch fixing
> that right now. Especially pwm[1-3]_enable == 0 MUST switch fans to
> full speed, pwmconfig and fancontrol rely on this.
>
>>  From 5a9cc351ca11375a81082a06212ac45ac900b213 Mon Sep 17 00:00:00 2001
>> From: Ken Milmore<ken.milmore at googlemail.com>
>> Date: Sat, 1 May 2010 13:26:41 +0100
>> Subject: [PATCH] hwmon: (asc7621) Bug fixes
>>
>> * Allow fan minimum RPM to be set to zero without triggering alarms.
>> * Fix voltage scaling arithmetic and correct scale factors.
>> * Correct fan1-fan4 alarm bit shifts.
>> * Correct register address for temp3_smoothing_enable.
>> * Read the alarm registers with high priority.
>>
>> Signed-off-by: Ken Milmore<ken.milmore at googlemail.com>
>> ---
>>   drivers/hwmon/asc7621.c |   63 +++++++++++++++++++++++------------------------
>>   1 files changed, 31 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/hwmon/asc7621.c b/drivers/hwmon/asc7621.c
>> index 7f94810..0f388ad 100644
>> --- a/drivers/hwmon/asc7621.c
>> +++ b/drivers/hwmon/asc7621.c
>> @@ -268,8 +268,11 @@ static ssize_t store_fan16(struct device *dev,
>>   	if (strict_strtol(buf, 10,&reqval))
>
> It would make sense to use strtoul here instead. Negative fan speeds
> make no sense.
>
>>   		return -EINVAL;
>>
>> +	/* If a minimum RPM of zero is requested, then we set the register to
>> +	   0xffff. This value allows the fan to be stopped completely without
>> +	   generating an alarm. */
>>   	reqval =
>> -	    (SENSORS_LIMIT((reqval)<= 0 ? 0 : 5400000 / (reqval), 0, 65534));
>> +	    (reqval<= 0 ? 0xffff : SENSORS_LIMIT(5400000 / reqval, 0, 0xfffe));
>
> And then this becomes == 0.
>
>>
>>   	mutex_lock(&data->update_lock);
>>   	data->reg[param->msb[0]] = (reqval>>  8)&  0xff;
>> @@ -285,8 +288,9 @@ static ssize_t store_fan16(struct device *dev,
>>    * Voltages are scaled in the device so that the nominal voltage
>>    * is 3/4ths of the 0-255 range (i.e. 192).
>>    * If all voltages are 'normal' then all voltage registers will
>> - * read 0xC0.  This doesn't help us if we don't have a point of refernce.
>> - * The data sheet however provides us with the full scale value for each
>> + * read 0xC0.
>> + *
>> + * The data sheet provides us with the 3/4 scale value for each voltage
>>    * which is stored in in_scaling.  The sda->index parameter value provides
>>    * the index into in_scaling.
>>    *
>> @@ -295,7 +299,7 @@ static ssize_t store_fan16(struct device *dev,
>>    */
>>
>>   static int asc7621_in_scaling[] = {
>> -	3320, 3000, 4380, 6640, 16000
>> +	2500, 2250, 3300, 5000, 12000
>>   };
>>
>>   static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
>> @@ -306,19 +310,12 @@ static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
>>   	u8 nr = sda->index;
>>
>>   	mutex_lock(&data->update_lock);
>> -	regval = (data->reg[param->msb[0]] * asc7621_in_scaling[nr]) / 256;
>> -
>> -	/* The LSB value is a 2-bit scaling of the MSB's LSbit value.
>> -	 * I.E.  If the maximim voltage for this input is 6640 millivolts then
>> -	 * a MSB register value of 0 = 0mv and 255 = 6640mv.
>> -	 * A 1 step change therefore represents 25.9mv (6640 / 256).
>> -	 * The extra 2-bits therefore represent increments of 6.48mv.
>> -	 */
>> -	regval += ((asc7621_in_scaling[nr] / 256) / 4) *
>> -	    (data->reg[param->lsb[0]]>>  6);
>> -
>> +	regval = (data->reg[param->msb[0]]<<  8) | (data->reg[param->lsb[0]]);
>>   	mutex_unlock(&data->update_lock);
>>
>> +	/* The LSB value is a 2-bit scaling of the MSB's LSbit value. */
>> +	regval = (regval>>  6) * asc7621_in_scaling[nr] / (0xc0<<  2);
>> +
>
> Looks much better :)
>
>>   	return sprintf(buf, "%u\n", regval);
>>   }
>>
>> @@ -331,7 +328,7 @@ static ssize_t show_in8(struct device *dev, struct device_attribute *attr,
>>
>>   	return sprintf(buf, "%u\n",
>>   		       ((data->reg[param->msb[0]] *
>> -			 asc7621_in_scaling[nr]) / 256));
>> +			 asc7621_in_scaling[nr]) / 0xc0));
>>   }
>>
>>   static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
>> @@ -344,9 +341,11 @@ static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
>>   	if (strict_strtol(buf, 10,&reqval))
>>   		return -EINVAL;
>>
>> -	reqval = SENSORS_LIMIT(reqval, 0, asc7621_in_scaling[nr]);
>> +	reqval = SENSORS_LIMIT(reqval, 0, 0xffff);
>
> Do you really need this? There's another call to SENSORS_LIMIT() below,
> I thought it would be enough.
>
>> +
>> +	reqval = reqval * 0xc0 / asc7621_in_scaling[nr];
>>
>> -	reqval = (reqval * 255 + 128) / asc7621_in_scaling[nr];
>> +	reqval = SENSORS_LIMIT(reqval, 0, 0xff);
>>
>>   	mutex_lock(&data->update_lock);
>>   	data->reg[param->msb[0]] = reqval;
>> @@ -846,11 +845,11 @@ static struct asc7621_param asc7621_params[] = {
>>   	PWRITE(in3_max, 3, PRI_LOW, 0x4b, 0, 0, 0, in8),
>>   	PWRITE(in4_max, 4, PRI_LOW, 0x4d, 0, 0, 0, in8),
>>
>> -	PREAD(in0_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 0, bitmask),
>> -	PREAD(in1_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 1, bitmask),
>> -	PREAD(in2_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 2, bitmask),
>> -	PREAD(in3_alarm, 3, PRI_LOW, 0x41, 0, 0x01, 3, bitmask),
>> -	PREAD(in4_alarm, 4, PRI_LOW, 0x42, 0, 0x01, 0, bitmask),
>> +	PREAD(in0_alarm, 0, PRI_HIGH, 0x41, 0, 0x01, 0, bitmask),
>> +	PREAD(in1_alarm, 1, PRI_HIGH, 0x41, 0, 0x01, 1, bitmask),
>> +	PREAD(in2_alarm, 2, PRI_HIGH, 0x41, 0, 0x01, 2, bitmask),
>> +	PREAD(in3_alarm, 3, PRI_HIGH, 0x41, 0, 0x01, 3, bitmask),
>> +	PREAD(in4_alarm, 4, PRI_HIGH, 0x42, 0, 0x01, 0, bitmask),
>>
>>   	PREAD(fan1_input, 0, PRI_HIGH, 0x29, 0x28, 0, 0, fan16),
>>   	PREAD(fan2_input, 1, PRI_HIGH, 0x2b, 0x2a, 0, 0, fan16),
>> @@ -862,10 +861,10 @@ static struct asc7621_param asc7621_params[] = {
>>   	PWRITE(fan3_min, 2, PRI_LOW, 0x59, 0x58, 0, 0, fan16),
>>   	PWRITE(fan4_min, 3, PRI_LOW, 0x5b, 0x5a, 0, 0, fan16),
>>
>> -	PREAD(fan1_alarm, 0, PRI_LOW, 0x42, 0, 0x01, 0, bitmask),
>> -	PREAD(fan2_alarm, 1, PRI_LOW, 0x42, 0, 0x01, 1, bitmask),
>> -	PREAD(fan3_alarm, 2, PRI_LOW, 0x42, 0, 0x01, 2, bitmask),
>> -	PREAD(fan4_alarm, 3, PRI_LOW, 0x42, 0, 0x01, 3, bitmask),
>> +	PREAD(fan1_alarm, 0, PRI_HIGH, 0x42, 0, 0x01, 2, bitmask),
>> +	PREAD(fan2_alarm, 1, PRI_HIGH, 0x42, 0, 0x01, 3, bitmask),
>> +	PREAD(fan3_alarm, 2, PRI_HIGH, 0x42, 0, 0x01, 4, bitmask),
>> +	PREAD(fan4_alarm, 3, PRI_HIGH, 0x42, 0, 0x01, 5, bitmask),
>>
>>   	PREAD(temp1_input, 0, PRI_HIGH, 0x25, 0x10, 0, 0, temp10),
>>   	PREAD(temp2_input, 1, PRI_HIGH, 0x26, 0x15, 0, 0, temp10),
>> @@ -886,10 +885,10 @@ static struct asc7621_param asc7621_params[] = {
>>   	PWRITE(temp3_max, 2, PRI_LOW, 0x53, 0, 0, 0, temp8),
>>   	PWRITE(temp4_max, 3, PRI_LOW, 0x35, 0, 0, 0, temp8),
>>
>> -	PREAD(temp1_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 4, bitmask),
>> -	PREAD(temp2_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 5, bitmask),
>> -	PREAD(temp3_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 6, bitmask),
>> -	PREAD(temp4_alarm, 3, PRI_LOW, 0x43, 0, 0x01, 0, bitmask),
>> +	PREAD(temp1_alarm, 0, PRI_HIGH, 0x41, 0, 0x01, 4, bitmask),
>> +	PREAD(temp2_alarm, 1, PRI_HIGH, 0x41, 0, 0x01, 5, bitmask),
>> +	PREAD(temp3_alarm, 2, PRI_HIGH, 0x41, 0, 0x01, 6, bitmask),
>> +	PREAD(temp4_alarm, 3, PRI_HIGH, 0x43, 0, 0x01, 0, bitmask),
>>
>>   	PWRITE(temp1_source, 0, PRI_LOW, 0x02, 0, 0x07, 4, bitmask),
>>   	PWRITE(temp2_source, 1, PRI_LOW, 0x02, 0, 0x07, 0, bitmask),
>> @@ -898,7 +897,7 @@ static struct asc7621_param asc7621_params[] = {
>>
>>   	PWRITE(temp1_smoothing_enable, 0, PRI_LOW, 0x62, 0, 0x01, 3, bitmask),
>>   	PWRITE(temp2_smoothing_enable, 1, PRI_LOW, 0x63, 0, 0x01, 7, bitmask),
>> -	PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x64, 0, 0x01, 3, bitmask),
>> +	PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x63, 0, 0x01, 3, bitmask),
>>   	PWRITE(temp4_smoothing_enable, 3, PRI_LOW, 0x3c, 0, 0x01, 3, bitmask),
>>
>>   	PWRITE(temp1_smoothing_time, 0, PRI_LOW, 0x62, 0, 0x07, 0, temp_st),
>> --
>> 1.7.1
>>
>
>




More information about the lm-sensors mailing list