netdev
[Top] [All Lists]

Re: Submission for S2io 10GbE driver

To: Leonid Grossman <leonid.grossman@xxxxxxxx>
Subject: Re: Submission for S2io 10GbE driver
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 17 Feb 2004 00:11:12 +0000
Cc: netdev@xxxxxxxxxxx, "'Andi Kleen'" <ak@xxxxxxx>, "'Jeff Garzik'" <jgarzik@xxxxxxxxx>, "'Stephen Hemminger'" <shemminger@xxxxxxxx>, "'Francois Romieu'" <romieu@xxxxxxxxxxxxx>, "'jamal'" <hadi@xxxxxxxxxx>, "'Grant Grundler'" <iod00d@xxxxxx>, "'Anton Blanchard'" <anton@xxxxxxxxx>, "'Jes Sorensen'" <jes@xxxxxxxxxxxxxxxxxx>, raghavendra.koushik@xxxxxxxx, "'ravinandan arakali'" <ravinandan.arakali@xxxxxxxx>
In-reply-to: <000201c3f4d2$2a5ddd90$7410100a@S2IOtech.com>; from leonid.grossman@s2io.com on Mon, Feb 16, 2004 at 01:16:35PM -0800
References: <20040205004952.GA27510@cup.hp.com> <000201c3f4d2$2a5ddd90$7410100a@S2IOtech.com>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mutt/1.2.5.1i
A bunch of comments:

 - if you want to submit the driver for inclusion please submit a patch against 
a kernel tree,
   not a tarball.
 - please try to avoid version ifdefs by provoding the newer APIs on older 
kernels, e.g.:

#ifndef IRQ_RETVAL
#define irqreturn_t                     void
#define IRQ_RETVAL(foo)
#endif

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,00)
#define free_netdev kfree
#endif

 - your AS_A_MODULE ifdef is bogs - everything under it is fine for a
   non-modular driver, too
 - your XENA_ARCH_64 is not good - just cast 64bit values to
   (unsigned long long) always and use the ll format specifier always.
   You missed a few 64bit arches, btw :)
 - can you get rid of all those BOOL/TRUE/FALSE/SUCCESS/etc.. ifdefs?
 - s2io_driver wants to be converted to C99 initializers (.foo instead of foo:)
 - In Linux comments usually are on the same indentation level as surrounding
   code
 - CONFIGURE_NAPI_SUPPORT should probably become CONFIG_XENA_NAPI or whatever
 - you want to use ethtool_ops instead of the ioctl variant
 - there's lots of non-static symbols in s2io.c - these shouldn't exist in a
   single source-file driver
 - you can't return -ENOMEM from the isr - just IRQ_HANDLED or IRQ_NONE
 - having the RCS log at the end of the source files looks ... odd

<Prev in Thread] Current Thread [Next in Thread>