Re: [patch/rft 2.6.17-rc2] swsusp resume must not device_suspend()

[Pavel Machek <pavel@ucw.cz>]

Hi!

> > It's Ok if it involves a drive change, so long as its an optional change, which
> > means that it shouldn't affect the interface very much (i.e. the calling 
> > convention). That's why it'd be good to augment and/or modify pm_message_t
> > to implement the changes, so we wouldn't have to change every single driver
> > again.. 
> 
> I'll post more patches after I sort out some oddness -- why is swsusp_suspend()
> leaving preempt_count() == 1, code I was nowhere near? -- but the patch appended
> here shows what I'm pursuing.  Same calling convention, new PRETHAW message
> that "pm-naive" drivers (most of them!) can handle just like FREEZE.
> 
> Other than this, it affects about 20 files with about 2/3 being drivers; say
> that maybe 5% of all in-tree drivers needed trivial changes, most of which
> could count as bugfixes _before_ defining the new message.

Okay, so you changed the interface, and it needed fixing at 16 places.


> @@ -143,29 +143,61 @@ typedef struct pm_message {
>  } pm_message_t;
>  
>  /*
> - * There are 4 important states driver can be in:
> - * ON     -- driver is working
> - * FREEZE -- stop operations and apply whatever policy is applicable to a
> - *           suspended driver of that class, freeze queues for block like IDE
> - *           does, drop packets for ethernet, etc... stop DMA engine too etc...
> - *           so a consistent image can be saved; but do not power any hardware
> - *           down.
> - * SUSPEND - like FREEZE, but hardware is doing as much powersaving as
> - *           possible. Roughly pci D3.
> - *
> - * Unfortunately, current drivers only recognize numeric values 0 (ON) and 3
> - * (SUSPEND).  We'll need to fix the drivers. So yes, putting 3 to all different
> - * defines is intentional, and will go away as soon as drivers are fixed.  Also
> - * note that typedef is neccessary, we'll probably want to switch to
> - *   typedef struct pm_message_t { int event; int flags; } pm_message_t
> - * or something similar soon.
> + * Several driver power state transitions are externally visible, affecting
> + * the state of pending I/O queues and (for drivers that touch hardware)
> + * interrupts, wakeups, and other hardware state.  There may also be
> + * internal transitions to various low power modes, which are transparent
> + * to the rest of the driver stack (such as a driver that's ON gating off
> + * clocks which are not in active use).
> + *
> + * One transition is triggered by resume(), after a suspend() call; the
> + * message is implicit:
> + *
> + * ON		Driver starts working again, responding to hardware events
> + * 		and software requests.  The hardware may have gone through
> + * 		a power-off reset, or it may have maintained state from the
> + * 		previous suspend() which the driver will rely on while
> + * 		resuming.  On most platforms, there are no restrictions on
> + * 		availability of resources like clocks during resume().
> + *
> + * Other transitions are triggered by messages sent using suspend().  All
> + * these transitions quiesce the driver, so that I/O queues are inactive.
> + * That commonly entails turning off IRQs and DMA; there may be rules
> + * about how to quiesce that are specific to the bus or the device's type.
> + * (For example, network drivers mark the link state.)  Other details may
> + * differ according to the message:
> + *
> + * SUSPEND	Quiesce, enter a low power device state appropriate for
> + * 		the upcoming system state (such as PCI_D3hot), and enable
> + * 		wakeup events as appropriate.
> + *
> + * FREEZE	Quiesce operations so that a consistent image can be saved;
> + * 		but do NOT otherwise enter a low power device state, and do
> + * 		NOT emit system wakeup events.
> + *
> + * PRETHAW	Quiesce as if for FREEZE; additionally, prepare for restoring
> + * 		the system from a snapshot taken after an earlier FREEZE.
> + * 		Some drivers will need to reset their hardware state instead
> + * 		of preserving it, to ensure that it's never mistaken for the
> + * 		state which that earlier snapshot had set up.

And you *do* realize that PRETHAW is like a FREEZE... So... can we use
FREEZE, and add aditional flag field that says it is preTHAW?

This will result in if (message == FREEZE || message == PRETHAW) that
is kind-of ugly.

Now, you are right that you only needed to fix 16 places, so it is not
_that_ bad...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html