cvs commit: src/sys/dev/re if_re.c

[Bill Paul]

> Hi,
> 
> On Fri, Sep 16, 2005 at 11:06:08PM +0000, Bill Paul wrote:
> > If you insist on sticking to the "no twiddling IFF_UP from inside
> > the driver" rule, then I think the problem can be avoided by simply not
> > being lazy in foo_ioctl() and actually having explicit code in SIOCSIFFLAGS
> > case that turns promisc mode on and off on an IFF_PROMISC transition,
> > and is careful not to do it unless IFF_RUNNING is set. It also
> > needs to handle IFF_UP separately from IFF_PROMISC. I think the
> > correct logic would look something like:
> > 
> > 	if (IFF_UP was toggled up)
> > 		foo_init(sc);
> > 	else if (IFF_UP was toggled down)
> > 		foo_stop(sc);
> > 
> > 	if (IFF_PROMISC was toggled up && ifp->if_flags & IFF_RUNNING)
> > 		foo_set_promisc(sc);
> > 	else if (IFF_PROMISC was toggled down && ifp->if_flags & IFF_RUNNING)
> > 		foo_clear_promisc(sc);
> > 
> How about prototyping the "lazy" SIOCSIFFLAGS handler as follows:
> 
> 	if (IFF_UP has toggled up)
> 		foo_init();	/* foo_init takes care of IFF_PROMISC etc. */
> 	else if (IFF_UP has toggled down)
> 		foo_stop();
> 	else if (IFF_DRV_RUNNING) {
> 		if (something interesting has toggled)
> 			foo_init();
> 	}
> 

I don't think that's quite right. Remember, it's possible to manipulate
serveral flags with a single call to SIOCSIFFLAGS. Supposing I call
SIOCSIFFLAGS to set both the IFF_UP and IFF_PROMISC bits at the same
time. With your logic, the interface will be brought up, but the
IFF_PROMISC setting will be ignored.
 
I think this is more like it:
 
        if (IFF_UP was toggled up)
                foo_init(sc);
        else if (IFF_UP was toggled down)
                foo_stop(sc);
 
        if (ifp->if_flags & IFF_RUNNING) {
                /* handle other state bits, PROMISC, ALLMULTI, etc... */
        }
 
If possible, you should avoid shortcutting the "handle other state bits"
part with foo_init(), since there are other side effects. For example,
foo_init() will usually reset the link state, so if the link is up, it'll
drop it and then restablish it. For ethernet, renegotiating the link
could take a couple of seconds. You don't want to clobber the link
for that length of time if all you want to do is toggle PROMISC or
ALLMULTI mode. These things can be programmed on the fly without
resetting the chip. Promisc mode is usually just a matter of setting one
bit in a register somewhere.

-Bill

--
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul at windriver.com | Wind River Systems
=============================================================================
              <adamw> you're just BEGGING to face the moose
=============================================================================