[PATCH] New driver "sfc" for Solarstorm SFC4000 controller - 3nd try

[PATCH] New driver "sfc" for Solarstorm SFC4000 controller - 3nd try

[Robert Stonehouse]

This is a resubmission of a new driver for Solarflare network controllers.

The driver supports several types of PHY (10Gbase-T, XFP, CX4) on six
different 10G and 1G boards.

The previous thread was:
  [PATCH] [RFC] New driver "sfc" for Solarstorm SFC4000 controller
  http://marc.info/?l=linux-netdev&m=119757352830103&w=2

Thanks to the people who looked at the previous patches. We have addressed
the following from received comments after the 2nd submission:
  - Use kzalloc where appropriate
  - Remove deprecated fastcall attributes
  - Use kernel routines for hex dumps and MAC address printing
  - Changes to logging MACROs

The last two patches were marked with RFC but I now think that this driver
is ready (withstanding any further review comments) and I would like to ask
that this driver is considered for merging.


The patch (against net-2.6.25) is at:
     https://support.solarflare.com/netdev/3/net-2.6.25-sfc-2.2.0029.patch

The new files may also be downloaded as a tarball:
     https://support.solarflare.com/netdev/3/net-2.6.25-sfc-2.2.0029.tgz

And for verification there is:
     https://support.solarflare.com/netdev/3/MD5SUMS


Happy Christmas

-- 
Robert Stonehouse

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller - 3nd try

[Randy Dunlap]

On Fri, 21 Dec 2007 16:53:40 +0000 Robert Stonehouse wrote:

> This is a resubmission of a new driver for Solarflare network controllers.
> 
> The driver supports several types of PHY (10Gbase-T, XFP, CX4) on six
> different 10G and 1G boards.
> 
> The previous thread was:
>   [PATCH] [RFC] New driver "sfc" for Solarstorm SFC4000 controller
>   http://marc.info/?l=linux-netdev&m=119757352830103&w=2
> 
> Thanks to the people who looked at the previous patches. We have addressed
> the following from received comments after the 2nd submission:
>   - Use kzalloc where appropriate
>   - Remove deprecated fastcall attributes
>   - Use kernel routines for hex dumps and MAC address printing
>   - Changes to logging MACROs
> 
> The last two patches were marked with RFC but I now think that this driver
> is ready (withstanding any further review comments) and I would like to ask
> that this driver is considered for merging.
> 
> 
> The patch (against net-2.6.25) is at:
>      https://support.solarflare.com/netdev/3/net-2.6.25-sfc-2.2.0029.patch

wow, 750+ KB

How many drivers is this?


> The new files may also be downloaded as a tarball:
>      https://support.solarflare.com/netdev/3/net-2.6.25-sfc-2.2.0029.tgz
> 
> And for verification there is:
>      https://support.solarflare.com/netdev/3/MD5SUMS


Hi,
I have just a few comments, mostly related to infrastructure.

a.  Don't use kernel-doc comment flags (i.e., "/**" to begin a comment
block) when the comment is not in kernel-doc format.  Or use
kernel-doc format.  :)
(as documented in Documentation/kernel-doc-nano-HOWTO.txt)

E.g.:
+/** @file
+ *
+ * Efx driverlink
+ *
+ * This header file defines the portions of the Efx driverlink
+ * interface that are used only by the kernel net driver, and are not
+ * part of the public interface specification.
+ */

b.  Kconfig file:

Use 1 tab (not 8 spaces) to indent below "config" items.
Use 1 tab + 2 spaces to indent help text below "help" lines.


c.  Driver contains MTD, SPI, & I2C (at least) code and needs to be
reviewed by people in those areas as well (IMO).

I see an MTD dependency in the Kconfig file.
What about the SPI and I2C parts?  Are they conditional or
how is that handled?  or does the driver not use the kernel
infrastructure for these?


---
~Randy

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller - 3nd try

[Ben Hutchings]

Randy Dunlap wrote:
> On Fri, 21 Dec 2007 16:53:40 +0000 Robert Stonehouse wrote:
> 
> > This is a resubmission of a new driver for Solarflare network controllers.
<snip>
> > The last two patches were marked with RFC but I now think that this driver
> > is ready (withstanding any further review comments) and I would like to ask
> > that this driver is considered for merging.
> > 
> > 
> > The patch (against net-2.6.25) is at:
> >      https://support.solarflare.com/netdev/3/net-2.6.25-sfc-2.2.0029.patch
> 
> wow, 750+ KB
> 
> How many drivers is this?

Just two: sfc (net) and sfc_mtd (MTD).  As Robert said, the net driver
supports a variety of PHYs.  It also has a fair amount of self-test
and (conditional) debug code, and comments.

I've noted your comments on kernel-doc and Kconfig format and will
address those in the next version.

<snip>
> c.  Driver contains MTD, SPI, & I2C (at least) code and needs to be
> reviewed by people in those areas as well (IMO).
> 
> I see an MTD dependency in the Kconfig file.
> What about the SPI and I2C parts?  Are they conditional or
> how is that handled?  or does the driver not use the kernel
> infrastructure for these?

We are not currently using the kernel infrastructure for those.
I'm not sure whether we could do.

I think everyone working on the net driver here will be away until
the new year, so please forgive our silence in the mean time.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.