Messing up driver model API

Messing up driver model API

[Pavel Machek]

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. 

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: Messing up driver model API

[Patrick Mochel]

> 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).

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. 

> 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

Re: Messing up driver model API

[Pavel Machek]

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).
> 
> 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.

> > 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. 

So we are stuck with the mess in 2.6; not good.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]