From owner-freebsd-current@FreeBSD.ORG Fri May 30 12:45:07 2008 Return-Path: Delivered-To: current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C73EA1065675; Fri, 30 May 2008 12:45:07 +0000 (UTC) (envelope-from cokane@FreeBSD.org) Received: from hrndva-omtalb.mail.rr.com (hrndva-omtalb.mail.rr.com [71.74.56.123]) by mx1.freebsd.org (Postfix) with ESMTP id 730DF8FC15; Fri, 30 May 2008 12:45:07 +0000 (UTC) (envelope-from cokane@FreeBSD.org) Received: from orion.intree.net ([70.62.16.218]) by hrndva-omta01.mail.rr.com with ESMTP id <20080530124506.PJBF17285.hrndva-omta01.mail.rr.com@orion.intree.net>; Fri, 30 May 2008 12:45:06 +0000 Received: from mail.cokane.org (unknown [172.31.0.6]) by orion.intree.net (Postfix) with ESMTP id B4BA0361C04A; Fri, 30 May 2008 08:45:05 -0400 (EDT) Received: by mail.cokane.org (Postfix, from userid 103) id 8DEE81DB2F9; Fri, 30 May 2008 08:45:05 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.1.8-gr1 (2007-02-13) on discordia X-Spam-Level: X-Spam-Status: No, score=-4.4 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.1.8-gr1 Received: from [172.20.1.3] (erwin.int.cokane.org [172.20.1.3]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.cokane.org (Postfix) with ESMTP id 0B58216B55B; Fri, 30 May 2008 08:44:52 -0400 (EDT) From: Coleman Kane To: Andrew Thompson In-Reply-To: <1212147970.1606.13.camel@localhost> References: <1212093692.1681.8.camel@localhost> <20080529214604.GA68753@citylink.fud.org.nz> <1212104731.1606.9.camel@localhost> <20080530060141.GB68753@citylink.fud.org.nz> <1212147970.1606.13.camel@localhost> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-lnBRs4Lg5yOx5WeImu28" Organization: FreeBSD Project Date: Fri, 30 May 2008 08:44:01 -0400 Message-Id: <1212151441.1802.0.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.22.1.1 FreeBSD GNOME Team Port Cc: current@FreeBSD.org Subject: Re: ndis(4) patch to replace obsolete if_watchdog interface X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 May 2008 12:45:08 -0000 --=-lnBRs4Lg5yOx5WeImu28 Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Fri, 2008-05-30 at 07:46 -0400, Coleman Kane wrote: > 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) wi= th a > > > > > local implementation. This should remove the obnoxious warning me= ssage > > > > > on device init. Anyone using -CURRENT with an ndis card, could yo= u send > > > > > me success/fails? > > > > >=20 > > > > > The patch is here: > > > > > * http://people.freebsd.org/~cokane/patches/if_ndis-new_wd.patc= h > > > >=20 > > > >=20 > > > > This works different to the rest of the network drivers. The existi= ng > > > > 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" functio= n > > > 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 allo= w > > 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 >=20 > Thanks this has all been very helpful. I'll test this out today and let > you know how it goes. >=20 Your rewrite is working pretty well for me. --=20 Coleman Kane --=-lnBRs4Lg5yOx5WeImu28 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/9owACgkQcMSxQcXat5crWgCfUb2QcVhPvBDAXgoDi18I/ba0 yq4An094ocoxViK/anhVWp1RMbP8hyPv =8apK -----END PGP SIGNATURE----- --=-lnBRs4Lg5yOx5WeImu28--