netdev - Re: Silicom bypass driver promote from staging

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:	Mon, 29 Sep 2014 13:07:15 -0400
From:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
To:	David Hendel <david@...icom.co.il>
Cc:	Francois Romieu <romieu@...zoreil.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"arnd@...db.de" <arnd@...db.de>, Anna Lukin <annal@...icom.co.il>
Subject: Re: Silicom bypass driver promote from staging

On Mon, Sep 29, 2014 at 07:18:35AM +0000, David Hendel wrote:
> 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.

I'm not going to accept a proc interface for a network driver, sorry,
please use netlink.  /proc is only for processes for any new
files/functionality.

> 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.
> 
> Here is the code,
> 
> 
> Bp_ctl patch,
> --------------------------------------------------------------------------------------------------
> Signed-off-by: Anna Lukin <annal@...icom.co.il>
> diff -uprN -X linux-3.17-rc1-vanilla/Documentation/dontdiff linux-3.17-rc1-vanilla/drivers/bypass/Kconfig linux-3.17-rc1/drivers/bypass/Kconfig
> --- linux-3.17-rc1-vanilla/drivers/bypass/Kconfig	1970-01-01 02:00:00.000000000 +0200
> +++ linux-3.17-rc1/drivers/bypass/Kconfig	2014-08-03 13:48:27.000000000 +0300

This is in an odd format, please read Documentation/SubmittingPatches
for the proper format to send a patch in.  There's nothing I can do with
this one, or your other one, sorry :(

greg k-h
--
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