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. 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. -- Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
