[PATCH 00/30] staging: wilc1000: checkpatch fixes and code cleanup

[PATCH 00/30] staging: wilc1000: checkpatch fixes and code cleanup

[Ajay Singh]

This patch series contains modification to remove checkpatch related issues,
mainly related to 'line over 80 chars'.

We are left with around '5' checkpatch warnings in WILC1000.

Also code cleanup related change to follow as per linux coding style are
included in this patch series.

Ajay Singh (30):
  staging: wilc1000: added complete() call for error scenario in
    handle_key()
  staging: wilc1000: remove 'ret' variable in handle_key()
  staging: wilc1000: fix line over 80 chars in handle_key()
  staging: wilc1000: fix line over 80 characters issue in
    handle_connect()
  staging: wilc1000: fix line over 80 chars in
    host_int_parse_assoc_resp_info()
  staging: wilc1000: fix line over 80 chars issue in
    host_int_handle_disconnect()
  staging: wilc1000: fix line over 80 characters in
    host_int_parse_join_bss_param()
  staging: wilc1000: fix line over 80 chars in
    host_int_parse_assoc_resp_info()
  staging: wilc1000: rename kmalloc with kmemdup() in
    handle_connect_timeout()
  staging: wilc1000: fix line over 80 chars in linux_mon
  staging: wilc1000: use sizeof(*wdev) to allocate memory in
    wilc_wfi_cfg_alloc()
  staging: wilc1000: use kmalloc(sizeof(*mgmt_tx)...) in mgmt_tx()
  staging: wilc1000: rename clear_duringIP() to avoid camelCase issue
  staging: wilc1000: fix line over 80 chars in add_network_to_shadow()
  staging: wilc1000: use kmemdup instead of kmalloc in
    add_network_to_shadow()
  staging: wilc1000: fix line over 80 charas in
    wilc_wfi_remain_on_channel_expired()
  staging: wilc1000: fix line over 80 chars in
    wilc_wfi_cfg_tx_vendor_spec()
  staging: wilc1000: fix line over 80 chars in get_station()
  staging: wilc1000: fix line over 80 chars in wilc_create_wiphy()
    declaration
  staging: wilc1000: fix line over 80 characters in add_key()
  staging: wilc1000: fix line over 80 chars in scan()
  staging: wilc1000: fix line over 80 chars issue in connect()
  staging: wilc1000: rename u8security to avoid datatype in variable
    name
  staging: wilc1000: refactor del_station() to avoid parenthesis
    misalignment
  staging: wilc1000: fix line over 80 chars in wilc_sdio struct
  staging: wilc1000: added #define for setting radiotap header
  staging: wilc1000: remove 'flag' argument from wilc_mac_indicate()
  staging: wilc1000: added comments for mutex and spinlock_t
  staging: wilc1000: remove unused 'lock' varible in 'wilc_priv'
    structure
  staging: wilc1000: rename s8idxarray to avoid datatype in variable
    name

 drivers/staging/wilc1000/host_interface.c         | 417 ++++++++++++----------
 drivers/staging/wilc1000/host_interface.h         |   2 +-
 drivers/staging/wilc1000/linux_mon.c              |  25 +-
 drivers/staging/wilc1000/linux_wlan.c             |  17 +-
 drivers/staging/wilc1000/wilc_sdio.c              |   3 +-
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 187 +++++-----
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.h |   3 +-
 drivers/staging/wilc1000/wilc_wfi_netdevice.h     |   8 +-
 drivers/staging/wilc1000/wilc_wlan.c              |   7 +-
 drivers/staging/wilc1000/wilc_wlan_if.h           |   3 -
 10 files changed, 350 insertions(+), 322 deletions(-)

-- 
2.7.4

[PATCH 01/30] staging: wilc1000: added complete() call for error scenario in handle_key()

[Ajay Singh]

During memory allocation failure in handle_key() the complete() was not
called for comp_test_key_block event. So now added the code to call
complete() for event during error scenario.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 28edd90..d903ae5 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1525,8 +1525,10 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 
 			key_buf = kmalloc(hif_key->attr.wep.key_len + 2,
 					  GFP_KERNEL);
-			if (!key_buf)
-				return -ENOMEM;
+			if (!key_buf) {
+				result = -ENOMEM;
+				goto out_wep;
+			}
 
 			key_buf[0] = hif_key->attr.wep.index;
 			key_buf[1] = hif_key->attr.wep.key_len;
@@ -1547,8 +1549,10 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 			kfree(key_buf);
 		} else if (hif_key->action & ADDKEY) {
 			key_buf = kmalloc(hif_key->attr.wep.key_len + 2, GFP_KERNEL);
-			if (!key_buf)
-				return -ENOMEM;
+			if (!key_buf) {
+				result = -ENOMEM;
+				goto out_wep;
+			}
 			key_buf[0] = hif_key->attr.wep.index;
 			memcpy(key_buf + 1, &hif_key->attr.wep.key_len, 1);
 			memcpy(key_buf + 2, hif_key->attr.wep.key,
@@ -1585,6 +1589,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 						      &wid, 1,
 						      wilc_get_vif_idx(vif));
 		}
+out_wep:
 		complete(&hif_drv->comp_test_key_block);
 		break;
 
@@ -1619,7 +1624,6 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 						      wilc_get_vif_idx(vif));
 
 			kfree(key_buf);
-			complete(&hif_drv->comp_test_key_block);
 		} else if (hif_key->action & ADDKEY) {
 			key_buf = kzalloc(RX_MIC_KEY_MSG_LEN, GFP_KERNEL);
 			if (!key_buf) {
@@ -1648,9 +1652,9 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 						      wilc_get_vif_idx(vif));
 
 			kfree(key_buf);
-			complete(&hif_drv->comp_test_key_block);
 		}
 out_wpa_rx_gtk:
+		complete(&hif_drv->comp_test_key_block);
 		kfree(hif_key->attr.wpa.key);
 		kfree(hif_key->attr.wpa.seq);
 		if (ret)
@@ -1686,7 +1690,6 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 						      wid_list, 2,
 						      wilc_get_vif_idx(vif));
 			kfree(key_buf);
-			complete(&hif_drv->comp_test_key_block);
 		} else if (hif_key->action & ADDKEY) {
 			key_buf = kmalloc(PTK_KEY_MSG_LEN, GFP_KERNEL);
 			if (!key_buf) {
@@ -1708,10 +1711,10 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 						      &wid, 1,
 						      wilc_get_vif_idx(vif));
 			kfree(key_buf);
-			complete(&hif_drv->comp_test_key_block);
 		}
 
 out_wpa_ptk:
+		complete(&hif_drv->comp_test_key_block);
 		kfree(hif_key->attr.wpa.key);
 		if (ret)
 			return ret;
-- 
2.7.4

[PATCH 02/30] staging: wilc1000: remove 'ret' variable in handle_key()

[Ajay Singh]

Remove the use of unnecessary 'ret' variable and use existing 'result'
variable to hold the status. Also changed type of 'result' from s32 to
int to confirm with the function return type.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index d903ae5..4ba868c 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1500,13 +1500,12 @@ static s32 handle_rcvd_gnrl_async_info(struct wilc_vif *vif,
 
 static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 {
-	s32 result = 0;
+	int result = 0;
 	struct wid wid;
 	struct wid wid_list[5];
 	u8 i;
 	u8 *key_buf;
 	s8 s8idxarray[1];
-	s8 ret = 0;
 	struct host_if_drv *hif_drv = vif->hif_drv;
 
 	switch (hif_key->type) {
@@ -1597,7 +1596,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 		if (hif_key->action & ADDKEY_AP) {
 			key_buf = kzalloc(RX_MIC_KEY_MSG_LEN, GFP_KERNEL);
 			if (!key_buf) {
-				ret = -ENOMEM;
+				result = -ENOMEM;
 				goto out_wpa_rx_gtk;
 			}
 
@@ -1627,7 +1626,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 		} else if (hif_key->action & ADDKEY) {
 			key_buf = kzalloc(RX_MIC_KEY_MSG_LEN, GFP_KERNEL);
 			if (!key_buf) {
-				ret = -ENOMEM;
+				result = -ENOMEM;
 				goto out_wpa_rx_gtk;
 			}
 
@@ -1657,16 +1656,13 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 		complete(&hif_drv->comp_test_key_block);
 		kfree(hif_key->attr.wpa.key);
 		kfree(hif_key->attr.wpa.seq);
-		if (ret)
-			return ret;
-
 		break;
 
 	case WPA_PTK:
 		if (hif_key->action & ADDKEY_AP) {
 			key_buf = kmalloc(PTK_KEY_MSG_LEN + 1, GFP_KERNEL);
 			if (!key_buf) {
-				ret = -ENOMEM;
+				result = -ENOMEM;
 				goto out_wpa_ptk;
 			}
 
@@ -1693,7 +1689,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 		} else if (hif_key->action & ADDKEY) {
 			key_buf = kmalloc(PTK_KEY_MSG_LEN, GFP_KERNEL);
 			if (!key_buf) {
-				ret = -ENOMEM;
+				result = -ENOMEM;
 				goto out_wpa_ptk;
 			}
 
@@ -1716,9 +1712,6 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 out_wpa_ptk:
 		complete(&hif_drv->comp_test_key_block);
 		kfree(hif_key->attr.wpa.key);
-		if (ret)
-			return ret;
-
 		break;
 
 	case PMKSA:
-- 
2.7.4

[PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()

[Ajay Singh]

Fix checkpatch reported issue of line over 80 char in handle_key().
Introduced new functions by spliting existing function to address the
checkpatch issue.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 59 +++++++++++++++++++------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 4ba868c..29f9907 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1498,12 +1498,45 @@ static s32 handle_rcvd_gnrl_async_info(struct wilc_vif *vif,
 	return result;
 }
 
+static int wilc_pmksa_key_copy(struct wilc_vif *vif, struct key_attr *hif_key)
+{
+	int i;
+	int ret;
+	struct wid wid;
+	u8 *key_buf;
+
+	key_buf = kmalloc((hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1,
+			  GFP_KERNEL);
+	if (!key_buf)
+		return -ENOMEM;
+
+	key_buf[0] = hif_key->attr.pmkid.numpmkid;
+
+	for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++) {
+		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + 1),
+		       hif_key->attr.pmkid.pmkidlist[i].bssid, ETH_ALEN);
+		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + ETH_ALEN + 1),
+		       hif_key->attr.pmkid.pmkidlist[i].pmkid, PMKID_LEN);
+	}
+
+	wid.id = (u16)WID_PMKID_INFO;
+	wid.type = WID_STR;
+	wid.val = (s8 *)key_buf;
+	wid.size = (hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1;
+
+	ret = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
+				   wilc_get_vif_idx(vif));
+
+	kfree(key_buf);
+
+	return ret;
+}
+
 static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 {
 	int result = 0;
 	struct wid wid;
 	struct wid wid_list[5];
-	u8 i;
 	u8 *key_buf;
 	s8 s8idxarray[1];
 	struct host_if_drv *hif_drv = vif->hif_drv;
@@ -1547,7 +1580,8 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 						      wilc_get_vif_idx(vif));
 			kfree(key_buf);
 		} else if (hif_key->action & ADDKEY) {
-			key_buf = kmalloc(hif_key->attr.wep.key_len + 2, GFP_KERNEL);
+			key_buf = kmalloc(hif_key->attr.wep.key_len + 2,
+					  GFP_KERNEL);
 			if (!key_buf) {
 				result = -ENOMEM;
 				goto out_wep;
@@ -1715,26 +1749,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 		break;
 
 	case PMKSA:
-		key_buf = kmalloc((hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1, GFP_KERNEL);
-		if (!key_buf)
-			return -ENOMEM;
-
-		key_buf[0] = hif_key->attr.pmkid.numpmkid;
-
-		for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++) {
-			memcpy(key_buf + ((PMKSA_KEY_LEN * i) + 1), hif_key->attr.pmkid.pmkidlist[i].bssid, ETH_ALEN);
-			memcpy(key_buf + ((PMKSA_KEY_LEN * i) + ETH_ALEN + 1), hif_key->attr.pmkid.pmkidlist[i].pmkid, PMKID_LEN);
-		}
-
-		wid.id = (u16)WID_PMKID_INFO;
-		wid.type = WID_STR;
-		wid.val = (s8 *)key_buf;
-		wid.size = (hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1;
-
-		result = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
-					      wilc_get_vif_idx(vif));
-
-		kfree(key_buf);
+		result = wilc_pmksa_key_copy(vif, hif_key);
 		break;
 	}
 
-- 
2.7.4

Re: [PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Fix checkpatch reported issue of line over 80 char in handle_key().
> Introduced new functions by spliting existing function to address the
> checkpatch issue.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 59 +++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 4ba868c..29f9907 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -1498,12 +1498,45 @@ static s32 handle_rcvd_gnrl_async_info(struct wilc_vif *vif,
>  	return result;
>  }
>  
> +static int wilc_pmksa_key_copy(struct wilc_vif *vif, struct key_attr *hif_key)
> +{
> +	int i;
> +	int ret;
> +	struct wid wid;
> +	u8 *key_buf;
> +
> +	key_buf = kmalloc((hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1,
> +			  GFP_KERNEL);
> +	if (!key_buf)
> +		return -ENOMEM;
> +
> +	key_buf[0] = hif_key->attr.pmkid.numpmkid;
> +
> +	for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++) {
> +		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + 1),
> +		       hif_key->attr.pmkid.pmkidlist[i].bssid, ETH_ALEN);
> +		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + ETH_ALEN + 1),
> +		       hif_key->attr.pmkid.pmkidlist[i].pmkid, PMKID_LEN);
> +	}
> +
> +	wid.id = (u16)WID_PMKID_INFO;
> +	wid.type = WID_STR;
> +	wid.val = (s8 *)key_buf;

Is this cast really needed?

> +	wid.size = (hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1;
> +
> +	ret = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
> +				   wilc_get_vif_idx(vif));
> +
> +	kfree(key_buf);
> +
> +	return ret;
> +}
> +
>  static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
>  {
>  	int result = 0;
>  	struct wid wid;
>  	struct wid wid_list[5];
> -	u8 i;
>  	u8 *key_buf;
>  	s8 s8idxarray[1];
>  	struct host_if_drv *hif_drv = vif->hif_drv;
> @@ -1547,7 +1580,8 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
>  						      wilc_get_vif_idx(vif));
>  			kfree(key_buf);
>  		} else if (hif_key->action & ADDKEY) {
> -			key_buf = kmalloc(hif_key->attr.wep.key_len + 2, GFP_KERNEL);
> +			key_buf = kmalloc(hif_key->attr.wep.key_len + 2,
> +					  GFP_KERNEL);
>  			if (!key_buf) {
>  				result = -ENOMEM;
>  				goto out_wep;
> @@ -1715,26 +1749,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
>  		break;
>  
>  	case PMKSA:
> -		key_buf = kmalloc((hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1, GFP_KERNEL);
> -		if (!key_buf)
> -			return -ENOMEM;
> -
> -		key_buf[0] = hif_key->attr.pmkid.numpmkid;
> -
> -		for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++) {
> -			memcpy(key_buf + ((PMKSA_KEY_LEN * i) + 1), hif_key->attr.pmkid.pmkidlist[i].bssid, ETH_ALEN);
> -			memcpy(key_buf + ((PMKSA_KEY_LEN * i) + ETH_ALEN + 1), hif_key->attr.pmkid.pmkidlist[i].pmkid, PMKID_LEN);
> -		}
> -
> -		wid.id = (u16)WID_PMKID_INFO;
> -		wid.type = WID_STR;
> -		wid.val = (s8 *)key_buf;
> -		wid.size = (hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN) + 1;
> -
> -		result = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
> -					      wilc_get_vif_idx(vif));
> -
> -		kfree(key_buf);
> +		result = wilc_pmksa_key_copy(vif, hif_key);
>  		break;
>  	}
>  
> 

Re: [PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()

[Ajay Singh]

On Wed, 9 May 2018 16:44:47 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Fix checkpatch reported issue of line over 80 char in handle_key().
> > Introduced new functions by spliting existing function to address
> > the checkpatch issue.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 59
> > +++++++++++++++++++------------ 1 file changed, 37 insertions(+),
> > 22 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index 4ba868c..29f9907
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -1498,12 +1498,45 @@ static s32
> > handle_rcvd_gnrl_async_info(struct wilc_vif *vif, return result;
> >  }
> >  
> > +static int wilc_pmksa_key_copy(struct wilc_vif *vif, struct
> > key_attr *hif_key) +{
> > +	int i;
> > +	int ret;
> > +	struct wid wid;
> > +	u8 *key_buf;
> > +
> > +	key_buf = kmalloc((hif_key->attr.pmkid.numpmkid *
> > PMKSA_KEY_LEN) + 1,
> > +			  GFP_KERNEL);
> > +	if (!key_buf)
> > +		return -ENOMEM;
> > +
> > +	key_buf[0] = hif_key->attr.pmkid.numpmkid;
> > +
> > +	for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++) {
> > +		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + 1),
> > +		       hif_key->attr.pmkid.pmkidlist[i].bssid,
> > ETH_ALEN);
> > +		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + ETH_ALEN +
> > 1),
> > +		       hif_key->attr.pmkid.pmkidlist[i].pmkid,
> > PMKID_LEN);
> > +	}
> > +
> > +	wid.id = (u16)WID_PMKID_INFO;
> > +	wid.type = WID_STR;
> > +	wid.val = (s8 *)key_buf;  
> 
> Is this cast really needed?
> 

This patch is only to address line over 80 chars checkpatch issue.
It is not good to club these change with type cast related
modification. For removing unnecessary cast we can submit
a separate patch series.
These are my views. What do you think?


> > +	wid.size = (hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN)
> > + 1; +
> > +	ret = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
> > +				   wilc_get_vif_idx(vif));
> > +
> > +	kfree(key_buf);
> > +
> > +	return ret;
> > +}
> > +
> >  static int handle_key(struct wilc_vif *vif, struct key_attr
> > *hif_key) {
> >  	int result = 0;
> >  	struct wid wid;
> >  	struct wid wid_list[5];
> > -	u8 i;
> >  	u8 *key_buf;
> >  	s8 s8idxarray[1];
> >  	struct host_if_drv *hif_drv = vif->hif_drv;
> > @@ -1547,7 +1580,8 @@ static int handle_key(struct wilc_vif *vif,
> > struct key_attr *hif_key) wilc_get_vif_idx(vif));
> >  			kfree(key_buf);
> >  		} else if (hif_key->action & ADDKEY) {
> > -			key_buf =
> > kmalloc(hif_key->attr.wep.key_len + 2, GFP_KERNEL);
> > +			key_buf =
> > kmalloc(hif_key->attr.wep.key_len + 2,
> > +					  GFP_KERNEL);
> >  			if (!key_buf) {
> >  				result = -ENOMEM;
> >  				goto out_wep;
> > @@ -1715,26 +1749,7 @@ static int handle_key(struct wilc_vif *vif,
> > struct key_attr *hif_key) break;
> >  
> >  	case PMKSA:
> > -		key_buf = kmalloc((hif_key->attr.pmkid.numpmkid *
> > PMKSA_KEY_LEN) + 1, GFP_KERNEL);
> > -		if (!key_buf)
> > -			return -ENOMEM;
> > -
> > -		key_buf[0] = hif_key->attr.pmkid.numpmkid;
> > -
> > -		for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++)
> > {
> > -			memcpy(key_buf + ((PMKSA_KEY_LEN * i) +
> > 1), hif_key->attr.pmkid.pmkidlist[i].bssid, ETH_ALEN);
> > -			memcpy(key_buf + ((PMKSA_KEY_LEN * i) +
> > ETH_ALEN + 1), hif_key->attr.pmkid.pmkidlist[i].pmkid, PMKID_LEN);
> > -		}
> > -
> > -		wid.id = (u16)WID_PMKID_INFO;
> > -		wid.type = WID_STR;
> > -		wid.val = (s8 *)key_buf;
> > -		wid.size = (hif_key->attr.pmkid.numpmkid *
> > PMKSA_KEY_LEN) + 1; -
> > -		result = wilc_send_config_pkt(vif, SET_CFG, &wid,
> > 1,
> > -
> > wilc_get_vif_idx(vif)); -
> > -		kfree(key_buf);
> > +		result = wilc_pmksa_key_copy(vif, hif_key);
> >  		break;
> >  	}
> >  
> >   

Re: [PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()

[Claudiu Beznea]

On 09.05.2018 21:36, Ajay Singh wrote:
> On Wed, 9 May 2018 16:44:47 +0300
> Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:
> 
>> On 07.05.2018 11:43, Ajay Singh wrote:
>>> Fix checkpatch reported issue of line over 80 char in handle_key().
>>> Introduced new functions by spliting existing function to address
>>> the checkpatch issue.
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> ---
>>>  drivers/staging/wilc1000/host_interface.c | 59
>>> +++++++++++++++++++------------ 1 file changed, 37 insertions(+),
>>> 22 deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/host_interface.c
>>> b/drivers/staging/wilc1000/host_interface.c index 4ba868c..29f9907
>>> 100644 --- a/drivers/staging/wilc1000/host_interface.c
>>> +++ b/drivers/staging/wilc1000/host_interface.c
>>> @@ -1498,12 +1498,45 @@ static s32
>>> handle_rcvd_gnrl_async_info(struct wilc_vif *vif, return result;
>>>  }
>>>  
>>> +static int wilc_pmksa_key_copy(struct wilc_vif *vif, struct
>>> key_attr *hif_key) +{
>>> +	int i;
>>> +	int ret;
>>> +	struct wid wid;
>>> +	u8 *key_buf;
>>> +
>>> +	key_buf = kmalloc((hif_key->attr.pmkid.numpmkid *
>>> PMKSA_KEY_LEN) + 1,
>>> +			  GFP_KERNEL);
>>> +	if (!key_buf)
>>> +		return -ENOMEM;
>>> +
>>> +	key_buf[0] = hif_key->attr.pmkid.numpmkid;
>>> +
>>> +	for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++) {
>>> +		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + 1),
>>> +		       hif_key->attr.pmkid.pmkidlist[i].bssid,
>>> ETH_ALEN);
>>> +		memcpy(key_buf + ((PMKSA_KEY_LEN * i) + ETH_ALEN +
>>> 1),
>>> +		       hif_key->attr.pmkid.pmkidlist[i].pmkid,
>>> PMKID_LEN);
>>> +	}
>>> +
>>> +	wid.id = (u16)WID_PMKID_INFO;
>>> +	wid.type = WID_STR;
>>> +	wid.val = (s8 *)key_buf;  
>>
>> Is this cast really needed?
>>
> 
> This patch is only to address line over 80 chars checkpatch issue.
> It is not good to club these change with type cast related
> modification. For removing unnecessary cast we can submit
> a separate patch series.
> These are my views. What do you think?
> 

