Re: [PATCH 06/11] staging: wilc1000: refactor mgmt_tx to fix line over 80 chars

[Claudiu Beznea]

On 20.03.2018 18:55, Ajay Singh wrote:
> Refactor mgmt_tx() to fix line over 80 characters issue. Split the
> function to avoid the checkpatch.pl warning. Returning the same error
> code in case of memory allocation failure.
> 
> Signed-off-by: Ajay Singh 
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 187 
> +-
>  1 file changed, 111 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index d7ff0a9..9950ca5 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1555,6 +1555,58 @@ static int cancel_remain_on_channel(struct wiphy 
> *wiphy,
>   priv->remain_on_ch_params.listen_session_id);
>  }
>  
> +static void wilc_wfi_cfg_tx_vendor_spec(struct p2p_mgmt_data *mgmt_tx,
> + struct cfg80211_mgmt_tx_params *params,
> + u8 iftype, u32 buf_len)
> +{
> + const u8 *buf = params->buf;
> + size_t len = params->len;
> + u32 i;
> + u8 subtype = buf[P2P_PUB_ACTION_SUBTYPE];
> +
> + if (subtype == GO_NEG_REQ || subtype == GO_NEG_RSP) {
> + if (p2p_local_random == 1 &&
> + p2p_recv_random < p2p_local_random) {
I think you can inner this if in the above one. Your choice.

> + get_random_bytes(_local_random, 1);
> + p2p_local_random++;
> + }
> + }
> +
> + if (p2p_local_random > p2p_recv_random && (subtype == GO_NEG_REQ ||
> +subtype == GO_NEG_RSP ||
> +subtype == P2P_INV_REQ ||
> +subtype == P2P_INV_RSP)) {
> + bool found = false;
> + bool oper_ch = false;
> +
> + for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
> + if (buf[i] == P2PELEM_ATTR_ID &&
> + !(memcmp(p2p_oui, [i + 2], 4))) {
You can remove "(" ")" around memcmp. Again, your choice.

> + if (subtype == P2P_INV_REQ ||
> + subtype == P2P_INV_RSP)
> + oper_ch = true;
> +
> + found = true;
> + break;
> + }
> + }
> +
> + if (found)
> + wilc_wfi_cfg_parse_tx_action(_tx->buff[i + 6],
> +  len - (i + 6), oper_ch,
> +  iftype);
> +
> + if (subtype != P2P_INV_REQ && subtype != P2P_INV_RSP) {
> + int vendor_spec_len = sizeof(p2p_vendor_spec);
> +
> + memcpy(_tx->buff[len], p2p_vendor_spec,
> +vendor_spec_len);
> + mgmt_tx->buff[len + vendor_spec_len] = p2p_local_random;
> + mgmt_tx->size = buf_len;
> + }
> + }
> +}
Looking at wilc_wfi_cfg_tx_vendor_spec() and
wilc_wfi_cfg_parse_rx_vendor_spec() from patch 4 from this series I can see
that conditions in these two are mostly the same but you refactor them
differently. I think the approach in wilc_wfi_cfg_parse_rx_vendor_spec()
from patch 4 is better and can help you avoid usage of bool variables like
found and oper_ch.

For instance, you can have:

