Date: Fri, 30 May 2008 07:46:10 -0400 From: Coleman Kane <cokane@FreeBSD.org> To: Andrew Thompson <thompsa@FreeBSD.org> Cc: current@FreeBSD.org Subject: Re: ndis(4) patch to replace obsolete if_watchdog interface Message-ID: <1212147970.1606.13.camel@localhost> In-Reply-To: <20080530060141.GB68753@citylink.fud.org.nz> References: <1212093692.1681.8.camel@localhost> <20080529214604.GA68753@citylink.fud.org.nz> <1212104731.1606.9.camel@localhost> <20080530060141.GB68753@citylink.fud.org.nz>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-5EdkLJEHnttaRgZoHuxY Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Thu, 2008-05-29 at 23:01 -0700, Andrew Thompson wrote: > On Thu, May 29, 2008 at 07:45:31PM -0400, Coleman Kane wrote: > > On Thu, 2008-05-29 at 14:46 -0700, Andrew Thompson wrote: > > > On Thu, May 29, 2008 at 04:41:32PM -0400, Coleman Kane wrote: > > > > Hi, > > > >=20 > > > > I just replaced the obsoleted if_watchdog interface in ndis(4) with= a > > > > local implementation. This should remove the obnoxious warning mess= age > > > > on device init. Anyone using -CURRENT with an ndis card, could you = send > > > > me success/fails? > > > >=20 > > > > The patch is here: > > > > * http://people.freebsd.org/~cokane/patches/if_ndis-new_wd.patch > > >=20 > > >=20 > > > This works different to the rest of the network drivers. The existing > > > drivers use a callout tick that runs while the driver is up and an > > > integer counter. > > >=20 > > > if (x && --x =3D=3D 0) > > > ...timeout... > > >=20 > > > You arm the callout and stop it after each Tx, does this have any > > > perfornace impact? > > >=20 > > >=20 > > > Andrew > > >=20 > >=20 > > I am attaching a new patch, where I use the method that you suggested > > and do away with the extra callout for the watchdog. A "tick" function > > was already implemented for timeouts related to the NDIS systems. > >=20 > > I am not sure if the ndis-timeout code using ndis_stat_callout > > duplicates the if_watchdog code (and what I replaced it with), but I > > don't think so as both were implemented side-by-side before... > >=20 >=20 > A few comments inline >=20 > >diff --git a/sys/dev/if_ndis/if_ndis.c b/sys/dev/if_ndis/if_ndis.c > >+ NDIS_LOCK(sc); > >+ if(sc->ndis_timer_countdown && > >+ (sc->ndis_timer_countdown -=3D > >+ sc->ndis_block->nmb_checkforhangsecs) < 1) { > >+ sc->ndis_timer_countdown =3D 0; > >+ NDIS_UNLOCK(sc); > >+ ndis_watchdog(xsc); > >+ } else { > >+ NDIS_UNLOCK(sc); > >+ } >=20 > I think this is harder than it needs to be. It locks, unlocks, and then > locks again in ndis_watchdog. I have attached a version of this diff > that nukes ndis_watchdog() and just merges the code. Its also a pain to > use nmb_checkforhangsecs for the timer period, the ndis driver blob can > set this to whatever it desires and it could be far greater than the > watchdog period of 5 secs. >=20 > I introduced two counters, ndis_tx_timer and ndis_hang_timer, that allow > these events to be independent of each other. >=20 > > return; > > } > >=20 > >@@ -1888,7 +1904,9 @@ ndis_start(ifp) > > /* > > * Set a timeout in case the chip goes out to lunch. > > */ > >- ifp->if_timer =3D 5; > >+ if(sc->ndis_timer_countdown =3D=3D 0) { > >+ sc->ndis_timer_countdown =3D 5; > >+ } >=20 > This isnt right. The timer doesnt need to expire in order to be reset, > it should just set it each time. Otherwise if the timer is at 1 and the > callout is _just_ about to fire then you will get a false watchdog > timeout. >=20 >=20 >=20 > cheers, > Andrew Thanks this has all been very helpful. I'll test this out today and let you know how it goes. --=20 Coleman Kane --=-5EdkLJEHnttaRgZoHuxY Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEABECAAYFAkg/6QAACgkQcMSxQcXat5fwCQCeJwM0aLFBamfuGyTpuMSTorIC /3oAnRvYP71xHJ4sig3of5eCl4Hiyal9 =5YFo -----END PGP SIGNATURE----- --=-5EdkLJEHnttaRgZoHuxY--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1212147970.1606.13.camel>