Date: Thu, 29 May 2008 23:01:41 -0700 From: Andrew Thompson <thompsa@FreeBSD.org> To: Coleman Kane <cokane@FreeBSD.org> Cc: current@FreeBSD.org Subject: Re: ndis(4) patch to replace obsolete if_watchdog interface Message-ID: <20080530060141.GB68753@citylink.fud.org.nz> In-Reply-To: <1212104731.1606.9.camel@localhost> References: <1212093692.1681.8.camel@localhost> <20080529214604.GA68753@citylink.fud.org.nz> <1212104731.1606.9.camel@localhost>
next in thread | previous in thread | raw e-mail | index | archive | help
--2oS5YaxWCcQjTEyO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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, > > > > > > I just replaced the obsoleted if_watchdog interface in ndis(4) with a > > > local implementation. This should remove the obnoxious warning message > > > on device init. Anyone using -CURRENT with an ndis card, could you send > > > me success/fails? > > > > > > The patch is here: > > > * http://people.freebsd.org/~cokane/patches/if_ndis-new_wd.patch > > > > > > 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. > > > > if (x && --x == 0) > > ...timeout... > > > > You arm the callout and stop it after each Tx, does this have any > > perfornace impact? > > > > > > Andrew > > > > 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. > > 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... > A few comments inline >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 -= >+ sc->ndis_block->nmb_checkforhangsecs) < 1) { >+ sc->ndis_timer_countdown = 0; >+ NDIS_UNLOCK(sc); >+ ndis_watchdog(xsc); >+ } else { >+ NDIS_UNLOCK(sc); >+ } 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. I introduced two counters, ndis_tx_timer and ndis_hang_timer, that allow these events to be independent of each other. > return; > } > >@@ -1888,7 +1904,9 @@ ndis_start(ifp) > /* > * Set a timeout in case the chip goes out to lunch. > */ >- ifp->if_timer = 5; >+ if(sc->ndis_timer_countdown == 0) { >+ sc->ndis_timer_countdown = 5; >+ } 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. cheers, Andrew --2oS5YaxWCcQjTEyO Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="ndis_watchdog.diff" Index: if_ndis.c =================================================================== RCS file: /home/ncvs/src/sys/dev/if_ndis/if_ndis.c,v retrieving revision 1.139 diff -u -p -r1.139 if_ndis.c --- if_ndis.c 12 May 2008 00:15:28 -0000 1.139 +++ if_ndis.c 30 May 2008 05:57:03 -0000 @@ -161,7 +161,6 @@ static void ndis_scan_curchan (struct ie static void ndis_scan_mindwell (struct ieee80211_scan_state *); static void ndis_init (void *); static void ndis_stop (struct ndis_softc *); -static void ndis_watchdog (struct ifnet *); static int ndis_ifmedia_upd (struct ifnet *); static void ndis_ifmedia_sts (struct ifnet *, struct ifmediareq *); static void ndis_auth (void *, int); @@ -537,6 +536,7 @@ ndis_attach(dev) KeInitializeSpinLock(&sc->ndis_spinlock); KeInitializeSpinLock(&sc->ndis_rxlock); InitializeListHead(&sc->ndis_shlist); + callout_init(&sc->ndis_stat_callout, CALLOUT_MPSAFE); if (sc->ndis_iftype == PCMCIABus) { error = ndis_alloc_amem(sc); @@ -695,7 +695,6 @@ ndis_attach(dev) ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; ifp->if_ioctl = ndis_ioctl; ifp->if_start = ndis_start; - ifp->if_watchdog = ndis_watchdog; ifp->if_init = ndis_init; ifp->if_baudrate = 10000000; IFQ_SET_MAXLEN(&ifp->if_snd, 50); @@ -1540,7 +1539,7 @@ ndis_txeof(adapter, packet, status) else ifp->if_oerrors++; - ifp->if_timer = 0; + sc->ndis_tx_timer = 0; ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; NDIS_UNLOCK(sc); @@ -1642,14 +1641,29 @@ ndis_tick(xsc) struct ndis_softc *sc; sc = xsc; + NDIS_LOCK(sc); - IoQueueWorkItem(sc->ndis_tickitem, - (io_workitem_func)ndis_ticktask_wrap, - WORKQUEUE_CRITICAL, sc); - callout_reset(&sc->ndis_stat_callout, - hz * sc->ndis_block->nmb_checkforhangsecs, ndis_tick, sc); + if (sc->ndis_hang_timer && --sc->ndis_hang_timer == 0) { + IoQueueWorkItem(sc->ndis_tickitem, + (io_workitem_func)ndis_ticktask_wrap, + WORKQUEUE_CRITICAL, sc); + sc->ndis_hang_timer = sc->ndis_block->nmb_checkforhangsecs; + } - return; + if (sc->ndis_tx_timer && --sc->ndis_tx_timer == 0) { + sc->ifp->if_oerrors++; + device_printf(sc->ndis_dev, "watchdog timeout\n"); + + IoQueueWorkItem(sc->ndis_resetitem, + (io_workitem_func)ndis_resettask_wrap, + WORKQUEUE_CRITICAL, sc); + IoQueueWorkItem(sc->ndis_startitem, + (io_workitem_func)ndis_starttask_wrap, + WORKQUEUE_CRITICAL, sc->ifp); + } + + callout_reset(&sc->ndis_stat_callout, hz, ndis_tick, sc); + NDIS_UNLOCK(sc); } static void @@ -1888,7 +1902,7 @@ ndis_start(ifp) /* * Set a timeout in case the chip goes out to lunch. */ - ifp->if_timer = 5; + sc->ndis_tx_timer = 5; NDIS_UNLOCK(sc); @@ -1982,11 +1996,7 @@ ndis_init(xsc) ifp->if_drv_flags |= IFF_DRV_RUNNING; ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; - - NDIS_UNLOCK(sc); - - /* XXX force handling */ - ieee80211_start_all(ic); /* start all vap's */ + sc->ndis_tx_timer = 0; /* * Some drivers don't set this value. The NDIS spec says @@ -1994,15 +2004,15 @@ ndis_init(xsc) * seconds." We use 3 seconds, because it seems for some * drivers, exactly 2 seconds is too fast. */ - if (sc->ndis_block->nmb_checkforhangsecs == 0) sc->ndis_block->nmb_checkforhangsecs = 3; - callout_init(&sc->ndis_stat_callout, CALLOUT_MPSAFE); - callout_reset(&sc->ndis_stat_callout, - hz * sc->ndis_block->nmb_checkforhangsecs, ndis_tick, sc); + sc->ndis_hang_timer = sc->ndis_block->nmb_checkforhangsecs; + callout_reset(&sc->ndis_stat_callout, hz, ndis_tick, sc); + NDIS_UNLOCK(sc); - return; + /* XXX force handling */ + ieee80211_start_all(ic); /* start all vap's */ } /* @@ -3112,29 +3122,6 @@ ndis_resettask(d, arg) return; } -static void -ndis_watchdog(ifp) - struct ifnet *ifp; -{ - struct ndis_softc *sc; - - sc = ifp->if_softc; - - NDIS_LOCK(sc); - ifp->if_oerrors++; - device_printf(sc->ndis_dev, "watchdog timeout\n"); - NDIS_UNLOCK(sc); - - IoQueueWorkItem(sc->ndis_resetitem, - (io_workitem_func)ndis_resettask_wrap, - WORKQUEUE_CRITICAL, sc); - IoQueueWorkItem(sc->ndis_startitem, - (io_workitem_func)ndis_starttask_wrap, - WORKQUEUE_CRITICAL, ifp); - - return; -} - /* * Stop the adapter and free any mbufs allocated to the * RX and TX lists. @@ -3150,7 +3137,7 @@ ndis_stop(sc) callout_drain(&sc->ndis_stat_callout); NDIS_LOCK(sc); - ifp->if_timer = 0; + sc->ndis_tx_timer = 0; sc->ndis_link = 0; ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); NDIS_UNLOCK(sc); Index: if_ndisvar.h =================================================================== RCS file: /home/ncvs/src/sys/dev/if_ndis/if_ndisvar.h,v retrieving revision 1.31 diff -u -p -r1.31 if_ndisvar.h --- if_ndisvar.h 10 May 2008 20:12:43 -0000 1.31 +++ if_ndisvar.h 30 May 2008 05:29:02 -0000 @@ -181,6 +181,8 @@ struct ndis_softc { struct task ndis_assoctask; int (*ndis_newstate)(struct ieee80211com *, enum ieee80211_state, int); + int ndis_tx_timer; + int ndis_hang_timer; }; #define NDIS_LOCK(_sc) KeAcquireSpinLock(&(_sc)->ndis_spinlock, \ --2oS5YaxWCcQjTEyO--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080530060141.GB68753>