Re: [PATCH 5] PM: Update comments describing device power management callbacks

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, November 21, 2011, Alan Stern wrote:
> On Mon, 21 Nov 2011, Rafael J. Wysocki wrote:
> 
> > > >   * @freeze: Hibernation-specific, executed before creating a hibernation image.
> > > > - *	Quiesce operations so that a consistent image can be created, but do NOT
> > > > - *	otherwise put the device into a low power device state and do NOT emit
> > > > - *	system wakeup events.  Save in main memory the device settings to be
> > > > - *	used by @restore() during the subsequent resume from hibernation or by
> > > > - *	the subsequent @thaw(), if the creation of the image or the restoration
> > > > - *	of main memory contents from it fails.
> > > > + *	Analogous to @suspend(), but it should not enable the device to signal
> > > > + *	wakeup events.  The majority of subsystems (with the notable exception
> > > > + *	of the PCI bus type) expect the driver-level @freeze() to save the
> > > > + *	device settings in memory to be used by @restore() during the subsequent
> > > > + *	resume from hibernation.
> > > > + *	Subsystem-level @freeze() is executed for all devices after invoking
> > > > + *	subsystem-level @prepare() for all of them.
> > > 
> > > The first three lines you removed contain some important points which I
> > > think should be retained.
> > 
> > Well, not really, because in fact what the callback is supposed to do depends
> > on the subsystem.  For example, on PCI freeze is not supposed to save the
> > the device state even and generally those routines don't emit any events.
> 
> But you removed the part saying that the freeze callback should quiesce
> the device but doesn't necessarily have to put the device into a
> low-power state (in fact, it should avoid changing the power state if
> possible).  And you removed the explanation that this is needed in
> order to guarantee a consistent memory image.  These two points are
> true for all subsystems, including PCI.

I said "Analogous to @suspend()" instead.  I'm not sure why this is not
sufficient?

> > > > @@ -174,29 +198,32 @@ typedef struct pm_message {
> > > >   * their children.
> > > >   *
> > > >   * It is allowed to unregister devices while the above callbacks are being
> > > > - * executed.  However, it is not allowed to unregister a device from within any
> > > > - * of its own callbacks.
> > > > + * executed.  However, it is NOT allowed to unregister a device from within any
> > > > + * of its driver's callbacks.
> > > 
> > > Please be a little more precise here.  If driver D manages two devices,
> > > A and B, then D _is_ allowed to unregister A from within a callback for
> > > B (except if A is an ancestor of B or something like that).  It's just
> > > not allowed to unregister B from within a callback for B.
> > 
> > I can leave that as is, but devices (or device objects to be precise) don't
> > have any callbacks.  Their drivers do.
> > 
> > I can add "(unless they are being executed for a different device)" or
> > something like this, but I'm not sure if that makes things any clearer.
> 
> "A callback routine must NOT try to unregister the device for which it
> was called, however it may unregister children of that device (for
> example, if it detects that a child was hot-unplugged while the system
> was asleep)."

That sounds good, thanks!
 
> > Do you know any situation in which that matters?
> 
> Not offhand, but I'm not familiar with too many subsystems.

Well, it seems quite extreme to me to be honest. :-)

Thanks,
Rafael