[PATCH 2.6] w83781d fan_div code refactoring

Mark M. Hoffman mhoffman at lightlink.com
Wed Mar 31 04:26:19 CEST 2004


(purposefully not trimming context this message is about a week late)

* Jean Delvare <khali at linux-fr.org> [2004-03-18 14:04:58 +0100]:
> Quoting "Mark M. Hoffman" <mhoffman at lightlink.com>:
> 
> > > While a corner case, we better fix it quickly since it is very
> > > likely to happen. We used to set fan divisors before fan mins in
> > > sensors.conf because setting fan divisors was breaking fan mins,
> > > and this will trigger the bug, since we invite users to run
> > > "sensors -s" prior to running "sensors".
> > 
> > But it's not real dangerous, since who inits fanX_div without also
> > init'ing fanX_min immediately after?
> 
> You're correct, this is what will most likely happen in a majority of
> cases. But not necessarily all the cases. One can comment out the set
> fanXmin lines in sensors.conf. And anyway, it still temporarily writes
> a random value for fanN_min into the chip, which could trigger an alarm
> and possibly other nasty actions. Remember these reports of "as I load
> module X, my computer instantly goes off"...
> 
> > > I can think of three ways to solve the issue and would like to
> > > hear opinions about which we should use:
> > > 
> > > 1* In the driver's init function, read fan_mins from the chip so
> > > they are initialized.
> > > 
> > > 2* In the store_fan_div function, call update_device.
> > > 
> > > 3* In the store_fan_div function, if data->valid == 0, read the
> > > fan_min from the chip.
> > > (...)
> > 
> > I prefer #1: it fixes the whole *class* of bugs which can arise from
> > interactions between set/show functions and uninit'ed <driver>_data
> > structure elements.  E.g. I can see this type of bug creeping in to
> > smart PWM control where it could be much more dangerous.
> 
> I don't think that #1 as I described it fixes the whole class, since it
> only works for fan_mins. Instead, what you seem to suggest would be:
> 
> 4* Call update_device upon driver init.

You're right, this is what I meant.

> This would really slow the driver loading, for sure. But then we're
> safe, and we even could get rid of data->valid (providing we add a
> "force" parameter to update_device; not sure it's worth it).
> 
> I also can think of another possibility now:
> 
> 5* Do not try to preserve fanN_min if data->valid == 0.
> 
> This one doesn't slow anything significantly. Ironically, this is the
> first thing I tried when finding the bug, and I forgot to mention it as
> a possible solution.
> 
> Of course, this one does only work for the store_fan_div case, other
> cases would have to handle the issue by themselves, in the same way or
> a different one.
> 
> Solution #4 can also be modified to something more subtle. We could
> divide internal register data in two sets. One set with registers we
> want to read at driver init, one set with the other registers. Each set
> would have its own "valid" and/or last_updated variables. This is
> somewhat similar to what Phil Pokorny does in the lm85 driver (one
> timestamp for limits, one for readings, each set has a different buffer
> validity limit) or me in the eeprom driver (each slice has its own
> validity bit and last_updated timestamp).
> 
> This requires more code, of course, but would slow down the init only
> as
> much as stricly necessary. I'm not sure the benefit is worth the work
> however.
> 
> I think I still prefer solution #1. Looks like the best tradeoff. If
> other sysfs functions need other registers, we simply add initializing
> reads for them in the init function.

I disagree, but I see that I'm a little late on this one since patches
have already been accepted.  As I mention in another message, I just
don't think that the update speed of these drivers is worth any extra
code... and I really do mean *any at all*.  What is the value of returning
a result 1/2 second faster using "sensors" at the command line?  More
likely people are using something like gkrellm, or capturing the data in
a script and plotting it, so they never experience that delay anyway.

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com



More information about the lm-sensors mailing list