Date: Thu, 16 May 2013 14:34:37 -0400 From: Justin Hibbits <jrh29@alumni.cwru.edu> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: late suspend/early resume Message-ID: <CAHSQbTBAaeavgnYeTepiw4G59ApUym=9RJy5xRDPrfmLNoYRUQ@mail.gmail.com> In-Reply-To: <519369C4.6060402@FreeBSD.org> References: <CAHSQbTBNjrx=9DT7vk29D=Y%2BOK0qV=Ld4qN-sxy%2B8_OONazKAA@mail.gmail.com> <AE98E779-E22B-434D-9BEE-BF66241BB2E6@bsdimp.com> <51913B7D.1040801@freebsd.org> <288C9B9E-E943-4C5B-BCFB-15B721CBE94C@bsdimp.com> <CAHSQbTCEOfCH5tLeWgP4gPVbsBwDKLKBboQ2d74npkQomoJJJQ@mail.gmail.com> <519369C4.6060402@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Wed, May 15, 2013 at 6:56 AM, John Baldwin <jhb@freebsd.org> wrote:
> On 5/14/13 1:14 PM, Justin Hibbits wrote:
> > You are right that the late suspend could lead to silly proliferation,
> and
> > an ordered list is much better, but another API would need to be added to
> > do that as well.
> >
> > My north bridge is first in the top list of the tree, right under the
> > nexus, so to suspend it last I wrote the nexus suspend to traverse its
> > children in reverse. The problem comes that the clock controller is
> under a
> > later PCI bus, not even the immediate following one, and the north bridge
> > children are i2c devices, so suspending them after their clock head away
> is
> > problematic. We can discuss this more at bsdcan, where it may be easier
> to
> > describe. But essentially I need the north bridge and that pesky clock
> > controller to be the last to suspend and the first to resume. I guess we
> > can take this as the starting discussion for modeling this relationship
> on
> > all platforms, since you mention it is common in embedded platforms.
>
> I think you can do this by having a notion of passes with drivers having
> a suspend pass level and doing passes over the tree suspending devices
> at each pass level and then walking the passes back up in reverse during
> resume. You could borrow from the multipass stuff used on probe/attach
> for this.
>
> --
> John Baldwin
Here's a (very) rough cut of what I think you're getting at. It uses the
existing pass identifier to convey the current pass number, and walks the
pass in reverse for suspend, and forwards for resume. However, I can't
stress enough, that I only compiled it, I have no hardware here at BSDCan
to test, so I have no idea if it will work as expected.
The basic idea is to start at the last pass number, and any device can
suspend and resume if able, or can check and return EAGAIN if it's not
ready to suspend itself (it can still suspend its children if it wants). I
could also add to bus_generic_resume() a check for the device's driver's
pass wrt bus_current_pass, as in the probe code.
Is this what you're thinking?
- Justin
[-- Attachment #2 --]
Index: sys/kern/subr_bus.c
===================================================================
--- sys/kern/subr_bus.c (revision 248255)
+++ sys/kern/subr_bus.c (working copy)
@@ -130,6 +130,7 @@
#define DF_DONENOMATCH 0x20 /* don't execute DEVICE_NOMATCH again */
#define DF_EXTERNALSOFTC 0x40 /* softc not allocated by us */
#define DF_REBID 0x80 /* Can rebid after attach */
+#define DF_SUSPENDED 0x100 /* Device is suspended. */
u_int order; /**< order from device_add_child_ordered() */
void *ivars; /**< instance variables */
void *softc; /**< current driver's variables */
@@ -3532,19 +3533,29 @@
bus_generic_suspend(device_t dev)
{
int error;
+ int again = 0;
device_t child, child2;
TAILQ_FOREACH(child, &dev->children, link) {
- error = DEVICE_SUSPEND(child);
- if (error) {
- for (child2 = TAILQ_FIRST(&dev->children);
- child2 && child2 != child;
- child2 = TAILQ_NEXT(child2, link))
- DEVICE_RESUME(child2);
- return (error);
+ if (!(child->flags & DF_SUSPENDED)) {
+ error = DEVICE_SUSPEND(child);
+ if (error && error != EAGAIN) {
+ for (child2 = TAILQ_FIRST(&dev->children);
+ child2 && child2 != child;
+ child2 = TAILQ_NEXT(child2, link)) {
+ DEVICE_RESUME(child2);
+ child2->flags &= ~DF_SUSPENDED;
+ }
+ return (error);
+ }
+ if (error == EAGAIN) {
+ again = EAGAIN;
+ continue;
+ }
+ child->flags |= DF_SUSPENDED;
}
}
- return (0);
+ return (again);
}
/**
@@ -3557,12 +3568,19 @@
bus_generic_resume(device_t dev)
{
device_t child;
+ int error = 0;
TAILQ_FOREACH(child, &dev->children, link) {
- DEVICE_RESUME(child);
- /* if resume fails, there's nothing we can usefully do... */
+ if (child->flags & DF_SUSPENDED) {
+ if (DEVICE_RESUME(child) == EAGAIN) {
+ error = EAGAIN;
+ continue;
+ }
+ /* if resume fails, there's nothing we can usefully do... */
+ child->flags &= ~DF_SUSPENDED;
+ }
}
- return (0);
+ return (error);
}
/**
@@ -4367,15 +4385,39 @@
static int
root_resume(device_t dev)
{
- int error;
+ struct driverlink *dl;
+ int error = 0;
- error = bus_generic_resume(dev);
+ TAILQ_FOREACH(dl, &passes, passlink) {
+ bus_current_pass = dl->pass;
+ error = bus_generic_resume(dev);
+
+ if (error != EAGAIN)
+ break;
+ }
+
if (error == 0)
devctl_notify("kern", "power", "resume", NULL);
return (error);
}
static int
+root_suspend(device_t dev)
+{
+ struct driverlink *dl;
+ int error = 0;
+
+ TAILQ_FOREACH_REVERSE(dl, &passes, driver_list, passlink) {
+ bus_current_pass = dl->pass;
+ error = bus_generic_resume(dev);
+ if (error != EAGAIN)
+ break;
+ }
+
+ return (error);
+}
+
+static int
root_print_child(device_t dev, device_t child)
{
int retval = 0;
@@ -4412,7 +4454,7 @@
static kobj_method_t root_methods[] = {
/* Device interface */
KOBJMETHOD(device_shutdown, bus_generic_shutdown),
- KOBJMETHOD(device_suspend, bus_generic_suspend),
+ KOBJMETHOD(device_suspend, root_suspend),
KOBJMETHOD(device_resume, root_resume),
/* Bus interface */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHSQbTBAaeavgnYeTepiw4G59ApUym=9RJy5xRDPrfmLNoYRUQ>