On Thu, 5 Jan 2017 15:14:50 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
