> If you did that, you'd get the old keys. > > Your complaint is like > > "Doctor, doctor, it hurts when I dig out my eyes with a dull spool" > > OF COURSE it hurts. Don't do it. > > Your example is insane. How so ? What is insane in expecting that settings you have done to your controller are restored to the last settings you did when you resume ? Keys are an example, as is the IDE timing one you mentionned yourself (and Im not talking about a user shooting himself in the foot with hdparm here, the IDE layer might do timing demotion in case of too many CRC errors for example), etc... So in all those examples where you said "don't do that", What should we do ? not restore things at all ? What if those keys are used to talk to your disk ? You resume and ... no disk ? What if userland sets some settings in a device via a driver ioctl/sysfs/whatever, system suspends, resumes, and you suddenly get the wrong settings because your save_state happened before the last userland call ? Have you read my mail completely ? I've talked to paulus about it, just to make sure I wasn't totally insane (or maybe we are both !) and so far, he doesn't see a failure in my reasoning. In fact, in every case where save_state() would be of any use for actually, it's also the cases where we hit the problem I described. It essentially boils down to the 3 categories of "state" I've described but I'll do it again: - Static. State that doesn't change. This is for example PCI config state, that sort of thing. Could be saved at _any_ time, as far back as ... driver initialisation. I don't see the need for a specific callback for these. - Volatile: That's what you have very well described in a lot of your examples: things that can be reconstructed, like current request pointer etc... In many cases, hw state is also "cached" by the driver (for example, your multicast filters setting are in your netdev structure iirc, etc...) and thus that state can be considered "volatile" on the hardware side since it can be reprogrammed in at resume time from those cached data. - Dynamic: That's the interesting case. That's state that gets set into the hardware upon client requests and that affects device operations. Examples of that are numerous, from controller timings, encryption keys, link type/speed/width, god knows what. Client here can be a dependent driver (the disk driver changes the settings of the controller for a given channel for example) or it can be userland (or a protocol stack, like softmac changing the speed and tx power of your wireless, etc etc etc...). That's exactly the sort of thing one may want to save and later restore. That is, if it's not already cached by the driver in some memory data structure in which case it goes into the volatile category and doesn't need save_state. Now if you think a bit about it... those states you want to save from your hardware to restore later... how can it make sense at ALL to save that state at any random point in time during suspend (which is bascially what your save_state) is while it can still and will be changed by the clients of that driver ? Essentially, what you propose is that on resume, devices that have such a state in the hardware will come back up with some random version of what you put there some time ago ... not the last you have set when suspending, no, wahtever was there some time before .... Please, show me the flaw in my argument, I haven't found it yet. I can't find a case where save_state is useful (for actually saving some hardware state) where it doesn't also need to be atomic to the actual suspend (or rather to the "stop processing user requests" part of suspend semantics). Examples of such states ? well, you found one yourself, IDE timings. It could be argued that the client (the disk drive here) should re-negociate timings on resume though, in which case it becomes a volatile state and doesn't need to be saved at all. SCSI link setup (same thing, could be renegociated, so either you save it, or it's volatile, but if you save it, you'd rather save something that matches what your client think it is, that is what your client last set). Encyrption keys in things like wireless, encrypted storage, etc... In fact, there is not that many of these things. Most of the time, state is volatile (that is cached by the driver). Now there is _one_ argument for having an early pass here is memory footprint vs. static state. That is, all this state that does not change (PCI config space, various video card registers that the BIOS has set that you may need to save/restore, firmware, etc etc ....). I said you can save it at init time. But you might not want to keep all that saved stuff around all the time in memory for no good use, thus indeed, it might be _convenient_ to have a call a bit before suspend to allocate storage for those things, and possibly save them at that point. In that case, save state becomes a convenience. But heh, we need that prepare() call for all the reasons I described, so why not make it the same. I do still think that the prepare() semantics (which is important and required) is more important though than this "convenience" save_state. Not only that, but save_state is confusing as it might lead the driver writer to think he can safely also save what I described as dynamic state in there, which he cannot safely as I explained already enough I think. Am I more clear or what ? Ben.
