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: pm list <linux-pm@lists.linux-foundation.org>,
	Alexey Starikovskiy <astarikovskiy@suse.de>,
	Pavel Machek <pavel@ucw.cz>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation
Date: Tue, 4 Mar 2008 22:52:50 +0100	[thread overview]
Message-ID: <200803042252.51440.rjw@sisk.pl> (raw)
In-Reply-To: <Pine.LNX.4.44L0.0803041041270.4039-100000@iolanthe.rowland.org>

On Tuesday, 4 of March 2008, Alan Stern wrote:
> On Tue, 4 Mar 2008, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > The appended patch is intended to fix the issue with the PM core that it allows
> > device registrations to complete successfully even if they run concurrently
> > with the suspending of their parents, which may lead to a wrong ordering of
> > devices on the dpm_active list and, as a result, to failures during suspend and
> > hibernation transitions.
> > 
> > Comments welcome.
> 
> > Index: linux-2.6/include/linux/pm.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/pm.h
> > +++ linux-2.6/include/linux/pm.h
> > @@ -186,6 +186,7 @@ struct dev_pm_info {
> >  #ifdef	CONFIG_PM_SLEEP
> >  	unsigned		should_wakeup:1;
> >  	struct list_head	entry;
> > +	bool			sleeping;	/* Owned by the PM core */
> >  #endif
> >  };
> 
> Drivers might want to use this field without having to add protective 
> "#ifdef CONFIG_PM_SLEEP" lines.

I guess you mean that driver writers may not want to make each piece of code
referring to this flag depend on CONFIG_PM_SLEEP.  Any driver that actually
writes to it will be buggy ...

> You can change it to a single-bit bitfield and place it adjacent to
> can_wakeup. 

Yeah, good catch.  I will.

> > -void device_pm_add(struct device *dev)
> > +int device_pm_add(struct device *dev)
> >  {
> > +	int error = 0;
> > +
> >  	pr_debug("PM: Adding info for %s:%s\n",
> >  		 dev->bus ? dev->bus->name : "No Bus",
> >  		 kobject_name(&dev->kobj));
> >  	mutex_lock(&dpm_list_mtx);
> > -	list_add_tail(&dev->power.entry, &dpm_active);
> > +	if (dev->parent && dev->parent->power.sleeping)
> > +		error = -EBUSY;
> 
> Add a stack dump?  When this isn't a race, it's the kind of bug we want
> to fix.

Yes, I'll do that.

> > +	else
> > +		list_add_tail(&dev->power.entry, &dpm_active);
> >  	mutex_unlock(&dpm_list_mtx);
> > +	return error;
> >  }
> 
> > @@ -426,6 +404,11 @@ static int dpm_suspend(pm_message_t stat
> >  		struct list_head *entry = dpm_active.prev;
> >  		struct device *dev = to_device(entry);
> >  
> > +		if (dev->parent && dev->parent->power.sleeping) {
> > +			error = -EAGAIN;
> > +			break;
> > +		}
> 
> It's not clear that we want to have this check.  It would cause
> problems if the device ordering got mixed up by device_move(), which is
> pretty much the only way it could be triggered.

Well, isn't it actually dangerous to suspend parents before their children?
What happens, for example, if putting the parent into D3 causes power to be
cut from the children and then we attempt to suspend them?

> If you do want to leave it in, add a stack dump (and perhaps make it 
> not return an error).  This would help force people to figure out safe 
> ways to use device_move().

Yes, I'll add a stack dump here.

In fact, I'm going to add WARN_ON(true) in both places.

> > +		dev->power.sleeping = true;;
> 
> Extra ';'.

Hah, thanks, will remove.

Rafael

  reply	other threads:[~2008-03-04 21:54 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-03 23:10 Rafael J. Wysocki
2008-03-04 16:01 ` Alan Stern
2008-03-04 21:52   ` Rafael J. Wysocki [this message]
2008-03-05 16:03     ` Alan Stern
2008-03-05 16:16       ` Cornelia Huck
2008-03-05  1:15   ` Rafael J. Wysocki
2008-03-05 16:27     ` Alan Stern
2008-03-05 21:49       ` Rafael J. Wysocki
2008-03-05 22:00         ` Alan Stern
2008-03-05 23:24           ` Rafael J. Wysocki
2008-03-06 15:26             ` Alan Stern
2008-03-06 16:26               ` Rafael J. Wysocki
2008-03-06 17:40                 ` Alan Stern
2008-03-06 20:18                   ` Rafael J. Wysocki
2008-03-06 20:28                     ` Alan Stern
2008-03-06 21:28                       ` Rafael J. Wysocki
2008-03-06 21:58                         ` Alan Stern
2008-03-06 22:30                           ` Rafael J. Wysocki

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=200803042252.51440.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=pavel@ucw.cz \
    --cc=stern@rowland.harvard.edu \
    --subject='Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation' \
    /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).