From owner-cvs-all@FreeBSD.ORG Sun Sep 18 03:06:10 2005 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: by hub.freebsd.org (Postfix, from userid 618) id 64B5616A420; Sun, 18 Sep 2005 03:06:10 +0000 (GMT) In-Reply-To: <20050917101552.GC22151@ip.net.ua> from Ruslan Ermilov at "Sep 17, 2005 01:15:52 pm" To: ru@FreeBSD.org (Ruslan Ermilov) Date: Sun, 18 Sep 2005 03:06:10 +0000 (GMT) X-Mailer: ELM [version 2.4ME+ PL54 (25)] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-Id: <20050918030610.64B5616A420@hub.freebsd.org> From: wpaul@FreeBSD.ORG (Bill Paul) Cc: cvs-all@FreeBSD.org, cvs-src@FreeBSD.org, src-committers@FreeBSD.org, jhb@FreeBSD.org, imp@bsdimp.com Subject: Re: cvs commit: src/sys/dev/re if_re.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 18 Sep 2005 03:06:10 -0000 > 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@windriver.com | Wind River Systems ============================================================================= you're just BEGGING to face the moose =============================================================================