Calling suspend() in halt/restart/shutdown -> not a good idea

Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

Hi !

Why are we calling driver suspend routines in these ? This is _not_ a
good idea ! On various machines, the mecanisms for shutting down are
quite different from suspend/resume, and current drivers have too many
bugs to make that safe. I keep getting all sort of reports of machines
not shutting down anymore.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Marc Ballarin]

On Mon, 01 Aug 2005 17:09:31 +0200
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Hi !
> 
> Why are we calling driver suspend routines in these ? This is _not_ a
> good idea ! On various machines, the mecanisms for shutting down are
> quite different from suspend/resume, and current drivers have too many
> bugs to make that safe. I keep getting all sort of reports of machines
> not shutting down anymore.

For example, my Centrino laptop will restart instead of power down with
-mm kernels.

To "fix" this I can either:
- unplug power. Shutdown works when on battery power.
- attach an external USB hard disk => power down always works.
- remove device_suspend(PMSG_SUSPEND) => power down always works.

Marc

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

On Mon, 2005-08-01 at 20:37 +0200, Marc Ballarin wrote:
> On Mon, 01 Aug 2005 17:09:31 +0200
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Hi !
> > 
> > Why are we calling driver suspend routines in these ? This is _not_ a
> > good idea ! On various machines, the mecanisms for shutting down are
> > quite different from suspend/resume, and current drivers have too many
> > bugs to make that safe. I keep getting all sort of reports of machines
> > not shutting down anymore.
> 
> For example, my Centrino laptop will restart instead of power down with
> -mm kernels.
> 
> To "fix" this I can either:
> - unplug power. Shutdown works when on battery power.
> - attach an external USB hard disk => power down always works.
> - remove device_suspend(PMSG_SUSPEND) => power down always works.-

Yes, this is just one of the gazillion setup that got broken by this
change. Drivers already have a shutdown() callback anyway, and if we
want to re-use the suspend one, then we need to define some sane
parameter, not "fake" a system suspend.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Pavel Machek]

Hi!

> Why are we calling driver suspend routines in these ? This is _not_

Well, reason is that if you remove device_suspend() you'll get
emergency hard disk park during powerdown. As harddrives can survive
only limited number of emergency stops, that is not a good idea.

								Pavel
-- 
teflon -- maybe it is a trademark, but it should not be.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Pavel Machek]

Hi!

> > > Why are we calling driver suspend routines in these ? This is _not_ a
> > > good idea ! On various machines, the mecanisms for shutting down are
> > > quite different from suspend/resume, and current drivers have too many
> > > bugs to make that safe. I keep getting all sort of reports of machines
> > > not shutting down anymore.
> > 
> > For example, my Centrino laptop will restart instead of power down with
> > -mm kernels.
> > 
> > To "fix" this I can either:
> > - unplug power. Shutdown works when on battery power.
> > - attach an external USB hard disk => power down always works.
> > - remove device_suspend(PMSG_SUSPEND) => power down always works.-
> 
> Yes, this is just one of the gazillion setup that got broken by this
> change. Drivers already have a shutdown() callback anyway, and if we
> want to re-use the suspend one, then we need to define some sane
> parameter, not "fake" a system suspend.

I'd like to get rid of shutdown callback. Having two copies of code
(one in callback, one in suspend) is ugly.
								Pavel

-- 
teflon -- maybe it is a trademark, but it should not be.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Pavel Machek]

Hi!

> Why are we calling driver suspend routines in these ? This is _not_ a
> good idea ! On various machines, the mecanisms for shutting down are
> quite different from suspend/resume, and current drivers have too many
> bugs to make that safe. I keep getting all sort of reports of machines

Well, powerdown at the end of suspend-to-disk should be *very* similar
to normal powerdown => if device_suspend() breaks something, it is a
bug in driver anyway.

Now, we may have a lot of such bugs, and the change went in too early,
but in the long run...
-- 
teflon -- maybe it is a trademark, but it should not be.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Eric W. Biederman]

Pavel Machek <pavel@ucw.cz> writes:

> Hi!
>
>> Why are we calling driver suspend routines in these ? This is _not_
>
> Well, reason is that if you remove device_suspend() you'll get
> emergency hard disk park during powerdown. As harddrives can survive
> only limited number of emergency stops, that is not a good idea.

Then the practical question is: do we suspend the disk by
calling device_suspend() for every device.  Or do we modify
the ->shutdown() method for the disk.

