ndis(4) patch to replace obsolete if_watchdog interface
Coleman Kane
cokane at FreeBSD.org
Fri May 30 12:45:08 UTC 2008
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,
> > > > >
> > > > > 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
>
> Thanks this has all been very helpful. I'll test this out today and let
> you know how it goes.
>
Your rewrite is working pretty well for me.
--
Coleman Kane
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part
Url : http://lists.freebsd.org/pipermail/freebsd-current/attachments/20080530/a4bab8f8/attachment.pgp
More information about the freebsd-current
mailing list