netdev - Re: [PATCH 5/6] e1000 driver remove checkpatch errors, warnings and checks.

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]

Message-ID: <561D846E.1050405@gmail.com>
Date:	Tue, 13 Oct 2015 15:23:42 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Janusz Wolak <januszwolak@...kados.com.pl>,
	jeffrey.t.kirsher@...el.com
Cc:	jesse.brandeburg@...el.com, shannon.nelson@...el.com,
	carolyn.wyborny@...el.com, donald.c.skidmore@...el.com,
	matthew.vick@...el.com, john.ronciak@...el.com,
	mitch.a.williams@...el.com, intel-wired-lan@...ts.osuosl.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Janusz Wolak <januszvdm@...il.com>
Subject: Re: [PATCH 5/6] e1000 driver remove checkpatch errors, warnings and
 checks.

On 10/13/2015 02:39 PM, Janusz Wolak wrote:
> From: Janusz Wolak <januszvdm@...il.com>
>
> Signed-off-by: Janusz Wolak <januszvdm@...il.com>
> ---
>   drivers/net/ethernet/intel/e1000/e1000_param.c | 114 ++++++++++++++++++-------
>   1 file changed, 82 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_param.c b/drivers/net/ethernet/intel/e1000/e1000_param.c
> index c9cde35..9ec730e 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_param.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_param.c
> @@ -1,5 +1,5 @@
>   /*******************************************************************************
> -
> +*
>     Intel PRO/1000 Linux driver
>     Copyright(c) 1999 - 2006 Intel Corporation.
>   
> @@ -45,10 +45,10 @@
>   
>   #define E1000_PARAM_INIT { [0 ... E1000_MAX_NIC] = OPTION_UNSET }
>   #define E1000_PARAM(X, desc) \
> -	static int X[E1000_MAX_NIC+1] = E1000_PARAM_INIT; \
> +	static int X[E1000_MAX_NIC + 1] = E1000_PARAM_INIT; \
>   	static unsigned int num_##X; \
>   	module_param_array_named(X, X, int, &num_##X, 0); \
> -	MODULE_PARM_DESC(X, desc);
> +	MODULE_PARM_DESC(X, desc)
>   
>   /* Transmit Descriptor Count
>    *
> @@ -200,6 +200,7 @@ struct e1000_option {
>   		} r;
>   		struct { /* list_option info */
>   			int nr;
> +
>   			const struct e1000_opt_list { int i; char *str; } *p;
>   		} l;
>   	} arg;

How is adding a space here adding any value?  Please don't just blindly 
follow checkpatch as it can give out erroneous information.

Looking over most of this patch series it seems like it is taking 
readability in the wrong direction and reducing the ability to maintain 
the driver since this code has been "maintenance only" for some time 
now.  If somebody comes up with a legitimate fix for an issue at some 
point in the future they will need to work around these patches in order 
to back-port it into a stable release and that just hurts maintainability.

I'd say this whole series should be rejected on the grounds that this 
driver is mostly stable and should only really be modified for bug fixes 
at this point.  If we really need to go through and do a checkpatch 
sweep we should probably just focus on serious errors only instead of 
going astray and chasing down things that are false hits or minor issues 
that are mostly a matter of preference.

- Alex
--
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