From owner-p4-projects@FreeBSD.ORG Thu Jul 8 15:20:54 2010 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 413E71065678; Thu, 8 Jul 2010 15:20:54 +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 050FC106566B; Thu, 8 Jul 2010 15:20:54 +0000 (UTC) (envelope-from pebu3op@googlemail.com) Received: from mail.net.t-labs.tu-berlin.de (mail.net.t-labs.tu-berlin.de [130.149.220.252]) by mx1.freebsd.org (Postfix) with ESMTP id 898E98FC16; Thu, 8 Jul 2010 15:20:53 +0000 (UTC) Received: from raven.net.t-labs.tu-berlin.de (raven.net.t-labs.tu-berlin.de [130.149.220.18]) by mail.net.t-labs.tu-berlin.de (Postfix) with ESMTP id 74D3D70015C0; Thu, 8 Jul 2010 17:20:52 +0200 (CEST) From: Alexander Fiveg Organization: Google To: John Baldwin Date: Thu, 8 Jul 2010 17:20:50 +0200 User-Agent: KMail/1.9.10 References: <201006171446.o5HEkRSP022515@repoman.freebsd.org> <201007081339.17321.pebu3op@googlemail.com> <201007080901.30049.jhb@freebsd.org> In-Reply-To: <201007080901.30049.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <201007081720.51257.pebu3op@googlemail.com> 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 Reply-To: pebu3op@googlemail.com List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jul 2010 15:20:54 -0000 On Thursday 08 July 2010 15:01:29 John Baldwin wrote: > 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. Ok, I see. what you suggest is a better architectural solution. I think I know how to implement it without to do a lot of changes in the native NIC-driver :) thanx a lot, Alex