netdev - Re: [PATCH v2 1/4] net: mvneta: driver for Marvell Armada 370/XP network unit

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] [day] [month] [year] [list]

Date:	Fri, 12 Oct 2012 12:03:29 -0400
From:	Jason Cooper <jason@...edaemon.net>
To:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
Cc:	Lior Amsalem <alior@...vell.com>, Andrew Lunn <andrew@...n.ch>,
	Rami Rosen <rosenr@...vell.com>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Maen Suleiman <maen@...vell.com>,
	Francois Romieu <romieu@...zoreil.com>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	Lennert Buytenhek <kernel@...tstofly.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/4] net: mvneta: driver for Marvell Armada 370/XP
 network unit

On Fri, Oct 12, 2012 at 04:59:19PM +0200, Thomas Petazzoni wrote:
> Jason,
> 
> On Fri, 12 Oct 2012 10:31:31 -0400, Jason Cooper wrote:
> 
> > I agree with Francois on most of these.  I prefer readability over
> > hard 80 column limits.
> 
> Sure, but checkpatch.pl is warning on every line exceeding the 80
> columns. Not that I think that all checkpatch.pl warnings should
> necessarily be religiously respected, but if you have gazillions of
> warnings regarding line exceeding 80 columns, it is very likely that
> you will miss more important warnings.

./scripts/checkpatch.pl --ignore LONG_LINE ...

Will yield the 'more important' warnings/errors.  After those are
cleared, you can run without --ignore to check for over-indentation,
etc.

> > Although, 80 columns is still sound
> > guidance.  For example, a majority of the broken lines are due to
> > long macro and constant names.  I did a 'git grep NETA' and didn't
> > see anything alarming.  So, above could become
> > 
> > 			val |= rx_filled << NETA_RXQ_ADD_NONOCC_SHIFT;
> 
> I don't mind, but then I would like to keep things consistent:
> 
>  * The driver file would be neta.c
> 
>  * All functions and data structure would be prefixed neta_ and not
>    mvneta_
> 
>  * The Kconfig option would become CONFIG_NETA. Do we really want such
>    a "simple" Kconfig option name for a driver?

Well, you could do mv_neta.c and CONFIG_MV_NETA, but at the end of the
day, we were both trying to put lipstick on a pig.  Your last paragraph
is the most important.

> Maybe the fact that those long macros are making long lines is also due
> to the code having sometimes a too deep indentation, and I need to fix
> that by using more auxiliary functions or something like that?

This is the intent of the 80 column warning.  Please review the patch
for over-indentation, and consider shortening the macros, eg
MVNETA_RXQ_ADD_NONOCC_SHFT.

thx,

Jason.
--
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