netdev - Re: [PATCH] New driver "sfc" for Solarstorm SFC4000 controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 21 Dec 2007 10:02:53 -0800
From:	Randy Dunlap <randy.dunlap@...cle.com>
To:	Robert Stonehouse <rstonehouse@...arflare.com>
Cc:	netdev@...r.kernel.org, linux-net-drivers@...arflare.com,
	spope@...arflare.com
Subject: Re: [PATCH]  New driver "sfc" for Solarstorm SFC4000 controller -
 3nd try

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html