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

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

[Aditya Shankar]

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;
 	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;
+			} else {
 				if (memcmp(last_scanned_shadow[i].bssid,
 					   sme->bssid,
-					   ETH_ALEN) == 0)
+					   ETH_ALEN) == 0){
+					sel_bssi_idx = i;
 					break;
+				}
+			}
 		}
 	}
 
-	if (i < last_scanned_cnt) {
-		pstrNetworkInfo = &last_scanned_shadow[i];
+	if (sel_bssi_idx < last_scanned_cnt) {
+		pstrNetworkInfo = &last_scanned_shadow[sel_bssi_idx];
 	} else {
 		s32Error = -ENOENT;
 		wilc_connecting = 0;
-- 
2.7.4

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

[Dan Carpenter]

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

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

[Aditya Shankar]

On Thu, 5 Jan 2017 15:14:50 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

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

Thanks. Will change this.
> 
> >  	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
> 
> 

I shall send an updated patch with the suggested changes and a separate
patch for the change at the beginning of the loop. 

Thanks for your review!

-- 
adiTya