headers
Rafael J. Wysocki | 1 Mar 2008 01:13
Picon
Gravatar

Re: Fundamental flaw in system suspend, exposed by freezer removal

On Friday, 29 of February 2008, Alan Stern wrote:
> On Fri, 29 Feb 2008, Rafael J. Wysocki wrote:
> 
> > I'm still not sure if this particular race would happen if only the registering
> > of children of already suspended partents were blocked.
> 
> That's different.  Before you were talking about acquiring
> dev->power.lock _before_ calling the suspend method.  Now you're
> talking about blocking child registration _after_ the parent is already
> suspended.

Hm, perhaps I don't understand the issue correctly, so please let me restate it.

We have the design issue that it's possible to register a child of a device
that was taken for suspend or even suspended which, in turn, leads to a wrong
ordering of devices on dpm_active.  Namely, suppose that device dev is taken
from the end of dpm_active and suspend_device(dev, state) is going to be called
If at the same time a child of dev is being registered and
suspend_device(dev, state) returns 0, dev will be taken out of dpm_active, but
the new device (its child) will be added to the end of dpm_active and
subsequently we'll attempt to suspend it.  That will be wrong.

Also, if a dev's child is registered after suspending all devices, it will go
to the end of dpm_active, so after the subsequent resume dev will end up
closer to the end of the list than the new child.  Thus, during the next
suspend it will be taken for suspending before this child and that will be
wrong either.  Note that this situation need not look dangerously from the
driver's perspective, since it doesn't know of the dpm_active ordering issue.

One of the possible solutions is to require suspend_device(dev, state) to
(Continue reading)

Permalink | Reply | 0 comments |
headers
Pavel Machek | 1 Mar 2008 12:47
Picon

Re: [RFC] Add extra PM_EVENT_* codes for use by subsystems

Hi!

> Is there any objection to adding extra PM_EVENT_* codes?  Although they
> won't ever be issued by the PM core, they will come in handy for
> internal uses in power-aware subsystems (like USB).

I guess we cando that.

> Alan Stern
> 
> 
> Index: usb-2.6/include/linux/pm.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm.h
> +++ usb-2.6/include/linux/pm.h
>  <at>  <at>  -164,21 +164,40  <at>  <at>  typedef struct pm_message {
>   * well as during system sleep states like PM_SUSPEND_STANDBY.  They may
>   * be able to use wakeup events to exit from runtime low-power states,
>   * or from system low-power states such as standby or suspend-to-RAM.
> + *
> + * The PM core will never issue the PMSG_ codes for USER_SUSPEND,
> + * USER_RESUME, REMOTE_WAKEUP, AUTOSUSPEND, or AUTORESUME.  They are
> + * provided for internal use by power-aware subsystems.
>   */
>  
> -#define PM_EVENT_ON 0
> -#define PM_EVENT_FREEZE 1
> -#define PM_EVENT_SUSPEND 2
> -#define PM_EVENT_HIBERNATE 4
> -#define PM_EVENT_PRETHAW 8
(Continue reading)

Permalink | Reply | 0 comments |
headers
Pierre Ossman | 1 Mar 2008 13:31

Re: [RFC][PATCH] cpuidle: avoid singing capacitors

On Fri, 29 Feb 2008 16:44:07 -0500
lsorense <at> csclub.uwaterloo.ca (Lennart Sorensen) wrote:

> On Fri, Feb 29, 2008 at 07:38:12PM +0100, Pierre Ossman wrote:
> > +/*
> > + * The minimum number of ticks needed to not oscillate faster than
> > + * 500 Hz.
> > + */
> > +#define MIN_DEEP_INTERVAL (HZ / 500)
> 
> What happens here if HZ < 500?  Or does the fact that you have less than
> 500HZ jiffies automatically imply that you can't go to sleep more than
> the jiffy rate times per second?

A low HZ will still go to sleep very often (provided NO_HZ is in effect). But a HZ < 500 makes that number up
there turn to zero.  But the check further down makes sure that at least 1 tick passes. So that means it will
not enter C3 more often than min(HZ, 500) Hz. Another reason to stop using jiffies.

--

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
Permalink | Reply | 0 comments |
headers
Pierre Ossman | 1 Mar 2008 14:40

Re: [RFC][PATCH] cpuidle: avoid singing capacitors

On Fri, 29 Feb 2008 19:38:12 +0100
Pierre Ossman <drzeus-list <at> drzeus.cx> wrote:

>  <at>  <at>  -50,9 +58,16  <at>  <at>  static int menu_select(struct cpuidle_device *dev)
>                         break;
>                 if (s->exit_latency > pm_qos_requirement(PM_QOS_CPU_DMA_LATENCY))
>                         break;
> +               if ((dev->states[i].flags & CPUIDLE_FLAG_DEEP) &&
> +                       time_before_eq(jiffies, data->last_deep_jif + MIN_DEEP_INTERVAL))
> +                       break;
>         }
>  

