Message-Id: <200803210314.55149.rjw@sisk.pl>
Date:	Fri, 21 Mar 2008 03:14:54 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	pm list <linux-pm@...ts.linux-foundation.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Greg KH <greg@...ah.com>, Len Brown <lenb@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Alexey Starikovskiy <astarikovskiy@...e.de>,
	David Brownell <david-b@...bell.net>,
	Pavel Machek <pavel@....cz>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [RFC][PATCH] PM: Introduce new top level suspend and hibernation callbacks (rev. 2)

On Friday, 21 of March 2008, Alan Stern wrote:
> On Fri, 21 Mar 2008, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > Below is an updated version of the $subject patch.  It has only been
> > compilation tested, but I'd like to get as much feedback as reasonably possible
> > at this stage to avoid redesigning things later in the process.
> > 
> > The most important changes from the previous one are the following:
> > * Callbacks to be executed with interrupts disabled are now separated from
> >   the "regular" ones.  There still are six of them, but IMHO this is what may
> >   be necessary in the most general case (eg. a driver may have to carry out
> >   some operations with interrupts disabled, which are different for SUSPEND,
> >   FREEZE and HIBERNATE; the complementary "resume" callbacks are needed for
> >   error recovery, for example)
> > * It occured to me that the ->freeze() and ->quiesce() callbacks were
> >   essentially the same, so I removed the latter
> > * The ->recover() callback was also dropped, as ->thaw() may well be used instead
> >   just fine (it seems)
> > * ->prepare() and ->complete() are now called in separate loops which causes
> >   some funny complications with error recovery.  Namely, we must remember which
> >   devices have already been ->prepare()d, so that we call ->complete() for them
> >   in the error path.  Moreover, there may be some ->prepare()d and some
> >   ->suspend()ed devices at the same time, so if there's an error we should
> >   ->resume() the ->suspend()ed ones and ->complete() everything etc.
> >   This may be handled in many different ways, but I decided to introduce a new
> >   list on which the ->complete()d devices are stored.  Accordingly, the
> >   registration of new children of a device is blocked between ->prepare() and
> >   ->complete() (it was only blocked between ->prepare() and ->resume() in the
> >   previous version).
> > 
> > Please have a look.
> 
> I don't have time to go through this in detail now, but a few things 
> stand out.
> 
> One trivial problem is that your dpm_prepare and dpm_complete routines 
> go through the list in the wrong order.

I'm not all so sure that the order is actually wrong.  What would be the
advantage of the forward order over the current one?

> More importantly, the situation with prepare and complete is getting
> rather complicated.  Now that you're adding dev->power.state, why not
> go all the way?  Get rid of all the different lists, keeping just
> dpm_active and possibly dpm_destroy.

dpm_destroy is not necessary and I'm going to drop it (later).

> Instead of moving devices between different lists, just store in
> dev->power.state an identification for which list the device is supposed
> to be on or is supposed to be moving to.  (Or else have a bunch of bitfields
> indicating which methods have been called.)  This has the advantage that
> the entries will never get out of order, even if devices are registered at
> the wrong time. 

That's correct.
 
> There's some question about when we want the PM core to start
> preventing new children from being registered.  Should this start right
> after prepare() returns, or should it start right before suspend() is
> called?  The first alternative sounds better to me.  Not only does it
> agree with the purpose of the prepare method, it also makes race
> situations easier to handle.

I agree and that's implemented in the patch (ie. the registrations of new
children are blocked immediately after ->prepare() has returned).

> Since dpm_prepare is supposed to go through the list in the forward
> direction, logically the "root" of the device tree should be the first
> thing "prepared".  This means you should not allow parentless devices
> to be registered any time after dpm_prepare has started.  Is that 
> liable to cause problems?

I'm still not seeing the advantage of the forward direction in the first place.

Although I don't see what particular problems that may cause, I think the
current approach (first, block registrations of new children for each
->prepare()d device and finally block any registrations of new devices) is
more natural.

> There may be similar problems with complete().  A number of drivers
> check during their resume method for the presence of new children
> plugged in while the system was asleep.  All these drivers would have
> to be converted over to the new scheme if they weren't permitted to
> register the new children before complete() was called.  Of course,
> this is easily fixed by permitting new children starting from when
> resume() is called rather than when complete() is called.

Well, the problem here was the protection of the correct ordering of the
various lists.  However, if the approach with changing 'status' is adopted
instead, which seems to be better, we'll be able to unblock the registering
of new children before ->resume().

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists