[PATCH] Remove if_watchdog use

John Baldwin jhb at freebsd.org
Fri Nov 6 20:08:40 UTC 2009

I have a patchset that converts all the remaining users of if_watchdog to 
using a private callout instead.  In some cases the the driver already used a 
private timer to drive a stats timer and I merely hooked into that timer.  In 
other cases a new callout needed to be added to the driver.  Some drivers 
even abused the if_watchdog interface to provide a stats timer that fired 
every second. :)  For a few drivers I also fixed other things such as busted 
locking, order-of-operations issues in detach, or just completely busted 
drivers (fea(4) and fpa(4) which share the pdq backend).  Please test.  
Barring any major screaming and shouting I plan to commit this in a week or 
so and after that to work on removing the if_watchdog/if_timer stuff from the 
network stack.

The patch is at http://www.FreeBSD.org/~jhb/patches/cleanup.patch

Driver details:
- an(4)
  - Locking fixes to not do silly things like drop the lock only to call a
    function that immediately reacquires the lock.  Also removes recursive
  - Hooks into the stat timer to drive the watchdog timer.
- bwi(4), cm(4), ep(4), fatm(4), malo(4), mwl(4), vx(4)
  - Adds a new private timer to drive the watchdog timer.
- ce(4), cp(4), ctau(4), cx(4), lmc(4)
  - These drivers have two modes, a netgraph mode and an ifnet mode.  In the
    netgraph mode they used a private timer to drive the watchdog.  In the
    ifnet mode they used if_watchdog.  Now they just always use the private
- de(4)
  - This driver abused the watchdog interface to manage its once-a-second
    stat timer.  It uses a callout to manage this now instead.
- ed(4)
  - This driver used to provide a hook to allow individual attachments to
    provide a 'tick' event to be called from an optional stats timer.
    The stats timer only ran if the tick function pointer was non-NULL
    and the attachment's tick routine had to call callout_reset(), etc.
    Now the driver always schedules a stat timer and manages the
    callout_reset() internally.  This timer is used to drive the watchdog
    and will also call the attachment's 'tick' handler if one is provided.
- ixgb(4)
  - Uses callout_init_mtx() instead of callout_init(..., CALLOUT_MPSAFE).
  - Remove silly callout handling in a few places (cancelling the callout
    only to rescheduled it again immediately afterwards).
  - Hooks into the stat timer to drive the watchdog timer.
- lge(4), nve(4), pcn(4)
  - Hooks into the stat timer to drive the watchdog timer.
- my(4)
  - This driver used the watchdog timer both as a watchdog on transmit and
    auto-negotiation.  To make this simpler and easier to understand I have
    split this out into two separate timers.  One just manages the auto-neg
    side of things and one is a transmit watchdog.
- fea(4), fpa(4)
  - These two drivers share a common backend, pdq, which was plagued with
    several issues.  I'm quite confident no one has used these drivers in
    years since the are guaranteed to panic during attach.
  - Add real locking.  Previously these drivers only acquired their lock
    in their interrupt handler or in the ioctl routine (but too broadly in
    the latter).  No locking was used for the stack calling down into the
    driver via if_init() or if_start(), for device shutdown or detach.  Also,
    the interrupt handler held the driver lock while calling if_input().  All
    this stuff should be fixed in the locking changes.
  - Really fix these drivers to handle if_alloc().  The front-end attachments
    were using if_initname() before the ifnet was allocated.  Fix this by
    moving some of the duplicated logic from each driver into pdq_ifattach().
    While here, make pdq_ifattach() return an error so that the driver just
    fails to attach if if_alloc() fails rather than panic'ing.
  - Adds a new private timer to drive the watchdog timer.
- sn(4)
  - Use bus_*() rather than bus_space_*().
  - Adds a new private timer to drive the watchdog timer.
  - Fixup detach.
- ste(4), ti(4)
  - Adds a new private timer to drive the watchdog timer.
  - Fixup detach.
- tl(4), wb(4)
  - Use bus_*() rather than bus_space_*().
  - Hooks into the stat timer to drive the watchdog timer.
  - Fixup detach.
- vge(4)
  - Overhaul the locking to avoid recursion and add missing locking in a few
  - Don't schedule a task to call vge_start() from contexts that are safe to
    call vge_start() directly.  Just invoke the routine directly instead
    (this is what all of the other NIC drivers I am familiar with do).  Note
    that vge(4) does not use an interrupt filter handler which is the primary
    reason some other drivers use tasks.
  - Adds a new private timer to drive the watchdog timer.
  - Use bus_*() rather than bus_space_*().
  - Fixup detach.
- netfront(4)
  - This doesn't actually implement a watchdog, it just sets if_watchdog to a
    non-existent function under '#ifdef notyet'.
- admsw(4)
  - This driver is a bit special in that it has no locking at all, not even
    a poor attempt. :)  It also appears to be for a specific MIPS board of
    some sort.
  - It has multiple ifnet's for multiple ports, but it only used if_timer and
    if_watchdog from the first ifnet.  For this driver I added a single
    private timer to replace the if_timer use on the first ifnet.  I marked
    the callout MPSAFE, but the driver really needs to have locking added at
    which point it could use callout_init_mtx().

John Baldwin

More information about the freebsd-net mailing list