Subject: Re: [PATCH] seastar - SeaStar Ethernet driver
From: "Kevin Pedretti" <ktpedre@sandia.gov>
To: "David Miller" <davem@davemloft.net>
cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
       "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
In-Reply-To: <20100202.134251.15604523.davem@davemloft.net>
References: <20100202205845.GE5246@hawkeye.sandia.gov>
 <20100202.134251.15604523.davem@davemloft.net>
Date: Tue, 2 Feb 2010 18:24:02 -0700
Message-ID: <1265160242.15726.20.camel@hawkeye.sandia.gov>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1374
Lines: 36

Thank you all for the review comments.  I believe most of the issues
have been addressed in the patch just posted.  I apologize if there are
still issues, and certainly appreciate further comments.

David Miller's comments:
1. Use u32, u16, etc.  -> Done.
2. Bad code formating -> Fixed, I believe. Went through everything.
3. Call netif_start_queue() after hw init -> Done.
4. Device only supports IPv4? -> Yes, that's correct. No IPv6 support.
The driver squashes everything but IPv4 in eth2ss().
5. No need for suspend/resume NOPs -> Done.  functions removed.

Randy Dunlap's comments:
1. Remove /** comments -> Done.
2. Odd spacing -> I'm not seeing this. Spacing looks correct to me.
3. Limit while (1) loops somehow -> Done. 
4. Limit while (1) in intr handler -> In practice we've never seen more
than a few packets processed per interrupt.

Ben Hutching's comments:
1. Spacing looks correct -> Thanks. 

Stephen Hemminger's comments:
1. Add ndo_validate_address -> Done
2. May want to use alloc_netdev() -> Didn't do this. Would there be a
substantial advantage to doing this?
3. memset() unnecessary -> removed

Kevin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/