static void wilc_wfi_cfg_tx_vendor_spec(struct p2p_mgmt_data *mgmt_tx,
struct cfg80211_mgmt_tx_params *params,
u8 iftype, u32 buf_len)
{
const u8 *buf = params->buf;
size_t len = params->len;
u32 i;
u8 subtype = buf[P2P_PUB_ACTION_SUBTYPE];

if ((subtype == GO_NEG_REQ || subtype == GO_NEG_RSP) &&
p2p_local_random == 1 && p2p_recv_random < p2p_local_random) {
get_random_bytes(_local_random, 1);
p2p_local_random++;
}

if (p2p_local_random <= p2p_recv_random)
return;

if (subtype == GO_NEG_REQ || subtype == GO_NEG_RSP ||
subtype == P2P_INV_REQ || subtype == P2P_INV_RSP) {
for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
if (buf[i] != P2PELEM_ATTR_ID ||
memcmp(p2p_oui, [i + 2], 4))
continue;

wilc_wfi_cfg_parse_tx_action(_tx->buff[i + 6],
 len - (i + 6),
 (subtype == P2P_INV_REQ ||
  subtype == P2P_INV_RSP),
  iftype);

Re: [PATCH 06/11] staging: wilc1000: refactor mgmt_tx to fix line over 80 chars

[Dan Carpenter]

This would have been easier for me if it were split up slightly
different again.

This patch is fine.  I have a couple comments for future patches which
you are free to ignore if you want because it's mostly just my personal
taste.

On Tue, Mar 20, 2018 at 10:25:39PM +0530, Ajay Singh wrote:
> + if (subtype != P2P_INV_REQ && subtype != P2P_INV_RSP) {
> + int vendor_spec_len = sizeof(p2p_vendor_spec);

I'm not a huge fan of making shorter names for sizeof()s just to get
around the 80 character rule.  The 80 character rule is ultimately
supposed to make the code more readable, and this is making less
readable so it's maybe better to just ignore the rule.

> +
> + memcpy(_tx->buff[len], p2p_vendor_spec,
> +vendor_spec_len);
> + mgmt_tx->buff[len + vendor_spec_len] = p2p_local_random;
> + mgmt_tx->size = buf_len;
> + }
> + }
> +}
> +
>  static int mgmt_tx(struct wiphy *wiphy,
>  struct wireless_dev *wdev,
>  struct cfg80211_mgmt_tx_params *params,
> @@ -1568,9 +1620,9 @@ static int mgmt_tx(struct wiphy *wiphy,
>   struct p2p_mgmt_data *mgmt_tx;
>   struct wilc_priv *priv;
>   struct host_if_drv *wfi_drv;
> - u32 i;
>   struct wilc_vif *vif;
>   u32 buf_len = len + sizeof(p2p_vendor_spec) + sizeof(p2p_local_random);
> + int ret = 0;
>  
>   vif = netdev_priv(wdev->netdev);
>   priv = wiphy_priv(wiphy);
> @@ -1580,92 +1632,75 @@ static int mgmt_tx(struct wiphy *wiphy,
>   priv->tx_cookie = *cookie;
>   mgmt = (const struct ieee80211_mgmt *)buf;
>  
> - if (ieee80211_is_mgmt(mgmt->frame_control)) {
> - mgmt_tx = kmalloc(sizeof(struct p2p_mgmt_data), GFP_KERNEL);
> - if (!mgmt_tx)
> - return -EFAULT;
> + if (!ieee80211_is_mgmt(mgmt->frame_control))
> + goto out;


I hate this "goto out;".  My first question when I see it is "what does
goto out; do?"  It's a kind of vague label name.  So I have to scroll
down to the bottom of the function.  And then I'm like "Oh, it doesn't
do anything.  Well that was a waste of time."  And then it occurs to me,
"Huh, what is 'ret' set to?" So now I have to scroll all the way to the
very start of the function...  All this scrolling could be avoided if we
just did a direct "return 0;"

regards,
dan carpenter

[PATCH 06/11] staging: wilc1000: refactor mgmt_tx to fix line over 80 chars

[Ajay Singh]

Refactor mgmt_tx() to fix line over 80 characters issue. Split the
function to avoid the checkpatch.pl warning. Returning the same error
code in case of memory allocation failure.

Signed-off-by: Ajay Singh 
---
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 187 +-
 1 file changed, 111 insertions(+), 76 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index d7ff0a9..9950ca5 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1555,6 +1555,58 @@ static int cancel_remain_on_channel(struct wiphy *wiphy,
priv->remain_on_ch_params.listen_session_id);
 }
 
+static void wilc_wfi_cfg_tx_vendor_spec(struct p2p_mgmt_data *mgmt_tx,
+   struct cfg80211_mgmt_tx_params *params,
+   u8 iftype, u32 buf_len)
+{
+   const u8 *buf = params->buf;
+   size_t len = params->len;
+   u32 i;
+   u8 subtype = buf[P2P_PUB_ACTION_SUBTYPE];
+
+   if (subtype == GO_NEG_REQ || subtype == GO_NEG_RSP) {
+   if (p2p_local_random == 1 &&
+   p2p_recv_random < p2p_local_random) {
+   get_random_bytes(_local_random, 1);
+   p2p_local_random++;
+   }
+   }
+
+   if (p2p_local_random > p2p_recv_random && (subtype == GO_NEG_REQ ||
+  subtype == GO_NEG_RSP ||
+  subtype == P2P_INV_REQ ||
+  subtype == P2P_INV_RSP)) {
+   bool found = false;
+   bool oper_ch = false;
+
+   for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
+   if (buf[i] == P2PELEM_ATTR_ID &&
+   !(memcmp(p2p_oui, [i + 2], 4))) {
+   if (subtype == P2P_INV_REQ ||
+   subtype == P2P_INV_RSP)
+   oper_ch = true;
+
+   found = true;
+   break;
+   }
+   }
+
+   if (found)
+   wilc_wfi_cfg_parse_tx_action(_tx->buff[i + 6],
+len - (i + 6), oper_ch,
+iftype);
+
+   if (subtype != P2P_INV_REQ && subtype != P2P_INV_RSP) {
+   int vendor_spec_len = sizeof(p2p_vendor_spec);
+
+   memcpy(_tx->buff[len], p2p_vendor_spec,
+  vendor_spec_len);
+   mgmt_tx->buff[len + vendor_spec_len] = p2p_local_random;
+   mgmt_tx->size = buf_len;
+   }
+   }
+}
+
 static int mgmt_tx(struct wiphy *wiphy,
   struct wireless_dev *wdev,
   struct cfg80211_mgmt_tx_params *params,
@@ -1568,9 +1620,9 @@ static int mgmt_tx(struct wiphy *wiphy,
struct p2p_mgmt_data *mgmt_tx;
struct wilc_priv *priv;
struct host_if_drv *wfi_drv;
-   u32 i;
struct wilc_vif *vif;
u32 buf_len = len + sizeof(p2p_vendor_spec) + sizeof(p2p_local_random);
+   int ret = 0;
 
vif = netdev_priv(wdev->netdev);
priv = wiphy_priv(wiphy);
@@ -1580,92 +1632,75 @@ static int mgmt_tx(struct wiphy *wiphy,
priv->tx_cookie = *cookie;
mgmt = (const struct ieee80211_mgmt *)buf;
 
-   if (ieee80211_is_mgmt(mgmt->frame_control)) {
-   mgmt_tx = kmalloc(sizeof(struct p2p_mgmt_data), GFP_KERNEL);
-   if (!mgmt_tx)
-   return -EFAULT;
+   if (!ieee80211_is_mgmt(mgmt->frame_control))
+   goto out;
 
-   mgmt_tx->buff = kmalloc(buf_len, GFP_KERNEL);
-   if (!mgmt_tx->buff) {
-   kfree(mgmt_tx);
-   return -ENOMEM;
-   }
+   mgmt_tx = kmalloc(sizeof(struct p2p_mgmt_data), GFP_KERNEL);
+   if (!mgmt_tx) {
+   ret = -ENOMEM;
+   goto out;
+   }
+
+   mgmt_tx->buff = kmalloc(buf_len, GFP_KERNEL);
+   if (!mgmt_tx->buff) {
+   ret = -ENOMEM;
+   kfree(mgmt_tx);
+   goto out;
+   }
+
+   memcpy(mgmt_tx->buff, buf, len);
+   mgmt_tx->size = len;
+
+   if (ieee80211_is_probe_resp(mgmt->frame_control)) {
+   wilc_set_mac_chnl_num(vif, chan->hw_value);
+   curr_channel = chan->hw_value;
+   goto out_txq_add_pkt;
+   }
 
-   memcpy(mgmt_tx->buff, buf, len);
-   mgmt_tx->size = len;
+   if (!ieee80211_is_action(mgmt->frame_control))
+   goto out_txq_add_pkt;
 
-   if