[PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[Ben Hutchings]

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.  It is accompanied by an MTD driver that
allows access to the flash/EEPROM.

NICs based on this controller are now available from SMC as part numbers
SMC10GPCIe-XFP and SMC10GPCIe-10BT.

The previous thread was:
  http://marc.info/?l=linux-netdev&m=120162616808659&w=2

Some explanation of the driver structure was posted in:
  http://marc.info/?l=linux-netdev&m=119999015817920&w=2

Since the last patch we have made some bug fixes and minor improvements:

 - Fix MAC stats during TX drain
 - Fix skb leak on self-test failure
 - Add workaround for RX flush timeout
 - Recover from more TX errors
 - Allow more time for recovery of TXC43128 PHY between resets
 - Fix signed-ness mismatches
 - Improve robustness of efx_{start,stop}_{all,port}()
 - Correct some netif_tx_lock/unlock calls to _bh variants
 - Make probe fail if the NIC becomes disabled
 - Fix efx_dl_search_device_info() macro
 - Validate MAC address in efx_set_mac_address()
 - Test for failure of efx_init_debugfs_netdev()
 - Fix potential loss of promiscuous flag
 - Improve comments on locking requirements
 - Remove some redundant initialisation code
 - Remove unused alaska_blink() function
 - Add byte-order annotations and fix byte-order bugs
 - Use C99 __func__, not gcc's __FUNCTION__
 - Eliminate suspend lock as redundant with rtnl_lock
 - Move Falcon hardware parameters from efx_nic into the nic_data field
 - Make RX refill workqueue global as it should be per-CPU not per-NIC
 - Use net_device::stats instead of efx_nic::stats
 - Correct some comments
 - Define a PCI_EXP_DEVCTL_PAYLOAD_LBN constant instead of using ffs()
   since ffs() is not evaluated at compile-time
 - Make use of PCI_DEVICE() macro for efx_pci_table initialisation
 - Remove unnecessary indirection between efx_pci_table and
   struct efx_nic_type
 - Change MSI-X setup to allocate an MSI-X interrupt per package, not
   per core, by default
 - Rename *_max to *_lim in struct efx_dl_falcon_resources for clarity
 - Fix error handling in efx_change_mtu()
 - Do not reconfigure XGXS block after resetting XAUI block
 - Simplify multicast hash setting
 - Fix array size calculation in efx_multicast_hash

We believe this is ready to be merged now and would appreciate a
thorough review.

The patch (against netdev-2.6) is at:
  https://support.solarflare.com/netdev/7/netdev-2.6-sfc-2.2.0106.patch

The new files may also be downloaded as a tarball:
  https://support.solarflare.com/netdev/7/netdev-2.6-sfc-2.2.0106.tar.gz

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

Ben.

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

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[Ben Hutchings]

Please send replies to my previous message to
linux-net-drivers@solarflare.com - I forgot to Cc this address as I
normally do.

Ben.

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

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 3 Mar 2008 18:56:24 +0000

> The patch (against netdev-2.6) is at:
>   https://support.solarflare.com/netdev/7/netdev-2.6-sfc-2.2.0106.patch

Nobody can properly review the driver if it's off on some external web
site instead of posted here.

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[Dan Williams]

On Mon, 2008-03-03 at 11:02 -0800, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Mon, 3 Mar 2008 18:56:24 +0000
> 
> > The patch (against netdev-2.6) is at:
> >   https://support.solarflare.com/netdev/7/netdev-2.6-sfc-2.2.0106.patch
> 
> Nobody can properly review the driver if it's off on some external web
> site instead of posted here.

The diff is 707K; I certainly thought that netdev had a message size
limit.  What's the proper policy on splitting up _new_ drivers?  There
may/may not be a good way of splitting up any given new driver for
piecemeal in-line review.  If there's not, what's the alternative?

Dan

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[Stephen Hemminger]

On Mon, 03 Mar 2008 14:22:55 -0500
Dan Williams <dcbw@redhat.com> wrote:

> On Mon, 2008-03-03 at 11:02 -0800, David Miller wrote:
> > From: Ben Hutchings <bhutchings@solarflare.com>
> > Date: Mon, 3 Mar 2008 18:56:24 +0000
> > 
> > > The patch (against netdev-2.6) is at:
> > >   https://support.solarflare.com/netdev/7/netdev-2.6-sfc-2.2.0106.patch
> > 
> > Nobody can properly review the driver if it's off on some external web
> > site instead of posted here.
> 
> The diff is 707K; I certainly thought that netdev had a message size
> limit.  What's the proper policy on splitting up _new_ drivers?  There
> may/may not be a good way of splitting up any given new driver for
> piecemeal in-line review.  If there's not, what's the alternative?

Part of the problem is that you put a lot of stuff all in one driver:
  * sensors support
  * large debugfs chunk
  * efx driver layer
  * event queue

You created a big monolith. No one likes reading big stuff, it requires
lots of time, as much as going over a whole subsystem. The fact that so
many callbacks and hooks are needed implies that the design got out of hand
for a simple device.

Maybe an alternative would be to make your device better match existing
infrastructure. The EFX code looks like a separate driver which should
show up as a bus in the driver model, not a network device. Other people
who don't just do network device could help as well.

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[Ben Hutchings]

Stephen Hemminger wrote:
> On Mon, 03 Mar 2008 14:22:55 -0500
> Dan Williams <dcbw@redhat.com> wrote:
> 
> > On Mon, 2008-03-03 at 11:02 -0800, David Miller wrote:
> > > From: Ben Hutchings <bhutchings@solarflare.com>
> > > Date: Mon, 3 Mar 2008 18:56:24 +0000
> > > 
> > > > The patch (against netdev-2.6) is at:
> > > >   https://support.solarflare.com/netdev/7/netdev-2.6-sfc-2.2.0106.patch
> > > 
> > > Nobody can properly review the driver if it's off on some external web
> > > site instead of posted here.
> > 
> > The diff is 707K; I certainly thought that netdev had a message size
> > limit.  What's the proper policy on splitting up _new_ drivers?  There
> > may/may not be a good way of splitting up any given new driver for
> > piecemeal in-line review.  If there's not, what's the alternative?
> 
> Part of the problem is that you put a lot of stuff all in one driver:

It's comparable in size to most other high-performance Ethernet
drivers.  It has to do a little more work talking to the hardware as
there is no firmware.

>   * sensors support

While this sort of protection is unusual, in this case I don't think
it's something that can reasonably be done outside the driver.

>   * large debugfs chunk

debugfs code is separated out, quite straightforward, and can't
really go anywhere but in this driver.

>   * efx driver layer

"efx" is a common prefix used in our driver code to be independent of
product and company names.  It isn't a separate layer.

>   * event queue

We could try writing a driver that just guessed what the NIC was
doing, but I don't think it would work very well. ;-)

> You created a big monolith. No one likes reading big stuff, it requires
> lots of time, as much as going over a whole subsystem. The fact that so
> many callbacks and hooks are needed implies that the design got out of hand
> for a simple device.

It's not a simple device.  There are callbacks into MAC-, PHY- and
board-specific code because this code supports several of each.  I
spent some weeks ruthlessly paring away unnecessary abstraction and
cruft, and I don't believe there is a significant amount left.

> Maybe an alternative would be to make your device better match existing
> infrastructure. The EFX code looks like a separate driver which should
> show up as a bus in the driver model, not a network device.

Are you talking about driverlink?

> Other people who don't just do network device could help as well.

The MTD driver has already benefitted from review on the linux-mtd
list.

Ben.

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

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 3 Mar 2008 21:17:44 +0000

> It's not a simple device.  There are callbacks into MAC-, PHY- and
> board-specific code because this code supports several of each.  I
> spent some weeks ruthlessly paring away unnecessary abstraction and
> cruft, and I don't believe there is a significant amount left.

That is your opinion.

We have drivers that handle 30 or so variants of a particular
ethernet chipset and they are not nearly so huge as this
driver, and some of those cases don't have firmware either.

The driver needs to be reviewed on at least some level, but nobody is
going to walk through a 700K driver.  And I do mean nobody.

You have to find a way to trim this driver down and make it
more reviewable.

If I can write a full 10Gbit driver in ~11,000 lines of code
(~200,000 bytes), so can anyone else.

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[Ben Hutchings]

David,

We concede that the driver is currently too large to be properly
reviewed in one go.

We propose to:

1. Remove what we can - i.e. internal HW support, extraneous comments,
unused HW register defines, and non-essential peripheral driver features
such as sensor support - leaving a minimal network driver.

2. Post the minimal driver in sub-100K patches to netdev.  This
minimal driver will still require several patches, but future patches
should represent independent pieces of functionality.

If you can see reasons why this will still not get us to a point where
a review can take place then please tell us what else is required.

We hope to complete this work within the next few days.

Ben.

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

Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller (try #7)

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 5 Mar 2008 14:22:04 +0000

> We propose to:
> 
> 1. Remove what we can - i.e. internal HW support, extraneous comments,
> unused HW register defines, and non-essential peripheral driver features
> such as sensor support - leaving a minimal network driver.
> 
> 2. Post the minimal driver in sub-100K patches to netdev.  This
> minimal driver will still require several patches, but future patches
> should represent independent pieces of functionality.

That sounds good.

You can even leave out the debugfs parts for now and add them
back in later, so that even that aspect can be reviewed seperately.