From owner-freebsd-current@FreeBSD.ORG Fri Nov 6 20:08:40 2009 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 CEDE0106566C; Fri, 6 Nov 2009 20:08:40 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id A31398FC21; Fri, 6 Nov 2009 20:08:40 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 26F1D46B09; Fri, 6 Nov 2009 15:08:40 -0500 (EST) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id A37898A01B; Fri, 6 Nov 2009 15:08:39 -0500 (EST) From: John Baldwin To: current@FreeBSD.org Date: Fri, 6 Nov 2009 15:08:22 -0500 User-Agent: KMail/1.9.7 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911061508.22482.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Fri, 06 Nov 2009 15:08:39 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: net@FreeBSD.org Subject: [PATCH] Remove if_watchdog use 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, 06 Nov 2009 20:08:40 -0000 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. - 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 timer. - 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 places. - 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