I guess another approach would be to refuse to enter deep sleep if the sleep time is less than 2 ms. That would
mean we would not lose the long sleeps, but if it is just doing short sleeps then we would never enter C3...

Is there a decent way of testing which approach is actually doing the least damage?

Rgds

--

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
Permalink | Reply | 0 comments |
headers
Pierre Ossman | 1 Mar 2008 15:11

Re: Bugs in MMC [was: [Bug 10030] Suspend doesn't work when SD card is inserted]

On Mon, 25 Feb 2008 15:00:51 -0500 (EST)
Alan Stern <stern <at> rowland.harvard.edu> wrote:

> 
> Maybe a better approach would be to leave the workqueue unfreezable,
> and keep cancel_delayed_work_sync() in mmc_suspend_host().  It would
> then be necessary to add a test to verify, if there is a card attached,
> that the card is indeed suspended.  After all, it's possible that the
> cancel_delayed_work_sync() ended up waiting for a job already running
> on the workqueue to register a new card!  (The same would be true even 
> with flush_scheduled_work.)

How would that be handled? I'd prefer a patch as I'm evidently not up to date with how to fondle the pm stuff
properly. :)

> 
> Also, as a bit of defensive programming, it might be a good idea to add
> a "suspended" flag to the mmc_host structure.  If mmc_rescan() sees
> that the flag is set then it should return immediately.  This would
> protect against host drivers that aren't careful to disable detect
> IRQs before calling mmc_suspend_host().

Indeed. I'll add that to my todo.

Rgds

--

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
(Continue reading)

Permalink | Reply | 0 comments |
headers
Alan Stern | 1 Mar 2008 15:36
Picon
Favicon

Re: Bugs in MMC [was: [Bug 10030] Suspend doesn't work when SD card is inserted]

On Sat, 1 Mar 2008, Pierre Ossman wrote:

> On Mon, 25 Feb 2008 15:00:51 -0500 (EST)
> Alan Stern <stern <at> rowland.harvard.edu> wrote:
> 
> > 
> > Maybe a better approach would be to leave the workqueue unfreezable,
> > and keep cancel_delayed_work_sync() in mmc_suspend_host().  It would
> > then be necessary to add a test to verify, if there is a card attached,
> > that the card is indeed suspended.  After all, it's possible that the
> > cancel_delayed_work_sync() ended up waiting for a job already running
> > on the workqueue to register a new card!  (The same would be true even 
> > with flush_scheduled_work.)
> 
> How would that be handled? I'd prefer a patch as I'm evidently not up to date with how to fondle the pm stuff
properly. :)

The easiest way to do this will be to wait until some planned updates 
are added to the PM core; then this will be a quick and simple change.  
That probably means waiting until after 2.6.25 is released, however.

> > Also, as a bit of defensive programming, it might be a good idea to add
> > a "suspended" flag to the mmc_host structure.  If mmc_rescan() sees
> > that the flag is set then it should return immediately.  This would
> > protect against host drivers that aren't careful to disable detect
> > IRQs before calling mmc_suspend_host().
> 
> Indeed. I'll add that to my todo.

That could go in along with the previous change.  The PM update
(Continue reading)

Permalink | Reply | 0 comments |
headers
Pierre Ossman | 1 Mar 2008 15:47

