Re: [lm-sensors] [PATCH] adm1026: fix setting fan_div

[Gabriele Gorla <gorlik@penguintown.net>]

On Mon, Nov 29, 2010 at 06:29:08PM -0800, Phil Pokorny wrote:
> Thanks for the patch.
> 
> On Mon, Nov 29, 2010 at 5:49 PM, Gabriele Gorla <gorlik@penguintown.net>wrote:
> 
> > Prevent setting fan_div from stomping on other fans that share the same i2c
> > register.
> >
> 
> Ugh...  Wow, that was clearly wrong logic now that you point it out...

:-)


> > Also allow div=1 (this is allowed in the ADM1026 datasheet)
> 
> 
> The old code didn't throw an EINVAL on most invalid divisors, it just
> silently converted them to the next reasonable value in the way that
> DIV_TO_REG worked.  If you're going to change the behavior and throw EINVAL
> for divisors greater than 8, perhaps you should check explicilty for the
> four legal values (val != 1 && val != 2 && val != 4 && val != 8)

the original code would refuse setting div to 1 since DIV_TO_REG(1) = 0
I changed to return -EINVAL for values not in the 1-8 range.
Anything in between gets the old behavior. If you think I should change to 
allow only 1,2,4,8 I can send another patch.

> Then you could eliminate the new_div DIV_TO_REG/REG_TO_DIV conversions and
> just test fan_div[nr] against val.  That would eliminate the "new_div"
> variable which would be good since it doesn't actually hold a "div" but a
> "reg" value, so it's somewhat mis-named.


> > and prevent the mutex lock
> > when no update is necessary.
> >
> If you don't take the mutex before testing the fan_div, then isn't there a
> possible race where you test first, but then it changes before you take the
> mutex to "change" it?  Is there any possible harm?  If multiple threads are
> trying to set the same divisor or different divisors for the same or
> different fans?  Just thinking out loud...

updates to data->fandiv[x] are atomic as there are no read-modify-writes to the 
structure members. If the values changes between the test and the lock, it will 
just be replaced with a new value.
The lock will take care of multiple threads trying to change div values for different 
fans. 
If multiple threads change the div to a different value for the same fan, I think, 
even with the lock around the entire function, it will not be possible to figure out 
which value will end up in the register at the end.
Let me know what you think.

thanks,
GG

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors