Re: vlan panic in -CURRENT

On Sat, Feb 07, 2004 at 08:50:32AM +0900, SUZUKI Shinsuke wrote:
> >>>>> On Fri, 6 Feb 2004 10:33:06 -0800
> >>>>> brooks_at_one-eyed-alien.net(Brooks Davis)  said:
> 
> > > This crashes -CURRENT (from a couple of days ago) for me:
> > > ifconfig vlan0 create
> > > ifconfig vlan0 vlan 2 vlandev fxp0
> > > ifconfig vlan0 up
> > > 
> > > (immediate trap 12/page fault while in kernel mode crash)
> > > 
> > > BUT, if I manually bring up fxp0 first, everything is fine.
> (snip)
> > Well, it's not related to simply upping a vlan on a down interface since
> > I can't reproduce it with fwe0.  I've got a shortage of systems
> > configured for debugging that have an extra nic in them so I can't seem
> > to replicate this.  Since it should be possiable to replicate in single
> > user mode, you might try setting hw.physmem to something small enough
> > that you can get a crashdump.
> 
> I can reproduce the problem, and here's the trace:
> 	  if_up->if_route->in6_if_up->in6_ifattach->in6_ifattach_link
> 	->in6_update_ifa->in6_addmulti->if_addmulti->vlan_ioctl
> 	->vlan_setmulti->if_delmulti->fxp_ioctl->fxp_mc_setup
> 
> The reason of this panic lies in if_fxp.c; fxp's
> ethernet-multicast-filter is configured before the initializtion of
> fxp driver.
> 
> The attached ad-hoc patch fixed the problem, but IMHO much further
> considerataion is necessary; this happens when vlan is initialized
> before the initialization of its physical interface, and there might
> be a similar different bug (in other driver or in different situation).
> 
> So could anyone see to it?
> #I'm afraid I cannot, because I'm not a device-driver expert...

I can't claim to fully understand what is going on, but it looks to me
like there is no point in fxp calling fxp_mc_setup when the interface
is not up (it might be OK if down and running, but I'm not sure about
that.)  The following untested patch avoids modifying the hardware
filter in favor of setting sc->need_mcsetup when the interface is down.
It looks like setting sc->need_mcsetup will cause fxp_mc_setup to be
called in the interupt handler so it will get taken care of when the
interface is brought up.

-- Brooks

==== //depot/user/brooks/cleanup/sys/dev/fxp/if_fxp.c#2 - /usr/p4/cleanup/sys/dev/fxp/if_fxp.c ====
_at__at_ -2460,18 +2460,28 _at__at_
 			sc->flags |= FXP_FLAG_ALL_MCAST;
 		else
 			sc->flags &= ~FXP_FLAG_ALL_MCAST;
-		/*
-		 * Multicast list has changed; set the hardware filter
-		 * accordingly.
-		 */
-		if ((sc->flags & FXP_FLAG_ALL_MCAST) == 0)
-			fxp_mc_setup(sc);
-		/*
-		 * fxp_mc_setup() can set FXP_FLAG_ALL_MCAST, so check it
-		 * again rather than else {}.
-		 */
-		if (sc->flags & FXP_FLAG_ALL_MCAST)
-			fxp_init_body(sc);
+
+		if (ifp->if_flags & IFF_UP) {
+			/*
+			 * Multicast list has changed; set the hardware
+			 * filter accordingly.
+			 */
+			if ((sc->flags & FXP_FLAG_ALL_MCAST) == 0)
+				fxp_mc_setup(sc);
+			/*
+			 * fxp_mc_setup() can set FXP_FLAG_ALL_MCAST, so
+			 * check it again rather than else {}.
+			 */
+			if (sc->flags & FXP_FLAG_ALL_MCAST)
+				fxp_init_body(sc);
+		} else {
+			/*
+			 * We can't set the hardware filter if we are
+			 * not up, so just signal that we need to do so
+			 * later.
+			 */
+			sc->need_mcsetup = 1;
+		}
 		error = 0;
 		break;
 

-- 
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529  9BF0 5D8E 8BE9 F238 1AD4