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

[Garrett D'Amore]

On 02/ 3/11 04:12 PM, Roland Mainz wrote:
> 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 ?
>    

No, these copyrights are as I received them from Exar.  I don't change them.

> 2. Copyright still goes to Oracle ? Or Nexenta ?
>    

Original copyrights are from Oracle, I don't change them.

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

Um.  I don't like using C99 keywords yet.  We don't have other use of 
this in kernel code yet.   Personally, I *hate* attempts to force the 
optimizer in this way... the optimizer should make the choice not a human.

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

Agreed.

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

If this is in device configuration, it probably isn't a hot code path, 
and not worth the optimization concern.

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

Probably, yes.

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

We should probably add a sparc Makefile, but I can't test, due to not 
having suitable sparc hardware.  I'll leave the code in place, and if 
someone wants sparc support they can add a Makefile trivially.

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

I don't know what it was tested with.  x04 has unique bug potentials as 
well.  I'd prefer not to override ... if x04 was safe to use everywhere, 
then we should use it everywhere . If it isn't universally safe, then it 
may not be safe here.   Again, I don't know the details of the 
differences and why we don't use xO4 by default.

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

I've not researched this myself... I've *not* read the 112KLOC myself... 
However you can look on paths that are called from interrupt service 
routines, and as a result of m_tx routines.

     - Garrett
> ----
>
> Bye,
> Roland
>
>