Re: if_iwn.c patch to support the 5100... (analysis + patch)

[David Young <dyoung%pobox.com@localhost>]

On Fri, Sep 18, 2009 at 11:27:01AM -0400, Christos Zoulas wrote:
> On Sep 18,  9:14am, sverre%viewmark.com@localhost (Sverre Froyen) wrote:
> -- Subject: Re: if_iwn.c patch to support the 5100... (analysis + patch)
> 
> | On Fri September 18 2009 04:05:19 Brett Lymn wrote:
> | > On Thu, Sep 17, 2009 at 09:39:20AM -0600, Sverre Froyen wrote:
> | > > It looks like these errors may be because iwn_init is called a second
> | > > time before the first call has completed.  It is called via the 
> "if_init"
> | > > pointer and also through iwn_ioctl.  The call via the ioctl path only
> | > > happens if the IFF_UP is set.  I notice that, e.g., net/if_ethersubr.c
> | > > sets this flag before the call to if_init.  While this seems
> | > > counterintuitive to me it is appears to be the way it's always worked. 
> | > > The simplest fix is to remove the call to iwn_init from iwn_ioctl:
> | > 
> | > Doesn't that have the unfortunate side effect of you then being unable
> | > to reset the interface by doing an ifconfig down/up?
> | 
> | You are correct.  Calling it a patch was an exaggeration.  I quick test 
> shows 
> | that ifconfig up/down indeed leaves the interface in a non-working state.  
> | (Still better than a panic at boot, though.)
> | 
> | In order to serialize access (or really prevent a second simultaneous 
> access) 
> | is the following the correct approach?
> 
> 
> cvs rdiff -u -r1.151 -r1.152 src/sys/netinet6/in6.c           
> cvs rdiff -u -r1.82 -r1.83 src/sys/netinet6/in6_ifattach.c    
> cvs rdiff -u -r1.11 -r1.12 src/sys/netinet6/in6_ifattach.h    
> 
> Can you try  backing out those three changes instead?

As I mentioned to Christos the other day, I strongly suspect that
backing out those changes of mine will fix iwn(4).  It will fix some
problems that Matthias Drochner has with an(4) and wi(4), too.  It
may also fix a problem that I am having with ath(4).  It is just a
coincidence that those are all wireless drivers, BTW: an, wi, and
iwn are affected because they sleep in if_init, which gives the IPv6
workqueue (introduced with my changes) a chance to invoke if_init,
again, before the first invocation is complete.  I am not sure if/why
ath(4) is affected.

In the short term, I will back out the IPv6 workqueue and rewrite agr(4)
and gre(4) locking to use an icky recursive lock.  I will probably
re-use the recursive lock that I use to synchronize pmf(9) access to
device_t's.  I will probably not get around to that until next week.

In the long run, we need to document the kernel's current expectations
about the ifnet entry points:

        No more than one thread may be in the same if_ioctl routine
        at once, however, a call stack if_ioctl -> ... -> if_ioctl
        is possible, if only through the IPv6 stack.

        if_init is not re-entrant.

We also need to think about how to synchronize access to ifnet's in the
future.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933