Re: [PATCH] staging: wilc1000: Connect to highest RSSI value for required SSID

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Jan 05, 2017 at 01:03:41PM +0530, Aditya Shankar wrote:
> Connect to the highest rssi with the required SSID in the shadow
> table if the connection criteria is based only on the SSID.
> For the first matching SSID, an index to the table is saved.
> Later the index is updated if matching SSID has a higher
> RSSI value than the last saved index.
> 
> However if decision is made based on BSSID, there is only one match
> in the table and corresponding index is used.
> 
> Signed-off-by: Aditya Shankar <aditya.shankar@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index c1a24f7..32206b8 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -665,6 +665,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>  {
>  	s32 s32Error = 0;
>  	u32 i;
> +	u32 sel_bssi_idx = last_scanned_cnt + 1;


My understanding from reading the code is that "last_scanned_cnt + 1"
is a randomly chosen invalid value.  Just saying:

	sel_bssi_idx = last_scanned_cnt;

would  also work because it's also invalid and slightly shorter to type.
But I suggest that you go with something like UINT_MAX because that's
more clearly invalid.

>  	u8 u8security = NO_ENCRYPT;
>  	enum AUTHTYPE tenuAuth_type = ANY;
>  
> @@ -688,18 +689,25 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
>  		    memcmp(last_scanned_shadow[i].ssid,
>  			   sme->ssid,
>  			   sme->ssid_len) == 0) {
> -			if (!sme->bssid)
> -				break;
> -			else
> +			if (!sme->bssid) {
> +				if (sel_bssi_idx == (last_scanned_cnt + 1))
> +					sel_bssi_idx = i;
> +				else if (last_scanned_shadow[i].rssi >
> +					 last_scanned_shadow[sel_bssi_idx].rssi)
> +					sel_bssi_idx = i;

Combine these with an ||.

			if (!sme->bssid) {
				if (sel_bssi_idx == UINT_MAX ||
				    last_scanned_shadow[i].rssi >
				    last_scanned_shadow[sel_bssi_idx].rssi)
					sel_bssi_idx = i;



In a separate patch, you can reverse the if statement at the start of
the loop:

		if (sme->ssid_len != last_scanned_shadow[i].ssid_len ||
		    memcmp(last_scanned_shadow[i].ssid, sme->ssid,
			   sme->ssid_len) != 0)
			continue;

That way you can pull these lines in a tab.


> +			} else {
>  				if (memcmp(last_scanned_shadow[i].bssid,
>  					   sme->bssid,
> -					   ETH_ALEN) == 0)
> +					   ETH_ALEN) == 0){

Add a space before the curly brace.

regards,
dan carpenter