PERFORCE change 179713 for review

[John Baldwin]

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