Re: [PATCH] Remove if_watchdog use

On Fri, 6 Nov 2009, John Baldwin wrote:

> 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
>    locking.
>  - Hooks into the stat timer to drive the watchdog timer.

I managed to get a panic when running wpa_supplicant:

System call ioctl returning with the following locks held:
exclusive sleep mutex an0 (network driver) r=0 (0xc58fc180) locked _at_
/usr/src/sys/dev/an/if_an.c:2341
panic: witness_warn

This seems to fix that:

--- /usr/src/sys/dev/an/if_an.c.orig	2009-11-10 19:26:21.000000000 
+0000
+++ /usr/src/sys/dev/an/if_an.c	2009-11-10 19:27:24.000000000 +0000
_at__at_ -2570,6 +2570,9 _at__at_
  			an_setdef(sc, &sc->areq);
  			AN_UNLOCK(sc);
  			break;
+		default:
+			AN_UNLOCK(sc);
+			break;
  		}

  		/*

I also get the following LOR on unplug (but see it before your patch too):

lock order reversal:
1st 0xc50f5208 if_afdata (if_afdata) _at_ /usr/src/sys/net/if.c:912
2nd 0xc0f9db68 mld_mtx (mld_mtx) _at_ /usr/src/sys/netinet6/mld6.c:569
KDB: stack backtrace:
db_trace_self_wrapper(c0cb835f,e54b1b20,c08e7b55,c08d891b,c0cbb215,...) at db_trace_self_wrapper+0x26
kdb_backtrace(c08d891b,c0cbb215,c4d30e18,c4d2a9c0,e54b1b7c,...) at kdb_backtrace+0x29
_witness_debugger(c0cbb215,c0f9db68,c0cbb365,c4d2a9c0,c0cd457d,...) at _witness_debugger+0x25
witness_checkorder(c0f9db68,9,c0cd457d,239,0,...) at witness_checkorder+0x839
_mtx_lock_flags(c0f9db68,0,c0cd457d,239,c5340ba0,...) at _mtx_lock_flags+0xc9
mld_domifdetach(c50f5000,c0db5e54,c0dc0880,e54b1c24,c09532a6,...) at mld_domifdetach+0x2c
in6_domifdetach(c50f5000,c5340ba0,390,4be,c50f522c,...) at in6_domifdetach+0x15
if_detach(c50f5000) at if_detach+0x916
ether_ifdetach(c50f5000,0,c0c6cbbc,34f,c555d380,...) at ether_ifdetach+0x3d
an_detach(c555d380,c4ead060,c0da0758,a76,e54b1c94,...) at an_detach+0xb5
device_detach(c555d380,0,c0d5a9b8,0,c4ff6500,...) at device_detach+0x8c
pccard_detach_card(c4ff6500,c4e3e8bc,c0d5a9b8) at pccard_detach_card+0x44
exca_removal(c4ed2004,0,c0c93731,1da,c8,...) at exca_removal+0x59
cbb_event_thread(c4ed2000,e54b1d38,c0cb02eb,343,c4d81aa0,...) at cbb_event_thread+0x107
fork_exit(c0720cb0,c4ed2000,e54b1d38) at fork_exit+0xb8
fork_trampoline() at fork_trampoline+0x8
  --- trap 0, eip = 0, esp = 0xe54b1d70, ebp = 0 ---
an0: detached

Other than that, the patches to an(4) seem to work as well before your 
patch as after, with light TCP traffic and heavy ping traffic.  However, 
an(4) appears to require a lot of work (unrelated to your patch) to bring 
it up to the state of other wireless drivers.

> - 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.

These changes to ixgb_detach() don't compile as ifp is no longer used in 
the !DEVICE_POLLING case.

/usr/src/sys/dev/ixgb/if_ixgb.c: In function 'ixgb_detach':
/usr/src/sys/dev/ixgb/if_ixgb.c:369: warning: unused variable 'ifp'

Hope that helps,

Gavin