linux-kernel - Re: [PATCH 2/5] drivers: staging: rtl8192u: fixing coding style issues in r8192U

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: <1437178294.2495.102.camel@perches.com>
Date:	Fri, 17 Jul 2015 17:11:34 -0700
From:	Joe Perches <joe@...ches.com>
To:	Joseph-Eugene Winzer <m999@...nmailbox.org>
Cc:	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] drivers: staging: rtl8192u: fixing coding style
 issues in r8192U_core.c

On Fri, 2015-07-17 at 15:59 +0200, Joseph-Eugene Winzer wrote:
> Reformatting the code without introducing other warnings
> like 'Avoid unnecessary line continuations' or breaking strings.

Very few of these seem to be improvements and many
of them should likely be in separate patches.

> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
[]
> @@ -143,17 +143,35 @@ struct CHANNEL_LIST {
>  };
>  
>  static struct CHANNEL_LIST ChannelPlan[] = {
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 36, 40, 44, 48, 52, 56, 60, 64, 149, 153, 157, 161, 165}, 24}, /* FCC */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}, 11}, /* IC */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 36, 40, 44, 48, 52, 56, 60, 64}, 21}, /* ETSI */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}, 13}, /* Spain. Change to ETSI. */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}, 13}, /* France. Change to ETSI. */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /* MKK //MKK */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /* MKK1 */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}, 13}, /* Israel. */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /*  For 11a , TELEC */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /* MIC */
> -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}, 14} /* For Global Domain. 1-11:active scan, 12-14 passive scan. //+YJ, 080626 */

If these were to be reformatted, it'd probably be better
to use 10 per line so that the Len value that follows is
more obvious.  Likely this struct should be const too.
Maybe something like:

static const struct CHANNEL_LIST ChannelPlan[] = {
	{
		.Channel = {
			  1,  2,  3,  4,  5,  6,  7,  8,  9, 10,
			 11, 36, 40, 44, 48, 52, 56, 60, 64,149,
			153,157,161,165
		},
		.Len = 24,
	},

etc.  Add more spaces between channels if you really want.

> @@ -179,7 +197,9 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
>  			min_chan = 1;
>  			max_chan = 14;
>  		} else {
> -			RT_TRACE(COMP_ERR, "unknown rf chip, can't set channel map in function:%s()\n", __func__);
> +			RT_TRACE(COMP_ERR,
> +				 "unknown rf chip, can't set channel map in function:%s()\n"
> +				 , __func__);

No leading commas please.  Put the comma after the format.

> @@ -187,7 +207,10 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
>  			       sizeof(GET_DOT11D_INFO(ieee)->channel_map));
>  			/*  Set new channel map */
>  			for (i = 0; i < ChannelPlan[channel_plan].Len; i++) {
> -				if (ChannelPlan[channel_plan].Channel[i] < min_chan || ChannelPlan[channel_plan].Channel[i] > max_chan)
> +				if (ChannelPlan[channel_plan].Channel[i] <
> +								min_chan ||
> +				    ChannelPlan[channel_plan].Channel[i] >
> +								max_chan)

Likely better to use a temporary for ChannelPlan[channel_plan]
so these could become:

				if (cp->Channel[i] < min_chan ||
				    cp->Channel[i] > max_chan)
					etc...

> @@ -1490,7 +1523,8 @@ static u8 QueryIsShort(u8 TxHT, u8 TxRate, cb_desc *tcb_desc)
>  {
>  	u8   tmp_Short;
>  
> -	tmp_Short = (TxHT == 1) ? ((tcb_desc->bUseShortGI) ? 1 : 0) : ((tcb_desc->bUseShortPreamble) ? 1 : 0);
> +	tmp_Short = (TxHT == 1) ? ((tcb_desc->bUseShortGI) ? 1 : 0) :
> +				  ((tcb_desc->bUseShortPreamble) ? 1 : 0);
>  
>  	if (TxHT == 1 && TxRate != DESC90_RATEMCS15)
>  		tmp_Short = 0;

Maybe using bool would be better.

> @@ -1673,7 +1711,8 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
>  					  0, tx_zero_isr, dev);
>  			status = usb_submit_urb(tx_urb_zero, GFP_ATOMIC);
>  			if (status) {
> -				RT_TRACE(COMP_ERR, "Error TX URB for zero byte %d, error %d", atomic_read(&priv->tx_pending[tcb_desc->queue_index]), status);
> +				RT_TRACE(COMP_ERR,
> +					 "Error TX URB for zero byte %d, error %d", atomic_read(&priv->tx_pending[tcb_desc->queue_index]), status);

Probably better line wrapped after format

etc...

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