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

[Ruslan Ermilov]

Hi,

On Sun, Sep 18, 2005 at 03:06:10AM +0000, Bill Paul wrote:
> > 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.
>  
The "/* foo_init takes care of IFF_PROMISC etc. */" comment
above was supposed to answer this question.  From what I can
tell, this holds true for all drivers.

> 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.
> 
That's clear.  I'm talking about fixing all drivers for the
BPF detach bug.


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20050918/940c98db/attachment.bin