Message-ID: <3BFAEADE799AF145974162DF00E013AF018F8C5848@exchange2010.silicom.local>
Date:	Sat, 27 Sep 2014 06:54:22 +0000
From:	David Hendel <david@...icom.co.il>
To:	Francois Romieu <romieu@...zoreil.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"arnd@...db.de" <arnd@...db.de>, Anna Lukin <annal@...icom.co.il>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: RE: Silicom bypass driver promote from staging

Hi all,
Sorry, for the late response, we had to go over all the code and update as requested and then test to verify, so here it is.

See below with some responses and attached, hope we can move forward with that. getting the bypass driver into the kernel.

On Thu, 7 Aug 2014 19:56 Stephen Hemminger <stephen@...workplumber.org> wrote:
>The current driver uses a device specific /proc interface.
>That API programming model will not likely be acceptable in a standard network driver.
>Please consider doing something generic with netlink.

Actually, this is not a network driver, many of the users use this interface to
 control the bypass/fail-to-wire to control this function. We can add netlink api as additional interface,
 but  not having  proc interface maybe a problem.

On Thu, 7 Aug 2014 23:20 gregkh@...uxfoundation.org wrote:
>Making your patch an attachment in base64 mode, makes it impossible to quote to review it :(
>
>Anyway, I stopped at the first header file.  The kernel already has BIT definitions, no driver should ever have to redefine these and do it on their own.
>  That leads me to believe that this code really isn't all that "cleaned up" at all.
>
>Should I just look at what is in drivers/staging/silicom/ right now as code to review?  Or have you changed it any by making this patch?

OK, will do that,
drivers/staging/silicom/ driver was created from silicom bypass driver released in December 2012. 
The driver we are providing now is based on this staging driver.
We updated it to our Latest release (adding support for new adapters, some changes in functionality) and fixed bug 
(loading the driver from staging led to kernel panic).


On Fri, 8 Aug 2014 00:51 Francois Romieu <romieu@...zoreil.com> wrote:
>Location is the easy part. Getting things reviewed is a different story.
>
>You shouldn't expect reviewers to swallow 300k of code (600k / 2 due to removal). Please split the submitted patch into smaller, self-consistent, logical ones.
>
We will include the patches in the following emails inline each will have smaller size, hope this will be as you expect.

>Notwithstanding Stephen and Jeff's remarks, you should also:
>- remove the (out-)commented code
>- reconsider the use of gorilla class macros vs plain functions
>- stop casting ioremap return into long, then later into void *. It's
>  void __iomem * and should stay so.
>- use a consistent locking style and remove BP_SYNC_FLAG
>- fix the 80 cols limit problem(s) (hardly surprizing after 5 levels of
 > nested "if")
>- avoid redefining stuff from include/uapi/linux/mii.h
> I may be wrong but BPCTLI_MII_CR_POWER_DOWN smells of BMCR_PDOWN and
>  BPCTLI_PHY_CONTROL, well, MII_BMCR ?
>- avoid returning with spinlock held (read_reg, wdt_pulse)
>- start explaining what did change between two submissions

This is what we changed in updated patch:
1. Avoid redefining BIT and MII .
2. Fixing returning with spinlock held
3. Remove BP_SYNC_FLAG
4. Use void __iomem * instead of long long in ioremap return
5. Cleanup with checkpatch.pl

Hope that it is OK now, or at least better.


David Hendel.

Download attachment "bp_patch3.tar.gz" of type "application/x-gzip" (43597 bytes)

Powered by blists - more mailing lists