Re: [PATCH] call drv->shutdown at rmmod

[ebiederm@xmission.com (Eric W. Biederman)]

Patrick Mochel <mochel@osdl.org> writes:

> > Assuming the device driver can get a device out of the suspend state
> > when the module is loaded.  This has been one of the biggest problem areas
> > with the e100 driver.  It's init code cannot bring the device out of a low
> > power state.
> 
> You're assuming that a driver can always bring a device out a shutdown
> state. 

I am assuming that is a requirement that it is possible for software
to bring a device out of the state is place in by the ->shutdown method.
->shutdown is called on the reboot path and if it results in a pure
software only reboot (i.e. the reset line on the motherboard is not toggled),
then the device driver for the OS loaded after the reboot must reinitialize it.

> That's one of the things we talked about at OLS, and the other half
> of the justification behind such a feature, not just making sure the
> device is queisced. Your argument against my suggestion are some of the
> same arguments for a feature like you're introducing. 

Yes.  My point is that I don't especially like having an additional argument
as that introduces more conditional control flow cases.  My experience
suggests unconditional control flow is more likely to be implemented properly.
 
> We have similar goals - introduce device power state code paths into 
> common operations (e.g module removal). Doing so gets the code paths 
> tested, which will help us ultimately achieve our utopian goals. And, it 
> can be done in one shot. 

If it can be done reasonably I am for it.

> > > The default would always be 'Do Nothing', but with a per-device sysfs 
> > > file, a developer/user/gui app could be used to set the policy from user 
> > > space. 
> > 
> > Possibly.  But this is getting complicated.  And while I do support things
> > being complicated enough to handle the problem this part of the API feels
> > like it is excessively complicated.  Which is why I was trying to simply
> > it by always calling shutdown on a device before we remove it.
> 
> Eh? This is complicated? especially compared your overall goal, kexec? Let 
> me explain again: 

Except that kexec suffers from the same class of issues a cooperative
multitasking (every component must do the right thing), it is quite
simple.  All of my code paths are written assuming they are the slow
uncommon path, so they place the smallest possible burden on the rest
of the kernel.

> I won't take the patch to unconditionally shutdown devices on module 
> removal, so you need some sort of flag. But, you must have some way to set 
> the flag, which is what sysfs is designed for. While you're at it, we 
> might as well make it an integer value, rather than a pure binary one, and 
> conditionally attempt to set the power state if the user says so. 
> 
> It's actually pretty simple, and if it is confusing, then sit back and 
> wait, and I'll provide documentation when I submit the patch. 

I think I now see where you are coming from.  Instead of changing the
production code paths.  Introducing a set of testing code paths to
assist with driver testing.  At least that is the feel I am getting.

Eric