I'm ok with this. I was thinking that since you introduced this new function
you may want to also address this, if any.

> 
>>> +	wid.size = (hif_key->attr.pmkid.numpmkid * PMKSA_KEY_LEN)
>>> + 1; +
>>> +	ret = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
>>> +				   wilc_get_vif_idx(vif));
>>> +
>>> +	kfree(key_buf);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  static int handle_key(struct wilc_vif *vif, struct key_attr
>>> *hif_key) {
>>>  	int result = 0;
>>>  	struct wid wid;
>>>  	struct wid wid_list[5];
>>> -	u8 i;
>>>  	u8 *key_buf;
>>>  	s8 s8idxarray[1];
>>>  	struct host_if_drv *hif_drv = vif->hif_drv;
>>> @@ -1547,7 +1580,8 @@ static int handle_key(struct wilc_vif *vif,
>>> struct key_attr *hif_key) wilc_get_vif_idx(vif));
>>>  			kfree(key_buf);
>>>  		} else if (hif_key->action & ADDKEY) {
>>> -			key_buf =
>>> kmalloc(hif_key->attr.wep.key_len + 2, GFP_KERNEL);
>>> +			key_buf =
>>> kmalloc(hif_key->attr.wep.key_len + 2,
>>> +					  GFP_KERNEL);
>>>  			if (!key_buf) {
>>>  				result = -ENOMEM;
>>>  				goto out_wep;
>>> @@ -1715,26 +1749,7 @@ static int handle_key(struct wilc_vif *vif,
>>> struct key_attr *hif_key) break;
>>>  
>>>  	case PMKSA:
>>> -		key_buf = kmalloc((hif_key->attr.pmkid.numpmkid *
>>> PMKSA_KEY_LEN) + 1, GFP_KERNEL);
>>> -		if (!key_buf)
>>> -			return -ENOMEM;
>>> -
>>> -		key_buf[0] = hif_key->attr.pmkid.numpmkid;
>>> -
>>> -		for (i = 0; i < hif_key->attr.pmkid.numpmkid; i++)
>>> {
>>> -			memcpy(key_buf + ((PMKSA_KEY_LEN * i) +
>>> 1), hif_key->attr.pmkid.pmkidlist[i].bssid, ETH_ALEN);
>>> -			memcpy(key_buf + ((PMKSA_KEY_LEN * i) +
>>> ETH_ALEN + 1), hif_key->attr.pmkid.pmkidlist[i].pmkid, PMKID_LEN);
>>> -		}
>>> -
>>> -		wid.id = (u16)WID_PMKID_INFO;
>>> -		wid.type = WID_STR;
>>> -		wid.val = (s8 *)key_buf;
>>> -		wid.size = (hif_key->attr.pmkid.numpmkid *
>>> PMKSA_KEY_LEN) + 1; -
>>> -		result = wilc_send_config_pkt(vif, SET_CFG, &wid,
>>> 1,
>>> -
>>> wilc_get_vif_idx(vif)); -
>>> -		kfree(key_buf);
>>> +		result = wilc_pmksa_key_copy(vif, hif_key);
>>>  		break;
>>>  	}
>>>  
>>>   
> 
> 
> 

Re: [PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()

[Dan Carpenter]

On Thu, May 10, 2018 at 08:21:52AM +0300, Claudiu Beznea wrote:
> >>> +	wid.val = (s8 *)key_buf;  
> >>
> >> Is this cast really needed?
> >>
> > 
> > This patch is only to address line over 80 chars checkpatch issue.
> > It is not good to club these change with type cast related
> > modification. For removing unnecessary cast we can submit
> > a separate patch series.
> > These are my views. What do you think?
> > 
> 
> I'm ok with this. I was thinking that since you introduced this new function
> you may want to also address this, if any.

No.  Please don't.  That kind of thing is sort of an unrelated change
from just moving the lines around and it messes up my review scripts.

It's really really easy to review the changes when they're split into
multiple chunks.

regards,
dan carpenter

[PATCH 04/30] staging: wilc1000: fix line over 80 characters issue in handle_connect()

[Ajay Singh]

Fix line over 80 character issue found by checkpatch.pl script by
aligning the input argument in function call.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 29f9907..55a61d5 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1117,10 +1117,8 @@ static s32 handle_connect(struct wilc_vif *vif,
 			}
 
 			conn_attr->result(CONN_DISCONN_EVENT_CONN_RESP,
-							       &conn_info,
-							       MAC_STATUS_DISCONNECTED,
-							       NULL,
-							       conn_attr->arg);
+					  &conn_info, MAC_STATUS_DISCONNECTED,
+					  NULL, conn_attr->arg);
 			hif_drv->hif_state = HOST_IF_IDLE;
 			kfree(conn_info.req_ies);
 			conn_info.req_ies = NULL;
-- 
2.7.4

[PATCH 05/30] staging: wilc1000: fix line over 80 chars in host_int_parse_assoc_resp_info()

[Ajay Singh]

Fix line over 80 characters issue reported by checkpatch.pl in
host_int_parse_assoc_resp_info().

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 37 ++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 55a61d5..a341ff1 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1293,6 +1293,23 @@ static inline void host_int_free_user_conn_req(struct host_if_drv *hif_drv)
 	hif_drv->usr_conn_req.ies = NULL;
 }
 
+static void host_int_copy_conn_info(struct connect_resp_info *conn_resp_info,
+				    struct connect_info *conn_info)
+{
+	conn_info->status = conn_resp_info->status;
+
+	if (conn_info->status == SUCCESSFUL_STATUSCODE && conn_resp_info->ies) {
+		conn_info->resp_ies = kmemdup(conn_resp_info->ies,
+					      conn_resp_info->ies_len,
+					      GFP_KERNEL);
+		if (conn_info->resp_ies)
+			conn_info->resp_ies_len = conn_resp_info->ies_len;
+	}
+
+	kfree(conn_resp_info->ies);
+	kfree(conn_resp_info);
+}
+
 static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
 						  u8 mac_status)
 {
@@ -1316,25 +1333,13 @@ static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
 
 			err = wilc_parse_assoc_resp_info(rcv_assoc_resp, rcvd_assoc_resp_info_len,
 							 &connect_resp_info);
-			if (err) {
+			if (err)
 				netdev_err(vif->ndev,
 					   "wilc_parse_assoc_resp_info() returned error %d\n",
 					   err);
-			} else {
-				conn_info.status = connect_resp_info->status;
-
-				if (conn_info.status == SUCCESSFUL_STATUSCODE &&
-				    connect_resp_info->ies) {
-					conn_info.resp_ies = kmemdup(connect_resp_info->ies,
-								     connect_resp_info->ies_len,
-								     GFP_KERNEL);
-					if (conn_info.resp_ies)
-						conn_info.resp_ies_len = connect_resp_info->ies_len;
-				}
-
-				kfree(connect_resp_info->ies);
-				kfree(connect_resp_info);
-			}
+			else
+				host_int_copy_conn_info(connect_resp_info,
+							&conn_info);
 		}
 	}
 
-- 
2.7.4

Re: [PATCH 05/30] staging: wilc1000: fix line over 80 chars in host_int_parse_assoc_resp_info()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Fix line over 80 characters issue reported by checkpatch.pl in
> host_int_parse_assoc_resp_info().
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 37 ++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 55a61d5..a341ff1 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -1293,6 +1293,23 @@ static inline void host_int_free_user_conn_req(struct host_if_drv *hif_drv)
>  	hif_drv->usr_conn_req.ies = NULL;
>  }
>  
> +static void host_int_copy_conn_info(struct connect_resp_info *conn_resp_info,
> +				    struct connect_info *conn_info)
> +{
> +	conn_info->status = conn_resp_info->status;
> +
> +	if (conn_info->status == SUCCESSFUL_STATUSCODE && conn_resp_info->ies) {
> +		conn_info->resp_ies = kmemdup(conn_resp_info->ies,
> +					      conn_resp_info->ies_len,
> +					      GFP_KERNEL);
> +		if (conn_info->resp_ies)
> +			conn_info->resp_ies_len = conn_resp_info->ies_len;
> +	}
> +
> +	kfree(conn_resp_info->ies);
> +	kfree(conn_resp_info);
> +}
> +

Instead of adding new function why not using wilc_parse_assoc_resp_info() to
return you the conn_info that you are copying it here? The connect_resp_info
object that you are passing to wilc_parse_assoc_resp_info() is not used 
anywhere in host_int_parse_assoc_resp_info() it is used only to copy again
from it to conn_info object.

Also, in wilc_parse_assoc_resp_info() you can get rid of these lines:
connect_resp_info->capability = get_assoc_resp_cap_info(buffer);
connect_resp_info->assoc_id = get_asoc_id(buffer);              


>  static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
>  						  u8 mac_status)
>  {
> @@ -1316,25 +1333,13 @@ static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
>  
>  			err = wilc_parse_assoc_resp_info(rcv_assoc_resp, rcvd_assoc_resp_info_len,
>  							 &connect_resp_info);
> -			if (err) {
> +			if (err)
>  				netdev_err(vif->ndev,
>  					   "wilc_parse_assoc_resp_info() returned error %d\n",
>  					   err);
> -			} else {
> -				conn_info.status = connect_resp_info->status;
> -
> -				if (conn_info.status == SUCCESSFUL_STATUSCODE &&
> -				    connect_resp_info->ies) {
> -					conn_info.resp_ies = kmemdup(connect_resp_info->ies,
> -								     connect_resp_info->ies_len,
> -								     GFP_KERNEL);
> -					if (conn_info.resp_ies)
> -						conn_info.resp_ies_len = connect_resp_info->ies_len;
> -				}
> -
> -				kfree(connect_resp_info->ies);
> -				kfree(connect_resp_info);
> -			}
> +			else
> +				host_int_copy_conn_info(connect_resp_info,
> +							&conn_info);
>  		}
>  	}
>  
> 

Re: [PATCH 05/30] staging: wilc1000: fix line over 80 chars in host_int_parse_assoc_resp_info()

[Ajay Singh]

On Wed, 9 May 2018 16:44:38 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Fix line over 80 characters issue reported by checkpatch.pl in
> > host_int_parse_assoc_resp_info().
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 37
> > ++++++++++++++++++------------- 1 file changed, 21 insertions(+),
> > 16 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index 55a61d5..a341ff1
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -1293,6 +1293,23 @@ static inline void
> > host_int_free_user_conn_req(struct host_if_drv *hif_drv)
> > hif_drv->usr_conn_req.ies = NULL; }
> >  
> > +static void host_int_copy_conn_info(struct connect_resp_info
> > *conn_resp_info,
> > +				    struct connect_info *conn_info)
> > +{
> > +	conn_info->status = conn_resp_info->status;
> > +
> > +	if (conn_info->status == SUCCESSFUL_STATUSCODE &&
> > conn_resp_info->ies) {
> > +		conn_info->resp_ies = kmemdup(conn_resp_info->ies,
> > +
> > conn_resp_info->ies_len,
> > +					      GFP_KERNEL);
> > +		if (conn_info->resp_ies)
> > +			conn_info->resp_ies_len =
> > conn_resp_info->ies_len;
> > +	}
> > +
> > +	kfree(conn_resp_info->ies);
> > +	kfree(conn_resp_info);
> > +}
> > +  
> 
> Instead of adding new function why not using
> wilc_parse_assoc_resp_info() to return you the conn_info that you are
> copying it here? The connect_resp_info object that you are passing to
> wilc_parse_assoc_resp_info() is not used anywhere in
> host_int_parse_assoc_resp_info() it is used only to copy again from
> it to conn_info object.

Yes, there are different function because the purpose is different.
Primarily wilc_parse_assoc_resp_info() is to take care of parsing and
filing the response info and later connect_resp_info is used to fill
conn_info values. The elements in conn_info and connect_resp_info are
different, but now most of the fields are not used in connect_resp_info.
Returning 'conn_info' from wilc_parse_assoc_resp_info() might not be
correct. I will check if we can go away with connect_resp_info
structure itself as most of the field are not used in it.

> 
> Also, in wilc_parse_assoc_resp_info() you can get rid of these lines:
> connect_resp_info->capability = get_assoc_resp_cap_info(buffer);
> connect_resp_info->assoc_id = get_asoc_id(buffer);              
> 

With above changes it can be taken care.

