LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@sisk.pl>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Linux-pm mailing list <linux-pm@lists.linux-foundation.org>,
Kernel development list <linux-kernel@vger.kernel.org>,
Alexey Starikovskiy <astarikovskiy@suse.de>
Subject: Re: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal
Date: Mon, 3 Mar 2008 21:47:09 +0100 [thread overview]
Message-ID: <200803032147.10223.rjw@sisk.pl> (raw)
In-Reply-To: <Pine.LNX.4.44L0.0803031217070.3611-100000@iolanthe.rowland.org>
On Monday, 3 of March 2008, Alan Stern wrote:
> On Mon, 3 Mar 2008, Rafael J. Wysocki wrote:
>
> > > After more thought, I'm not so sure about this. It might be a good
> > > idea to call the begin_sleep method just before the suspend method (or
> > > any of its variants: freeze, hibernate, prethaw, etc.) and call the
> > > end_sleep method just after the resume method. This minimizes the time
> > > drivers will spend in a peculiar non-hotplug-aware state, although it
> > > means that begin_sleep would have to be idempotent.
> > >
> > > It also allows sophisticated drivers to do all their processing in the
> > > begin_sleep (and end_sleep) method: both preventing new child
> > > registrations and powering down the device. At the moment I'm not sure
> > > whether this would turn out to be a good strategy, but it might.
> >
> > Well, I think there should be a window between ->suspend_begin()
> > and ->suspend() allowing the core to cancel the suspending of given
> > device and to select another one. For example, if there's a child
> > registration concurrent wrt ->suspend_begin() which completes after
> > ->suspend_begin() has been called, but before ->suspend_begin() has a chance
> > to block it, the core should not call ->suspend() for the device, but select
> > another one (the child).
>
> With a sophisticated driver that would never happen, because after
> blocking new child registrations, the driver would check that the
> power.sleeping flag is set in all the children before powering down the
> device.
The _core_ needs the window, not the driver. Even if the driver discovers that
there are active children, it can only fail ->suspend(), in which the core will
faill the entire power state transition (unless we reserve an error code for
signaling such situations by ->suspend(), which I'd prefer to avoid).
> But like I said, I'm not sure if this would be a good strategy.
>
> (This partly has to do with the requirements for runtime PM. During a
> runtime suspend the driver does have to check the children's status; it
> can't rely on the PM core. So if the check has to be done anyway, why
> not also check during a system sleep?)
>
> With non-sophisticated drivers, it definitely could happen that a new
> child is registered concurrently with begin_sleep. Then the core would
> go back and suspend the child first, as you say. Eventually the core
> would return to the parent device, at which time it would call the
> parent's begin_sleep method again -- unless we add another flag to
> indicate that it had already been called.
>
> > Of course, it won't be necessary if the ->suspend_begin() methods are called
> > in an initial forward pass through dpm_active.
>
> Right. That would be simpler.
>
> > > (BTW, I wonder if it's a good idea for device_add() to call
> > > device_pm_add() before calling bus_add_device(). If a suspend occurs
> > > in between, we could end up in a strange situation with a driver being
> > > asked to suspend a device before that device has been fully registered
> > > -- in fact the registration might still fail.)
> >
> > That's correct. Perhaps we should change device_add().
>
> I had a change like that in my version of the patch. It's excerpted
> below.
Hm, I wonder why you didn't move dpm_sysfs_add() along with device_pm_add()?
Perhaps it's better to include dpm_sysfs_add() into device_pm_add(), since we
are going the make the return a result anyway?
> > > > Well, we can add new callbacks for notifying drivers of an impending suspend.
> > > > In that case, say we add a ->begin() callback for this purpose (in fact that
> > > > would be two callbacks, ->suspend_begin() and ->hibernate_begin(), but let's
> > > > simplify things a bit for now), so there are the following questions:
> > >
> > > In theory you could even expand it to freeze_begin and prethaw_begin.
> >
> > I meant ->hibernate_begin() as ->freeze_begin() and I don't see any reason why
> > ->prethaw_begin() would be different from it (the difference between ->freeze()
> > and ->prethaw() -- now called ->quiesce(), btw -- is that the former saves
> > device settings and the latter doesn't).
>
> AFAICS there needs to be only one begin_sleep method. It should apply
> equally well to suspend, freeze, quiesce, and hibernate. (But not to
> suspend_late.)
It definitely would be simpler to assume so and introduce just one common
begin_sleep().
> > > > * Perhaps we can require ->suspend() to always succeed after ->begin() has
> > > > succeeded?
> > >
> > > No. Some drivers might implement just one and some drivers just the
> > > other.
> >
> > I thought ->suspend() would be mandatory, even if it's to be empty.
>
> There's no need for that. If it isn't implemented, treat it as though
> it was successful.
Well, I'm not sure. Right now we have a problem with distinguishing drivers
that don't implement ->suspend() purposefully from the ones that just don't
support suspend/hibernation ...
OTOH, since we are going to have a pointer to 'struct pm_ops', we can safely
assume that if it's not NULL, the driver writer knows what he's doing.
> But if a driver implements suspend() and leaves begin_sleep() as just a
> stub (which many drivers might reasonably do, if they never register
> any children), then the core shouldn't require suspend() to succeed
> merely because begin_sleep() did.
>
> And especially not if begin_sleep() is called in a separate pass.
Agreed.
> > > Here's something else to think about. We might want to allow some
> > > devices to be "power-irrelevant". That is, the device exists in the
> > > kernel only as a representation of some software state, not as a
> > > physical device. It doesn't consume power, it doesn't have any state
> > > to lose during a sleep, and its driver doesn't implement suspend or
> > > resume methods. For these sorts of devices, we might allow
> > > device_add() to skip calling device_pm_add() altogether. USB
> > > interfaces are a little like this, as are SCSI hosts and MMC hosts.
> >
> > If such devices serve as logical parents of some "real" ones, we should
> > at least mark them as "sleeping" during suspend to protect the dpm_active
> > ordering.
>
> They wouldn't have to be marked at all. They would never get on any of
> the PM lists, because they would never be passed to device_pm_add().
But dev->parent will be not NULL for their children being on dpm_active ...
IMO it's simpler to just add those devices without any suspend callbacks
defined to dpm_active and handle them normally than to introduce a special
case.
Thanks,
Rafael
next prev parent reply other threads:[~2008-03-03 20:48 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-02-25 15:39 Alan Stern
2008-02-25 19:46 ` [linux-pm] " Alan Stern
2008-02-25 22:25 ` Rafael J. Wysocki
2008-02-25 23:37 ` Alan Stern
2008-02-26 0:07 ` Rafael J. Wysocki
2008-02-26 15:49 ` Alan Stern
2008-02-26 23:17 ` Rafael J. Wysocki
2008-02-27 16:03 ` Alan Stern
2008-02-27 19:50 ` Rafael J. Wysocki
2008-02-27 20:15 ` Alan Stern
2008-02-28 22:49 ` Alan Stern
2008-02-29 0:01 ` Rafael J. Wysocki
2008-02-29 14:26 ` Rafael J. Wysocki
2008-02-29 15:53 ` Alan Stern
2008-02-29 17:02 ` Rafael J. Wysocki
2008-02-29 18:42 ` Alan Stern
2008-02-29 21:57 ` Rafael J. Wysocki
2008-02-29 22:46 ` Alan Stern
2008-03-01 0:13 ` Rafael J. Wysocki
2008-03-01 15:30 ` Alan Stern
2008-03-02 13:37 ` Rafael J. Wysocki
2008-03-02 16:22 ` Alan Stern
2008-03-02 19:11 ` Rafael J. Wysocki
2008-03-03 3:54 ` Alan Stern
2008-03-03 16:32 ` Rafael J. Wysocki
2008-03-03 17:43 ` Alan Stern
2008-03-03 20:47 ` Rafael J. Wysocki [this message]
2008-03-03 22:48 ` Alan Stern
2008-03-03 22:56 ` Rafael J. Wysocki
2008-03-03 23:12 ` Alan Stern
2008-03-03 23:18 ` Rafael J. Wysocki
2008-02-26 7:13 ` David Brownell
2008-02-26 8:25 ` David Newall
2008-02-26 9:16 ` David Brownell
2008-02-26 13:36 ` David Newall
2008-02-26 15:58 ` Alan Stern
2008-02-25 22:24 ` Rafael J. Wysocki
2008-02-27 20:36 ` Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200803032147.10223.rjw@sisk.pl \
--to=rjw@sisk.pl \
--cc=astarikovskiy@suse.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@lists.linux-foundation.org \
--cc=stern@rowland.harvard.edu \
--subject='Re: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).