Date: Sun, 18 Sep 2005 19:34:57 +0300 From: Ruslan Ermilov <ru@FreeBSD.org> To: Bill Paul <wpaul@FreeBSD.org> 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 Message-ID: <20050918163457.GA43796@ip.net.ua> In-Reply-To: <20050918030610.64B5616A420@hub.freebsd.org> References: <20050917101552.GC22151@ip.net.ua> <20050918030610.64B5616A420@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--YiEDa0DAkWCtVeE4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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 n=
ot
> > > being lazy in foo_ioctl() and actually having explicit code in SIOCSI=
FFLAGS
> > > 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:
> > >=20
> > > if (IFF_UP was toggled up)
> > > foo_init(sc);
> > > else if (IFF_UP was toggled down)
> > > foo_stop(sc);
> > >=20
> > > 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);
> > >=20
> > How about prototyping the "lazy" SIOCSIFFLAGS handler as follows:
> >=20
> > 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();
> > }
> >=20
>=20
> 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.
> =20
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:
> =20
> if (IFF_UP was toggled up)
> foo_init(sc);
> else if (IFF_UP was toggled down)
> foo_stop(sc);
> =20
> if (ifp->if_flags & IFF_RUNNING) {
> /* handle other state bits, PROMISC, ALLMULTI, etc... */
> }
> =20
> 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.
>=20
That's clear. I'm talking about fixing all drivers for the
BPF detach bug.
Cheers,
--=20
Ruslan Ermilov
ru@FreeBSD.org
FreeBSD committer
--YiEDa0DAkWCtVeE4
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (FreeBSD)
iD8DBQFDLZcxqRfpzJluFF4RAo8rAJ0WF6pNyEbiWQmd5x/U2ZkP+v+vhgCgnj69
jJCmTTbUkGlEGkeAj7+QhZQ=
=qWe/
-----END PGP SIGNATURE-----
--YiEDa0DAkWCtVeE4--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050918163457.GA43796>