Eric

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Pavel Machek]

Hi!

> >> Why are we calling driver suspend routines in these ? This is _not_
> >
> > Well, reason is that if you remove device_suspend() you'll get
> > emergency hard disk park during powerdown. As harddrives can survive
> > only limited number of emergency stops, that is not a good idea.
> 
> Then the practical question is: do we suspend the disk by
> calling device_suspend() for every device.  Or do we modify
> the ->shutdown() method for the disk.

The additional data in pm_message_t are usefull, and sharing code
between suspend-to-ram and suspend-to-disk is usefull => option #1...

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

On Tue, 2005-08-02 at 11:53 +0200, Pavel Machek wrote:
> Hi!
> 
> > Why are we calling driver suspend routines in these ? This is _not_
> 
> Well, reason is that if you remove device_suspend() you'll get
> emergency hard disk park during powerdown. As harddrives can survive
> only limited number of emergency stops, that is not a good idea.

No, that is bogus. We have shutdown() already for that and it used to be
implemented at least by IDE.

You are just blindly "dropping" the device_suspend() call in there
without thinking. There is a lot of differences between suspend and
shutdown, you can't "just do that". In addition to the number of drivers
with broken suspend of course, causing many boxes to not shutdown
anymore (and not only PPCs)

Andrew, please back that off before 2.6.13. I'll try to send a patch if
you want later today if I find some time with a kernel source at hand.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

> I'd like to get rid of shutdown callback. Having two copies of code
> (one in callback, one in suspend) is ugly.

Well, it's obviously not a good time for this. First, suspend and
shutdown don't necessarily do the same thing, then it just doesn't work
in practice. So either do it right completely or not at all, but 2.6.13
isn't the place for an half-assed hack that looks like a solution to
you.

I do agree that there are enough similarities between the suspend and
shutdown process that we could use the same callback, but then, please,
at least with a different argument and with drivers beeing fixed to
handle it. Most drivers should probably just "ignore" shutdown anyway.

BTW. I suppose we still have the same constant for PMSG_FREEZE and
PMSG_SUSPEND ? That could have been fixed ages ago and is more important
imho....

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

On Tue, 2005-08-02 at 12:04 +0200, Pavel Machek wrote:
> Hi!
> 
> > Why are we calling driver suspend routines in these ? This is _not_ a
> > good idea ! On various machines, the mecanisms for shutting down are
> > quite different from suspend/resume, and current drivers have too many
> > bugs to make that safe. I keep getting all sort of reports of machines
> 
> Well, powerdown at the end of suspend-to-disk should be *very* similar
> to normal powerdown => if device_suspend() breaks something, it is a
> bug in driver anyway.

Power Down != Suspend. Period.

> Now, we may have a lot of such bugs, and the change went in too early,
> but in the long run...

Crap. It's not the same thing.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

On Tue, 2005-08-02 at 08:40 -0600, Eric W. Biederman wrote:
> Pavel Machek <pavel@ucw.cz> writes:
> 
> > Hi!
> >
> >> Why are we calling driver suspend routines in these ? This is _not_
> >
> > Well, reason is that if you remove device_suspend() you'll get
> > emergency hard disk park during powerdown. As harddrives can survive
> > only limited number of emergency stops, that is not a good idea.
> 
> Then the practical question is: do we suspend the disk by
> calling device_suspend() for every device.  Or do we modify
> the ->shutdown() method for the disk.

afaik, IDE used to have a shutdown callback or a shutdown notifier
already anyway. If that was lost, then this is a different problem. If
SATA and/or SCSI aren't doing it, then they need fixing, but suspend()
isn't the solution.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

On Tue, 2005-08-02 at 16:45 +0200, Pavel Machek wrote:
> Hi!
> 
> > >> Why are we calling driver suspend routines in these ? This is _not_
> > >
> > > Well, reason is that if you remove device_suspend() you'll get
> > > emergency hard disk park during powerdown. As harddrives can survive
> > > only limited number of emergency stops, that is not a good idea.
> > 
> > Then the practical question is: do we suspend the disk by
> > calling device_suspend() for every device.  Or do we modify
> > the ->shutdown() method for the disk.
> 
> The additional data in pm_message_t are usefull, and sharing code
> between suspend-to-ram and suspend-to-disk is usefull => option #1...