> 
> >  static inline void host_int_parse_assoc_resp_info(struct wilc_vif
> > *vif, u8 mac_status)
> >  {
> > @@ -1316,25 +1333,13 @@ static inline void
> > host_int_parse_assoc_resp_info(struct wilc_vif *vif, 
> >  			err =
> > wilc_parse_assoc_resp_info(rcv_assoc_resp,
> > rcvd_assoc_resp_info_len, &connect_resp_info);
> > -			if (err) {
> > +			if (err)
> >  				netdev_err(vif->ndev,
> >  					   "wilc_parse_assoc_resp_info()
> > returned error %d\n", err);
> > -			} else {
> > -				conn_info.status =
> > connect_resp_info->status; -
> > -				if (conn_info.status ==
> > SUCCESSFUL_STATUSCODE &&
> > -				    connect_resp_info->ies) {
> > -					conn_info.resp_ies =
> > kmemdup(connect_resp_info->ies,
> > -
> > connect_resp_info->ies_len,
> > -
> > GFP_KERNEL);
> > -					if (conn_info.resp_ies)
> > -
> > conn_info.resp_ies_len = connect_resp_info->ies_len;
> > -				}
> > -
> > -				kfree(connect_resp_info->ies);
> > -				kfree(connect_resp_info);
> > -			}
> > +			else
> > +
> > host_int_copy_conn_info(connect_resp_info,
> > +
> > &conn_info); }
> >  	}
> >  
> >   

[PATCH 06/30] staging: wilc1000: fix line over 80 chars issue in host_int_handle_disconnect()

[Ajay Singh]

Fix line over 80 char issue in host_int_handle_disconnect() by using
temp variable to hold the 'wilc_connect_result' function pointer.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index a341ff1..0d84af9 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1401,6 +1401,7 @@ static inline void host_int_handle_disconnect(struct wilc_vif *vif)
 {
 	struct disconnect_info disconn_info;
 	struct host_if_drv *hif_drv = vif->hif_drv;
+	wilc_connect_result conn_result = hif_drv->usr_conn_req.conn_result;
 
 	memset(&disconn_info, 0, sizeof(struct disconnect_info));
 
@@ -1413,13 +1414,12 @@ static inline void host_int_handle_disconnect(struct wilc_vif *vif)
 	disconn_info.ie = NULL;
 	disconn_info.ie_len = 0;
 
-	if (hif_drv->usr_conn_req.conn_result) {
+	if (conn_result) {
 		wilc_optaining_ip = false;
 		wilc_set_power_mgmt(vif, 0, 0);
 
-		hif_drv->usr_conn_req.conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF,
-						  NULL, 0, &disconn_info,
-						  hif_drv->usr_conn_req.arg);
+		conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF, NULL, 0,
+			    &disconn_info, hif_drv->usr_conn_req.arg);
 	} else {
 		netdev_err(vif->ndev, "Connect result NULL\n");
 	}
-- 
2.7.4

Re: [PATCH 06/30] staging: wilc1000: fix line over 80 chars issue in host_int_handle_disconnect()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Fix line over 80 char issue in host_int_handle_disconnect() by using
> temp variable to hold the 'wilc_connect_result' function pointer.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index a341ff1..0d84af9 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -1401,6 +1401,7 @@ static inline void host_int_handle_disconnect(struct wilc_vif *vif)
>  {
>  	struct disconnect_info disconn_info;
>  	struct host_if_drv *hif_drv = vif->hif_drv;
> +	wilc_connect_result conn_result = hif_drv->usr_conn_req.conn_result;
>  

Is there a scenario you can end up here with a null hif_drv? I'm seeing that some
of the functions from this layer are checking for hif_drv at the beginning, some
are not.


>  	memset(&disconn_info, 0, sizeof(struct disconnect_info));
>  
> @@ -1413,13 +1414,12 @@ static inline void host_int_handle_disconnect(struct wilc_vif *vif)
>  	disconn_info.ie = NULL;
>  	disconn_info.ie_len = 0;
>  
> -	if (hif_drv->usr_conn_req.conn_result) {
> +	if (conn_result) {
>  		wilc_optaining_ip = false;
>  		wilc_set_power_mgmt(vif, 0, 0);
>  
> -		hif_drv->usr_conn_req.conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF,
> -						  NULL, 0, &disconn_info,
> -						  hif_drv->usr_conn_req.arg);
> +		conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF, NULL, 0,
> +			    &disconn_info, hif_drv->usr_conn_req.arg);
>  	} else {
>  		netdev_err(vif->ndev, "Connect result NULL\n");
>  	}
> 

Re: [PATCH 06/30] staging: wilc1000: fix line over 80 chars issue in host_int_handle_disconnect()

[Ajay Singh]

On Wed, 9 May 2018 16:44:13 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Fix line over 80 char issue in host_int_handle_disconnect() by using
> > temp variable to hold the 'wilc_connect_result' function pointer.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index a341ff1..0d84af9
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -1401,6 +1401,7 @@ static inline void
> > host_int_handle_disconnect(struct wilc_vif *vif) {
> >  	struct disconnect_info disconn_info;
> >  	struct host_if_drv *hif_drv = vif->hif_drv;
> > +	wilc_connect_result conn_result =
> > hif_drv->usr_conn_req.conn_result; 
> 
> Is there a scenario you can end up here with a null hif_drv? I'm
> seeing that some of the functions from this layer are checking for
> hif_drv at the beginning, some are not.
>

Before calling host_int_handle_disconnect() there is already a NULL
check in handle_rcvd_gnrl_async_info(), so I think we don't need to add
another check here. In handle_scan_done() there is NULL check
inside the function because its also called from different places 
without NULL check.

> 
> >  	memset(&disconn_info, 0, sizeof(struct disconnect_info));
> >  
> > @@ -1413,13 +1414,12 @@ static inline void
> > host_int_handle_disconnect(struct wilc_vif *vif) disconn_info.ie =
> > NULL; disconn_info.ie_len = 0;
> >  
> > -	if (hif_drv->usr_conn_req.conn_result) {
> > +	if (conn_result) {
> >  		wilc_optaining_ip = false;
> >  		wilc_set_power_mgmt(vif, 0, 0);
> >  
> > -
> > hif_drv->usr_conn_req.conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF,
> > -						  NULL, 0,
> > &disconn_info,
> > -
> > hif_drv->usr_conn_req.arg);
> > +		conn_result(CONN_DISCONN_EVENT_DISCONN_NOTIF,
> > NULL, 0,
> > +			    &disconn_info,
> > hif_drv->usr_conn_req.arg); } else {
> >  		netdev_err(vif->ndev, "Connect result NULL\n");
> >  	}
> >   

[PATCH 07/30] staging: wilc1000: fix line over 80 characters in host_int_parse_join_bss_param()

[Ajay Singh]

Split host_int_parse_join_bss_param() to avoid the line over 80
character issue reported by checkpatch.pl script.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 247 ++++++++++++++++--------------
 1 file changed, 131 insertions(+), 116 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 0d84af9..c9c5d352 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -3856,150 +3856,165 @@ int wilc_setup_multicast_filter(struct wilc_vif *vif, bool enabled,
 	return result;
 }
 
-static void *host_int_parse_join_bss_param(struct network_info *info)
+static void host_int_fill_join_bss_param(struct join_bss_param *param, u8 *ies,
+					 u16 *out_index, u8 *pcipher_tc,
+					 u8 *auth_total_cnt, u32 tsf_lo)
 {
-	struct join_bss_param *param = NULL;
-	u8 *ies;
-	u16 ies_len;
-	u16 index = 0;
 	u8 rates_no = 0;
 	u8 ext_rates_no;
 	u16 offset;
 	u8 pcipher_cnt;
 	u8 auth_cnt;
-	u8 pcipher_total_cnt = 0;
-	u8 auth_total_cnt = 0;
 	u8 i, j;
+	u16 index = *out_index;
 
-	ies = info->ies;
-	ies_len = info->ies_len;
+	if (ies[index] == SUPP_RATES_IE) {
+		rates_no = ies[index + 1];
+		param->supp_rates[0] = rates_no;
+		index += 2;
 
-	param = kzalloc(sizeof(*param), GFP_KERNEL);
-	if (!param)
-		return NULL;
+		for (i = 0; i < rates_no; i++)
+			param->supp_rates[i + 1] = ies[index + i];
 
-	param->dtim_period = info->dtim_period;
-	param->beacon_period = info->beacon_period;
-	param->cap_info = info->cap_info;
-	memcpy(param->bssid, info->bssid, 6);
-	memcpy((u8 *)param->ssid, info->ssid, info->ssid_len + 1);
-	param->ssid_len = info->ssid_len;
-	memset(param->rsn_pcip_policy, 0xFF, 3);
-	memset(param->rsn_auth_policy, 0xFF, 3);
+		index += rates_no;
+	} else if (ies[index] == EXT_SUPP_RATES_IE) {
+		ext_rates_no = ies[index + 1];
+		if (ext_rates_no > (MAX_RATES_SUPPORTED - rates_no))
+			param->supp_rates[0] = MAX_RATES_SUPPORTED;
+		else
+			param->supp_rates[0] += ext_rates_no;
+		index += 2;
+		for (i = 0; i < (param->supp_rates[0] - rates_no); i++)
+			param->supp_rates[rates_no + i + 1] = ies[index + i];
+
+		index += ext_rates_no;
+	} else if (ies[index] == HT_CAPABILITY_IE) {
+		param->ht_capable = true;
+		index += ies[index + 1] + 2;
+	} else if ((ies[index] == WMM_IE) &&
+		   (ies[index + 2] == 0x00) && (ies[index + 3] == 0x50) &&
+		   (ies[index + 4] == 0xF2) && (ies[index + 5] == 0x02) &&
+		   ((ies[index + 6] == 0x00) || (ies[index + 6] == 0x01)) &&
+		   (ies[index + 7] == 0x01)) {
+		param->wmm_cap = true;
+
+		if (ies[index + 8] & BIT(7))
+			param->uapsd_cap = true;
+		index += ies[index + 1] + 2;
+	} else if ((ies[index] == P2P_IE) &&
+		 (ies[index + 2] == 0x50) && (ies[index + 3] == 0x6f) &&
+		 (ies[index + 4] == 0x9a) &&
+		 (ies[index + 5] == 0x09) && (ies[index + 6] == 0x0c)) {
+		u16 p2p_cnt;
+
+		param->tsf = tsf_lo;
+		param->noa_enabled = 1;
+		param->idx = ies[index + 9];
+
+		if (ies[index + 10] & BIT(7)) {
+			param->opp_enabled = 1;
+			param->ct_window = ies[index + 10];
+		} else {
+			param->opp_enabled = 0;
+		}
 
-	while (index < ies_len) {
-		if (ies[index] == SUPP_RATES_IE) {
-			rates_no = ies[index + 1];
-			param->supp_rates[0] = rates_no;
-			index += 2;
+		param->cnt = ies[index + 11];
+		p2p_cnt = index + 12;
 
-			for (i = 0; i < rates_no; i++)
-				param->supp_rates[i + 1] = ies[index + i];
+		memcpy(param->duration, ies + p2p_cnt, 4);
+		p2p_cnt += 4;
 
-			index += rates_no;
-		} else if (ies[index] == EXT_SUPP_RATES_IE) {
-			ext_rates_no = ies[index + 1];
-			if (ext_rates_no > (MAX_RATES_SUPPORTED - rates_no))
-				param->supp_rates[0] = MAX_RATES_SUPPORTED;
-			else
-				param->supp_rates[0] += ext_rates_no;
-			index += 2;
-			for (i = 0; i < (param->supp_rates[0] - rates_no); i++)
-				param->supp_rates[rates_no + i + 1] = ies[index + i];
-
-			index += ext_rates_no;
-		} else if (ies[index] == HT_CAPABILITY_IE) {
-			param->ht_capable = true;
-			index += ies[index + 1] + 2;
-		} else if ((ies[index] == WMM_IE) &&
-			   (ies[index + 2] == 0x00) && (ies[index + 3] == 0x50) &&
-			   (ies[index + 4] == 0xF2) &&
-			   (ies[index + 5] == 0x02) &&
-			   ((ies[index + 6] == 0x00) || (ies[index + 6] == 0x01)) &&
-			   (ies[index + 7] == 0x01)) {
-			param->wmm_cap = true;
-
-			if (ies[index + 8] & BIT(7))
-				param->uapsd_cap = true;
-			index += ies[index + 1] + 2;
-		} else if ((ies[index] == P2P_IE) &&
-			 (ies[index + 2] == 0x50) && (ies[index + 3] == 0x6f) &&
-			 (ies[index + 4] == 0x9a) &&
-			 (ies[index + 5] == 0x09) && (ies[index + 6] == 0x0c)) {
-			u16 p2p_cnt;
-
-			param->tsf = info->tsf_lo;
-			param->noa_enabled = 1;
-			param->idx = ies[index + 9];
-
-			if (ies[index + 10] & BIT(7)) {
-				param->opp_enabled = 1;
-				param->ct_window = ies[index + 10];
-			} else {
-				param->opp_enabled = 0;
-			}
+		memcpy(param->interval, ies + p2p_cnt, 4);
+		p2p_cnt += 4;
 
-			param->cnt = ies[index + 11];
-			p2p_cnt = index + 12;
+		memcpy(param->start_time, ies + p2p_cnt, 4);
 
-			memcpy(param->duration, ies + p2p_cnt, 4);
-			p2p_cnt += 4;
+		index += ies[index + 1] + 2;
+	} else if ((ies[index] == RSN_IE) ||
+		 ((ies[index] == WPA_IE) && (ies[index + 2] == 0x00) &&
+		  (ies[index + 3] == 0x50) && (ies[index + 4] == 0xF2) &&
+		  (ies[index + 5] == 0x01))) {
+		u16 rsn_idx = index;
 
-			memcpy(param->interval, ies + p2p_cnt, 4);
-			p2p_cnt += 4;
+		if (ies[rsn_idx] == RSN_IE) {
+			param->mode_802_11i = 2;
+		} else {
+			if (param->mode_802_11i == 0)
+				param->mode_802_11i = 1;
+			rsn_idx += 4;
+		}
 
-			memcpy(param->start_time, ies + p2p_cnt, 4);
+		rsn_idx += 7;
+		param->rsn_grp_policy = ies[rsn_idx];
+		rsn_idx++;
+		offset = ies[rsn_idx] * 4;
+		pcipher_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
+		rsn_idx += 2;
 
-			index += ies[index + 1] + 2;
-		} else if ((ies[index] == RSN_IE) ||
-			 ((ies[index] == WPA_IE) && (ies[index + 2] == 0x00) &&
-			  (ies[index + 3] == 0x50) && (ies[index + 4] == 0xF2) &&
-			  (ies[index + 5] == 0x01)))	{
-			u16 rsn_idx = index;
+		i = *pcipher_tc;
+		j = 0;
+		for (; i < (pcipher_cnt + *pcipher_tc) && i < 3; i++, j++) {
+			u8 *policy =  &param->rsn_pcip_policy[i];
 
-			if (ies[rsn_idx] == RSN_IE)	{
-				param->mode_802_11i = 2;
-			} else {
-				if (param->mode_802_11i == 0)
-					param->mode_802_11i = 1;
-				rsn_idx += 4;
-			}
+			*policy = ies[rsn_idx + ((j + 1) * 4) - 1];
+		}
 
-			rsn_idx += 7;
-			param->rsn_grp_policy = ies[rsn_idx];
-			rsn_idx++;
-			offset = ies[rsn_idx] * 4;
-			pcipher_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
-			rsn_idx += 2;
+		*pcipher_tc += pcipher_cnt;
+		rsn_idx += offset;
 
-			for (i = pcipher_total_cnt, j = 0; i < pcipher_cnt + pcipher_total_cnt && i < 3; i++, j++)
-				param->rsn_pcip_policy[i] = ies[rsn_idx + ((j + 1) * 4) - 1];
+		offset = ies[rsn_idx] * 4;
 
-			pcipher_total_cnt += pcipher_cnt;
-			rsn_idx += offset;
+		auth_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
+		rsn_idx += 2;
+		i = *auth_total_cnt;
+		j = 0;
+		for (; i < (*auth_total_cnt + auth_cnt); i++, j++) {
+			u8 *policy =  &param->rsn_auth_policy[i];
 
-			offset = ies[rsn_idx] * 4;
+			*policy = ies[rsn_idx + ((j + 1) * 4) - 1];
+		}
+
+		auth_total_cnt += auth_cnt;
+		rsn_idx += offset;
 
-			auth_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
+		if (ies[index] == RSN_IE) {
+			param->rsn_cap[0] = ies[rsn_idx];
+			param->rsn_cap[1] = ies[rsn_idx + 1];
 			rsn_idx += 2;
+		}
+		param->rsn_found = true;
+		index += ies[index + 1] + 2;
+	} else {
+		index += ies[index + 1] + 2;
+	}
 
-			for (i = auth_total_cnt, j = 0; i < auth_total_cnt + auth_cnt; i++, j++)
-				param->rsn_auth_policy[i] = ies[rsn_idx + ((j + 1) * 4) - 1];
+	*out_index = index;
+}
 
-			auth_total_cnt += auth_cnt;
-			rsn_idx += offset;
+static void *host_int_parse_join_bss_param(struct network_info *info)
+{
+	struct join_bss_param *param = NULL;
+	u16 index = 0;
+	u8 pcipher_total_cnt = 0;
+	u8 auth_total_cnt = 0;
 
-			if (ies[index] == RSN_IE) {
-				param->rsn_cap[0] = ies[rsn_idx];
-				param->rsn_cap[1] = ies[rsn_idx + 1];
-				rsn_idx += 2;
-			}
-			param->rsn_found = true;
-			index += ies[index + 1] + 2;
-		} else {
-			index += ies[index + 1] + 2;
-		}
-	}
+	param = kzalloc(sizeof(*param), GFP_KERNEL);
+	if (!param)
+		return NULL;
+
+	param->dtim_period = info->dtim_period;
+	param->beacon_period = info->beacon_period;
+	param->cap_info = info->cap_info;
+	memcpy(param->bssid, info->bssid, 6);
+	memcpy((u8 *)param->ssid, info->ssid, info->ssid_len + 1);
+	param->ssid_len = info->ssid_len;
+	memset(param->rsn_pcip_policy, 0xFF, 3);
+	memset(param->rsn_auth_policy, 0xFF, 3);
+
+	while (index < info->ies_len)
+		host_int_fill_join_bss_param(param, info->ies, &index,
+					     &pcipher_total_cnt,
+					     &auth_total_cnt, info->tsf_lo);
 
 	return (void *)param;
 }
-- 
2.7.4

Re: [PATCH 07/30] staging: wilc1000: fix line over 80 characters in host_int_parse_join_bss_param()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Split host_int_parse_join_bss_param() to avoid the line over 80
> character issue reported by checkpatch.pl script.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 247 ++++++++++++++++--------------
>  1 file changed, 131 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 0d84af9..c9c5d352 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -3856,150 +3856,165 @@ int wilc_setup_multicast_filter(struct wilc_vif *vif, bool enabled,
>  	return result;
>  }
>  
> -static void *host_int_parse_join_bss_param(struct network_info *info)
> +static void host_int_fill_join_bss_param(struct join_bss_param *param, u8 *ies,
> +					 u16 *out_index, u8 *pcipher_tc,
> +					 u8 *auth_total_cnt, u32 tsf_lo)
>  {
> -	struct join_bss_param *param = NULL;
> -	u8 *ies;
> -	u16 ies_len;
> -	u16 index = 0;
>  	u8 rates_no = 0;
>  	u8 ext_rates_no;
>  	u16 offset;
>  	u8 pcipher_cnt;
>  	u8 auth_cnt;
> -	u8 pcipher_total_cnt = 0;
> -	u8 auth_total_cnt = 0;
>  	u8 i, j;
> +	u16 index = *out_index;

Why not having a single index, the one passed as argument?

>  
> -	ies = info->ies;
> -	ies_len = info->ies_len;
> +	if (ies[index] == SUPP_RATES_IE) {
> +		rates_no = ies[index + 1];
> +		param->supp_rates[0] = rates_no;
> +		index += 2;
>  
> -	param = kzalloc(sizeof(*param), GFP_KERNEL);
> -	if (!param)
> -		return NULL;
> +		for (i = 0; i < rates_no; i++)
> +			param->supp_rates[i + 1] = ies[index + i];
>  
> -	param->dtim_period = info->dtim_period;
> -	param->beacon_period = info->beacon_period;
> -	param->cap_info = info->cap_info;
> -	memcpy(param->bssid, info->bssid, 6);
> -	memcpy((u8 *)param->ssid, info->ssid, info->ssid_len + 1);
> -	param->ssid_len = info->ssid_len;
> -	memset(param->rsn_pcip_policy, 0xFF, 3);
> -	memset(param->rsn_auth_policy, 0xFF, 3);
> +		index += rates_no;
> +	} else if (ies[index] == EXT_SUPP_RATES_IE) {
> +		ext_rates_no = ies[index + 1];
> +		if (ext_rates_no > (MAX_RATES_SUPPORTED - rates_no))
> +			param->supp_rates[0] = MAX_RATES_SUPPORTED;
> +		else
> +			param->supp_rates[0] += ext_rates_no;
> +		index += 2;
> +		for (i = 0; i < (param->supp_rates[0] - rates_no); i++)
> +			param->supp_rates[rates_no + i + 1] = ies[index + i];
> +
> +		index += ext_rates_no;
> +	} else if (ies[index] == HT_CAPABILITY_IE) {
> +		param->ht_capable = true;
> +		index += ies[index + 1] + 2;
> +	} else if ((ies[index] == WMM_IE) &&
> +		   (ies[index + 2] == 0x00) && (ies[index + 3] == 0x50) &&
> +		   (ies[index + 4] == 0xF2) && (ies[index + 5] == 0x02) &&
> +		   ((ies[index + 6] == 0x00) || (ies[index + 6] == 0x01)) &&
> +		   (ies[index + 7] == 0x01)) {
> +		param->wmm_cap = true;
> +
> +		if (ies[index + 8] & BIT(7))
> +			param->uapsd_cap = true;
> +		index += ies[index + 1] + 2;
> +	} else if ((ies[index] == P2P_IE) &&
> +		 (ies[index + 2] == 0x50) && (ies[index + 3] == 0x6f) &&
> +		 (ies[index + 4] == 0x9a) &&
> +		 (ies[index + 5] == 0x09) && (ies[index + 6] == 0x0c)) {
> +		u16 p2p_cnt;
> +
> +		param->tsf = tsf_lo;
> +		param->noa_enabled = 1;
> +		param->idx = ies[index + 9];
> +
> +		if (ies[index + 10] & BIT(7)) {
> +			param->opp_enabled = 1;
> +			param->ct_window = ies[index + 10];
> +		} else {
> +			param->opp_enabled = 0;
> +		}
>  
> -	while (index < ies_len) {
> -		if (ies[index] == SUPP_RATES_IE) {
> -			rates_no = ies[index + 1];
> -			param->supp_rates[0] = rates_no;
> -			index += 2;
> +		param->cnt = ies[index + 11];
> +		p2p_cnt = index + 12;
>  
> -			for (i = 0; i < rates_no; i++)
> -				param->supp_rates[i + 1] = ies[index + i];
> +		memcpy(param->duration, ies + p2p_cnt, 4);
> +		p2p_cnt += 4;
>  
> -			index += rates_no;
> -		} else if (ies[index] == EXT_SUPP_RATES_IE) {
> -			ext_rates_no = ies[index + 1];
> -			if (ext_rates_no > (MAX_RATES_SUPPORTED - rates_no))
> -				param->supp_rates[0] = MAX_RATES_SUPPORTED;
> -			else
> -				param->supp_rates[0] += ext_rates_no;
> -			index += 2;
> -			for (i = 0; i < (param->supp_rates[0] - rates_no); i++)
> -				param->supp_rates[rates_no + i + 1] = ies[index + i];
> -
> -			index += ext_rates_no;
> -		} else if (ies[index] == HT_CAPABILITY_IE) {
> -			param->ht_capable = true;
> -			index += ies[index + 1] + 2;
> -		} else if ((ies[index] == WMM_IE) &&
> -			   (ies[index + 2] == 0x00) && (ies[index + 3] == 0x50) &&
> -			   (ies[index + 4] == 0xF2) &&
> -			   (ies[index + 5] == 0x02) &&
> -			   ((ies[index + 6] == 0x00) || (ies[index + 6] == 0x01)) &&
> -			   (ies[index + 7] == 0x01)) {
> -			param->wmm_cap = true;
> -
> -			if (ies[index + 8] & BIT(7))
> -				param->uapsd_cap = true;
> -			index += ies[index + 1] + 2;
> -		} else if ((ies[index] == P2P_IE) &&
> -			 (ies[index + 2] == 0x50) && (ies[index + 3] == 0x6f) &&
> -			 (ies[index + 4] == 0x9a) &&
> -			 (ies[index + 5] == 0x09) && (ies[index + 6] == 0x0c)) {
> -			u16 p2p_cnt;
> -
> -			param->tsf = info->tsf_lo;
> -			param->noa_enabled = 1;
> -			param->idx = ies[index + 9];
> -
> -			if (ies[index + 10] & BIT(7)) {
> -				param->opp_enabled = 1;
> -				param->ct_window = ies[index + 10];
> -			} else {
> -				param->opp_enabled = 0;
> -			}
> +		memcpy(param->interval, ies + p2p_cnt, 4);
> +		p2p_cnt += 4;
>  
> -			param->cnt = ies[index + 11];
> -			p2p_cnt = index + 12;
> +		memcpy(param->start_time, ies + p2p_cnt, 4);
>  
> -			memcpy(param->duration, ies + p2p_cnt, 4);
> -			p2p_cnt += 4;
> +		index += ies[index + 1] + 2;
> +	} else if ((ies[index] == RSN_IE) ||
> +		 ((ies[index] == WPA_IE) && (ies[index + 2] == 0x00) &&
> +		  (ies[index + 3] == 0x50) && (ies[index + 4] == 0xF2) &&
> +		  (ies[index + 5] == 0x01))) {
> +		u16 rsn_idx = index;
>  
> -			memcpy(param->interval, ies + p2p_cnt, 4);
> -			p2p_cnt += 4;
> +		if (ies[rsn_idx] == RSN_IE) {
> +			param->mode_802_11i = 2;
> +		} else {
> +			if (param->mode_802_11i == 0)
> +				param->mode_802_11i = 1;
> +			rsn_idx += 4;
> +		}
>  
> -			memcpy(param->start_time, ies + p2p_cnt, 4);
> +		rsn_idx += 7;
> +		param->rsn_grp_policy = ies[rsn_idx];
> +		rsn_idx++;
> +		offset = ies[rsn_idx] * 4;
> +		pcipher_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
> +		rsn_idx += 2;
>  
> -			index += ies[index + 1] + 2;
> -		} else if ((ies[index] == RSN_IE) ||
> -			 ((ies[index] == WPA_IE) && (ies[index + 2] == 0x00) &&
> -			  (ies[index + 3] == 0x50) && (ies[index + 4] == 0xF2) &&
> -			  (ies[index + 5] == 0x01)))	{
> -			u16 rsn_idx = index;
> +		i = *pcipher_tc;
> +		j = 0;
> +		for (; i < (pcipher_cnt + *pcipher_tc) && i < 3; i++, j++) {
> +			u8 *policy =  &param->rsn_pcip_policy[i];
>  
> -			if (ies[rsn_idx] == RSN_IE)	{
> -				param->mode_802_11i = 2;
> -			} else {
> -				if (param->mode_802_11i == 0)
> -					param->mode_802_11i = 1;
> -				rsn_idx += 4;
> -			}
> +			*policy = ies[rsn_idx + ((j + 1) * 4) - 1];
> +		}
>  
> -			rsn_idx += 7;
> -			param->rsn_grp_policy = ies[rsn_idx];
> -			rsn_idx++;
> -			offset = ies[rsn_idx] * 4;
> -			pcipher_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
> -			rsn_idx += 2;
> +		*pcipher_tc += pcipher_cnt;
> +		rsn_idx += offset;
>  
> -			for (i = pcipher_total_cnt, j = 0; i < pcipher_cnt + pcipher_total_cnt && i < 3; i++, j++)
> -				param->rsn_pcip_policy[i] = ies[rsn_idx + ((j + 1) * 4) - 1];
> +		offset = ies[rsn_idx] * 4;
>  
> -			pcipher_total_cnt += pcipher_cnt;
> -			rsn_idx += offset;
> +		auth_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
> +		rsn_idx += 2;
> +		i = *auth_total_cnt;
> +		j = 0;

I prefer keeping these inside for (). I think some of the "line over 80" should 
be fixed by refactoring code not introducing new variables and so on.

> +		for (; i < (*auth_total_cnt + auth_cnt); i++, j++) {
> +			u8 *policy =  &param->rsn_auth_policy[i];
>  
> -			offset = ies[rsn_idx] * 4;
> +			*policy = ies[rsn_idx + ((j + 1) * 4) - 1];
> +		}
> +
> +		auth_total_cnt += auth_cnt;
> +		rsn_idx += offset;
>  
> -			auth_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
> +		if (ies[index] == RSN_IE) {
> +			param->rsn_cap[0] = ies[rsn_idx];
> +			param->rsn_cap[1] = ies[rsn_idx + 1];
>  			rsn_idx += 2;
> +		}
> +		param->rsn_found = true;
> +		index += ies[index + 1] + 2;
> +	} else {
> +		index += ies[index + 1] + 2;
> +	}
>  
> -			for (i = auth_total_cnt, j = 0; i < auth_total_cnt + auth_cnt; i++, j++)
> -				param->rsn_auth_policy[i] = ies[rsn_idx + ((j + 1) * 4) - 1];
> +	*out_index = index;
> +}
>  
> -			auth_total_cnt += auth_cnt;
> -			rsn_idx += offset;
> +static void *host_int_parse_join_bss_param(struct network_info *info)
> +{
> +	struct join_bss_param *param = NULL;
> +	u16 index = 0;
> +	u8 pcipher_total_cnt = 0;
> +	u8 auth_total_cnt = 0;
>  
> -			if (ies[index] == RSN_IE) {
> -				param->rsn_cap[0] = ies[rsn_idx];
> -				param->rsn_cap[1] = ies[rsn_idx + 1];
> -				rsn_idx += 2;
> -			}
> -			param->rsn_found = true;
> -			index += ies[index + 1] + 2;
> -		} else {
> -			index += ies[index + 1] + 2;
> -		}
> -	}
> +	param = kzalloc(sizeof(*param), GFP_KERNEL);
> +	if (!param)
> +		return NULL;
> +
> +	param->dtim_period = info->dtim_period;
> +	param->beacon_period = info->beacon_period;
> +	param->cap_info = info->cap_info;
> +	memcpy(param->bssid, info->bssid, 6);
> +	memcpy((u8 *)param->ssid, info->ssid, info->ssid_len + 1);
> +	param->ssid_len = info->ssid_len;
> +	memset(param->rsn_pcip_policy, 0xFF, 3);
> +	memset(param->rsn_auth_policy, 0xFF, 3);
> +
> +	while (index < info->ies_len)
> +		host_int_fill_join_bss_param(param, info->ies, &index,
> +					     &pcipher_total_cnt,
> +					     &auth_total_cnt, info->tsf_lo);
>  
>  	return (void *)param;
>  }
> 

Re: [PATCH 07/30] staging: wilc1000: fix line over 80 characters in host_int_parse_join_bss_param()

[Ajay Singh]

On Wed, 9 May 2018 16:43:59 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Split host_int_parse_join_bss_param() to avoid the line over 80
> > character issue reported by checkpatch.pl script.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 247
> > ++++++++++++++++-------------- 1 file changed, 131 insertions(+),
> > 116 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index 0d84af9..c9c5d352
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -3856,150 +3856,165 @@ int wilc_setup_multicast_filter(struct
> > wilc_vif *vif, bool enabled, return result;
> >  }
> >  
> > -static void *host_int_parse_join_bss_param(struct network_info
> > *info) +static void host_int_fill_join_bss_param(struct
> > join_bss_param *param, u8 *ies,
> > +					 u16 *out_index, u8
> > *pcipher_tc,
> > +					 u8 *auth_total_cnt, u32
> > tsf_lo) {
> > -	struct join_bss_param *param = NULL;
> > -	u8 *ies;
> > -	u16 ies_len;
> > -	u16 index = 0;
> >  	u8 rates_no = 0;
> >  	u8 ext_rates_no;
> >  	u16 offset;
> >  	u8 pcipher_cnt;
> >  	u8 auth_cnt;
> > -	u8 pcipher_total_cnt = 0;
> > -	u8 auth_total_cnt = 0;
> >  	u8 i, j;
> > +	u16 index = *out_index;  
> 
> Why not having a single index, the one passed as argument?
> 

If we use 'index' argument then we have to change all the lines below
with '*index' and it would make code look more complicated. So to
reduce those changes below I have the above logic.

> >  
> > -	ies = info->ies;
> > -	ies_len = info->ies_len;
> > +	if (ies[index] == SUPP_RATES_IE) {
> > +		rates_no = ies[index + 1];
> > +		param->supp_rates[0] = rates_no;
> > +		index += 2;
> >  
> > -	param = kzalloc(sizeof(*param), GFP_KERNEL);
> > -	if (!param)
> > -		return NULL;
> > +		for (i = 0; i < rates_no; i++)
> > +			param->supp_rates[i + 1] = ies[index + i];
> >  
> > -	param->dtim_period = info->dtim_period;
> > -	param->beacon_period = info->beacon_period;
> > -	param->cap_info = info->cap_info;
> > -	memcpy(param->bssid, info->bssid, 6);
> > -	memcpy((u8 *)param->ssid, info->ssid, info->ssid_len + 1);
> > -	param->ssid_len = info->ssid_len;
> > -	memset(param->rsn_pcip_policy, 0xFF, 3);
> > -	memset(param->rsn_auth_policy, 0xFF, 3);
> > +		index += rates_no;
> > +	} else if (ies[index] == EXT_SUPP_RATES_IE) {
> > +		ext_rates_no = ies[index + 1];
> > +		if (ext_rates_no > (MAX_RATES_SUPPORTED -
> > rates_no))
> > +			param->supp_rates[0] = MAX_RATES_SUPPORTED;
> > +		else
> > +			param->supp_rates[0] += ext_rates_no;
> > +		index += 2;
> > +		for (i = 0; i < (param->supp_rates[0] - rates_no);
> > i++)
> > +			param->supp_rates[rates_no + i + 1] =
> > ies[index + i]; +
> > +		index += ext_rates_no;
> > +	} else if (ies[index] == HT_CAPABILITY_IE) {
> > +		param->ht_capable = true;
> > +		index += ies[index + 1] + 2;
> > +	} else if ((ies[index] == WMM_IE) &&
> > +		   (ies[index + 2] == 0x00) && (ies[index + 3] ==
> > 0x50) &&
> > +		   (ies[index + 4] == 0xF2) && (ies[index + 5] ==
> > 0x02) &&
> > +		   ((ies[index + 6] == 0x00) || (ies[index + 6] ==
> > 0x01)) &&
> > +		   (ies[index + 7] == 0x01)) {
> > +		param->wmm_cap = true;
> > +
> > +		if (ies[index + 8] & BIT(7))
> > +			param->uapsd_cap = true;
> > +		index += ies[index + 1] + 2;
> > +	} else if ((ies[index] == P2P_IE) &&
> > +		 (ies[index + 2] == 0x50) && (ies[index + 3] ==
> > 0x6f) &&
> > +		 (ies[index + 4] == 0x9a) &&
> > +		 (ies[index + 5] == 0x09) && (ies[index + 6] ==
> > 0x0c)) {
> > +		u16 p2p_cnt;
> > +
> > +		param->tsf = tsf_lo;
> > +		param->noa_enabled = 1;
> > +		param->idx = ies[index + 9];
> > +
> > +		if (ies[index + 10] & BIT(7)) {
> > +			param->opp_enabled = 1;
> > +			param->ct_window = ies[index + 10];
> > +		} else {
> > +			param->opp_enabled = 0;
> > +		}
> >  
> > -	while (index < ies_len) {
> > -		if (ies[index] == SUPP_RATES_IE) {
> > -			rates_no = ies[index + 1];
> > -			param->supp_rates[0] = rates_no;
> > -			index += 2;
> > +		param->cnt = ies[index + 11];
> > +		p2p_cnt = index + 12;
> >  
> > -			for (i = 0; i < rates_no; i++)
> > -				param->supp_rates[i + 1] =
> > ies[index + i];
> > +		memcpy(param->duration, ies + p2p_cnt, 4);
> > +		p2p_cnt += 4;
> >  
> > -			index += rates_no;
> > -		} else if (ies[index] == EXT_SUPP_RATES_IE) {
> > -			ext_rates_no = ies[index + 1];
> > -			if (ext_rates_no > (MAX_RATES_SUPPORTED -
> > rates_no))
> > -				param->supp_rates[0] =
> > MAX_RATES_SUPPORTED;
> > -			else
> > -				param->supp_rates[0] +=
> > ext_rates_no;
> > -			index += 2;
> > -			for (i = 0; i < (param->supp_rates[0] -
> > rates_no); i++)
> > -				param->supp_rates[rates_no + i +
> > 1] = ies[index + i]; -
> > -			index += ext_rates_no;
> > -		} else if (ies[index] == HT_CAPABILITY_IE) {
> > -			param->ht_capable = true;
> > -			index += ies[index + 1] + 2;
> > -		} else if ((ies[index] == WMM_IE) &&
> > -			   (ies[index + 2] == 0x00) && (ies[index
> > + 3] == 0x50) &&
> > -			   (ies[index + 4] == 0xF2) &&
> > -			   (ies[index + 5] == 0x02) &&
> > -			   ((ies[index + 6] == 0x00) || (ies[index
> > + 6] == 0x01)) &&
> > -			   (ies[index + 7] == 0x01)) {
> > -			param->wmm_cap = true;
> > -
> > -			if (ies[index + 8] & BIT(7))
> > -				param->uapsd_cap = true;
> > -			index += ies[index + 1] + 2;
> > -		} else if ((ies[index] == P2P_IE) &&
> > -			 (ies[index + 2] == 0x50) && (ies[index +
> > 3] == 0x6f) &&
> > -			 (ies[index + 4] == 0x9a) &&
> > -			 (ies[index + 5] == 0x09) && (ies[index +
> > 6] == 0x0c)) {
> > -			u16 p2p_cnt;
> > -
> > -			param->tsf = info->tsf_lo;
> > -			param->noa_enabled = 1;
> > -			param->idx = ies[index + 9];
> > -
> > -			if (ies[index + 10] & BIT(7)) {
> > -				param->opp_enabled = 1;
> > -				param->ct_window = ies[index + 10];
> > -			} else {
> > -				param->opp_enabled = 0;
> > -			}
> > +		memcpy(param->interval, ies + p2p_cnt, 4);
> > +		p2p_cnt += 4;
> >  
> > -			param->cnt = ies[index + 11];
> > -			p2p_cnt = index + 12;
> > +		memcpy(param->start_time, ies + p2p_cnt, 4);
> >  
> > -			memcpy(param->duration, ies + p2p_cnt, 4);
> > -			p2p_cnt += 4;
> > +		index += ies[index + 1] + 2;
> > +	} else if ((ies[index] == RSN_IE) ||
> > +		 ((ies[index] == WPA_IE) && (ies[index + 2] ==
> > 0x00) &&
> > +		  (ies[index + 3] == 0x50) && (ies[index + 4] ==
> > 0xF2) &&
> > +		  (ies[index + 5] == 0x01))) {
> > +		u16 rsn_idx = index;
> >  
> > -			memcpy(param->interval, ies + p2p_cnt, 4);
> > -			p2p_cnt += 4;
> > +		if (ies[rsn_idx] == RSN_IE) {
> > +			param->mode_802_11i = 2;
> > +		} else {
> > +			if (param->mode_802_11i == 0)
> > +				param->mode_802_11i = 1;
> > +			rsn_idx += 4;
> > +		}
> >  
> > -			memcpy(param->start_time, ies + p2p_cnt,
> > 4);
> > +		rsn_idx += 7;
> > +		param->rsn_grp_policy = ies[rsn_idx];
> > +		rsn_idx++;
> > +		offset = ies[rsn_idx] * 4;
> > +		pcipher_cnt = (ies[rsn_idx] > 3) ? 3 :
> > ies[rsn_idx];
> > +		rsn_idx += 2;
> >  
> > -			index += ies[index + 1] + 2;
> > -		} else if ((ies[index] == RSN_IE) ||
> > -			 ((ies[index] == WPA_IE) && (ies[index +
> > 2] == 0x00) &&
> > -			  (ies[index + 3] == 0x50) && (ies[index +
> > 4] == 0xF2) &&
> > -			  (ies[index + 5] == 0x01)))	{
> > -			u16 rsn_idx = index;
> > +		i = *pcipher_tc;
> > +		j = 0;
> > +		for (; i < (pcipher_cnt + *pcipher_tc) && i < 3;
> > i++, j++) {
> > +			u8 *policy =  &param->rsn_pcip_policy[i];
> >  
> > -			if (ies[rsn_idx] == RSN_IE)	{
> > -				param->mode_802_11i = 2;
> > -			} else {
> > -				if (param->mode_802_11i == 0)
> > -					param->mode_802_11i = 1;
> > -				rsn_idx += 4;
> > -			}
> > +			*policy = ies[rsn_idx + ((j + 1) * 4) - 1];
> > +		}
> >  
> > -			rsn_idx += 7;
> > -			param->rsn_grp_policy = ies[rsn_idx];
> > -			rsn_idx++;
> > -			offset = ies[rsn_idx] * 4;
> > -			pcipher_cnt = (ies[rsn_idx] > 3) ? 3 :
> > ies[rsn_idx];
> > -			rsn_idx += 2;
> > +		*pcipher_tc += pcipher_cnt;
> > +		rsn_idx += offset;
> >  
> > -			for (i = pcipher_total_cnt, j = 0; i <
> > pcipher_cnt + pcipher_total_cnt && i < 3; i++, j++)
> > -				param->rsn_pcip_policy[i] =
> > ies[rsn_idx + ((j + 1) * 4) - 1];
> > +		offset = ies[rsn_idx] * 4;
> >  
> > -			pcipher_total_cnt += pcipher_cnt;
> > -			rsn_idx += offset;
> > +		auth_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
> > +		rsn_idx += 2;
> > +		i = *auth_total_cnt;
> > +		j = 0;  
> 
> I prefer keeping these inside for (). I think some of the "line over
> 80" should be fixed by refactoring code not introducing new variables
> and so on.

Do you think for the below line the 'line over 80 chars' can be solved
by refactoring alone and without using temp/intermediate variables
or short names of variables. 

The original 'for' loop line itself was about 65+ characters long. 

> 
> > +		for (; i < (*auth_total_cnt + auth_cnt); i++, j++)
> > {
> > +			u8 *policy =  &param->rsn_auth_policy[i];
> >  
> > -			offset = ies[rsn_idx] * 4;
> > +			*policy = ies[rsn_idx + ((j + 1) * 4) - 1];
> > +		}
> > +
> > +		auth_total_cnt += auth_cnt;
> > +		rsn_idx += offset;
> >  
> > -			auth_cnt = (ies[rsn_idx] > 3) ? 3 :
> > ies[rsn_idx];
> > +		if (ies[index] == RSN_IE) {
> > +			param->rsn_cap[0] = ies[rsn_idx];
> > +			param->rsn_cap[1] = ies[rsn_idx + 1];
> >  			rsn_idx += 2;
> > +		}
> > +		param->rsn_found = true;
> > +		index += ies[index + 1] + 2;
> > +	} else {
> > +		index += ies[index + 1] + 2;
> > +	}
> >  
> > -			for (i = auth_total_cnt, j = 0; i <
> > auth_total_cnt + auth_cnt; i++, j++)
> > -				param->rsn_auth_policy[i] =
> > ies[rsn_idx + ((j + 1) * 4) - 1];
> > +	*out_index = index;
> > +}
> >  
> > -			auth_total_cnt += auth_cnt;
> > -			rsn_idx += offset;
> > +static void *host_int_parse_join_bss_param(struct network_info
> > *info) +{
> > +	struct join_bss_param *param = NULL;
> > +	u16 index = 0;
> > +	u8 pcipher_total_cnt = 0;
> > +	u8 auth_total_cnt = 0;
> >  
> > -			if (ies[index] == RSN_IE) {
> > -				param->rsn_cap[0] = ies[rsn_idx];
> > -				param->rsn_cap[1] = ies[rsn_idx +
> > 1];
> > -				rsn_idx += 2;
> > -			}
> > -			param->rsn_found = true;
> > -			index += ies[index + 1] + 2;
> > -		} else {
> > -			index += ies[index + 1] + 2;
> > -		}
> > -	}
> > +	param = kzalloc(sizeof(*param), GFP_KERNEL);
> > +	if (!param)
> > +		return NULL;
> > +
> > +	param->dtim_period = info->dtim_period;
> > +	param->beacon_period = info->beacon_period;
> > +	param->cap_info = info->cap_info;
> > +	memcpy(param->bssid, info->bssid, 6);
> > +	memcpy((u8 *)param->ssid, info->ssid, info->ssid_len + 1);
> > +	param->ssid_len = info->ssid_len;
> > +	memset(param->rsn_pcip_policy, 0xFF, 3);
> > +	memset(param->rsn_auth_policy, 0xFF, 3);
> > +
> > +	while (index < info->ies_len)
> > +		host_int_fill_join_bss_param(param, info->ies,
> > &index,
> > +					     &pcipher_total_cnt,
> > +					     &auth_total_cnt,
> > info->tsf_lo); 
> >  	return (void *)param;
> >  }
> >   

[PATCH 08/30] staging: wilc1000: fix line over 80 chars in host_int_parse_assoc_resp_info()

[Ajay Singh]

Fix line over 80 characters issue in host_int_parse_assoc_resp_info() by
using shorter name for the local variable.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index c9c5d352..b1f67a7 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1320,18 +1320,19 @@ static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
 	memset(&conn_info, 0, sizeof(struct connect_info));
 
 	if (mac_status == MAC_STATUS_CONNECTED) {
-		u32 rcvd_assoc_resp_info_len;
+		u32 assoc_resp_info_len;
 
 		memset(rcv_assoc_resp, 0, MAX_ASSOC_RESP_FRAME_SIZE);
 
 		host_int_get_assoc_res_info(vif, rcv_assoc_resp,
 					    MAX_ASSOC_RESP_FRAME_SIZE,
-					    &rcvd_assoc_resp_info_len);
+					    &assoc_resp_info_len);
 
-		if (rcvd_assoc_resp_info_len != 0) {
+		if (assoc_resp_info_len != 0) {
 			s32 err = 0;
 
-			err = wilc_parse_assoc_resp_info(rcv_assoc_resp, rcvd_assoc_resp_info_len,
+			err = wilc_parse_assoc_resp_info(rcv_assoc_resp,
+							 assoc_resp_info_len,
 							 &connect_resp_info);
 			if (err)
 				netdev_err(vif->ndev,
-- 
2.7.4

Re: [PATCH 08/30] staging: wilc1000: fix line over 80 chars in host_int_parse_assoc_resp_info()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Fix line over 80 characters issue in host_int_parse_assoc_resp_info() by
> using shorter name for the local variable.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index c9c5d352..b1f67a7 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -1320,18 +1320,19 @@ static inline void host_int_parse_assoc_resp_info(struct wilc_vif *vif,
>  	memset(&conn_info, 0, sizeof(struct connect_info));
>  
>  	if (mac_status == MAC_STATUS_CONNECTED) {
> -		u32 rcvd_assoc_resp_info_len;
> +		u32 assoc_resp_info_len;

I would simply use len or size. It is just my preference.

>  
>  		memset(rcv_assoc_resp, 0, MAX_ASSOC_RESP_FRAME_SIZE);
>  
>  		host_int_get_assoc_res_info(vif, rcv_assoc_resp,
>  					    MAX_ASSOC_RESP_FRAME_SIZE,
> -					    &rcvd_assoc_resp_info_len);
> +					    &assoc_resp_info_len);
>  
> -		if (rcvd_assoc_resp_info_len != 0) {
> +		if (assoc_resp_info_len != 0) {
>  			s32 err = 0;
>  
> -			err = wilc_parse_assoc_resp_info(rcv_assoc_resp, rcvd_assoc_resp_info_len,
> +			err = wilc_parse_assoc_resp_info(rcv_assoc_resp,
> +							 assoc_resp_info_len,
>  							 &connect_resp_info);
>  			if (err)
>  				netdev_err(vif->ndev,
> 

Re: [PATCH 08/30] staging: wilc1000: fix line over 80 chars in host_int_parse_assoc_resp_info()

[Ajay Singh]

On Wed, 9 May 2018 16:43:37 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Fix line over 80 characters issue in
> > host_int_parse_assoc_resp_info() by using shorter name for the
> > local variable.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index c9c5d352..b1f67a7
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -1320,18 +1320,19 @@ static inline void
> > host_int_parse_assoc_resp_info(struct wilc_vif *vif,
> > memset(&conn_info, 0, sizeof(struct connect_info)); 
> >  	if (mac_status == MAC_STATUS_CONNECTED) {
> > -		u32 rcvd_assoc_resp_info_len;
> > +		u32 assoc_resp_info_len;  
> 
> I would simply use len or size. It is just my preference.

I would prefer to keep it same(as submitted patch), as its more
readable.

> 
> >  
> >  		memset(rcv_assoc_resp, 0,
> > MAX_ASSOC_RESP_FRAME_SIZE); 
> >  		host_int_get_assoc_res_info(vif, rcv_assoc_resp,
> >  					    MAX_ASSOC_RESP_FRAME_SIZE,
> > -
> > &rcvd_assoc_resp_info_len);
> > +					    &assoc_resp_info_len);
> >  
> > -		if (rcvd_assoc_resp_info_len != 0) {
> > +		if (assoc_resp_info_len != 0) {
> >  			s32 err = 0;
> >  
> > -			err =
> > wilc_parse_assoc_resp_info(rcv_assoc_resp, rcvd_assoc_resp_info_len,
> > +			err =
> > wilc_parse_assoc_resp_info(rcv_assoc_resp,
> > +
> > assoc_resp_info_len, &connect_resp_info);
> >  			if (err)
> >  				netdev_err(vif->ndev,
> >   

[PATCH 09/30] staging: wilc1000: rename kmalloc with kmemdup() in handle_connect_timeout()

[Ajay Singh]

Instead of kmalloc and memcpy use kmemdup in handle_connect_timeout().
Also return -ENOMEM incase of failure to allocate the memory.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index b1f67a7..ad9270e 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1166,10 +1166,11 @@ static s32 handle_connect_timeout(struct wilc_vif *vif)
 
 		if (hif_drv->usr_conn_req.ies) {
 			info.req_ies_len = hif_drv->usr_conn_req.ies_len;
-			info.req_ies = kmalloc(hif_drv->usr_conn_req.ies_len, GFP_KERNEL);
-			memcpy(info.req_ies,
-			       hif_drv->usr_conn_req.ies,
-			       hif_drv->usr_conn_req.ies_len);
+			info.req_ies = kmemdup(hif_drv->usr_conn_req.ies,
+					       hif_drv->usr_conn_req.ies_len,
+					       GFP_KERNEL);
+			if (!info.req_ies)
+				return -ENOMEM;
 		}
 
 		hif_drv->usr_conn_req.conn_result(CONN_DISCONN_EVENT_CONN_RESP,
-- 
2.7.4

[PATCH 10/30] staging: wilc1000: fix line over 80 chars in linux_mon

[Ajay Singh]

Fix line over 80 char issue reported by checkpatch.pl script.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/linux_mon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_mon.c b/drivers/staging/wilc1000/linux_mon.c
index 2f4b3f5..c372e0f 100644
--- a/drivers/staging/wilc1000/linux_mon.c
+++ b/drivers/staging/wilc1000/linux_mon.c
@@ -46,18 +46,18 @@ void wilc_wfi_monitor_rx(u8 *buff, u32 size)
 	if (pkt_offset & IS_MANAGMEMENT_CALLBACK) {
 		/* hostapd callback mgmt frame */
 
-		skb = dev_alloc_skb(size + sizeof(struct wilc_wfi_radiotap_cb_hdr));
+		skb = dev_alloc_skb(size + sizeof(*cb_hdr));
 		if (!skb)
 			return;
 
 		skb_put_data(skb, buff, size);
 
 		cb_hdr = skb_push(skb, sizeof(*cb_hdr));
-		memset(cb_hdr, 0, sizeof(struct wilc_wfi_radiotap_cb_hdr));
+		memset(cb_hdr, 0, sizeof(*cb_hdr));
 
 		cb_hdr->hdr.it_version = 0; /* PKTHDR_RADIOTAP_VERSION; */
 
-		cb_hdr->hdr.it_len = cpu_to_le16(sizeof(struct wilc_wfi_radiotap_cb_hdr));
+		cb_hdr->hdr.it_len = cpu_to_le16(sizeof(*cb_hdr));
 
 		cb_hdr->hdr.it_present = cpu_to_le32(
 				(1 << IEEE80211_RADIOTAP_RATE) |
@@ -73,7 +73,7 @@ void wilc_wfi_monitor_rx(u8 *buff, u32 size)
 		}
 
 	} else {
-		skb = dev_alloc_skb(size + sizeof(struct wilc_wfi_radiotap_hdr));
+		skb = dev_alloc_skb(size + sizeof(*hdr));
 
 		if (!skb)
 			return;
@@ -82,7 +82,7 @@ void wilc_wfi_monitor_rx(u8 *buff, u32 size)
 		hdr = skb_push(skb, sizeof(*hdr));
 		memset(hdr, 0, sizeof(struct wilc_wfi_radiotap_hdr));
 		hdr->hdr.it_version = 0; /* PKTHDR_RADIOTAP_VERSION; */
-		hdr->hdr.it_len = cpu_to_le16(sizeof(struct wilc_wfi_radiotap_hdr));
+		hdr->hdr.it_len = cpu_to_le16(sizeof(*hdr));
 		hdr->hdr.it_present = cpu_to_le32
 				(1 << IEEE80211_RADIOTAP_RATE); /* | */
 		hdr->rate = 5; /* txrate->bitrate / 5; */
@@ -164,7 +164,7 @@ static netdev_tx_t wilc_wfi_mon_xmit(struct sk_buff *skb,
 	skb_pull(skb, rtap_len);
 
 	if (skb->data[0] == 0xc0 && is_broadcast_ether_addr(&skb->data[4])) {
-		skb2 = dev_alloc_skb(skb->len + sizeof(struct wilc_wfi_radiotap_cb_hdr));
+		skb2 = dev_alloc_skb(skb->len + sizeof(*cb_hdr));
 		if (!skb2)
 			return -ENOMEM;
 
@@ -175,7 +175,7 @@ static netdev_tx_t wilc_wfi_mon_xmit(struct sk_buff *skb,
 
 		cb_hdr->hdr.it_version = 0; /* PKTHDR_RADIOTAP_VERSION; */
 
-		cb_hdr->hdr.it_len = cpu_to_le16(sizeof(struct wilc_wfi_radiotap_cb_hdr));
+		cb_hdr->hdr.it_len = cpu_to_le16(sizeof(*cb_hdr));
 
 		cb_hdr->hdr.it_present = cpu_to_le32(
 				(1 << IEEE80211_RADIOTAP_RATE) |
-- 
2.7.4

[PATCH 11/30] staging: wilc1000: use sizeof(*wdev) to allocate memory in wilc_wfi_cfg_alloc()

[Ajay Singh]

Fix below reported checkpatch issues in wilc_wfi_cfg_alloc().
kzalloc(sizeof(*wdev)...) over kzalloc(sizeof(struct wireless_dev)

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 8be3c4c..102facd 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -2194,7 +2194,7 @@ static struct wireless_dev *wilc_wfi_cfg_alloc(void)
 {
 	struct wireless_dev *wdev;
 
-	wdev = kzalloc(sizeof(struct wireless_dev), GFP_KERNEL);
+	wdev = kzalloc(sizeof(*wdev), GFP_KERNEL);
 	if (!wdev)
 		goto _fail_;
 
-- 
2.7.4

[PATCH 12/30] staging: wilc1000: use kmalloc(sizeof(*mgmt_tx)...) in mgmt_tx()

[Ajay Singh]

Fix below checkpatch issue found in mgmt_tx()

Prefer kmalloc(sizeof(*mgmt_tx)...) over kmalloc(sizeof(struct
p2p_mgmt_data)...)

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 102facd..d0fb31a 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1628,7 +1628,7 @@ static int mgmt_tx(struct wiphy *wiphy,
 	if (!ieee80211_is_mgmt(mgmt->frame_control))
 		goto out;
 
-	mgmt_tx = kmalloc(sizeof(struct p2p_mgmt_data), GFP_KERNEL);
+	mgmt_tx = kmalloc(sizeof(*mgmt_tx), GFP_KERNEL);
 	if (!mgmt_tx) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.7.4

[PATCH 13/30] staging: wilc1000: rename clear_duringIP() to avoid camelCase issue

[Ajay Singh]

Rename clear_duringIP() function to avoid camelCase issue reported by
checkpatch.pl script.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index d0fb31a..f1ebaea 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -269,7 +269,7 @@ static void remove_network_from_shadow(struct timer_list *unused)
 		mod_timer(&aging_timer, jiffies + msecs_to_jiffies(AGING_TIME));
 }
 
-static void clear_duringIP(struct timer_list *unused)
+static void clear_duringip(struct timer_list *unused)
 {
 	wilc_optaining_ip = false;
 }
@@ -2272,7 +2272,7 @@ int wilc_init_host_int(struct net_device *net)
 	priv = wdev_priv(net->ieee80211_ptr);
 	if (op_ifcs == 0) {
 		timer_setup(&aging_timer, remove_network_from_shadow, 0);
-		timer_setup(&wilc_during_ip_timer, clear_duringIP, 0);
+		timer_setup(&wilc_during_ip_timer, clear_duringip, 0);
 	}
 	op_ifcs++;
 
-- 
2.7.4

Re: [PATCH 13/30] staging: wilc1000: rename clear_duringIP() to avoid camelCase issue

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Rename clear_duringIP() function to avoid camelCase issue reported by
> checkpatch.pl script.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index d0fb31a..f1ebaea 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -269,7 +269,7 @@ static void remove_network_from_shadow(struct timer_list *unused)
>  		mod_timer(&aging_timer, jiffies + msecs_to_jiffies(AGING_TIME));
>  }
>  
> -static void clear_duringIP(struct timer_list *unused)
> +static void clear_duringip(struct timer_list *unused)

Maybe add an '_' for better understanding: like clear_during_ip() ?

>  {
>  	wilc_optaining_ip = false;
>  }
> @@ -2272,7 +2272,7 @@ int wilc_init_host_int(struct net_device *net)
>  	priv = wdev_priv(net->ieee80211_ptr);
>  	if (op_ifcs == 0) {
>  		timer_setup(&aging_timer, remove_network_from_shadow, 0);
> -		timer_setup(&wilc_during_ip_timer, clear_duringIP, 0);
> +		timer_setup(&wilc_during_ip_timer, clear_duringip, 0);
>  	}
>  	op_ifcs++;
>  
> 

[PATCH 14/30] staging: wilc1000: fix line over 80 chars in add_network_to_shadow()

[Ajay Singh]

Fix line over 80 characters issue reported by checkpatch in
add_network_to_shadow() by using temporary variable.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52 +++++++++++------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index f1ebaea..0ae2065 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -300,6 +300,7 @@ static void add_network_to_shadow(struct network_info *nw_info,
 	int ap_found = is_network_in_shadow(nw_info, user_void);
 	u32 ap_index = 0;
 	u8 rssi_index = 0;
+	struct network_info *shadow_nw_info;
 
 	if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
 		return;
@@ -310,37 +311,34 @@ static void add_network_to_shadow(struct network_info *nw_info,
 	} else {
 		ap_index = ap_found;
 	}
-	rssi_index = last_scanned_shadow[ap_index].rssi_history.index;
-	last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++] = nw_info->rssi;
+	shadow_nw_info = &last_scanned_shadow[ap_index];
+	rssi_index = shadow_nw_info->rssi_history.index;
+	shadow_nw_info->rssi_history.samples[rssi_index++] = nw_info->rssi;
 	if (rssi_index == NUM_RSSI) {
 		rssi_index = 0;
-		last_scanned_shadow[ap_index].rssi_history.full = true;
-	}
-	last_scanned_shadow[ap_index].rssi_history.index = rssi_index;
-	last_scanned_shadow[ap_index].rssi = nw_info->rssi;
-	last_scanned_shadow[ap_index].cap_info = nw_info->cap_info;
-	last_scanned_shadow[ap_index].ssid_len = nw_info->ssid_len;
-	memcpy(last_scanned_shadow[ap_index].ssid,
-	       nw_info->ssid, nw_info->ssid_len);
-	memcpy(last_scanned_shadow[ap_index].bssid,
-	       nw_info->bssid, ETH_ALEN);
-	last_scanned_shadow[ap_index].beacon_period = nw_info->beacon_period;
-	last_scanned_shadow[ap_index].dtim_period = nw_info->dtim_period;
-	last_scanned_shadow[ap_index].ch = nw_info->ch;
-	last_scanned_shadow[ap_index].ies_len = nw_info->ies_len;
-	last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
+		shadow_nw_info->rssi_history.full = true;
+	}
+	shadow_nw_info->rssi_history.index = rssi_index;
+	shadow_nw_info->rssi = nw_info->rssi;
+	shadow_nw_info->cap_info = nw_info->cap_info;
+	shadow_nw_info->ssid_len = nw_info->ssid_len;
+	memcpy(shadow_nw_info->ssid, nw_info->ssid, nw_info->ssid_len);
+	memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
+	shadow_nw_info->beacon_period = nw_info->beacon_period;
+	shadow_nw_info->dtim_period = nw_info->dtim_period;
+	shadow_nw_info->ch = nw_info->ch;
+	shadow_nw_info->ies_len = nw_info->ies_len;
+	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
 	if (ap_found != -1)
-		kfree(last_scanned_shadow[ap_index].ies);
-	last_scanned_shadow[ap_index].ies = kmalloc(nw_info->ies_len,
-						    GFP_KERNEL);
-	memcpy(last_scanned_shadow[ap_index].ies,
-	       nw_info->ies, nw_info->ies_len);
-	last_scanned_shadow[ap_index].time_scan = jiffies;
-	last_scanned_shadow[ap_index].time_scan_cached = jiffies;
-	last_scanned_shadow[ap_index].found = 1;
+		kfree(shadow_nw_info->ies);
+	shadow_nw_info->ies = kmalloc(nw_info->ies_len, GFP_KERNEL);
+	memcpy(shadow_nw_info->ies, nw_info->ies, nw_info->ies_len);
+	shadow_nw_info->time_scan = jiffies;
+	shadow_nw_info->time_scan_cached = jiffies;
+	shadow_nw_info->found = 1;
 	if (ap_found != -1)
-		kfree(last_scanned_shadow[ap_index].join_params);
-	last_scanned_shadow[ap_index].join_params = join_params;
+		kfree(shadow_nw_info->join_params);
+	shadow_nw_info->join_params = join_params;
 }
 
 static void cfg_scan_result(enum scan_event scan_event,
-- 
2.7.4

Re: [PATCH 14/30] staging: wilc1000: fix line over 80 chars in add_network_to_shadow()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Fix line over 80 characters issue reported by checkpatch in
> add_network_to_shadow() by using temporary variable.

I, personally, don't like this way of fixing line over 80. From my
point of view this introduces a new future patch. Maybe, in future,
somebody will remove this temporary variable stating that there is
no need for it.

> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52 +++++++++++------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index f1ebaea..0ae2065 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -300,6 +300,7 @@ static void add_network_to_shadow(struct network_info *nw_info,
>  	int ap_found = is_network_in_shadow(nw_info, user_void);
>  	u32 ap_index = 0;
>  	u8 rssi_index = 0;
> +	struct network_info *shadow_nw_info;
>  
>  	if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
>  		return;
> @@ -310,37 +311,34 @@ static void add_network_to_shadow(struct network_info *nw_info,
>  	} else {
>  		ap_index = ap_found;
>  	}
> -	rssi_index = last_scanned_shadow[ap_index].rssi_history.index;
> -	last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++] = nw_info->rssi;
> +	shadow_nw_info = &last_scanned_shadow[ap_index];
> +	rssi_index = shadow_nw_info->rssi_history.index;
> +	shadow_nw_info->rssi_history.samples[rssi_index++] = nw_info->rssi;
>  	if (rssi_index == NUM_RSSI) {
>  		rssi_index = 0;
> -		last_scanned_shadow[ap_index].rssi_history.full = true;
> -	}
> -	last_scanned_shadow[ap_index].rssi_history.index = rssi_index;
> -	last_scanned_shadow[ap_index].rssi = nw_info->rssi;
> -	last_scanned_shadow[ap_index].cap_info = nw_info->cap_info;
> -	last_scanned_shadow[ap_index].ssid_len = nw_info->ssid_len;
> -	memcpy(last_scanned_shadow[ap_index].ssid,
> -	       nw_info->ssid, nw_info->ssid_len);
> -	memcpy(last_scanned_shadow[ap_index].bssid,
> -	       nw_info->bssid, ETH_ALEN);
> -	last_scanned_shadow[ap_index].beacon_period = nw_info->beacon_period;
> -	last_scanned_shadow[ap_index].dtim_period = nw_info->dtim_period;
> -	last_scanned_shadow[ap_index].ch = nw_info->ch;
> -	last_scanned_shadow[ap_index].ies_len = nw_info->ies_len;
> -	last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
> +		shadow_nw_info->rssi_history.full = true;
> +	}
> +	shadow_nw_info->rssi_history.index = rssi_index;
> +	shadow_nw_info->rssi = nw_info->rssi;
> +	shadow_nw_info->cap_info = nw_info->cap_info;
> +	shadow_nw_info->ssid_len = nw_info->ssid_len;
> +	memcpy(shadow_nw_info->ssid, nw_info->ssid, nw_info->ssid_len);
> +	memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
> +	shadow_nw_info->beacon_period = nw_info->beacon_period;
> +	shadow_nw_info->dtim_period = nw_info->dtim_period;
> +	shadow_nw_info->ch = nw_info->ch;
> +	shadow_nw_info->ies_len = nw_info->ies_len;
> +	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
>  	if (ap_found != -1)
> -		kfree(last_scanned_shadow[ap_index].ies);
> -	last_scanned_shadow[ap_index].ies = kmalloc(nw_info->ies_len,
> -						    GFP_KERNEL);
> -	memcpy(last_scanned_shadow[ap_index].ies,
> -	       nw_info->ies, nw_info->ies_len);
> -	last_scanned_shadow[ap_index].time_scan = jiffies;
> -	last_scanned_shadow[ap_index].time_scan_cached = jiffies;
> -	last_scanned_shadow[ap_index].found = 1;
> +		kfree(shadow_nw_info->ies);
> +	shadow_nw_info->ies = kmalloc(nw_info->ies_len, GFP_KERNEL);
> +	memcpy(shadow_nw_info->ies, nw_info->ies, nw_info->ies_len);
> +	shadow_nw_info->time_scan = jiffies;
> +	shadow_nw_info->time_scan_cached = jiffies;
> +	shadow_nw_info->found = 1;
>  	if (ap_found != -1)
> -		kfree(last_scanned_shadow[ap_index].join_params);
> -	last_scanned_shadow[ap_index].join_params = join_params;
> +		kfree(shadow_nw_info->join_params);
> +	shadow_nw_info->join_params = join_params;
>  }
>  
>  static void cfg_scan_result(enum scan_event scan_event,
> 

Re: [PATCH 14/30] staging: wilc1000: fix line over 80 chars in add_network_to_shadow()

[Ajay Singh]

On Wed, 9 May 2018 16:43:14 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Fix line over 80 characters issue reported by checkpatch in
> > add_network_to_shadow() by using temporary variable.  
> 
> I, personally, don't like this way of fixing line over 80. From my
> point of view this introduces a new future patch. Maybe, in future,
> somebody will remove this temporary variable stating that there is
> no need for it.
> 

In my opinion, just by removing this temporary variable the patch
might not go through because it will definitely have line over
80 character issue. As per guideline its recommended to run the
checkpatch before submitting the patch.

Only using short variables names might help to resolve that issue but
using short variable names will not give clear meaning for the code. 
I  don't want to shorten the variable name as they don't convey the
complete meaning.

Do you have any suggestion/code which can help to understand how to
resolve this without using temp/variables name changes.

> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52
> > +++++++++++------------ 1 file changed, 25 insertions(+), 27
> > deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> > f1ebaea..0ae2065 100644 ---
> > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -300,6
> > +300,7 @@ static void add_network_to_shadow(struct network_info
> > *nw_info, int ap_found = is_network_in_shadow(nw_info, user_void);
> > u32 ap_index = 0; u8 rssi_index = 0;
> > +	struct network_info *shadow_nw_info;
> >  
> >  	if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
> >  		return;
> > @@ -310,37 +311,34 @@ static void add_network_to_shadow(struct
> > network_info *nw_info, } else {
> >  		ap_index = ap_found;
> >  	}
> > -	rssi_index =
> > last_scanned_shadow[ap_index].rssi_history.index;
> > -
> > last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++] =
> > nw_info->rssi;
> > +	shadow_nw_info = &last_scanned_shadow[ap_index];
> > +	rssi_index = shadow_nw_info->rssi_history.index;
> > +	shadow_nw_info->rssi_history.samples[rssi_index++] =
> > nw_info->rssi; if (rssi_index == NUM_RSSI) {
> >  		rssi_index = 0;
> > -		last_scanned_shadow[ap_index].rssi_history.full =
> > true;
> > -	}
> > -	last_scanned_shadow[ap_index].rssi_history.index =
> > rssi_index;
> > -	last_scanned_shadow[ap_index].rssi = nw_info->rssi;
> > -	last_scanned_shadow[ap_index].cap_info = nw_info->cap_info;
> > -	last_scanned_shadow[ap_index].ssid_len = nw_info->ssid_len;
> > -	memcpy(last_scanned_shadow[ap_index].ssid,
> > -	       nw_info->ssid, nw_info->ssid_len);
> > -	memcpy(last_scanned_shadow[ap_index].bssid,
> > -	       nw_info->bssid, ETH_ALEN);
> > -	last_scanned_shadow[ap_index].beacon_period =
> > nw_info->beacon_period;
> > -	last_scanned_shadow[ap_index].dtim_period =
> > nw_info->dtim_period;
> > -	last_scanned_shadow[ap_index].ch = nw_info->ch;
> > -	last_scanned_shadow[ap_index].ies_len = nw_info->ies_len;
> > -	last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
> > +		shadow_nw_info->rssi_history.full = true;
> > +	}
> > +	shadow_nw_info->rssi_history.index = rssi_index;
> > +	shadow_nw_info->rssi = nw_info->rssi;
> > +	shadow_nw_info->cap_info = nw_info->cap_info;
> > +	shadow_nw_info->ssid_len = nw_info->ssid_len;
> > +	memcpy(shadow_nw_info->ssid, nw_info->ssid,
> > nw_info->ssid_len);
> > +	memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
> > +	shadow_nw_info->beacon_period = nw_info->beacon_period;
> > +	shadow_nw_info->dtim_period = nw_info->dtim_period;
> > +	shadow_nw_info->ch = nw_info->ch;
> > +	shadow_nw_info->ies_len = nw_info->ies_len;
> > +	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
> >  	if (ap_found != -1)
> > -		kfree(last_scanned_shadow[ap_index].ies);
> > -	last_scanned_shadow[ap_index].ies =
> > kmalloc(nw_info->ies_len,
> > -						    GFP_KERNEL);
> > -	memcpy(last_scanned_shadow[ap_index].ies,
> > -	       nw_info->ies, nw_info->ies_len);
> > -	last_scanned_shadow[ap_index].time_scan = jiffies;
> > -	last_scanned_shadow[ap_index].time_scan_cached = jiffies;
> > -	last_scanned_shadow[ap_index].found = 1;
> > +		kfree(shadow_nw_info->ies);
> > +	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
> > GFP_KERNEL);
> > +	memcpy(shadow_nw_info->ies, nw_info->ies,
> > nw_info->ies_len);
> > +	shadow_nw_info->time_scan = jiffies;
> > +	shadow_nw_info->time_scan_cached = jiffies;
> > +	shadow_nw_info->found = 1;
> >  	if (ap_found != -1)
> > -		kfree(last_scanned_shadow[ap_index].join_params);
> > -	last_scanned_shadow[ap_index].join_params = join_params;
> > +		kfree(shadow_nw_info->join_params);
> > +	shadow_nw_info->join_params = join_params;
> >  }
> >  
> >  static void cfg_scan_result(enum scan_event scan_event,
> >   

Re: [PATCH 14/30] staging: wilc1000: fix line over 80 chars in add_network_to_shadow()

[Claudiu Beznea]

On 09.05.2018 21:42, Ajay Singh wrote:
> On Wed, 9 May 2018 16:43:14 +0300
> Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:
> 
>> On 07.05.2018 11:43, Ajay Singh wrote:
>>> Fix line over 80 characters issue reported by checkpatch in
>>> add_network_to_shadow() by using temporary variable.  
>>
>> I, personally, don't like this way of fixing line over 80. From my
>> point of view this introduces a new future patch. Maybe, in future,
>> somebody will remove this temporary variable stating that there is
>> no need for it.
>>
> 
> In my opinion, just by removing this temporary variable the patch
> might not go through because it will definitely have line over
> 80 character issue. As per guideline its recommended to run the
> checkpatch before submitting the patch.
> 
> Only using short variables names might help to resolve that issue but
> using short variable names will not give clear meaning for the code. 
> I  don't want to shorten the variable name as they don't convey the
> complete meaning.
> 
> Do you have any suggestion/code which can help to understand how to
> resolve this without using temp/variables name changes.

No, for this one I have not. Maybe further refactoring...

> 
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> ---
>>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52
>>> +++++++++++------------ 1 file changed, 25 insertions(+), 27
>>> deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
>>> f1ebaea..0ae2065 100644 ---
>>> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -300,6
>>> +300,7 @@ static void add_network_to_shadow(struct network_info
>>> *nw_info, int ap_found = is_network_in_shadow(nw_info, user_void);
>>> u32 ap_index = 0; u8 rssi_index = 0;
>>> +	struct network_info *shadow_nw_info;
>>>  
>>>  	if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
>>>  		return;
>>> @@ -310,37 +311,34 @@ static void add_network_to_shadow(struct
>>> network_info *nw_info, } else {
>>>  		ap_index = ap_found;
>>>  	}
>>> -	rssi_index =
>>> last_scanned_shadow[ap_index].rssi_history.index;
>>> -
>>> last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++] =
>>> nw_info->rssi;
>>> +	shadow_nw_info = &last_scanned_shadow[ap_index];
>>> +	rssi_index = shadow_nw_info->rssi_history.index;
>>> +	shadow_nw_info->rssi_history.samples[rssi_index++] =
>>> nw_info->rssi; if (rssi_index == NUM_RSSI) {
>>>  		rssi_index = 0;
>>> -		last_scanned_shadow[ap_index].rssi_history.full =
>>> true;
>>> -	}
>>> -	last_scanned_shadow[ap_index].rssi_history.index =
>>> rssi_index;
>>> -	last_scanned_shadow[ap_index].rssi = nw_info->rssi;
>>> -	last_scanned_shadow[ap_index].cap_info = nw_info->cap_info;
>>> -	last_scanned_shadow[ap_index].ssid_len = nw_info->ssid_len;
>>> -	memcpy(last_scanned_shadow[ap_index].ssid,
>>> -	       nw_info->ssid, nw_info->ssid_len);
>>> -	memcpy(last_scanned_shadow[ap_index].bssid,
>>> -	       nw_info->bssid, ETH_ALEN);
>>> -	last_scanned_shadow[ap_index].beacon_period =
>>> nw_info->beacon_period;
>>> -	last_scanned_shadow[ap_index].dtim_period =
>>> nw_info->dtim_period;
>>> -	last_scanned_shadow[ap_index].ch = nw_info->ch;
>>> -	last_scanned_shadow[ap_index].ies_len = nw_info->ies_len;
>>> -	last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
>>> +		shadow_nw_info->rssi_history.full = true;
>>> +	}
>>> +	shadow_nw_info->rssi_history.index = rssi_index;
>>> +	shadow_nw_info->rssi = nw_info->rssi;
>>> +	shadow_nw_info->cap_info = nw_info->cap_info;
>>> +	shadow_nw_info->ssid_len = nw_info->ssid_len;
>>> +	memcpy(shadow_nw_info->ssid, nw_info->ssid,
>>> nw_info->ssid_len);
>>> +	memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
>>> +	shadow_nw_info->beacon_period = nw_info->beacon_period;
>>> +	shadow_nw_info->dtim_period = nw_info->dtim_period;
>>> +	shadow_nw_info->ch = nw_info->ch;
>>> +	shadow_nw_info->ies_len = nw_info->ies_len;
>>> +	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
>>>  	if (ap_found != -1)
>>> -		kfree(last_scanned_shadow[ap_index].ies);
>>> -	last_scanned_shadow[ap_index].ies =
>>> kmalloc(nw_info->ies_len,
>>> -						    GFP_KERNEL);
>>> -	memcpy(last_scanned_shadow[ap_index].ies,
>>> -	       nw_info->ies, nw_info->ies_len);
>>> -	last_scanned_shadow[ap_index].time_scan = jiffies;
>>> -	last_scanned_shadow[ap_index].time_scan_cached = jiffies;
>>> -	last_scanned_shadow[ap_index].found = 1;
>>> +		kfree(shadow_nw_info->ies);
>>> +	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
>>> GFP_KERNEL);
>>> +	memcpy(shadow_nw_info->ies, nw_info->ies,
>>> nw_info->ies_len);
>>> +	shadow_nw_info->time_scan = jiffies;
>>> +	shadow_nw_info->time_scan_cached = jiffies;
>>> +	shadow_nw_info->found = 1;
>>>  	if (ap_found != -1)
>>> -		kfree(last_scanned_shadow[ap_index].join_params);
>>> -	last_scanned_shadow[ap_index].join_params = join_params;
>>> +		kfree(shadow_nw_info->join_params);
>>> +	shadow_nw_info->join_params = join_params;
>>>  }
>>>  
>>>  static void cfg_scan_result(enum scan_event scan_event,
>>>   
> 
> 
> 

Re: [PATCH 14/30] staging: wilc1000: fix line over 80 chars in add_network_to_shadow()

[Claudiu Beznea]

Hi Ajay,

On 10.05.2018 08:27, Claudiu Beznea wrote:
> 
> 
> On 09.05.2018 21:42, Ajay Singh wrote:
>> On Wed, 9 May 2018 16:43:14 +0300
>> Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:
>>
>>> On 07.05.2018 11:43, Ajay Singh wrote:
>>>> Fix line over 80 characters issue reported by checkpatch in
>>>> add_network_to_shadow() by using temporary variable.  
>>>
>>> I, personally, don't like this way of fixing line over 80. From my
>>> point of view this introduces a new future patch. Maybe, in future,
>>> somebody will remove this temporary variable stating that there is
>>> no need for it.
>>>
>>
>> In my opinion, just by removing this temporary variable the patch
>> might not go through because it will definitely have line over
>> 80 character issue. As per guideline its recommended to run the
>> checkpatch before submitting the patch.
>>
>> Only using short variables names might help to resolve that issue but
>> using short variable names will not give clear meaning for the code. 
>> I  don't want to shorten the variable name as they don't convey the
>> complete meaning.
>>
>> Do you have any suggestion/code which can help to understand how to
>> resolve this without using temp/variables name changes.
> 
> No, for this one I have not. Maybe further refactoring...
> 

Looking over the v2 of this series you send, and over wilc_wfi_cfgoperations.c,
and remembering your last question on this patch, I was thinking that
one suggestion for this would be to replace last_scanned_shadow with
just scanned_shadow or nw_info or scanned_nw_info. Just an idea.

Claudiu

>>
>>>>
>>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>>> ---
>>>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52
>>>> +++++++++++------------ 1 file changed, 25 insertions(+), 27
>>>> deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
>>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
>>>> f1ebaea..0ae2065 100644 ---
>>>> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
>>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -300,6
>>>> +300,7 @@ static void add_network_to_shadow(struct network_info
>>>> *nw_info, int ap_found = is_network_in_shadow(nw_info, user_void);
>>>> u32 ap_index = 0; u8 rssi_index = 0;
>>>> +	struct network_info *shadow_nw_info;
>>>>  
>>>>  	if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
>>>>  		return;
>>>> @@ -310,37 +311,34 @@ static void add_network_to_shadow(struct
>>>> network_info *nw_info, } else {
>>>>  		ap_index = ap_found;
>>>>  	}
>>>> -	rssi_index =
>>>> last_scanned_shadow[ap_index].rssi_history.index;
>>>> -
>>>> last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++] =
>>>> nw_info->rssi;
>>>> +	shadow_nw_info = &last_scanned_shadow[ap_index];
>>>> +	rssi_index = shadow_nw_info->rssi_history.index;
>>>> +	shadow_nw_info->rssi_history.samples[rssi_index++] =
>>>> nw_info->rssi; if (rssi_index == NUM_RSSI) {
>>>>  		rssi_index = 0;
>>>> -		last_scanned_shadow[ap_index].rssi_history.full =
>>>> true;
>>>> -	}
>>>> -	last_scanned_shadow[ap_index].rssi_history.index =
>>>> rssi_index;
>>>> -	last_scanned_shadow[ap_index].rssi = nw_info->rssi;
>>>> -	last_scanned_shadow[ap_index].cap_info = nw_info->cap_info;
>>>> -	last_scanned_shadow[ap_index].ssid_len = nw_info->ssid_len;
>>>> -	memcpy(last_scanned_shadow[ap_index].ssid,
>>>> -	       nw_info->ssid, nw_info->ssid_len);
>>>> -	memcpy(last_scanned_shadow[ap_index].bssid,
>>>> -	       nw_info->bssid, ETH_ALEN);
>>>> -	last_scanned_shadow[ap_index].beacon_period =
>>>> nw_info->beacon_period;
>>>> -	last_scanned_shadow[ap_index].dtim_period =
>>>> nw_info->dtim_period;
>>>> -	last_scanned_shadow[ap_index].ch = nw_info->ch;
>>>> -	last_scanned_shadow[ap_index].ies_len = nw_info->ies_len;
>>>> -	last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
>>>> +		shadow_nw_info->rssi_history.full = true;
>>>> +	}
>>>> +	shadow_nw_info->rssi_history.index = rssi_index;
>>>> +	shadow_nw_info->rssi = nw_info->rssi;
>>>> +	shadow_nw_info->cap_info = nw_info->cap_info;
>>>> +	shadow_nw_info->ssid_len = nw_info->ssid_len;
>>>> +	memcpy(shadow_nw_info->ssid, nw_info->ssid,
>>>> nw_info->ssid_len);
>>>> +	memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
>>>> +	shadow_nw_info->beacon_period = nw_info->beacon_period;
>>>> +	shadow_nw_info->dtim_period = nw_info->dtim_period;
>>>> +	shadow_nw_info->ch = nw_info->ch;
>>>> +	shadow_nw_info->ies_len = nw_info->ies_len;
>>>> +	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
>>>>  	if (ap_found != -1)
>>>> -		kfree(last_scanned_shadow[ap_index].ies);
>>>> -	last_scanned_shadow[ap_index].ies =
>>>> kmalloc(nw_info->ies_len,
>>>> -						    GFP_KERNEL);
>>>> -	memcpy(last_scanned_shadow[ap_index].ies,
>>>> -	       nw_info->ies, nw_info->ies_len);
>>>> -	last_scanned_shadow[ap_index].time_scan = jiffies;
>>>> -	last_scanned_shadow[ap_index].time_scan_cached = jiffies;
>>>> -	last_scanned_shadow[ap_index].found = 1;
>>>> +		kfree(shadow_nw_info->ies);
>>>> +	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
>>>> GFP_KERNEL);
>>>> +	memcpy(shadow_nw_info->ies, nw_info->ies,
>>>> nw_info->ies_len);
>>>> +	shadow_nw_info->time_scan = jiffies;
>>>> +	shadow_nw_info->time_scan_cached = jiffies;
>>>> +	shadow_nw_info->found = 1;
>>>>  	if (ap_found != -1)
>>>> -		kfree(last_scanned_shadow[ap_index].join_params);
>>>> -	last_scanned_shadow[ap_index].join_params = join_params;
>>>> +		kfree(shadow_nw_info->join_params);
>>>> +	shadow_nw_info->join_params = join_params;
>>>>  }
>>>>  
>>>>  static void cfg_scan_result(enum scan_event scan_event,
>>>>   
>>
>>
>>
> 

Re: [PATCH 14/30] staging: wilc1000: fix line over 80 chars in add_network_to_shadow()

[Ajay Singh]

Hi Claudiu,

On Mon, 14 May 2018 11:57:24 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> Hi Ajay,
> 
> On 10.05.2018 08:27, Claudiu Beznea wrote:
> > 
> > 
> > On 09.05.2018 21:42, Ajay Singh wrote:  
> >> On Wed, 9 May 2018 16:43:14 +0300
> >> Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:
> >>  
> >>> On 07.05.2018 11:43, Ajay Singh wrote:  
> >>>> Fix line over 80 characters issue reported by checkpatch in
> >>>> add_network_to_shadow() by using temporary variable.    
> >>>
> >>> I, personally, don't like this way of fixing line over 80. From my
> >>> point of view this introduces a new future patch. Maybe, in
> >>> future, somebody will remove this temporary variable stating that
> >>> there is no need for it.
> >>>  
> >>
> >> In my opinion, just by removing this temporary variable the patch
> >> might not go through because it will definitely have line over
> >> 80 character issue. As per guideline its recommended to run the
> >> checkpatch before submitting the patch.
> >>
> >> Only using short variables names might help to resolve that issue
> >> but using short variable names will not give clear meaning for the
> >> code. I  don't want to shorten the variable name as they don't
> >> convey the complete meaning.
> >>
> >> Do you have any suggestion/code which can help to understand how to
> >> resolve this without using temp/variables name changes.  
> > 
> > No, for this one I have not. Maybe further refactoring...
> >   
> 
> Looking over the v2 of this series you send, and over
> wilc_wfi_cfgoperations.c, and remembering your last question on this
> patch, I was thinking that one suggestion for this would be to
> replace last_scanned_shadow with just scanned_shadow or nw_info or
> scanned_nw_info. Just an idea.
> 

I avoided use of short name for 'last_scanned_shadow' because it might
not give clear meaning as there are variables like 'last_scanned_cnt',
which also uses same prefix 'last_' and its helpful to know its related
data.


> >>  
> >>>>
> >>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> >>>> ---
> >>>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52
> >>>> +++++++++++------------ 1 file changed, 25 insertions(+), 27
> >>>> deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> >>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> >>>> f1ebaea..0ae2065 100644 ---
> >>>> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> >>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -300,6
> >>>> +300,7 @@ static void add_network_to_shadow(struct network_info
> >>>> *nw_info, int ap_found = is_network_in_shadow(nw_info,
> >>>> user_void); u32 ap_index = 0; u8 rssi_index = 0;
> >>>> +	struct network_info *shadow_nw_info;
> >>>>  
> >>>>  	if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
> >>>>  		return;
> >>>> @@ -310,37 +311,34 @@ static void add_network_to_shadow(struct
> >>>> network_info *nw_info, } else {
> >>>>  		ap_index = ap_found;
> >>>>  	}
> >>>> -	rssi_index =
> >>>> last_scanned_shadow[ap_index].rssi_history.index;
> >>>> -
> >>>> last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++]
> >>>> = nw_info->rssi;
> >>>> +	shadow_nw_info = &last_scanned_shadow[ap_index];
> >>>> +	rssi_index = shadow_nw_info->rssi_history.index;
> >>>> +	shadow_nw_info->rssi_history.samples[rssi_index++] =
> >>>> nw_info->rssi; if (rssi_index == NUM_RSSI) {
> >>>>  		rssi_index = 0;
> >>>> -		last_scanned_shadow[ap_index].rssi_history.full
> >>>> = true;
> >>>> -	}
> >>>> -	last_scanned_shadow[ap_index].rssi_history.index =
> >>>> rssi_index;
> >>>> -	last_scanned_shadow[ap_index].rssi = nw_info->rssi;
> >>>> -	last_scanned_shadow[ap_index].cap_info =
> >>>> nw_info->cap_info;
> >>>> -	last_scanned_shadow[ap_index].ssid_len =
> >>>> nw_info->ssid_len;
> >>>> -	memcpy(last_scanned_shadow[ap_index].ssid,
> >>>> -	       nw_info->ssid, nw_info->ssid_len);
> >>>> -	memcpy(last_scanned_shadow[ap_index].bssid,
> >>>> -	       nw_info->bssid, ETH_ALEN);
> >>>> -	last_scanned_shadow[ap_index].beacon_period =
> >>>> nw_info->beacon_period;
> >>>> -	last_scanned_shadow[ap_index].dtim_period =
> >>>> nw_info->dtim_period;
> >>>> -	last_scanned_shadow[ap_index].ch = nw_info->ch;
> >>>> -	last_scanned_shadow[ap_index].ies_len =
> >>>> nw_info->ies_len;
> >>>> -	last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
> >>>> +		shadow_nw_info->rssi_history.full = true;
> >>>> +	}
> >>>> +	shadow_nw_info->rssi_history.index = rssi_index;
> >>>> +	shadow_nw_info->rssi = nw_info->rssi;
> >>>> +	shadow_nw_info->cap_info = nw_info->cap_info;
> >>>> +	shadow_nw_info->ssid_len = nw_info->ssid_len;
> >>>> +	memcpy(shadow_nw_info->ssid, nw_info->ssid,
> >>>> nw_info->ssid_len);
> >>>> +	memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
> >>>> +	shadow_nw_info->beacon_period = nw_info->beacon_period;
> >>>> +	shadow_nw_info->dtim_period = nw_info->dtim_period;
> >>>> +	shadow_nw_info->ch = nw_info->ch;
> >>>> +	shadow_nw_info->ies_len = nw_info->ies_len;
> >>>> +	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
> >>>>  	if (ap_found != -1)
> >>>> -		kfree(last_scanned_shadow[ap_index].ies);
> >>>> -	last_scanned_shadow[ap_index].ies =
> >>>> kmalloc(nw_info->ies_len,
> >>>> -						    GFP_KERNEL);
> >>>> -	memcpy(last_scanned_shadow[ap_index].ies,
> >>>> -	       nw_info->ies, nw_info->ies_len);
> >>>> -	last_scanned_shadow[ap_index].time_scan = jiffies;
> >>>> -	last_scanned_shadow[ap_index].time_scan_cached =
> >>>> jiffies;
> >>>> -	last_scanned_shadow[ap_index].found = 1;
> >>>> +		kfree(shadow_nw_info->ies);
> >>>> +	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
> >>>> GFP_KERNEL);
> >>>> +	memcpy(shadow_nw_info->ies, nw_info->ies,
> >>>> nw_info->ies_len);
> >>>> +	shadow_nw_info->time_scan = jiffies;
> >>>> +	shadow_nw_info->time_scan_cached = jiffies;
> >>>> +	shadow_nw_info->found = 1;
> >>>>  	if (ap_found != -1)
> >>>> -
> >>>> kfree(last_scanned_shadow[ap_index].join_params);
> >>>> -	last_scanned_shadow[ap_index].join_params = join_params;
> >>>> +		kfree(shadow_nw_info->join_params);
> >>>> +	shadow_nw_info->join_params = join_params;
> >>>>  }
> >>>>  
> >>>>  static void cfg_scan_result(enum scan_event scan_event,
> >>>>     
> >>
> >>
> >>  
> >   

[PATCH 15/30] staging: wilc1000: use kmemdup instead of kmalloc in add_network_to_shadow()

[Ajay Singh]

Use kmemdup instead of kmalloc & memcpy in add_network_to_shadow().

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 0ae2065..ca221f1 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -331,8 +331,8 @@ static void add_network_to_shadow(struct network_info *nw_info,
 	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
 	if (ap_found != -1)
 		kfree(shadow_nw_info->ies);
-	shadow_nw_info->ies = kmalloc(nw_info->ies_len, GFP_KERNEL);
-	memcpy(shadow_nw_info->ies, nw_info->ies, nw_info->ies_len);
+	shadow_nw_info->ies = kmemdup(nw_info->ies, nw_info->ies_len,
+				      GFP_KERNEL);
 	shadow_nw_info->time_scan = jiffies;
 	shadow_nw_info->time_scan_cached = jiffies;
 	shadow_nw_info->found = 1;
-- 
2.7.4

Re: [PATCH 15/30] staging: wilc1000: use kmemdup instead of kmalloc in add_network_to_shadow()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Use kmemdup instead of kmalloc & memcpy in add_network_to_shadow().
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 0ae2065..ca221f1 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -331,8 +331,8 @@ static void add_network_to_shadow(struct network_info *nw_info,
>  	shadow_nw_info->tsf_hi = nw_info->tsf_hi;
>  	if (ap_found != -1)
>  		kfree(shadow_nw_info->ies);
> -	shadow_nw_info->ies = kmalloc(nw_info->ies_len, GFP_KERNEL);
> -	memcpy(shadow_nw_info->ies, nw_info->ies, nw_info->ies_len);
> +	shadow_nw_info->ies = kmemdup(nw_info->ies, nw_info->ies_len,
> +				      GFP_KERNEL);

Maybe, in case of NULL, you will want to set ies_len = 0 ?

>  	shadow_nw_info->time_scan = jiffies;
>  	shadow_nw_info->time_scan_cached = jiffies;
>  	shadow_nw_info->found = 1;
> 

Re: [PATCH 15/30] staging: wilc1000: use kmemdup instead of kmalloc in add_network_to_shadow()

[Ajay Singh]

On Wed, 9 May 2018 16:42:59 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Use kmemdup instead of kmalloc & memcpy in add_network_to_shadow().
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> > 0ae2065..ca221f1 100644 ---
> > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -331,8
> > +331,8 @@ static void add_network_to_shadow(struct network_info
> > *nw_info, shadow_nw_info->tsf_hi = nw_info->tsf_hi; if (ap_found !=
> > -1) kfree(shadow_nw_info->ies);
> > -	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
> > GFP_KERNEL);
> > -	memcpy(shadow_nw_info->ies, nw_info->ies,
> > nw_info->ies_len);
> > +	shadow_nw_info->ies = kmemdup(nw_info->ies,
> > nw_info->ies_len,
> > +				      GFP_KERNEL);  
> 
> Maybe, in case of NULL, you will want to set ies_len = 0 ?


I couldn't find code where 'ies_len' is check to validity of data.
Mostly we use NULL check for "ies" pointer for data
validity.So in my opinion setting it to zero would be
irrelevant.


> 
> >  	shadow_nw_info->time_scan = jiffies;
> >  	shadow_nw_info->time_scan_cached = jiffies;
> >  	shadow_nw_info->found = 1;
> >   


Regards,
Ajay

Re: [PATCH 15/30] staging: wilc1000: use kmemdup instead of kmalloc in add_network_to_shadow()

[Claudiu Beznea]

On 09.05.2018 22:17, Ajay Singh wrote:
> On Wed, 9 May 2018 16:42:59 +0300
> Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:
> 
>> On 07.05.2018 11:43, Ajay Singh wrote:
>>> Use kmemdup instead of kmalloc & memcpy in add_network_to_shadow().
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> ---
>>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
>>> 0ae2065..ca221f1 100644 ---
>>> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -331,8
>>> +331,8 @@ static void add_network_to_shadow(struct network_info
>>> *nw_info, shadow_nw_info->tsf_hi = nw_info->tsf_hi; if (ap_found !=
>>> -1) kfree(shadow_nw_info->ies);
>>> -	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
>>> GFP_KERNEL);
>>> -	memcpy(shadow_nw_info->ies, nw_info->ies,
>>> nw_info->ies_len);
>>> +	shadow_nw_info->ies = kmemdup(nw_info->ies,
>>> nw_info->ies_len,
>>> +				      GFP_KERNEL);  
>>
>> Maybe, in case of NULL, you will want to set ies_len = 0 ?
> 
> 
> I couldn't find code where 'ies_len' is check to validity of data.
> Mostly we use NULL check for "ies" pointer for data
> validity.So in my opinion setting it to zero would be
> irrelevant.

I'm seeing this in refresh_scan():
                network_info = &last_scanned_shadow[i];                         
                                                                                
                if (!memcmp("DIRECT-", network_info->ssid, 7) && !direct_scan)  
                        continue;                                               
                                                                                
                freq = ieee80211_channel_to_frequency((s32)network_info->ch,    
                                                      NL80211_BAND_2GHZ);       
                channel = ieee80211_get_channel(wiphy, freq);                   
                rssi = get_rssi_avg(network_info);                              
                bss = cfg80211_inform_bss(wiphy,                                
                                          channel,                              
                                          CFG80211_BSS_FTYPE_UNKNOWN,           
                                          network_info->bssid,                  
                                          network_info->tsf_hi,                 
                                          network_info->cap_info,               
                                          network_info->beacon_period,          
                                          (const u8 *)network_info->ies,        
                                          (size_t)network_info->ies_len,        
                                          (s32)rssi * 100,                      
                                          GFP_KERNEL);                          

Looking further into cfg80211_inform_bss():
	-> cfg80211_inform_bss_data()
	-> cfg80211_get_bss_channel()
	-> cfg80211_find_ie()
	-> cfg80211_find_ie_match():
        while (len >= 2 && len >= ies[1] + 2) {                                 
                if ((ies[0] == eid) &&                                          
                    (ies[1] + 2 >= match_offset + match_len) &&                 
                    !memcmp(ies + match_offset, match, match_len))              
                        return ies;                                             
                                                                                
                len -= ies[1] + 2;                                              
                ies += ies[1] + 2;                                              
        }                                                                       


> 
> 
>>
>>>  	shadow_nw_info->time_scan = jiffies;
>>>  	shadow_nw_info->time_scan_cached = jiffies;
>>>  	shadow_nw_info->found = 1;
>>>   
> 
> 
> Regards,
> Ajay
> 
> 

Re: [PATCH 15/30] staging: wilc1000: use kmemdup instead of kmalloc in add_network_to_shadow()

[Ajay Singh]

On Thu, 10 May 2018 08:35:29 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 09.05.2018 22:17, Ajay Singh wrote:
> > On Wed, 9 May 2018 16:42:59 +0300
> > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:
> >   
> >> On 07.05.2018 11:43, Ajay Singh wrote:  
> >>> Use kmemdup instead of kmalloc & memcpy in
> >>> add_network_to_shadow().
> >>>
> >>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> >>> ---
> >>>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> >>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> >>> 0ae2065..ca221f1 100644 ---
> >>> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> >>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -331,8
> >>> +331,8 @@ static void add_network_to_shadow(struct network_info
> >>> *nw_info, shadow_nw_info->tsf_hi = nw_info->tsf_hi; if
> >>> (ap_found != -1) kfree(shadow_nw_info->ies);
> >>> -	shadow_nw_info->ies = kmalloc(nw_info->ies_len,
> >>> GFP_KERNEL);
> >>> -	memcpy(shadow_nw_info->ies, nw_info->ies,
> >>> nw_info->ies_len);
> >>> +	shadow_nw_info->ies = kmemdup(nw_info->ies,
> >>> nw_info->ies_len,
> >>> +				      GFP_KERNEL);    
> >>
> >> Maybe, in case of NULL, you will want to set ies_len = 0 ?  
> > 
> > 
> > I couldn't find code where 'ies_len' is check to validity of data.
> > Mostly we use NULL check for "ies" pointer for data
> > validity.So in my opinion setting it to zero would be
> > irrelevant.  
> 
> I'm seeing this in refresh_scan():
>                 network_info =
> &last_scanned_shadow[i]; 
>                 if (!memcmp("DIRECT-", network_info->ssid, 7)
> && !direct_scan)
> continue; 
>                 freq =
> ieee80211_channel_to_frequency((s32)network_info->ch,
> NL80211_BAND_2GHZ); channel = ieee80211_get_channel(wiphy,
> freq); rssi =
> get_rssi_avg(network_info); bss =
> cfg80211_inform_bss(wiphy, channel,                              
>                                           CFG80211_BSS_FTYPE_UNKNOWN,           
>                                           network_info->bssid,                  
>                                           network_info->tsf_hi,                 
>                                           network_info->cap_info,               
>                                           network_info->beacon_period,          
>                                           (const u8
> *)network_info->ies, (size_t)network_info->ies_len,        
>                                           (s32)rssi *
> 100, GFP_KERNEL);                          
> 
> Looking further into cfg80211_inform_bss():
> 	-> cfg80211_inform_bss_data()
> 	-> cfg80211_get_bss_channel()
> 	-> cfg80211_find_ie()
> 	-> cfg80211_find_ie_match():  
>         while (len >= 2 && len >= ies[1] + 2)
> { if ((ies[0] == eid) &&                                          
>                     (ies[1] + 2 >= match_offset + match_len)
> && !memcmp(ies + match_offset, match, match_len))              
>                         return
> ies; 
>                 len -= ies[1] +
> 2; ies += ies[1] + 2;                                              
>         }                                                                       
> 
> 

Got it. I will also include the code to set ies_len to 0 during 
memory allocations failure scenario.


> > 
> >   
> >>  
> >>>  	shadow_nw_info->time_scan = jiffies;
> >>>  	shadow_nw_info->time_scan_cached = jiffies;
> >>>  	shadow_nw_info->found = 1;
> >>>     
> > 
> > 
> > Regards,
> > Ajay
> > 
> >   

[PATCH 16/30] staging: wilc1000: fix line over 80 charas in wilc_wfi_remain_on_channel_expired()

[Ajay Singh]

Refactor wilc_wfi_remain_on_channel_expired() to avoid line over 80
character issue reported by checkpatch.pl script. Also assigned value in the
variable at the time of declaration.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index ca221f1..4f35178 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1490,18 +1490,16 @@ static void wilc_wfi_remain_on_channel_ready(void *priv_data)
 
 static void wilc_wfi_remain_on_channel_expired(void *data, u32 session_id)
 {
-	struct wilc_priv *priv;
+	struct wilc_priv *priv = data;
+	struct wilc_wfi_p2p_listen_params *params = &priv->remain_on_ch_params;
 
-	priv = data;
+	if (session_id != params->listen_session_id)
+		return;
 
-	if (session_id == priv->remain_on_ch_params.listen_session_id) {
-		priv->p2p_listen_state = false;
+	priv->p2p_listen_state = false;
 
-		cfg80211_remain_on_channel_expired(priv->wdev,
-						   priv->remain_on_ch_params.listen_cookie,
-						   priv->remain_on_ch_params.listen_ch,
-						   GFP_KERNEL);
-	}
+	cfg80211_remain_on_channel_expired(priv->wdev, params->listen_cookie,
+					   params->listen_ch, GFP_KERNEL);
 }
 
 static int remain_on_channel(struct wiphy *wiphy,
-- 
2.7.4

[PATCH 17/30] staging: wilc1000: fix line over 80 chars in wilc_wfi_cfg_tx_vendor_spec()

[Ajay Singh]

Fix line over 80 characters issues reported by checkpatch.pl script in
wilc_wfi_cfg_tx_vendor_spec() by using temporary variable. Simplified
'if else' condition with 'if'.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 4f35178..8dea414 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1573,14 +1573,14 @@ static void wilc_wfi_cfg_tx_vendor_spec(struct p2p_mgmt_data *mgmt_tx,
 	for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
 		if (buf[i] == P2PELEM_ATTR_ID &&
 		    !memcmp(p2p_oui, &buf[i + 2], 4)) {
+			bool oper_ch = false;
+			u8 *tx_buff = &mgmt_tx->buff[i + 6];
+
 			if (subtype == P2P_INV_REQ || subtype == P2P_INV_RSP)
-				wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
-							     len - (i + 6),
-							     true, iftype);
-			else
-				wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
-							     len - (i + 6),
-							     false, iftype);
+				oper_ch = true;
+
+			wilc_wfi_cfg_parse_tx_action(tx_buff, len - (i + 6),
+						     oper_ch, iftype);
 
 			break;
 		}
-- 
2.7.4

Re: [PATCH 17/30] staging: wilc1000: fix line over 80 chars in wilc_wfi_cfg_tx_vendor_spec()

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Fix line over 80 characters issues reported by checkpatch.pl script in
> wilc_wfi_cfg_tx_vendor_spec() by using temporary variable. Simplified
> 'if else' condition with 'if'.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 4f35178..8dea414 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1573,14 +1573,14 @@ static void wilc_wfi_cfg_tx_vendor_spec(struct p2p_mgmt_data *mgmt_tx,
>  	for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
>  		if (buf[i] == P2PELEM_ATTR_ID &&
>  		    !memcmp(p2p_oui, &buf[i + 2], 4)) {
> +			bool oper_ch = false;
> +			u8 *tx_buff = &mgmt_tx->buff[i + 6];
> +
>  			if (subtype == P2P_INV_REQ || subtype == P2P_INV_RSP)
> -				wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
> -							     len - (i + 6),
> -							     true, iftype);
> -			else
> -				wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
> -							     len - (i + 6),
> -							     false, iftype);
> +				oper_ch = true;
> +
> +			wilc_wfi_cfg_parse_tx_action(tx_buff, len - (i + 6),
> +						     oper_ch, iftype);

What about:
			wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
						     len - (i + 6),
						     (subtype == P2P_INV_REQ ||
						      subtype == P2P_INV_RSP),
						     iftype);


instead all the temporary variables?

>  
>  			break;
>  		}
> 

Re: [PATCH 17/30] staging: wilc1000: fix line over 80 chars in wilc_wfi_cfg_tx_vendor_spec()

[Ajay Singh]

On Wed, 9 May 2018 16:42:45 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Fix line over 80 characters issues reported by checkpatch.pl script
> > in wilc_wfi_cfg_tx_vendor_spec() by using temporary variable.
> > Simplified 'if else' condition with 'if'.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 14
> > +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> > 4f35178..8dea414 100644 ---
> > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -1573,14
> > +1573,14 @@ static void wilc_wfi_cfg_tx_vendor_spec(struct
> > p2p_mgmt_data *mgmt_tx, for (i = P2P_PUB_ACTION_SUBTYPE + 2; i <
> > len; i++) { if (buf[i] == P2PELEM_ATTR_ID && !memcmp(p2p_oui,
> > &buf[i + 2], 4)) {
> > +			bool oper_ch = false;
> > +			u8 *tx_buff = &mgmt_tx->buff[i + 6];
> > +
> >  			if (subtype == P2P_INV_REQ || subtype ==
> > P2P_INV_RSP)
> > -
> > wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
> > -							     len -
> > (i + 6),
> > -							     true,
> > iftype);
> > -			else
> > -
> > wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
> > -							     len -
> > (i + 6),
> > -
> > false, iftype);
> > +				oper_ch = true;
> > +
> > +			wilc_wfi_cfg_parse_tx_action(tx_buff, len
> > - (i + 6),
> > +						     oper_ch,
> > iftype);  
> 
> What about:
> 			wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i
> + 6], len - (i + 6),
> 						     (subtype ==
> P2P_INV_REQ || subtype == P2P_INV_RSP),
> 						     iftype);
> 
> 
> instead all the temporary variables?

In my opinion adding one bool variable making the code more readable
then using adding extra logic with parameters for function call.

> 
> >  
> >  			break;
> >  		}
> >   

[PATCH 18/30] staging: wilc1000: fix line over 80 chars in get_station()

[Ajay Singh]

Fix line over 80 characters issue in get_station().

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 8dea414..3deef5b 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1124,7 +1124,9 @@ static int get_station(struct wiphy *wiphy, struct net_device *dev,
 
 	if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
 		for (i = 0; i < NUM_STA_ASSOCIATED; i++) {
-			if (!(memcmp(mac, priv->assoc_stainfo.sta_associated_bss[i], ETH_ALEN))) {
+			if (!(memcmp(mac,
+				     priv->assoc_stainfo.sta_associated_bss[i],
+				     ETH_ALEN))) {
 				associatedsta = i;
 				break;
 			}
-- 
2.7.4

[PATCH 19/30] staging: wilc1000: fix line over 80 chars in wilc_create_wiphy() declaration

[Ajay Singh]

Fix line over 80 characters issue found by checkpatch.pl script in
function declaration.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
index c1a2421..a69103b 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.h
@@ -3,7 +3,8 @@
 #define NM_WFI_CFGOPERATIONS
 #include "wilc_wfi_netdevice.h"
 
-struct wireless_dev *wilc_create_wiphy(struct net_device *net, struct device *dev);
+struct wireless_dev *wilc_create_wiphy(struct net_device *net,
+				       struct device *dev);
 void wilc_free_wiphy(struct net_device *net);
 int wilc_deinit_host_int(struct net_device *net);
 int wilc_init_host_int(struct net_device *net);
-- 
2.7.4

[PATCH 20/30] staging: wilc1000: fix line over 80 characters in add_key()

[Ajay Singh]

Fix line over 80 character issue found by checkpatch.pl script in
add_key().

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 3deef5b..20ba9cc 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -905,8 +905,7 @@ static int wilc_wfi_cfg_copy_wpa_info(struct wilc_wfi_key *key_info,
 }
 
 static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
-		   bool pairwise,
-		   const u8 *mac_addr, struct key_params *params)
+		   bool pairwise, const u8 *mac_addr, struct key_params *params)
 
 {
 	s32 ret = 0, keylen = params->key_len;
@@ -954,6 +953,8 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
 	case WLAN_CIPHER_SUITE_CCMP:
 		if (priv->wdev->iftype == NL80211_IFTYPE_AP ||
 		    priv->wdev->iftype == NL80211_IFTYPE_P2P_GO) {
+			struct wilc_wfi_key *key;
+
 			ret = wilc_wfi_cfg_allocate_wpa_entry(priv, key_index);
 			if (ret)
 				return -ENOMEM;
@@ -973,21 +974,19 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
 
 				priv->wilc_groupkey = mode;
 
-				ret = wilc_wfi_cfg_copy_wpa_info(priv->wilc_gtk[key_index],
-								 params);
-				if (ret)
-					return -ENOMEM;
+				key = priv->wilc_gtk[key_index];
 			} else {
 				if (params->cipher == WLAN_CIPHER_SUITE_TKIP)
 					mode = ENCRYPT_ENABLED | WPA | TKIP;
 				else
 					mode = priv->wilc_groupkey | AES;
 
-				ret = wilc_wfi_cfg_copy_wpa_info(priv->wilc_ptk[key_index],
-								 params);
-				if (ret)
-					return -ENOMEM;
+				key = priv->wilc_ptk[key_index];
 			}
+			ret = wilc_wfi_cfg_copy_wpa_info(key, params);
+			if (ret)
+				return -ENOMEM;
+
 			op_mode = AP_MODE;
 		} else {
 			if (params->key_len > 16 &&
-- 
2.7.4

[PATCH 21/30] staging: wilc1000: fix line over 80 chars in scan()

[Ajay Singh]

Fix line over 80 characters issues found by checkpatch.pl script with
the help of local variable.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 20ba9cc..ec4fa1c 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -633,8 +633,11 @@ static int scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
 
 	priv->cfg_scanning = true;
 	if (request->n_channels <= MAX_NUM_SCANNED_NETWORKS) {
-		for (i = 0; i < request->n_channels; i++)
-			scan_ch_list[i] = (u8)ieee80211_frequency_to_channel(request->channels[i]->center_freq);
+		for (i = 0; i < request->n_channels; i++) {
+			u16 freq = request->channels[i]->center_freq;
+
+			scan_ch_list[i] = ieee80211_frequency_to_channel(freq);
+		}
 
 		if (request->n_ssids >= 1) {
 			if (wilc_wfi_cfg_alloc_fill_ssid(request,
-- 
2.7.4

[PATCH 22/30] staging: wilc1000: fix line over 80 chars issue in connect()

[Ajay Singh]

Fix line over 80 characters in connect() by using temporary variables.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 29 +++++++++++++----------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index ec4fa1c..00381c9 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -676,7 +676,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 	u32 sel_bssi_idx = UINT_MAX;
 	u8 u8security = NO_ENCRYPT;
 	enum AUTHTYPE auth_type = ANY;
-
+	u32 cipher_group;
 	struct wilc_priv *priv;
 	struct host_if_drv *wfi_drv;
 	struct network_info *nw_info = NULL;
@@ -724,32 +724,35 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 	memset(priv->wep_key, 0, sizeof(priv->wep_key));
 	memset(priv->wep_key_len, 0, sizeof(priv->wep_key_len));
 
-	if (sme->crypto.cipher_group != NO_ENCRYPT) {
-		if (sme->crypto.cipher_group == WLAN_CIPHER_SUITE_WEP40) {
+	cipher_group = sme->crypto.cipher_group;
+	if (cipher_group != NO_ENCRYPT) {
+		if (cipher_group == WLAN_CIPHER_SUITE_WEP40) {
 			u8security = ENCRYPT_ENABLED | WEP;
 
 			priv->wep_key_len[sme->key_idx] = sme->key_len;
-			memcpy(priv->wep_key[sme->key_idx], sme->key, sme->key_len);
+			memcpy(priv->wep_key[sme->key_idx], sme->key,
+			       sme->key_len);
 
 			wilc_set_wep_default_keyid(vif, sme->key_idx);
 			wilc_add_wep_key_bss_sta(vif, sme->key, sme->key_len,
 						 sme->key_idx);
-		} else if (sme->crypto.cipher_group == WLAN_CIPHER_SUITE_WEP104)   {
+		} else if (cipher_group == WLAN_CIPHER_SUITE_WEP104) {
 			u8security = ENCRYPT_ENABLED | WEP | WEP_EXTENDED;
 
 			priv->wep_key_len[sme->key_idx] = sme->key_len;
-			memcpy(priv->wep_key[sme->key_idx], sme->key, sme->key_len);
+			memcpy(priv->wep_key[sme->key_idx], sme->key,
+			       sme->key_len);
 
 			wilc_set_wep_default_keyid(vif, sme->key_idx);
 			wilc_add_wep_key_bss_sta(vif, sme->key, sme->key_len,
 						 sme->key_idx);
-		} else if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_2)   {
-			if (sme->crypto.cipher_group == WLAN_CIPHER_SUITE_TKIP)
+		} else if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_2) {
+			if (cipher_group == WLAN_CIPHER_SUITE_TKIP)
 				u8security = ENCRYPT_ENABLED | WPA2 | TKIP;
 			else
 				u8security = ENCRYPT_ENABLED | WPA2 | AES;
-		} else if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_1)   {
-			if (sme->crypto.cipher_group == WLAN_CIPHER_SUITE_TKIP)
+		} else if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_1) {
+			if (cipher_group == WLAN_CIPHER_SUITE_TKIP)
 				u8security = ENCRYPT_ENABLED | WPA | TKIP;
 			else
 				u8security = ENCRYPT_ENABLED | WPA | AES;
@@ -764,14 +767,16 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 	if ((sme->crypto.wpa_versions & NL80211_WPA_VERSION_1) ||
 	    (sme->crypto.wpa_versions & NL80211_WPA_VERSION_2)) {
 		for (i = 0; i < sme->crypto.n_ciphers_pairwise; i++) {
-			if (sme->crypto.ciphers_pairwise[i] == WLAN_CIPHER_SUITE_TKIP)
+			u32 ciphers_pairwise = sme->crypto.ciphers_pairwise[i];
+
+			if (ciphers_pairwise == WLAN_CIPHER_SUITE_TKIP)
 				u8security = u8security | TKIP;
 			else
 				u8security = u8security | AES;
 		}
 	}
 
-	switch (sme->auth_type)	{
+	switch (sme->auth_type) {
 	case NL80211_AUTHTYPE_OPEN_SYSTEM:
 		auth_type = OPEN_SYSTEM;
 		break;
-- 
2.7.4

[PATCH 23/30] staging: wilc1000: rename u8security to avoid datatype in variable name

[Ajay Singh]

Cleanup patch to avoid use of datatype in variable name to follow as
per linux coding style.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 00381c9..4600f4a 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -674,7 +674,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 	s32 ret = 0;
 	u32 i;
 	u32 sel_bssi_idx = UINT_MAX;
-	u8 u8security = NO_ENCRYPT;
+	u8 security = NO_ENCRYPT;
 	enum AUTHTYPE auth_type = ANY;
 	u32 cipher_group;
 	struct wilc_priv *priv;
@@ -727,7 +727,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 	cipher_group = sme->crypto.cipher_group;
 	if (cipher_group != NO_ENCRYPT) {
 		if (cipher_group == WLAN_CIPHER_SUITE_WEP40) {
-			u8security = ENCRYPT_ENABLED | WEP;
+			security = ENCRYPT_ENABLED | WEP;
 
 			priv->wep_key_len[sme->key_idx] = sme->key_len;
 			memcpy(priv->wep_key[sme->key_idx], sme->key,
@@ -737,7 +737,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 			wilc_add_wep_key_bss_sta(vif, sme->key, sme->key_len,
 						 sme->key_idx);
 		} else if (cipher_group == WLAN_CIPHER_SUITE_WEP104) {
-			u8security = ENCRYPT_ENABLED | WEP | WEP_EXTENDED;
+			security = ENCRYPT_ENABLED | WEP | WEP_EXTENDED;
 
 			priv->wep_key_len[sme->key_idx] = sme->key_len;
 			memcpy(priv->wep_key[sme->key_idx], sme->key,
@@ -748,14 +748,14 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 						 sme->key_idx);
 		} else if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_2) {
 			if (cipher_group == WLAN_CIPHER_SUITE_TKIP)
-				u8security = ENCRYPT_ENABLED | WPA2 | TKIP;
+				security = ENCRYPT_ENABLED | WPA2 | TKIP;
 			else
-				u8security = ENCRYPT_ENABLED | WPA2 | AES;
+				security = ENCRYPT_ENABLED | WPA2 | AES;
 		} else if (sme->crypto.wpa_versions & NL80211_WPA_VERSION_1) {
 			if (cipher_group == WLAN_CIPHER_SUITE_TKIP)
-				u8security = ENCRYPT_ENABLED | WPA | TKIP;
+				security = ENCRYPT_ENABLED | WPA | TKIP;
 			else
-				u8security = ENCRYPT_ENABLED | WPA | AES;
+				security = ENCRYPT_ENABLED | WPA | AES;
 		} else {
 			ret = -ENOTSUPP;
 			netdev_err(dev, "Not supported cipher\n");
@@ -770,9 +770,9 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 			u32 ciphers_pairwise = sme->crypto.ciphers_pairwise[i];
 
 			if (ciphers_pairwise == WLAN_CIPHER_SUITE_TKIP)
-				u8security = u8security | TKIP;
+				security = security | TKIP;
 			else
-				u8security = u8security | AES;
+				security = security | AES;
 		}
 	}
 
@@ -804,7 +804,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
 	ret = wilc_set_join_req(vif, nw_info->bssid, sme->ssid,
 				sme->ssid_len, sme->ie, sme->ie_len,
 				cfg_connect_result, (void *)priv,
-				u8security, auth_type,
+				security, auth_type,
 				nw_info->ch,
 				nw_info->join_params);
 	if (ret != 0) {
-- 
2.7.4

[PATCH 24/30] staging: wilc1000: refactor del_station() to avoid parenthesis misalignment

[Ajay Singh]

Refactor the code to fix open parenthesis alignment issue reported by
checkpatch.pl script in del_station().

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 4600f4a..7f49d60 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1997,6 +1997,7 @@ static int del_station(struct wiphy *wiphy, struct net_device *dev,
 	s32 ret = 0;
 	struct wilc_priv *priv;
 	struct wilc_vif *vif;
+	struct sta_info *info;
 
 	if (!wiphy)
 		return -EFAULT;
@@ -2004,16 +2005,17 @@ static int del_station(struct wiphy *wiphy, struct net_device *dev,
 	priv = wiphy_priv(wiphy);
 	vif = netdev_priv(dev);
 
-	if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
-		if (!mac)
-			ret = wilc_del_allstation(vif,
-				     priv->assoc_stainfo.sta_associated_bss);
+	if (!(vif->iftype == AP_MODE || vif->iftype == GO_MODE))
+		return ret;
 
-		ret = wilc_del_station(vif, mac);
+	info = &priv->assoc_stainfo;
 
-		if (ret)
-			netdev_err(dev, "Host delete station fail\n");
-	}
+	if (!mac)
+		ret = wilc_del_allstation(vif, info->sta_associated_bss);
+
+	ret = wilc_del_station(vif, mac);
+	if (ret)
+		netdev_err(dev, "Host delete station fail\n");
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH 24/30] staging: wilc1000: refactor del_station() to avoid parenthesis misalignment

[Dan Carpenter]

I feel sort of bad complaining about this patchset when your co-workers
already nit picked it to death...  :P

On Mon, May 07, 2018 at 02:13:28PM +0530, Ajay Singh wrote:
> Refactor the code to fix open parenthesis alignment issue reported by
> checkpatch.pl script in del_station().

I no idea what an "open parenthesis alignment issue" is.  It's sort of
surprising because I deal with checkpatch patches a lot.

> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 4600f4a..7f49d60 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1997,6 +1997,7 @@ static int del_station(struct wiphy *wiphy, struct net_device *dev,
>  	s32 ret = 0;
>  	struct wilc_priv *priv;
>  	struct wilc_vif *vif;
> +	struct sta_info *info;
>  
>  	if (!wiphy)
>  		return -EFAULT;
> @@ -2004,16 +2005,17 @@ static int del_station(struct wiphy *wiphy, struct net_device *dev,
>  	priv = wiphy_priv(wiphy);
>  	vif = netdev_priv(dev);
>  
> -	if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
> -		if (!mac)
> -			ret = wilc_del_allstation(vif,
> -				     priv->assoc_stainfo.sta_associated_bss);
> +	if (!(vif->iftype == AP_MODE || vif->iftype == GO_MODE))

I feel like this is better as:
	if (vif->iftype != AP_MODE && vif->iftype != GO_MODE)

> +		return ret;

What is "ret" here?  I haven't looked at this patch in context, but
it's probably zero.  Just return the literal.

regards,
dan carpenter

Re: [PATCH 24/30] staging: wilc1000: refactor del_station() to avoid parenthesis misalignment

[Ajay Singh]

Hi Dan,

On Tue, 15 May 2018 12:01:12 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> I feel sort of bad complaining about this patchset when your
> co-workers already nit picked it to death...  :P
> 
> On Mon, May 07, 2018 at 02:13:28PM +0530, Ajay Singh wrote:
> > Refactor the code to fix open parenthesis alignment issue reported
> > by checkpatch.pl script in del_station().  
> 
> I no idea what an "open parenthesis alignment issue" is.  It's sort of
> surprising because I deal with checkpatch patches a lot.

The exact issue description reported by checkpatch.pl was 

"CHECK: Alignment should match open parenthesis"

To resolve that issue tried by reducing the leading tabs before
wilc_del_allstations() call, which helped in resolving checkpatch
issue without introducing line over 80 chars.

> 
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18
> > ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> > 4600f4a..7f49d60 100644 ---
> > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -1997,6
> > +1997,7 @@ static int del_station(struct wiphy *wiphy, struct
> > net_device *dev, s32 ret = 0; struct wilc_priv *priv;
> >  	struct wilc_vif *vif;
> > +	struct sta_info *info;
> >  
> >  	if (!wiphy)
> >  		return -EFAULT;
> > @@ -2004,16 +2005,17 @@ static int del_station(struct wiphy *wiphy,
> > struct net_device *dev, priv = wiphy_priv(wiphy);
> >  	vif = netdev_priv(dev);
> >  
> > -	if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
> > -		if (!mac)
> > -			ret = wilc_del_allstation(vif,
> > -
> > priv->assoc_stainfo.sta_associated_bss);
> > +	if (!(vif->iftype == AP_MODE || vif->iftype == GO_MODE))  
> 
> I feel like this is better as:
> 	if (vif->iftype != AP_MODE && vif->iftype != GO_MODE)
> 

Thanks for your suggestion.
Currently the patch is accepted, i will check and try to include it in
future patch as per your inputs.

> > +		return ret;  
> 
> What is "ret" here?  I haven't looked at this patch in context, but
> it's probably zero.  Just return the literal.

The value for 'ret' is zero till this point, only if there is
failure to post the command in wilc_eneque_cmd() then 'ret' receives
-ENOMEM value in below code.



Regards,
Ajay

[PATCH 25/30] staging: wilc1000: fix line over 80 chars in wilc_sdio struct

[Ajay Singh]

Fix line over 80 chars issue found by checkpatch.pl script by placing
the comment message above the macro preprocessor.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_sdio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_sdio.c b/drivers/staging/wilc1000/wilc_sdio.c
index 211be73..4ab43f9 100644
--- a/drivers/staging/wilc1000/wilc_sdio.c
+++ b/drivers/staging/wilc1000/wilc_sdio.c
@@ -26,7 +26,8 @@ struct wilc_sdio {
 	bool irq_gpio;
 	u32 block_size;
 	int nint;
-#define MAX_NUN_INT_THRPT_ENH2 (5) /* Max num interrupts allowed in registers 0xf7, 0xf8 */
+/* Max num interrupts allowed in registers 0xf7, 0xf8 */
+#define MAX_NUN_INT_THRPT_ENH2 (5)
 	int has_thrpt_enh3;
 };
 
-- 
2.7.4

[PATCH 26/30] staging: wilc1000: added #define for setting radiotap header

[Ajay Singh]

Added new macro to resolve below checkpatch issues in linux_mon.

"Lines should not end with a '('"

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/linux_mon.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_mon.c b/drivers/staging/wilc1000/linux_mon.c
index c372e0f..1c7e6e1 100644
--- a/drivers/staging/wilc1000/linux_mon.c
+++ b/drivers/staging/wilc1000/linux_mon.c
@@ -22,6 +22,9 @@ static u8 bssid[6];
 #define IEEE80211_RADIOTAP_F_TX_FAIL	0x0001  /* failed due to excessive*/
 #define GET_PKT_OFFSET(a) (((a) >> 22) & 0x1ff)
 
+#define TX_RADIOTAP_PRESENT ((1 << IEEE80211_RADIOTAP_RATE) |	\
+			     (1 << IEEE80211_RADIOTAP_TX_FLAGS))
+
 void wilc_wfi_monitor_rx(u8 *buff, u32 size)
 {
 	u32 header, pkt_offset;
@@ -59,9 +62,7 @@ void wilc_wfi_monitor_rx(u8 *buff, u32 size)
 
 		cb_hdr->hdr.it_len = cpu_to_le16(sizeof(*cb_hdr));
 
-		cb_hdr->hdr.it_present = cpu_to_le32(
-				(1 << IEEE80211_RADIOTAP_RATE) |
-				(1 << IEEE80211_RADIOTAP_TX_FLAGS));
+		cb_hdr->hdr.it_present = cpu_to_le32(TX_RADIOTAP_PRESENT);
 
 		cb_hdr->rate = 5; /* txrate->bitrate / 5; */
 
@@ -177,9 +178,7 @@ static netdev_tx_t wilc_wfi_mon_xmit(struct sk_buff *skb,
 
 		cb_hdr->hdr.it_len = cpu_to_le16(sizeof(*cb_hdr));
 
-		cb_hdr->hdr.it_present = cpu_to_le32(
-				(1 << IEEE80211_RADIOTAP_RATE) |
-				(1 << IEEE80211_RADIOTAP_TX_FLAGS));
+		cb_hdr->hdr.it_present = cpu_to_le32(TX_RADIOTAP_PRESENT);
 
 		cb_hdr->rate = 5; /* txrate->bitrate / 5; */
 		cb_hdr->tx_flags = 0x0004;
-- 
2.7.4

[PATCH 27/30] staging: wilc1000: remove 'flag' argument from wilc_mac_indicate()

[Ajay Singh]

Remove 'flag' function parameter in wilc_mac_indicate() as only one
condition was handled using that parameter. Also removed unnecessary
call to wilc_mac_indicate() as no operation was performed in that
function call.
After above changes below macro are not required.

This changes also helped in resolving the line over 80 chars issue
found by checkatpch.pl script.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/linux_wlan.c         | 17 +++++++----------
 drivers/staging/wilc1000/wilc_wfi_netdevice.h |  2 +-
 drivers/staging/wilc1000/wilc_wlan.c          |  7 +------
 drivers/staging/wilc1000/wilc_wlan_if.h       |  3 ---
 4 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 7b883c0..02e6b13 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -176,19 +176,16 @@ static void deinit_irq(struct net_device *dev)
 	}
 }
 
-void wilc_mac_indicate(struct wilc *wilc, int flag)
+void wilc_mac_indicate(struct wilc *wilc)
 {
 	int status;
 
-	if (flag == WILC_MAC_INDICATE_STATUS) {
-		wilc_wlan_cfg_get_val(WID_STATUS,
-				      (unsigned char *)&status, 4);
-		if (wilc->mac_status == MAC_STATUS_INIT) {
-			wilc->mac_status = status;
-			complete(&wilc->sync_event);
-		} else {
-			wilc->mac_status = status;
-		}
+	wilc_wlan_cfg_get_val(WID_STATUS, (unsigned char *)&status, 4);
+	if (wilc->mac_status == MAC_STATUS_INIT) {
+		wilc->mac_status = status;
+		complete(&wilc->sync_event);
+	} else {
+		wilc->mac_status = status;
 	}
 }
 
diff --git a/drivers/staging/wilc1000/wilc_wfi_netdevice.h b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
index 8849924..607dae0 100644
--- a/drivers/staging/wilc1000/wilc_wfi_netdevice.h
+++ b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
@@ -182,7 +182,7 @@ struct wilc_wfi_mon_priv {
 };
 
 void wilc_frmw_to_linux(struct wilc *wilc, u8 *buff, u32 size, u32 pkt_offset);
-void wilc_mac_indicate(struct wilc *wilc, int flag);
+void wilc_mac_indicate(struct wilc *wilc);
 void wilc_netdev_cleanup(struct wilc *wilc);
 int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
 		     int gpio, const struct wilc_hif_func *ops);
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index 7147e0c..d4ebbf6 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -816,12 +816,7 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size)
 					if (wilc->cfg_seq_no == rsp.seq_no)
 						complete(&wilc->cfg_event);
 				} else if (rsp.type == WILC_CFG_RSP_STATUS) {
-					wilc_mac_indicate(wilc,
-							  WILC_MAC_INDICATE_STATUS);
-
-				} else if (rsp.type == WILC_CFG_RSP_SCAN) {
-					wilc_mac_indicate(wilc,
-							  WILC_MAC_INDICATE_SCAN);
+					wilc_mac_indicate(wilc);
 				}
 			}
 		}
diff --git a/drivers/staging/wilc1000/wilc_wlan_if.h b/drivers/staging/wilc1000/wilc_wlan_if.h
index aa0731e..e4a7bf5 100644
--- a/drivers/staging/wilc1000/wilc_wlan_if.h
+++ b/drivers/staging/wilc1000/wilc_wlan_if.h
@@ -47,9 +47,6 @@ struct sdio_cmd53 {
 	u32 block_size;
 };
 
-#define WILC_MAC_INDICATE_STATUS	0x1
-#define WILC_MAC_INDICATE_SCAN		0x2
-
 #define MAC_STATUS_INIT			-1
 #define MAC_STATUS_CONNECTED		1
 #define MAC_STATUS_DISCONNECTED		0
-- 
2.7.4

[PATCH 28/30] staging: wilc1000: added comments for mutex and spinlock_t

[Ajay Singh]

Added comments for mutex and spinlock_t to avoid checkpatch.pl script.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.h     | 2 +-
 drivers/staging/wilc1000/wilc_wfi_netdevice.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
index 7a26f34..5e00dde 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -271,7 +271,7 @@ struct host_if_drv {
 
 	u8 assoc_bssid[ETH_ALEN];
 	struct cfg_param_attr cfg_values;
-
+/*lock to protect concurrent setting of cfg params*/
 	struct mutex cfg_values_lock;
 	struct completion comp_test_key_block;
 	struct completion comp_test_disconn_block;
diff --git a/drivers/staging/wilc1000/wilc_wfi_netdevice.h b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
index 607dae0..e517768 100644
--- a/drivers/staging/wilc1000/wilc_wfi_netdevice.h
+++ b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
@@ -131,10 +131,11 @@ struct wilc {
 	u8 vif_num;
 	struct wilc_vif *vif[NUM_CONCURRENT_IFC];
 	u8 open_ifcs;
-
+/*protect head of transmit queue*/
 	struct mutex txq_add_to_head_cs;
+/*protect txq_entry_t transmit queue*/
 	spinlock_t txq_spinlock;
-
+/*protect rxq_entry_t receiver queue*/
 	struct mutex rxq_cs;
 	struct mutex hif_cs;
 
-- 
2.7.4

Re: [PATCH 28/30] staging: wilc1000: added comments for mutex and spinlock_t

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Added comments for mutex and spinlock_t to avoid checkpatch.pl script.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.h     | 2 +-
>  drivers/staging/wilc1000/wilc_wfi_netdevice.h | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
> index 7a26f34..5e00dde 100644
> --- a/drivers/staging/wilc1000/host_interface.h
> +++ b/drivers/staging/wilc1000/host_interface.h
> @@ -271,7 +271,7 @@ struct host_if_drv {
>  
>  	u8 assoc_bssid[ETH_ALEN];
>  	struct cfg_param_attr cfg_values;
> -
> +/*lock to protect concurrent setting of cfg params*/
>  	struct mutex cfg_values_lock;
>  	struct completion comp_test_key_block;
>  	struct completion comp_test_disconn_block;
> diff --git a/drivers/staging/wilc1000/wilc_wfi_netdevice.h b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
> index 607dae0..e517768 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_netdevice.h
> +++ b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
> @@ -131,10 +131,11 @@ struct wilc {
>  	u8 vif_num;
>  	struct wilc_vif *vif[NUM_CONCURRENT_IFC];
>  	u8 open_ifcs;
> -
> +/*protect head of transmit queue*/

Add an extra tag to align it with the rest of the structure's members. 

>  	struct mutex txq_add_to_head_cs;
> +/*protect txq_entry_t transmit queue*/
Ditto

>  	spinlock_t txq_spinlock;
> -
> +/*protect rxq_entry_t receiver queue*/

Ditto

>  	struct mutex rxq_cs;
>  	struct mutex hif_cs;
>  
> 

[PATCH 29/30] staging: wilc1000: remove unused 'lock' varible in 'wilc_priv' structure

[Ajay Singh]

Cleanup patch to remove the unused variable from 'wilc_priv' structure.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_wfi_netdevice.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_netdevice.h b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
index e517768..3875065 100644
--- a/drivers/staging/wilc1000/wilc_wfi_netdevice.h
+++ b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
@@ -78,7 +78,6 @@ struct wilc_priv {
 	u8 monitor_flag;
 	int status;
 	struct sk_buff *skb;
-	spinlock_t lock;
 	struct net_device *dev;
 	struct host_if_drv *hif_drv;
 	struct host_if_pmkid_attr pmkid_list;
-- 
2.7.4

[PATCH 30/30] staging: wilc1000: rename s8idxarray to avoid datatype in variable name

[Ajay Singh]

Cleanup patch to have variable names as per linux coding style.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index ad9270e..e27010c 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1543,7 +1543,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 	struct wid wid;
 	struct wid wid_list[5];
 	u8 *key_buf;
-	s8 s8idxarray[1];
+	s8 idxarray[1];
 	struct host_if_drv *hif_drv = vif->hif_drv;
 
 	switch (hif_key->type) {
@@ -1610,8 +1610,8 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
 			wid.id = (u16)WID_REMOVE_WEP_KEY;
 			wid.type = WID_STR;
 
-			s8idxarray[0] = (s8)hif_key->attr.wep.index;
-			wid.val = s8idxarray;
+			idxarray[0] = (s8)hif_key->attr.wep.index;
+			wid.val = idxarray;
 			wid.size = 1;
 
 			result = wilc_send_config_pkt(vif, SET_CFG,
-- 
2.7.4

Re: [PATCH 30/30] staging: wilc1000: rename s8idxarray to avoid datatype in variable name

[Claudiu Beznea]

On 07.05.2018 11:43, Ajay Singh wrote:
> Cleanup patch to have variable names as per linux coding style.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index ad9270e..e27010c 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -1543,7 +1543,7 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
>  	struct wid wid;
>  	struct wid wid_list[5];
>  	u8 *key_buf;
> -	s8 s8idxarray[1];
> +	s8 idxarray[1];
>  	struct host_if_drv *hif_drv = vif->hif_drv;
>  
>  	switch (hif_key->type) {
> @@ -1610,8 +1610,8 @@ static int handle_key(struct wilc_vif *vif, struct key_attr *hif_key)
>  			wid.id = (u16)WID_REMOVE_WEP_KEY;
>  			wid.type = WID_STR;
>  
> -			s8idxarray[0] = (s8)hif_key->attr.wep.index;
> -			wid.val = s8idxarray;
> +			idxarray[0] = (s8)hif_key->attr.wep.index;

I think you can get rid of idxarray at all.

> +			wid.val = idxarray;>  			wid.size = 1;
>  
>  			result = wilc_send_config_pkt(vif, SET_CFG,
> 

Re: [PATCH 30/30] staging: wilc1000: rename s8idxarray to avoid datatype in variable name

[Ajay Singh]

On Wed, 9 May 2018 16:42:20 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 07.05.2018 11:43, Ajay Singh wrote:
> > Cleanup patch to have variable names as per linux coding style.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index ad9270e..e27010c
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -1543,7 +1543,7 @@ static int handle_key(struct wilc_vif *vif,
> > struct key_attr *hif_key) struct wid wid;
> >  	struct wid wid_list[5];
> >  	u8 *key_buf;
> > -	s8 s8idxarray[1];
> > +	s8 idxarray[1];
> >  	struct host_if_drv *hif_drv = vif->hif_drv;
> >  
> >  	switch (hif_key->type) {
> > @@ -1610,8 +1610,8 @@ static int handle_key(struct wilc_vif *vif,
> > struct key_attr *hif_key) wid.id = (u16)WID_REMOVE_WEP_KEY;
> >  			wid.type = WID_STR;
> >  
> > -			s8idxarray[0] =
> > (s8)hif_key->attr.wep.index;
> > -			wid.val = s8idxarray;
> > +			idxarray[0] =
> > (s8)hif_key->attr.wep.index;  
> 
> I think you can get rid of idxarray at all.
> 

Yes, we can remove idxarray variable. I will update and resubmit this
patch.

> > +			wid.val = idxarray;>
> > 			wid.size = 1; 
> >  			result = wilc_send_config_pkt(vif, SET_CFG,
> >