Date:	Thu, 25 Feb 2010 16:11:43 +0300
From:	Dan Carpenter <error27@...il.com>
To:	Jim Dog <jimdog@...thern-indymedia.org>
Cc:	gregkh@...e.de, jmm@...ian.org, mithlesh@...syssoft.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: wlan-ng: fixed multiple styling issues in
	prism2sta.c This is a patch to the prism2sta.c file that fixes
	multiple warnings generated by the checkpatch.pl tool, mostly
	related to the 80 character limit, though there were also a few
	related to having a space following the & operator. code has been
	refactored and no longer generates errors Signed-off-by: Jim Dog
	<jimdog@...thern-indymedia.org>

On Thu, Feb 25, 2010 at 12:36:52PM +0000, Jim Dog wrote:
> ---
>  drivers/staging/wlan-ng/prism2sta.c |   74 +++++++++++++++++++----------------
>  1 files changed, 40 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/prism2sta.c b/drivers/staging/wlan-ng/prism2sta.c
> index 50f301d..871d8f6 100644
> --- a/drivers/staging/wlan-ng/prism2sta.c
> +++ b/drivers/staging/wlan-ng/prism2sta.c
> @@ -64,7 +64,7 @@
>  #include <linux/byteorder/generic.h>
>  #include <linux/ctype.h>
>  
> -#include <asm/io.h>
> +#include <linux/io.h>
>  #include <linux/delay.h>
>  #include <asm/byteorder.h>
>  #include <linux/if_arp.h>
> @@ -455,10 +455,10 @@ u32 prism2sta_ifstate(wlandevice_t *wlandev, u32 ifstate)
>  			result = hfa384x_drvr_start(hw);
>  			if (result) {
>  				printk(KERN_ERR
> -				       "hfa384x_drvr_start() failed,"
> -				       "result=%d\n", (int)result);
> +				 "hfa384x_drvr_start() failed,"
> +				 "result=%d\n", (int)result);
>  				result =
> -				    P80211ENUM_resultcode_implementation_failure;
> +				 P80211ENUM_resultcode_implementation_failure;
>  				wlandev->msdstate = WLAN_MSD_HWPRESENT;
>  				break;
>  			}
> @@ -503,7 +503,7 @@ u32 prism2sta_ifstate(wlandevice_t *wlandev, u32 ifstate)
>  				       "hfa384x_drvr_start() failed,"
>  				       "result=%d\n", (int)result);
>  				result =
> -				    P80211ENUM_resultcode_implementation_failure;
> +				 P80211ENUM_resultcode_implementation_failure;
>  				wlandev->msdstate = WLAN_MSD_HWPRESENT;
>  				break;
>  			}
> @@ -514,7 +514,7 @@ u32 prism2sta_ifstate(wlandevice_t *wlandev, u32 ifstate)
>  				       "prism2sta_getcardinfo() failed,"
>  				       "result=%d\n", (int)result);
>  				result =
> -				    P80211ENUM_resultcode_implementation_failure;
> +				 P80211ENUM_resultcode_implementation_failure;
>  				hfa384x_drvr_stop(hw);
>  				wlandev->msdstate = WLAN_MSD_HWPRESENT;
>  				break;
> @@ -525,7 +525,7 @@ u32 prism2sta_ifstate(wlandevice_t *wlandev, u32 ifstate)
>  				       "prism2sta_globalsetup() failed,"
>  				       "result=%d\n", (int)result);
>  				result =
> -				    P80211ENUM_resultcode_implementation_failure;
> +				 P80211ENUM_resultcode_implementation_failure;
>  				hfa384x_drvr_stop(hw);
>  				wlandev->msdstate = WLAN_MSD_HWPRESENT;
>  				break;
> @@ -1023,13 +1023,13 @@ static void prism2sta_inf_tallies(wlandevice_t *wlandev,
>  
>  	cnt = sizeof(hfa384x_CommTallies32_t) / sizeof(u32);
>  	if (inf->framelen > 22) {
> -		dst = (u32 *) & hw->tallies;
> -		src32 = (u32 *) & inf->info.commtallies32;
> +		dst = (u32 *) &hw->tallies;
> +		src32 = (u32 *) &inf->info.commtallies32;
>  		for (i = 0; i < cnt; i++, dst++, src32++)
>  			*dst += le32_to_cpu(*src32);
>  	} else {
> -		dst = (u32 *) & hw->tallies;
> -		src16 = (u16 *) & inf->info.commtallies16;
> +		dst = (u32 *) &hw->tallies;
> +		src16 = (u16 *) &inf->info.commtallies16;
>  		for (i = 0; i < cnt; i++, dst++, src16++)
>  			*dst += le16_to_cpu(*src16);
>  	}
> @@ -1179,7 +1179,8 @@ static void prism2sta_inf_chinforesults(wlandevice_t *wlandev,
>  			    le16_to_cpu(inf->info.chinforesult.result[n].
>  					active);
>  			pr_debug
> -			    ("chinfo: channel %d, %s level (avg/peak)=%d/%d dB, pcf %d\n",
> +			("chinfo:
> +			 channel %d, %s level (avg/peak)=%d/%d dB, pcf %d\n",

You introduced a bug here.

I want to encourage newbies but really these patches are the wrong 
idea.

We could easily do this with a script.  It is scripts that should be 
slaving away for humans and not the other way round.  In the case of 
handling string literals, you're slaving away to do what a _buggy_ 
version of checkpatch.pl tells you to do.

It's not that hard to find actual bugs to fix.  Here is a list of 11
buffer overflows I posted earlier.
http://lkml.org/lkml/2010/2/25/38

If you think about it, it's better to start with the goal of fixing an 
actual bug.  Otherwise you have the possibility of introducing a bug, 
but no possibility of fixing a bug.

Anyway.  Aim higher is all I'm saying.  Stop with the checkpatch.pl.

regards,
dan carpenter

>  			     channel + 1,
>  			     chinforesult->
>  			     active & HFA384x_CHINFORESULT_BSSACTIVE ? "signal"
> @@ -1246,7 +1247,8 @@ void prism2sta_processing_defer(struct work_struct *data)
>  
>  		netif_carrier_on(wlandev->netdev);
>  
> -		/* If we are joining a specific AP, set our state and reset retries */
> +		/* If we are joining a specific AP, */
> +		/* set our state and reset retries */
>  		if (hw->join_ap == 1)
>  			hw->join_ap = 2;
>  		hw->join_retries = 60;
> @@ -1260,10 +1262,10 @@ void prism2sta_processing_defer(struct work_struct *data)
>  			/* For non-usb devices, we can use the sync versions */
>  			/* Collect the BSSID, and set state to allow tx */
>  
> -			result = hfa384x_drvr_getconfig(hw,
> -							HFA384x_RID_CURRENTBSSID,
> -							wlandev->bssid,
> -							WLAN_BSSID_LEN);
> +			result = hfa384x_drvr_getconfig
> +				    (hw, HFA384x_RID_CURRENTBSSID,
> +				     wlandev->bssid,
> +				     WLAN_BSSID_LEN);
>  			if (result) {
>  				pr_debug
>  				    ("getconfig(0x%02x) failed, result = %d\n",
> @@ -1280,14 +1282,14 @@ void prism2sta_processing_defer(struct work_struct *data)
>  				     HFA384x_RID_CURRENTSSID, result);
>  				goto failed;
>  			}
> -			prism2mgmt_bytestr2pstr((hfa384x_bytestr_t *) & ssid,
> +			prism2mgmt_bytestr2pstr((hfa384x_bytestr_t *) &ssid,
>  						(p80211pstrd_t *) &
>  						wlandev->ssid);
>  
>  			/* Collect the port status */
> -			result = hfa384x_drvr_getconfig16(hw,
> -							  HFA384x_RID_PORTSTATUS,
> -							  &portstatus);
> +			result = hfa384x_drvr_getconfig16
> +				    (hw, HFA384x_RID_PORTSTATUS,
> +				     &portstatus);
>  			if (result) {
>  				pr_debug
>  				    ("getconfig(0x%02x) failed, result = %d\n",
> @@ -1322,7 +1324,7 @@ void prism2sta_processing_defer(struct work_struct *data)
>  					       &joinreq,
>  					       HFA384x_RID_JOINREQUEST_LEN);
>  			printk(KERN_INFO
> -			       "linkstatus=DISCONNECTED (re-submitting join)\n");
> +			      "linkstatus=DISCONNECTED (re-submitting join)\n");
>  		} else {
>  			if (wlandev->netdev->type == ARPHRD_ETHER)
>  				printk(KERN_INFO
> @@ -1368,8 +1370,8 @@ void prism2sta_processing_defer(struct work_struct *data)
>  				 HFA384x_RID_CURRENTSSID, result);
>  			goto failed;
>  		}
> -		prism2mgmt_bytestr2pstr((hfa384x_bytestr_t *) & ssid,
> -					(p80211pstrd_t *) & wlandev->ssid);
> +		prism2mgmt_bytestr2pstr((hfa384x_bytestr_t *) &ssid,
> +					(p80211pstrd_t *) &wlandev->ssid);
>  
>  		hw->link_status = HFA384x_LINK_CONNECTED;
>  		netif_carrier_on(wlandev->netdev);
> @@ -1509,14 +1511,15 @@ static void prism2sta_inf_assocstatus(wlandevice_t *wlandev,
>  	rec.reason = le16_to_cpu(rec.reason);
>  
>  	/*
> -	 ** Find the address in the list of authenticated stations.  If it wasn't
> +	 ** Find the address in the list of authenticated stations. If it wasn't
>  	 ** found, then this address has not been previously authenticated and
>  	 ** something weird has happened if this is anything other than an
>  	 ** "authentication failed" message.  If the address was found, then
>  	 ** set the "associated" flag for that station, based on whether the
> -	 ** station is associating or losing its association.  Something weird
> -	 ** has also happened if we find the address in the list of authenticated
> -	 ** stations but we are getting an "authentication failed" message.
> +	 ** station is associating or losing its association. Something weird
> +	 ** has also happened if we find the address in the list of
> +	 ** authenticated stations but we are getting an "authentication failed"
> +	 ** message.
>  	 */
>  
>  	for (i = 0; i < hw->authlist.cnt; i++)
> @@ -1526,7 +1529,8 @@ static void prism2sta_inf_assocstatus(wlandevice_t *wlandev,
>  	if (i >= hw->authlist.cnt) {
>  		if (rec.assocstatus != HFA384x_ASSOCSTATUS_AUTHFAIL)
>  			printk(KERN_WARNING
> -			       "assocstatus info frame received for non-authenticated station.\n");
> +				"assocstatus info frame received
> +				 for non-authenticated station.\n");
>  	} else {
>  		hw->authlist.assoc[i] =
>  		    (rec.assocstatus == HFA384x_ASSOCSTATUS_STAASSOC ||
> @@ -1534,7 +1538,8 @@ static void prism2sta_inf_assocstatus(wlandevice_t *wlandev,
>  
>  		if (rec.assocstatus == HFA384x_ASSOCSTATUS_AUTHFAIL)
>  			printk(KERN_WARNING
> -			       "authfail assocstatus info frame received for authenticated station.\n");
> +				"authfail assocstatus info frame received
> +				 for authenticated station.\n");
>  	}
>  
>  	return;
> @@ -1682,7 +1687,7 @@ static void prism2sta_inf_authreq_defer(wlandevice_t *wlandev,
>  
>  	/*
>  	 ** If the authentication is okay, then add the MAC address to the list
> -	 ** of authenticated stations.  Don't add the address if it is already in
> +	 ** of authenticated stations.  Don't add the address if it's already in
>  	 ** the list.  (802.11b does not seem to disallow a station from issuing
>  	 ** an authentication request when the station is already authenticated.
>  	 ** Does this sort of thing ever happen?  We might as well do the check
> @@ -1997,7 +2002,8 @@ void prism2sta_commsqual_defer(struct work_struct *data)
>  	if (wlandev->macmode != WLAN_MACMODE_IBSS_STA) {
>  		result = hfa384x_drvr_getconfig(hw, HFA384x_RID_DBMCOMMSQUALITY,
>  						&hw->qual,
> -						HFA384x_RID_DBMCOMMSQUALITY_LEN);
> +						HFA384x_RID_DBMCOMMSQUALITY_LEN)
> +						;
>  
>  		if (result) {
>  			printk(KERN_ERR "error fetching commsqual\n");
> @@ -2028,8 +2034,8 @@ void prism2sta_commsqual_defer(struct work_struct *data)
>  			 HFA384x_RID_CURRENTSSID, result);
>  		goto done;
>  	}
> -	prism2mgmt_bytestr2pstr((hfa384x_bytestr_t *) & ssid,
> -				(p80211pstrd_t *) & wlandev->ssid);
> +	prism2mgmt_bytestr2pstr((hfa384x_bytestr_t *) &ssid,
> +				(p80211pstrd_t *) &wlandev->ssid);
>  
>  	/* Reschedule timer */
>  	mod_timer(&hw->commsqual_timer, jiffies + HZ);
> -- 
> 1.6.3.3
> 
> --
> 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/
--
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