No.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Kyle Moffett]

On Aug 3, 2005, at 07:40:54, Benjamin Herrenschmidt wrote:
>> I'd like to get rid of shutdown callback. Having two copies of code
>> (one in callback, one in suspend) is ugly.
>
> Well, it's obviously not a good time for this. First, suspend and
> shutdown don't necessarily do the same thing, then it just doesn't  
> work
> in practice. So either do it right completely or not at all, but  
> 2.6.13
> isn't the place for an half-assed hack that looks like a solution to
> you.

One possible way to proceed might be to add a new callback that takes a
pm_message_t: powerdown()  If it exists, it would be called in both the
suspend and shutdown paths, before the suspend() and shutdown() calls to
that driver are made.  As drivers are fixed to clean up and combine that
code, they could put the merged result into the powerdown() function,
and remove their suspend() and shutdown() functions.

Cheers,
Kyle Moffett

--
I lost interest in "blade servers" when I found they didn't throw  
knives at
people who weren't supposed to be in your machine room.
   -- Anthony de Boer

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Benjamin Herrenschmidt]

On Wed, 2005-08-03 at 12:53 -0400, Kyle Moffett wrote:
> On Aug 3, 2005, at 07:40:54, Benjamin Herrenschmidt wrote:
> >> I'd like to get rid of shutdown callback. Having two copies of code
> >> (one in callback, one in suspend) is ugly.
> >
> > Well, it's obviously not a good time for this. First, suspend and
> > shutdown don't necessarily do the same thing, then it just doesn't  
> > work
> > in practice. So either do it right completely or not at all, but  
> > 2.6.13
> > isn't the place for an half-assed hack that looks like a solution to
> > you.
> 
> One possible way to proceed might be to add a new callback that takes a
> pm_message_t: powerdown()  If it exists, it would be called in both the
> suspend and shutdown paths, before the suspend() and shutdown() calls to
> that driver are made.  As drivers are fixed to clean up and combine that
> code, they could put the merged result into the powerdown() function,
> and remove their suspend() and shutdown() functions.

We already have shutdown() for that.

Ben.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Andrew Morton]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> Andrew, please back that off before 2.6.13. I'll try to send a patch if
>  you want later today if I find some time with a kernel source at hand.

Please do.

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Pavel Machek]

Hi!

> >>I'd like to get rid of shutdown callback. Having two copies of code
> >>(one in callback, one in suspend) is ugly.
> >
> >Well, it's obviously not a good time for this. First, suspend and
> >shutdown don't necessarily do the same thing, then it just doesn't  
> >work
> >in practice. So either do it right completely or not at all, but  
> >2.6.13
> >isn't the place for an half-assed hack that looks like a solution to
> >you.
> 
> One possible way to proceed might be to add a new callback that takes a
> pm_message_t: powerdown()  If it exists, it would be called in both the
> suspend and shutdown paths, before the suspend() and shutdown() calls to
> that driver are made.  As drivers are fixed to clean up and combine

No; please don't make driver model more complex than it already is.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: Calling suspend() in halt/restart/shutdown -> not a good idea

[Pavel Machek]

Hi!

> > I'd like to get rid of shutdown callback. Having two copies of code
> > (one in callback, one in suspend) is ugly.
> 
> Well, it's obviously not a good time for this. First, suspend and
> shutdown don't necessarily do the same thing, then it just doesn't work
> in practice. So either do it right completely or not at all, but 2.6.13
> isn't the place for an half-assed hack that looks like a solution to
> you.

Well, if that "hack" broke something, it was broken
w.r.t. suspend-to-disk already, but... it caused enough problems that
it is not good idea for 2.6.13, agreed.

> I do agree that there are enough similarities between the suspend and
> shutdown process that we could use the same callback, but then, please,
> at least with a different argument and with drivers beeing fixed to
> handle it. Most drivers should probably just "ignore" shutdown
> anyway.

Yes, we should have different argument, but code should be really
similar in suspend-to-disk-powerdown and normal powerdown, so I'd like
to use same callback. This is post-2.6.13 material (but I'd like patch
to stay in -mm, so that drivers keep getting tested).

> BTW. I suppose we still have the same constant for PMSG_FREEZE and
> PMSG_SUSPEND ? That could have been fixed ages ago and is more important
> imho....

That one is fixed in -mm already. But I guess it will not make it into
2.6.13.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address