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:	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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists