Messing up driver model API
Hi!
So you just had to mess it up... Having suspend(device *, state, level) might be bad, but having suspend(device *, state, level) in one piece of code and {suspend,save}(device *, state) is *way* worse. (And I did not see any proposal on l-k. I hope I just missed it).
So are you going to revert it or convert whole driver model to use {suspend,save}(device *, state)?
Pavel
[driver model] Add save() and restore() methods for system device drivers.
It turns out that at least some system device drivers need to allocate memory and/or sleep for one reason or another when either saving or restoring state.
Instead of adding a 'level' paramter to the suspend() and resume() methods, which I despise and think is a horrible programming interface, two new methods have been added to struct sysdev_driver:
int (*save)(struct sys_device *, u32 state);
int (*restore)(struct sys_device *);that are called explicitly before and after suspend() and resume() respectively, with interrupts enabled. This gives the drivers the flexibility to allocate memory and sleep, if necessary.
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 5 lines
So you just had to mess it up... Having
suspend(device *, state, level)might be bad, but havingsuspend(device *, state, level)in one piece of code and {suspend,save}(device *, state) is *way* worse. (And I did not see any proposal on l-k. I hope I just missed it).
Calm down, Pavel. From a technical standpoint, it's a superior interface. Having the 'level' parameter was a mistake made out of speculative ignorance - we didn't know exactly how the suspend/resume code was going to work, or exactly what we had to do.
Having explicit calls to save state, power down the device, power on the deivce and restore state is a Good Thing - it dictates exactly what should be done at each level. And, it makes more drivers conform to the same style. Should there be any special cases, then we will deal with them. Based on the conversations and the code of the last year, those should be rare.
Quoted 2 lines
So are you going to revert it or convert whole driver model to use {suspend,save}(device *, state)?
Today: neither. I'm going to see how this works, and if it does, then I may convert all the users of struct device_driver to use the same model.
The interfaces are obvious, and they are documented in the changesets, as well as the code. Soon, they will be documented in text files shipped with the kernel. I have complete confidence that anyone that takes the time to become versed enough in current power management semantics will have no problem with the new methods for system devices.
-pat
Hi!
Quoted 9 lines
Quoted 4 lines
So you just had to mess it up... Having
suspend(device *, state, level)might be bad, but havingsuspend(device *, state, level)in one piece of code and {suspend,save}(device *, state) is *way* worse. (And I did not see any proposal on l-k. I hope I just missed it).Calm down, Pavel. From a technical standpoint, it's a superior interface.
From a technical standpoint, its now mess with half a kernel using one
interface and second one using another. And you did not bother to mail the patch to l-k for the review :-(, and then you call me a troll.
Quoted 6 lines
Quoted 2 lines
So are you going to revert it or convert whole driver model to use {suspend,save}(device *, state)?
Today: neither. I'm going to see how this works, and if it does, then I may convert all the users of struct
device_driverto use the same model.
So we are stuck with the mess in 2.6; not good.
Pavel
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]