[illumos-Developer] webrev: new vxge driver (Exar 3100 series)

[Roland Mainz]

On Thu, Feb 3, 2011 at 10:13 PM, Garrett D'Amore <garrett at damore.org> wrote:
> I've posted a webrev containing a very large (!) device driver contributed
> by Exar for their Neterion X-Frame 3100 series of 10 GbE NICs.
>
> This is located at:  http://mexico.purplecow.org/gdamore/webrev/vxge/

Some quick notes after a 5min race through the code (mostly nits (and
I'm no networking hardware expert either... ;-/)):
1. Copyright years should AFAIK be updated to 2011, right ?

2. Copyright still goes to Oracle ? Or Nexenta ?

3. In  usr/src/uts/common/io/vxge/vxge-defs.h there are CPP macros like this:
-- snip --
#define	bVAL1(bits, loc)  ((((u64)bits) >> (64-(loc+1))) & 0x1)
-- snip --
Please add brackets around each argument, e.g. change the line above to:
-- snip --
#define	bVAL1(bits, loc)  ((((u64)(bits)) >> (64-((loc)+1))) & 0x1)
-- snip --
There are other locations in the code where brackets around arguments
are missing in #define macros...

4. usr/src/uts/common/io/vxge/vxge-list.h
The header uses #pragma inline. Would it be usefull to use the C99
keyword |inline| instead in this case ?

5. usr/src/uts/common/io/vxge/vxge.c
There are a couple of arrays which could be |const|, e.g.
|vxge_func_mode_names|, |vxge_port_mode_names| etc. and
|vpath_selector|

6. usr/src/uts/common/io/vxge/vxge.c function |vxge_config_init_vpath()|:
There is code like this:
-- snip --
+		VXGE_CONFIG_TTI(index).timer_ac_en =
+		    ddi_prop_get_int(DDI_DEV_T_ANY, dev_info,
+		    DDI_PROP_DONTPASS, "tti_timer_ac_en",
+		    VXGE_HAL_TIM_TIMER_AC_USE_FLASH_DEFAULT);
+
+		VXGE_CONFIG_TTI(index).timer_ci_en =
+		    ddi_prop_get_int(DDI_DEV_T_ANY, dev_info,
+		    DDI_PROP_DONTPASS, "tti_timer_ci_en",
+		    VXGE_HAL_TIM_TIMER_CI_USE_FLASH_DEFAULT);
+
+		VXGE_CONFIG_TTI(index).timer_ri_en =
+		    ddi_prop_get_int(DDI_DEV_T_ANY, dev_info,
+		    DDI_PROP_DONTPASS, "tti_timer_ri_en",
+		    VXGE_HAL_TIM_TIMER_RI_USE_FLASH_DEFAULT);
etc. etc.
-- snip --
Would it be usefull to just save  the result of
|VXGE_CONFIG_TTI(index)| and re-use it instead of hoping that the
compiler will recognise&&optimise this ?
This issue occurs repeatedly, e.g. see usage of
|VXGE_CONFIG_RTI()|&co. later in the function and in other locations
of the driver.

7. vxge_firmware.h:
AFAIK a |const| for the array |X3fw_ncf| would be nice

8. The code has |#ifdef __sparc| but I don't see the Makefile stuff
for the SPARC architecture... is this intentional ?

9. Since this is a 10000baseT driver which is expected to shuffle
around _lots_ of data and consume lots of CPU time... would it be
usefull to compile it with -xO4 instead of the default -xO3 ?

10. Which functions are "hotspots" in this driver (I like to have a
2nd look at them then...) ?

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)