svn commit: r215544 - head/sys/kern

[John Baldwin]

On Friday, November 19, 2010 4:31:44 pm Attilio Rao wrote:
> 2010/11/19 John Baldwin <jhb at freebsd.org>:
> > On Friday, November 19, 2010 4:09:28 pm Jung-uk Kim wrote:
> >> On Friday 19 November 2010 02:43 pm, Attilio Rao wrote:
> >> > Author: attilio
> >> > Date: Fri Nov 19 19:43:56 2010
> >> > New Revision: 215544
> >> > URL: http://svn.freebsd.org/changeset/base/215544
> >> >
> >> > Log:
> >> >   Scan the list in reverse order for the shutdown handlers of
> >> > loaded modules. This way, when there is a dependency between two
> >> > modules, the handler of the latter probed runs first.
> >> >
> >> >   This is a similar approach as the modules are unloaded in the
> >> > same linkerfile.
> >> >
> >> >   Sponsored by:     Sandvine Incorporated
> >> >   Submitted by:     Nima Misaghian <nmisaghian at sandvine dot com>
> >> >   MFC after:        1 week
> >>
> >> Hmm...  It is not directly related but I was thinking about doing
> >> similar things for sys/kern/subr_bus.c.  What do you think about the
> >> attached patch?
> >
> > Hmm, the patches for suspend and resume that I had for this took the opposite
> > order, they did suspend in forward order, but resume in backwards order.
> > Like so:
> >
> > --- //depot/vendor/freebsd/src/sys/kern/subr_bus.c      2010-11-17 22:30:24.000000000 0000
> > +++ //depot/user/jhb/acpipci/kern/subr_bus.c    2010-11-19 17:19:02.000000000 00
> > @@ -3426,9 +3429,9 @@
> >        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))
> > +                       for (child2 = TAILQ_PREV(child, device_list, link);
> > +                            child2 != NULL;
> > +                            child2 = TAILQ_PREV(child2, device_list, link))
> >                                DEVICE_RESUME(child2);
> >                        return (error);
> >                }
> > @@ -3447,7 +3450,7 @@
> >  {
> >        device_t        child;
> >
> > -       TAILQ_FOREACH(child, &dev->children, link) {
> > +       TAILQ_FOREACH_REVERSE(child, &dev->children, device_list, link) {
> >                DEVICE_RESUME(child);
> >                /* if resume fails, there's nothing we can usefully do... */
> >        }
> >
> > (Likely mangled whitespace.)
> >
> > I couldn't convince myself which order was "more" correct for suspend and resume.
> 
> Considering loading in starting point, I think suspend should go in
> reverse logic and resume in the same module load logic.
> So that dependent modules are suspended first and resumed after.
> Don't you agree?

These are devices, and the ordering here is the order of sibling devices on a
given bus.  That is, if you have a PCI bus with two em interfaces, does it
really matter if em0 suspends before em1 vs after em1?  I think it actually
doesn't matter.  The passes from the multipass boot probe might make some
sense to order on.  However, I don't think the order of siblings on a bus is
meaningful for suspend and resume (which is why I've never committed the
above patches).

Specifically, there is no dependency relationship between siblings on a bus.
Certain buses may in fact have a dependency order of sorts (vgapci0 comes to
mind), but those buses should manage that.  There is no generic dependency
between siblings that should be encoded into subr_bus.c

-- 
John Baldwin