svn commit: r215544 - head/sys/kern

[John Baldwin]

On Friday, November 19, 2010 5:41:00 pm Attilio Rao wrote:
> 2010/11/19 John Baldwin <jhb at freebsd.org>:
> > On Friday, November 19, 2010 5:03:25 pm Jung-uk Kim wrote:
> >> On Friday 19 November 2010 04:46 pm, John Baldwin wrote:
> >> > 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
> >>
> >> Generally siblings don't interact with each other directly, no.
> >> However, some modern chipsets *do* have strong relationship.  For
> >> example, some chipsets reference SMB controller to get current
> >> configuration, e.g., function A depends on function B on the same
> >> chip.
> >
> > That may be true, but that is not generic to all buses and devices.
> > That isn't even really generic to PCI.  If there are specific instances
> > where there are dependencies, the drivers for that hardware should
> > manage that.  If specific buses have specific orders, then they can
> > manage that order explicitly in their own suspend and resume routines.
> > However, I don't see a valid reason for enforcing a particular order
> > among siblings for all devices.  We certainly do enforce some orders
> > with respect to children and parents (parents attach first and resume
> > first, children detach first and suspend first, PCI even mandates this
> > for powering down a bus), but the same is not true for siblings.
> 
> However, it would be good if any ordering is maintained between
> devices linked via MODDEPEND stuff.
> It happens they may also be sibillings, and not havign parent-child
> relationship in some cases, but I'm also wondering if maybe they
> should ensure consistency in their code itself rather than relying
> strongly on ordering by the PCi/whatever subsystem?

Almost all of those devices have parent -> child relationships, not
sibling relationships (e.g. NIC drivers that depend on certain phy
drivers).  Do you have a specific example of drivers that have
dependencies where the current order is not correct?

-- 
John Baldwin