Re: Bugs in MMC [was: [Bug 10030] Suspend doesn't work when SD card is inserted]

On Sat, 1 Mar 2008 09:36:36 -0500 (EST)
Alan Stern <stern <at> rowland.harvard.edu> wrote:

> 
> The easiest way to do this will be to wait until some planned updates 
> are added to the PM core; then this will be a quick and simple change.  
> That probably means waiting until after 2.6.25 is released, however.
> 

Ok. Just give me a poke when it's time.

--
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org
Permalink | Reply | 0 comments |
headers
Alan Stern | 1 Mar 2008 16:30
Picon
Favicon

Re: Fundamental flaw in system suspend, exposed by freezer removal

On Sat, 1 Mar 2008, Rafael J. Wysocki wrote:

> On Friday, 29 of February 2008, Alan Stern wrote:
> > On Fri, 29 Feb 2008, Rafael J. Wysocki wrote:
> > 
> > > I'm still not sure if this particular race would happen if only the registering
> > > of children of already suspended partents were blocked.
> > 
> > That's different.  Before you were talking about acquiring
> > dev->power.lock _before_ calling the suspend method.  Now you're
> > talking about blocking child registration _after_ the parent is already
> > suspended.
> 
> Hm, perhaps I don't understand the issue correctly, so please let me restate it.
> 
> We have the design issue that it's possible to register a child of a device
> that was taken for suspend or even suspended which, in turn, leads to a wrong
> ordering of devices on dpm_active.  Namely, suppose that device dev is taken
> from the end of dpm_active and suspend_device(dev, state) is going to be called
> If at the same time a child of dev is being registered and
> suspend_device(dev, state) returns 0, dev will be taken out of dpm_active, but
> the new device (its child) will be added to the end of dpm_active and
> subsequently we'll attempt to suspend it.  That will be wrong.

That's right.

> Also, if a dev's child is registered after suspending all devices, it will go
> to the end of dpm_active, so after the subsequent resume dev will end up
> closer to the end of the list than the new child.  Thus, during the next
> suspend it will be taken for suspending before this child and that will be
(Continue reading)

Permalink | Reply | 0 comments |
headers
Rafael J. Wysocki | 2 Mar 2008 13:57
Picon
Gravatar

Re: [RFC] Add extra PM_EVENT_* codes for use by subsystems

On Friday, 29 of February 2008, Alan Stern wrote:
> Is there any objection to adding extra PM_EVENT_* codes?  Although they
> won't ever be issued by the PM core, they will come in handy for
> internal uses in power-aware subsystems (like USB).

Well, this is in a (slight) conflict with the hibernation/suspend callbacks
rework that I'm working on with Alex.  I'll send the first patch in a few
minutes for comments.

Thanks,
Rafael

> Index: usb-2.6/include/linux/pm.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm.h
> +++ usb-2.6/include/linux/pm.h
>  <at>  <at>  -164,21 +164,40  <at>  <at>  typedef struct pm_message {
>   * well as during system sleep states like PM_SUSPEND_STANDBY.  They may
>   * be able to use wakeup events to exit from runtime low-power states,
>   * or from system low-power states such as standby or suspend-to-RAM.
> + *
> + * The PM core will never issue the PMSG_ codes for USER_SUSPEND,
> + * USER_RESUME, REMOTE_WAKEUP, AUTOSUSPEND, or AUTORESUME.  They are
> + * provided for internal use by power-aware subsystems.
>   */
>  
> -#define PM_EVENT_ON 0
> -#define PM_EVENT_FREEZE 1
> -#define PM_EVENT_SUSPEND 2
> -#define PM_EVENT_HIBERNATE 4
(Continue reading)

Permalink | Reply | 2 comments |
headers
Rafael J. Wysocki | 2 Mar 2008 14:37
Picon
Gravatar

Re: Fundamental flaw in system suspend, exposed by freezer removal

On Saturday, 1 of March 2008, Alan Stern wrote:
> On Sat, 1 Mar 2008, Rafael J. Wysocki wrote:
> 
> > On Friday, 29 of February 2008, Alan Stern wrote:
> > > On Fri, 29 Feb 2008, Rafael J. Wysocki wrote:
> > > 
> > > > I'm still not sure if this particular race would happen if only the registering
> > > > of children of already suspended partents were blocked.
> > > 
> > > That's different.  Before you were talking about acquiring
> > > dev->power.lock _before_ calling the suspend method.  Now you're
> > > talking about blocking child registration _after_ the parent is already
> > > suspended.
> > 
> > Hm, perhaps I don't understand the issue correctly, so please let me restate it.
> > 
> > We have the design issue that it's possible to register a child of a device
> > that was taken for suspend or even suspended which, in turn, leads to a wrong
> > ordering of devices on dpm_active.  Namely, suppose that device dev is taken
> > from the end of dpm_active and suspend_device(dev, state) is going to be called
> > If at the same time a child of dev is being registered and
> > suspend_device(dev, state) returns 0, dev will be taken out of dpm_active, but
> > the new device (its child) will be added to the end of dpm_active and
> > subsequently we'll attempt to suspend it.  That will be wrong.
> 
> That's right.
> 
> > Also, if a dev's child is registered after suspending all devices, it will go
> > to the end of dpm_active, so after the subsequent resume dev will end up
> > closer to the end of the list than the new child.  Thus, during the next
(Continue reading)

Permalink | Reply | 0 comments |

Gmane