From owner-p4-projects@FreeBSD.ORG Thu Jul 8 13:26:01 2010 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id B07981065677; Thu, 8 Jul 2010 13:26:01 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5B9E91065674; Thu, 8 Jul 2010 13:26:01 +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 2E1448FC1E; Thu, 8 Jul 2010 13:26:01 +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 ACBA946B95; Thu, 8 Jul 2010 09:26:00 -0400 (EDT) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 67ECD8A03C; Thu, 8 Jul 2010 09:25:52 -0400 (EDT) From: John Baldwin To: pebu3op@googlemail.com Date: Thu, 8 Jul 2010 09:01:29 -0400 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20100217; KDE/4.4.5; amd64; ; ) References: <201006171446.o5HEkRSP022515@repoman.freebsd.org> <201006231319.49258.jhb@freebsd.org> <201007081339.17321.pebu3op@googlemail.com> In-Reply-To: <201007081339.17321.pebu3op@googlemail.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201007080901.30049.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Thu, 08 Jul 2010 09:25:52 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.6 required=4.2 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: Perforce Change Reviews , Alexandre Fiveg Subject: Re: PERFORCE change 179713 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jul 2010 13:26:01 -0000 On Thursday, July 08, 2010 7:39:17 am Alexander Fiveg wrote: > On Wednesday 23 June 2010 19:19:49 John Baldwin wrote: > > On Thursday 17 June 2010 10:46:27 am Alexandre Fiveg wrote: > > > http://p4web.freebsd.org/@@179713?ac=10 > > > > > > Change 179713 by afiveg@cottonmouth on 2010/06/17 14:46:03 > > > > > > Begin with new design for ringmap: > > > 1. The new structure with pointers to hardware dependent functions: > > > "struct ringmap_functions" (/net/ringmap.h) > > > 2. Pointer to this structure placed in ringmap structure. > > > 3. In the ringmap_attach function look for pci Id of network > > > controller, and then, depending on controllers type, initialize the > > > functions pointers: (ringmap.c: set_ringmap_funcs()) > Hello John, > > > > I think 3) is the wrong way to go about it. Can't you have the NIC driver > > attach a ringmap and supply the function pointers to the NIC-specific > > functionality instead? > I think I didn't exactly understand what do you mean :( > > Actually the NIC driver in its attach() function calls ringmap_attach(), and > all settings which appears in the ringmap_attach() are related only to one > specific NIC. Instead of having ringmap_attach() figure out the per-NIC function pointers based on device IDs, can't you have ringmap_attach() accept the function pointers for the device-specific routines directly? That is, instead of having a set_ringmap_funcs() function that queries device IDs, instead have the list of functions (or perhaps a full blown function switch ala cdevsw) passed in as a parameter to ringmap_attach(). > > You really don't want to have two separate lists of > > device IDs. The ringmap list will invariably become stale. > The second device's list contains only ID's of devices supported by ringmap. > The "em" driver in -CURRENT supports both 8254x and 8257x controllers. But > ringmap supports currently only 8254x. In the future ringmap should support > all devices supported by the driver which ringmap is based on. This means the > second ringmap-device-list will be unnecessary. I think the driver should only call ringmap_attach() for supported devices and that the logic decision for which devices are supported should be in the driver, not in the ringmap code. The current organization will make it harder to add support for ringmap in newer drivers far more complicated and will likely not work with modules, etc. -- John Baldwin