Date: Thu, 8 Jul 2010 17:20:50 +0200 From: Alexander Fiveg <pebu3op@googlemail.com> To: John Baldwin <jhb@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org>, Alexandre Fiveg <afiveg@freebsd.org> Subject: Re: PERFORCE change 179713 for review Message-ID: <201007081720.51257.pebu3op@googlemail.com> In-Reply-To: <201007080901.30049.jhb@freebsd.org> References: <201006171446.o5HEkRSP022515@repoman.freebsd.org> <201007081339.17321.pebu3op@googlemail.com> <201007080901.30049.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201007081720.51257.pebu3op>