svn commit: r250956 - projects/pmac_pmu/sys/kern

[Justin Hibbits]

On Fri, May 24, 2013 at 5:49 AM, John Baldwin <jhb at freebsd.org> wrote:

> On Thursday, May 23, 2013 11:58:31 pm Justin Hibbits wrote:
> > Author: jhibbits
> > Date: Fri May 24 03:58:31 2013
> > New Revision: 250956
> > URL: http://svnweb.freebsd.org/changeset/base/250956
> >
> > Log:
> >   Add multipass for suspend/resume.  This allows some devices to be
> resumed before
> >   others, even their peers, as required by PowerPC Mac hardware.
>
> I think this is a good start.  One question I have is why not reuse the
> pass number
> from the driver instead of stuffing it in the devclass?  (It is
> conceivable that
> different drivers with different passes might share a devclass even,
> though unlikely).
>
> That is, can't you use child->driver->dl_pass directly?  (If a child
> doesn't have a
> driver it can't suspend or resume anyway).
>

I was going to do this, but I don't see a dl_pass in the driver_t
structure, the pass number currently goes into the driverlink list, so this
was the quickest way to add and test this, as well as the most
nonintrusive.  I can change the driver_t definition to add a pass, though,
it's not difficult, but would change the module ABI, since the driver
definition is public.


> Also, can you explain the EAGAIN logic?  It's not obvious.  Is this how
> you are
> enforcing that already resumed drivers keep traversing their tree in
> subsequent
> passes (if not you need to deal with that in some fashion).
>

Yes, I use EAGAIN to mean "Make another pass at this".  I discussed it with
Warner at BSDCan and that seemed a simple solution.  Drivers that are
pass-aware can check bus_current_pass for the pass they want to resume
themselves on, and just call bus_generic_resume() if it's not their pass.
Drivers that are not pass aware would behave as they do now.


> I think we might want to be more explicit about forcing suspend and resume
> to
> traverse the tree for each pass.  The boot-time probe has a BUS_NEW_PASS
> callback
> it uses for this, and bus_generic_new_pass() is what it uses for that.  I
> think
> you might want to create BUS_SUSPEND_PASS and BUS_RESUME_PASS with generic
> versions similar to bus_generic_new_pass.  You would check the DF_SUSPENDED
> flag there to decide whether or not you wanted to call
> BUS_SUSPEND/RESUME_PASS
> or DEVICE_SUSPEND().
>

I did consider this as well.  However, I'm not certain what that would add
over this way.  I found this to be relatively nonintrusive and mostly
straight forward.

The other thing that makes this more complicated is that unlike
> probe/attach,
> we might need to actually ask the bus to suspend and resume the device so
> that
> the bus can do power management.  Right now this works because the bus
> devices
> suspend everything in one pass so they can order things correctly (e.g.
> pci_suspend()).  If we want to shut down some devices early we might
> consider
> having a new bus_if method which takes a child and handles suspending that
> specific child (it would call DEVICE_SUSPEND).  For PCI you might get
> something
> like this:
>
> int
> pci_suspend_child(device_t bus, device_t child)
> {
>         struct pci_devinfo *dinfo;
>         int error;
>
>         dinfo = device_get_ivars(child);
>         pci_cfg_save(child, dinfo, 0);
>         error = DEVICE_SUSPEND(child);
>         if (error)
>                 return (error);
>         if (pci_do_power_suspend)
>                 /* set power state for just this child to D3 */
>         return (error);
> }
>
> I need to think a bit more, but I think this is more of a correct model to
> handle
> passes, and will also be cleaner for suspend/resume in general.


I agree that adding the ability to suspend children independently is a good
idea.  I'm interested in any more thoughts you have on this, so we could
get it working for everything.

I do want to get this correct before merging into HEAD.  There are still
other bugs I need to fix, such as suspend/resume of the video (might need
to wait until the DRM2 branch is merged for the video reset, so this will
add some time, too).

- Justin