> >Is there any way values can be written to the driver's register > >without telling us? > > I think any other communication with the chip which isn't through our > driver (Bios? or some other code?) might change the values, although > it is unlikely. No other code should. If they do, they are bad, not us. As for the BIOS, is there any BIOS code running after boot time? I would have said no, but you seem to think the other way. Of course, one can use i2cset to write some arbitrary value to any register, and break the driver if it is designed my way, while it woudln't if designed your way. But someone doing so has to know what he/she does, isn't it? I2cset is meant for developing/debugging purposes, not regular use. > It's also possible that a driver may be misdetected or forced. > It would be odd if the driver gave misleading 'correct' > limit readings in these cases. I don't think it is worth the risk. As I was proposing, we still can read the limit back once, at the time we write it at first (and any later time we change it) so if it really is wrong (such as the register is read only) we'll see it exactly the same way we would with the current policy. As far as I know, the chips only seldom, if ever, change their R/W registers on their own, except at init time (none of our business) and reset (this could happen, I admit, but shouldn't after module init). So the only two errors I can think of if the chip isn't what it is supposed to be are writing a limit to a read only register (which can be handled as described right above) and hitting some reset register or bit, which is likely to show the same way (reading the value back probably won't be what was expected) and will have other side effects the user will probably notice. All these are problems caused by the driver being used with wrong hardware, which will not work anyway. It looks like we "optimize" our drivers for the very very few cases were the things go bad. I don't pretend we must ignore problems and remove error checking from our code ;) but still I believe we should optimize the working, not the faulty. One thing I'm strongly thinking of is disabling the limit readings *unless* the module is loaded with any force parameter (still mainly thinking of my ADM1025/NE1619 driver). This makes the secure path back in case a user is forcing the driver, but makes the driver faster for regular users. > In a nutshell, I think lower level drivers which talk to hardware > shouldn't do any internal caching or optimizing which isn't nessesary. And I don't quite agree. We are already caching since we limit the frequency at which new data can be obtained. Also, note that I don't advocate in favor of caching too much. For example, one could think of caching VID readings since it's unlikely to change after boot time, still I think it could in some cases (I guess Speedstep technology and the like could want to change that value) so we have to "sample" it continuously. I just want to limit what I consider useless re-readings, (mainly limits). > I.e., when values are asked for, it should talk directly with the > hardware to ask for them and not make any assumptions. I understand your point of view. I think it's motivated by the fact that you are a developer (I am too BTW ;)) and you must love the idea of watching whatever happens to the internal registers of every chipset in your computer. I love playing with them too, don't doubt it :) But what about the end-user? He/she wants an efficient driver. He/she event just wants temperature, etc. readings. The fact that a driver is required for that, and how the driver does it, is far beyond his/her interest. Again, I don't mean we can do anything that would make the driver faster, at any cost. I don't want speed. I want efficiency. > Now, for user-space apps, it could be more justifiable to cache or > assume values. At least, I'd rather see those sorts of hacks done in > user-space than kernel driver space. OK, maybe I see what you mean now, but I think you're missing something. At the time being, the user-space applications cannot cache a given set of values and refresh only the rest, because most of our drivers, if not all, have a single update function. If *one* value is queried and the data isn't up-to-date, *all* values are refreshed. This is the reason why I want to limit what is refreshed. (Next paragraph is me thinking while writing, not necessary very clear, feel free to plain skip or ask for better explanations) We could think of splitting all values into groups for separate refreshes, but it ain't that obvious. The groups would be "vertical" (understand: current values form one group, limits form one group...) from the driver's point of view, while they would be "horizontal" (understand: in0, in0_min and in0_max form one group...) from user-space (because for example reading /proc/dev/sys/.../in0 needs these three values). Or we can have individual refreshed (that is, each group only has one value in it), but considering you need one timestamp per group, it will be really painful. What's more, I think most applications will read all values at a time, which justifies there is a single update function in our drivers. This is how I came to the idea of not reading the limits as being the better optimization one could think of. > For this paricular issue (if we are trying to optimize speed or > limit transaction numbers over the I2C bus), it doesn't buy > anything so it is a moot point. I guess the I2C bus isn't overloaded by our drivers, for sure ;) On the other hand, speed (I prefer the term of quickness or reactivity) could be discussed. For example, running "sensors" on my desktop system takes about 500ms (three busses, one chip on each) when an update is needed. OK, 500ms isn't that long, but think of it. It doesn't do that many things in 500ms. It could be faster, isn't it? If it ain't much of an issue for the sensors program, I think it is for apps that monitor the values continuously (such as gkrellm). My proposal could lower the time to say 200ms, which is far better IMHO. > >Thus my question, and I ask it again, maybe more clearly than the > >first time. How could the chip's register be changed without using > >our driver? > > Well, I guess it is a matter of ideals. I've thought of kernel > hardware drivers as being an inbetween for the user-space apps and the > actual hardware with absolutely as little tainting of the data as > possible. We already do many transformations, even between the registers and the internal vars. > Even if the values in the hardware's registers are unlikely > to change or change infrequently, we still should pass values straight > through and not make any assumptions on the data in code. My point isn't about values that are unlikely to change (see my opinion about VID above) but about values that *should* not change. It's slightly different (in the spirit if not in the facts). > As long as overhead and processor loading isn't an issue, I'd suggest > we not cache or code in any value/limit assumptions into the code. > I2C transactions are slow, but they cause little (if any) CPU loading > or resource hogging. Even as far as percentage usage of the I2C bus, > it is small. I can't see any possible significant performance gains > by caching things like limits. Also, there are at least some > potential drawbacks (at least marginally more than simply 'no > drawbacks'), so I see some additional risk with no gain. Hardly anything I can say against this, you're of course right. It's more about having a "beautiful" driver than anything else. > BTW- The worst offender of reading values over and over with little > chance of change is the eeprom driver. For each device, if I > remember correctly, each byte read takes two I2C transactions for > 256 bytes! So, if you have 4 dimms (with an eeprom on each), you > are doing over 2 thousand transactions. Still, though, this > take little time and almost no CPU overhead, so it doesn't seem > to hurt anything. You're definitely right. I guess this is the reason why the update frequency is limited to 1 each 5 minutes as opposed to 1 each 1 or 2 seconds for the rest of our drivers. Refreshing the two eeproms on my laptops takes 5 full seconds (and I was complaining about the 500ms on the other system...) The worst part of the story is that over 50% of the data we retrieve isn't even used. Anyway it's a bit different, the EEPROM driver isn't a sensor-like driver. It isn't meant to be refreshed frequently. All data in the EEPROM are the same (update-frequency-wise), so that a user-space app can cache the whole thing and never use the driver again, while it's different with usual chipsets. While it's useful for our tests, few people want to use that driver (I even wonder if we should tell the users to modprobe that module at the end of sensors-detect). > Anyways, that's my $0.02. :') I add my 0.02 Eur (not much more ;)). Long way still before we can afford a drink ;) Thanks a lot for the clarifications, I really appreciate when we can exchange our views like that. (My state of mind after this step: I think I'll use "my" policy in the ADM1025 as a test. It's not widely used AFAIK so we don't risk much. If we have feedback of any kind, we'll could either revert the driver to the usual policy, of extend the experiment to other drivers. Or I can just be quiet and leave the ADM1025 driver as the other ones, if you or anyone doesn't want me to experiment. What is certain after our discussion is that I won't change all drivers to my policy right now ;)) -- Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
