Re: [PATCHv3 02/15] staging: rtl8192u: r8192U_core: add temporary variables to keep lines under 80 characters

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sun, Sep 20, 2015 at 01:14:14PM -0400, Raphaël Beamonte wrote:
> Add some temporary variables to reduce line length under the maximum
> of 80 characters, as per the kernel code style.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 139 ++++++++++++++++++++++-----------
>  1 file changed, 94 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 28b54ba..2abc3e77 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -171,6 +171,7 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
>  {
>  	int i, max_chan = -1, min_chan = -1;
>  	struct ieee80211_device *ieee = priv->ieee80211;
> +	struct CHANNEL_LIST cl;

A whole structure on the stack?  Are you sure you want to do that?

>  
>  	switch (channel_plan) {
>  	case COUNTRY_CODE_FCC:
> @@ -194,15 +195,18 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
>  				 "unknown rf chip, can't set channel map in function:%s()\n",
>  				 __func__);
>  		}
> -		if (ChannelPlan[channel_plan].Len != 0) {
> +		cl = ChannelPlan[channel_plan];

You just did a memory copy of a whole structure, did you really want to
do that?  If so, why?  You just changed the logic of this function,
potentially slowing things down and setting yourself up for causing real
bugs (hint, anything you now change in that structure is not going to be
ever saved, it will go away after the function is exited.)

Please be more aware of how structures and pointers work in C before
doing changes like this, I can't take this change.

> +		if (cl.Len != 0) {
>  			/* Clear old channel map */
>  			memset(GET_DOT11D_INFO(ieee)->channel_map, 0,
>  			       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)
> +			for (i = 0; i < cl.Len; i++) {
> +				u8 chan = cl.Channel[i];
> +
> +				if (chan < min_chan || chan > max_chan)
>  					break;
> -				GET_DOT11D_INFO(ieee)->channel_map[ChannelPlan[channel_plan].Channel[i]] = 1;
> +				GET_DOT11D_INFO(ieee)->channel_map[chan] = 1;
>  			}
>  		}
>  		break;
> @@ -1649,9 +1653,12 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
>  					  &zero, 0, tx_zero_isr, dev);
>  			status = usb_submit_urb(tx_urb_zero, GFP_ATOMIC);
>  			if (status) {
> +				atomic_t tx =
> +					priv->tx_pending[tcb_desc->queue_index];

Oh that's funny, think about what you just did here.

Please get some more experience with C before doing kernel development.
C is tricky and will let you shoot yourself in the foot and any other
body part if you are not _VERY_ careful.  You just blew off a few limbs
without realizing it at all in just the first two changes you made.

thanks,

greg k-h