This patch series contains fixes to avoid checkpatch issues and removed
unused code. Few patch contains changes related to NULL check and freeing of
dynamically allocated memory.
Ajay Singh (11):
staging: wilc1000: refactor scan() to free kmalloc memory on failure
cases
staging: wilc1000: removed unused global variables for gtk and ptk
information
staging: wilc1000: remove line over 80 char warnings in
set_wiphy_params()
staging: wilc1000: refactor WILC_WFI_p2p_rx() to avoid line over 80
char
staging: wilc1000: rename WILC_WFI_p2p_rx & s32Freq to avoid camelCase
staging: wilc1000: refactor mgmt_tx to fix line over 80 chars
staging: wilc1000: rename hAgingTimer to avoid camelCase issue
staging: wilc1000: fix line over 80 char issue in clear_shadow_scan()
staging: wilc1000: remove line over 80 char in cfg_connect_result()
staging: wilc1000: remove unused 'struct add_key_params'
staging: wilc1000: remove line over 80 char warning in few functions
drivers/staging/wilc1000/linux_wlan.c | 2 +-
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 577 +++++++++++-----------
drivers/staging/wilc1000/wilc_wlan.h | 2 +-
3 files changed, 297 insertions(+), 284 deletions(-)
--
2.7.4
Hi Dan,
On Wed, 21 Mar 2018 10:51:16 +0300
Dan Carpenter <[email protected]> wrote:
> These look good. I've reviewed them all.
>
> Reviewed-by: Dan Carpenter <[email protected]>
Thanks for reviewing all the patches.
>
> I had some small process complaints but it doesn't make life easier for
> me if you resend them and I have to review everything twice.... :P
>
In future patches, I will take care as per your suggestions. Instead of
resubmitting this patch series, I will include the additional changes
required for [PATCH 01/11] in separate patch series.
Regards,
Ajay
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(&mgmt_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
Fix 'Avoid camelCase' issue found by checkpatch.pl script.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 2 +-
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18 +++++++++---------
drivers/staging/wilc1000/wilc_wlan.h | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 38a83bd..2236967 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1166,7 +1166,7 @@ void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size)
vif = netdev_priv(wilc->vif[1]->ndev);
if ((buff[0] == vif->frame_reg[0].type && vif->frame_reg[0].reg) ||
(buff[0] == vif->frame_reg[1].type && vif->frame_reg[1].reg))
- WILC_WFI_p2p_rx(wilc->vif[1]->ndev, buff, size);
+ wilc_wfi_p2p_rx(wilc->vif[1]->ndev, buff, size);
}
void wilc_netdev_cleanup(struct wilc *wilc)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 090d59d..d7ff0a9 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -360,7 +360,7 @@ static void cfg_scan_result(enum scan_event scan_event,
{
struct wilc_priv *priv;
struct wiphy *wiphy;
- s32 s32Freq;
+ s32 freq;
struct ieee80211_channel *channel;
struct cfg80211_bss *bss = NULL;
@@ -379,9 +379,9 @@ static void cfg_scan_result(enum scan_event scan_event,
((s32)network_info->rssi * 100) > 100))
return;
- s32Freq = ieee80211_channel_to_frequency((s32)network_info->ch,
- NL80211_BAND_2GHZ);
- channel = ieee80211_get_channel(wiphy, s32Freq);
+ freq = ieee80211_channel_to_frequency((s32)network_info->ch,
+ NL80211_BAND_2GHZ);
+ channel = ieee80211_get_channel(wiphy, freq);
if (!channel)
return;
@@ -1403,12 +1403,12 @@ static void wilc_wfi_cfg_parse_rx_vendor_spec(struct wilc_priv *priv, u8 *buff,
}
}
-void WILC_WFI_p2p_rx(struct net_device *dev, u8 *buff, u32 size)
+void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size)
{
struct wilc_priv *priv;
u32 header, pkt_offset;
struct host_if_drv *wfi_drv;
- s32 s32Freq;
+ s32 freq;
priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
wfi_drv = (struct host_if_drv *)priv->hif_drv;
@@ -1429,10 +1429,10 @@ void WILC_WFI_p2p_rx(struct net_device *dev, u8 *buff, u32 size)
return;
}
- s32Freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ);
+ freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ);
if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) {
- cfg80211_rx_mgmt(priv->wdev, s32Freq, 0, buff, size, 0);
+ cfg80211_rx_mgmt(priv->wdev, freq, 0, buff, size, 0);
return;
}
@@ -1468,7 +1468,7 @@ void WILC_WFI_p2p_rx(struct net_device *dev, u8 *buff, u32 size)
}
}
- cfg80211_rx_mgmt(priv->wdev, s32Freq, 0, buff, size, 0);
+ cfg80211_rx_mgmt(priv->wdev, freq, 0, buff, size, 0);
}
static void wilc_wfi_mgmt_tx_complete(void *priv, int status)
diff --git a/drivers/staging/wilc1000/wilc_wlan.h b/drivers/staging/wilc1000/wilc_wlan.h
index fa157a6..4abbfa7 100644
--- a/drivers/staging/wilc1000/wilc_wlan.h
+++ b/drivers/staging/wilc1000/wilc_wlan.h
@@ -300,7 +300,7 @@ void wilc_enable_tcp_ack_filter(bool value);
int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc);
int wilc_mac_xmit(struct sk_buff *skb, struct net_device *dev);
-void WILC_WFI_p2p_rx(struct net_device *dev, u8 *buff, u32 size);
+void wilc_wfi_p2p_rx(struct net_device *dev, u8 *buff, u32 size);
void host_wakeup_notify(struct wilc *wilc);
void host_sleep_notify(struct wilc *wilc);
extern bool wilc_enable_ps;
--
2.7.4
Fix 'line over 80 characters' issue found by checkpatch.pl script.
Refactor and split the function to avoid the checkpatch reported issues.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 145 ++++++++++++----------
1 file changed, 82 insertions(+), 63 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 9d35a08..090d59d 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1365,12 +1365,49 @@ static void wilc_wfi_cfg_parse_tx_action(u8 *buf, u32 len, bool oper_ch,
op_channel_attr_index);
}
+static void wilc_wfi_cfg_parse_rx_vendor_spec(struct wilc_priv *priv, u8 *buff,
+ u32 size)
+{
+ int i;
+ u8 subtype;
+ struct wilc_vif *vif = netdev_priv(priv->dev);
+
+ subtype = buff[P2P_PUB_ACTION_SUBTYPE];
+ if ((subtype == GO_NEG_REQ || subtype == GO_NEG_RSP) && !wilc_ie) {
+ for (i = P2P_PUB_ACTION_SUBTYPE; i < size; i++) {
+ if (!memcmp(p2p_vendor_spec, &buff[i], 6)) {
+ p2p_recv_random = buff[i + 6];
+ wilc_ie = true;
+ break;
+ }
+ }
+ }
+
+ if (p2p_local_random <= p2p_recv_random) {
+ netdev_dbg(vif->ndev,
+ "PEER WILL BE GO LocaRand=%02x RecvRand %02x\n",
+ 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 < size; i++) {
+ if (buff[i] == P2PELEM_ATTR_ID &&
+ !(memcmp(p2p_oui, &buff[i + 2], 4))) {
+ wilc_wfi_cfg_parse_rx_action(&buff[i + 6],
+ size - (i + 6));
+ break;
+ }
+ }
+ }
+}
+
void WILC_WFI_p2p_rx(struct net_device *dev, u8 *buff, u32 size)
{
struct wilc_priv *priv;
u32 header, pkt_offset;
struct host_if_drv *wfi_drv;
- u32 i = 0;
s32 s32Freq;
priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
@@ -1381,75 +1418,57 @@ void WILC_WFI_p2p_rx(struct net_device *dev, u8 *buff, u32 size)
pkt_offset = GET_PKT_OFFSET(header);
if (pkt_offset & IS_MANAGMEMENT_CALLBACK) {
- if (buff[FRAME_TYPE_ID] == IEEE80211_STYPE_PROBE_RESP) {
- cfg80211_mgmt_tx_status(priv->wdev, priv->tx_cookie, buff, size, true, GFP_KERNEL);
- return;
- } else {
- if (pkt_offset & IS_MGMT_STATUS_SUCCES)
- cfg80211_mgmt_tx_status(priv->wdev, priv->tx_cookie, buff, size, true, GFP_KERNEL);
- else
- cfg80211_mgmt_tx_status(priv->wdev, priv->tx_cookie, buff, size, false, GFP_KERNEL);
- return;
- }
- } else {
- s32Freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ);
+ bool ack = false;
- if (ieee80211_is_action(buff[FRAME_TYPE_ID])) {
- if (priv->cfg_scanning && time_after_eq(jiffies, (unsigned long)wfi_drv->p2p_timeout)) {
- netdev_dbg(dev, "Receiving action wrong ch\n");
- return;
- }
- if (buff[ACTION_CAT_ID] == PUB_ACTION_ATTR_ID) {
- switch (buff[ACTION_SUBTYPE_ID]) {
- case GAS_INITIAL_REQ:
- break;
+ if (buff[FRAME_TYPE_ID] == IEEE80211_STYPE_PROBE_RESP ||
+ pkt_offset & IS_MGMT_STATUS_SUCCES)
+ ack = true;
- case GAS_INITIAL_RSP:
- break;
+ cfg80211_mgmt_tx_status(priv->wdev, priv->tx_cookie, buff, size,
+ ack, GFP_KERNEL);
+ return;
+ }
- case PUBLIC_ACT_VENDORSPEC:
- if (!memcmp(p2p_oui, &buff[ACTION_SUBTYPE_ID + 1], 4)) {
- if ((buff[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buff[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP)) {
- if (!wilc_ie) {
- for (i = P2P_PUB_ACTION_SUBTYPE; i < size; i++) {
- if (!memcmp(p2p_vendor_spec, &buff[i], 6)) {
- p2p_recv_random = buff[i + 6];
- wilc_ie = true;
- break;
- }
- }
- }
- }
- if (p2p_local_random > p2p_recv_random) {
- if ((buff[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buff[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP ||
- buff[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_REQ || buff[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_RSP)) {
- for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < size; i++) {
- if (buff[i] == P2PELEM_ATTR_ID && !(memcmp(p2p_oui, &buff[i + 2], 4))) {
- wilc_wfi_cfg_parse_rx_action(&buff[i + 6], size - (i + 6));
- break;
- }
- }
- }
- } else {
- netdev_dbg(dev, "PEER WILL BE GO LocaRand=%02x RecvRand %02x\n", p2p_local_random, p2p_recv_random);
- }
- }
+ s32Freq = ieee80211_channel_to_frequency(curr_channel, NL80211_BAND_2GHZ);
- if ((buff[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buff[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP) && (wilc_ie)) {
- cfg80211_rx_mgmt(priv->wdev, s32Freq, 0, buff, size - 7, 0);
- return;
- }
- break;
+ if (!ieee80211_is_action(buff[FRAME_TYPE_ID])) {
+ cfg80211_rx_mgmt(priv->wdev, s32Freq, 0, buff, size, 0);
+ return;
+ }
- default:
- netdev_dbg(dev, "NOT HANDLED PUBLIC ACTION FRAME TYPE:%x\n", buff[ACTION_SUBTYPE_ID]);
- break;
- }
- }
- }
+ if (priv->cfg_scanning &&
+ time_after_eq(jiffies, (unsigned long)wfi_drv->p2p_timeout)) {
+ netdev_dbg(dev, "Receiving action wrong ch\n");
+ return;
+ }
+ if (buff[ACTION_CAT_ID] == PUB_ACTION_ATTR_ID) {
+ u8 subtype = buff[P2P_PUB_ACTION_SUBTYPE];
- cfg80211_rx_mgmt(priv->wdev, s32Freq, 0, buff, size, 0);
+ switch (buff[ACTION_SUBTYPE_ID]) {
+ case GAS_INITIAL_REQ:
+ case GAS_INITIAL_RSP:
+ break;
+
+ case PUBLIC_ACT_VENDORSPEC:
+ if (!memcmp(p2p_oui, &buff[ACTION_SUBTYPE_ID + 1], 4))
+ wilc_wfi_cfg_parse_rx_vendor_spec(priv, buff,
+ size);
+
+ if ((subtype == GO_NEG_REQ || subtype == GO_NEG_RSP) &&
+ wilc_ie)
+ size -= 7;
+
+ break;
+
+ default:
+ netdev_dbg(dev,
+ "NOT HANDLED PUBLIC ACTION FRAME TYPE:%x\n",
+ buff[ACTION_SUBTYPE_ID]);
+ break;
+ }
}
+
+ cfg80211_rx_mgmt(priv->wdev, s32Freq, 0, buff, size, 0);
}
static void wilc_wfi_mgmt_tx_complete(void *priv, int status)
--
2.7.4
Fix 'line over 80 characters' issues reported by checkpatch.pl script in
cfg_connect_result().
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 34 +++++++++++++++--------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 1b6fe64..af1b8aa 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -460,6 +460,17 @@ static void cfg_scan_result(enum scan_event scan_event,
}
}
+static inline bool wilc_wfi_cfg_scan_time_expired(int i)
+{
+ unsigned long now = jiffies;
+
+ if (time_after(now, last_scanned_shadow[i].time_scan_cached +
+ (unsigned long)(nl80211_SCAN_RESULT_EXPIRE - (1 * HZ))))
+ return true;
+ else
+ return false;
+}
+
int wilc_connecting;
static void cfg_connect_result(enum conn_event conn_disconn_evt,
@@ -505,17 +516,14 @@ static void cfg_connect_result(enum conn_event conn_disconn_evt,
bool scan_refresh = false;
u32 i;
- memcpy(priv->associated_bss, conn_info->bssid, ETH_ALEN);
+ memcpy(priv->associated_bss, conn_info->bssid,
+ ETH_ALEN);
for (i = 0; i < last_scanned_cnt; i++) {
if (memcmp(last_scanned_shadow[i].bssid,
conn_info->bssid,
ETH_ALEN) == 0) {
- unsigned long now = jiffies;
-
- if (time_after(now,
- last_scanned_shadow[i].time_scan_cached +
- (unsigned long)(nl80211_SCAN_RESULT_EXPIRE - (1 * HZ))))
+ if (wilc_wfi_cfg_scan_time_expired(i))
scan_refresh = true;
break;
@@ -527,9 +535,11 @@ static void cfg_connect_result(enum conn_event conn_disconn_evt,
}
cfg80211_connect_result(dev, conn_info->bssid,
- conn_info->req_ies, conn_info->req_ies_len,
- conn_info->resp_ies, conn_info->resp_ies_len,
- connect_status, GFP_KERNEL);
+ conn_info->req_ies,
+ conn_info->req_ies_len,
+ conn_info->resp_ies,
+ conn_info->resp_ies_len, connect_status,
+ GFP_KERNEL);
} else if (conn_disconn_evt == CONN_DISCONN_EVENT_DISCONN_NOTIF) {
wilc_optaining_ip = false;
p2p_local_random = 0x01;
@@ -546,9 +556,9 @@ static void cfg_connect_result(enum conn_event conn_disconn_evt,
else if (!wfi_drv->IFC_UP && dev == wl->vif[1]->ndev)
disconn_info->reason = 1;
- cfg80211_disconnected(dev, disconn_info->reason, disconn_info->ie,
- disconn_info->ie_len, false,
- GFP_KERNEL);
+ cfg80211_disconnected(dev, disconn_info->reason,
+ disconn_info->ie, disconn_info->ie_len,
+ false, GFP_KERNEL);
}
}
--
2.7.4
Please let me know, in case I have to rework and resubmit this patch
series to make them into staging branch.
Regards,
Ajay
On 27.03.2018 10:22, Ajay Singh wrote:
>
> Please let me know, in case I have to rework and resubmit this patch
> series to make them into staging branch.
>
As I suggested in patch 6, I prefer having the same format for
wilc_wfi_cfg_tx_vendor_spec() and wilc_wfi_cfg_parse_rx_vendor_spec(). I
think this could be achieved. To merge them I don't think would be feasible.
This series could go as is but I would like to see another future patch to
treat the format of wilc_wfi_cfg_tx_vendor_spec().
Thank you,
Claudiu
> Regards,
> Ajay
>
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 <[email protected]>
---
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(&p2p_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, &buf[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(&mgmt_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(&mgmt_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 (ieee80211_is_probe_resp(mgmt->frame_control)) {
+ if (buf[ACTION_CAT_ID] == PUB_ACTION_ATTR_ID) {
+ if (buf[ACTION_SUBTYPE_ID] != PUBLIC_ACT_VENDORSPEC ||
+ buf[P2P_PUB_ACTION_SUBTYPE] != GO_NEG_CONF) {
wilc_set_mac_chnl_num(vif, chan->hw_value);
curr_channel = chan->hw_value;
- } else if (ieee80211_is_action(mgmt->frame_control)) {
- if (buf[ACTION_CAT_ID] == PUB_ACTION_ATTR_ID) {
- if (buf[ACTION_SUBTYPE_ID] != PUBLIC_ACT_VENDORSPEC ||
- buf[P2P_PUB_ACTION_SUBTYPE] != GO_NEG_CONF) {
- wilc_set_mac_chnl_num(vif,
- chan->hw_value);
- curr_channel = chan->hw_value;
- }
- switch (buf[ACTION_SUBTYPE_ID]) {
- case GAS_INITIAL_REQ:
- break;
+ }
+ switch (buf[ACTION_SUBTYPE_ID]) {
+ case GAS_INITIAL_REQ:
+ case GAS_INITIAL_RSP:
+ break;
- case GAS_INITIAL_RSP:
- break;
+ case PUBLIC_ACT_VENDORSPEC:
+ if (!memcmp(p2p_oui, &buf[ACTION_SUBTYPE_ID + 1], 4))
+ wilc_wfi_cfg_tx_vendor_spec(mgmt_tx, params,
+ vif->iftype,
+ buf_len);
+ else
+ netdev_dbg(vif->ndev,
+ "Not a P2P public action frame\n");
- case PUBLIC_ACT_VENDORSPEC:
- {
- if (!memcmp(p2p_oui, &buf[ACTION_SUBTYPE_ID + 1], 4)) {
- if ((buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP)) {
- if (p2p_local_random == 1 && p2p_recv_random < p2p_local_random) {
- get_random_bytes(&p2p_local_random, 1);
- p2p_local_random++;
- }
- }
-
- if ((buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP ||
- buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_RSP)) {
- if (p2p_local_random > p2p_recv_random) {
- for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
- if (buf[i] == P2PELEM_ATTR_ID && !(memcmp(p2p_oui, &buf[i + 2], 4))) {
- if (buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_RSP)
- wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6], len - (i + 6), true, vif->iftype);
- else
- wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6], len - (i + 6), false, vif->iftype);
- break;
- }
- }
-
- if (buf[P2P_PUB_ACTION_SUBTYPE] != P2P_INV_REQ && buf[P2P_PUB_ACTION_SUBTYPE] != P2P_INV_RSP) {
- memcpy(&mgmt_tx->buff[len], p2p_vendor_spec, sizeof(p2p_vendor_spec));
- mgmt_tx->buff[len + sizeof(p2p_vendor_spec)] = p2p_local_random;
- mgmt_tx->size = buf_len;
- }
- }
- }
-
- } else {
- netdev_dbg(vif->ndev, "Not a P2P public action frame\n");
- }
+ break;
- break;
- }
+ default:
+ netdev_dbg(vif->ndev,
+ "NOT HANDLED PUBLIC ACTION FRAME TYPE:%x\n",
+ buf[ACTION_SUBTYPE_ID]);
+ break;
+ }
+ }
- default:
- {
- netdev_dbg(vif->ndev, "NOT HANDLED PUBLIC ACTION FRAME TYPE:%x\n", buf[ACTION_SUBTYPE_ID]);
- break;
- }
- }
- }
+ wfi_drv->p2p_timeout = (jiffies + msecs_to_jiffies(wait));
- wfi_drv->p2p_timeout = (jiffies + msecs_to_jiffies(wait));
- }
+out_txq_add_pkt:
- wilc_wlan_txq_add_mgmt_pkt(wdev->netdev, mgmt_tx,
- mgmt_tx->buff, mgmt_tx->size,
- wilc_wfi_mgmt_tx_complete);
- }
- return 0;
+ wilc_wlan_txq_add_mgmt_pkt(wdev->netdev, mgmt_tx,
+ mgmt_tx->buff, mgmt_tx->size,
+ wilc_wfi_mgmt_tx_complete);
+
+out:
+
+ return ret;
}
static int mgmt_tx_cancel_wait(struct wiphy *wiphy,
--
2.7.4
Also good for me, only one minor thing mentioned below.
On 20.03.2018 18:55, Ajay Singh wrote:
> + 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 < size; i++) {
> + if (buff[i] == P2PELEM_ATTR_ID &&
> + !(memcmp(p2p_oui, &buff[i + 2], 4))) {
You can remove "(" ")" around memcmp. Your choice.
> + wilc_wfi_cfg_parse_rx_action(&buff[i + 6],
> + size - (i + 6));
> + break;
> + }
> + }
> + }
Cleanup patch to remove unused struct data structure.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index af1b8aa..b9bba86 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -160,12 +160,6 @@ static struct ieee80211_supported_band WILC_WFI_band_2ghz = {
.n_bitrates = ARRAY_SIZE(ieee80211_bitrates),
};
-struct add_key_params {
- u8 key_idx;
- bool pairwise;
- u8 *mac_addr;
-};
-
#define AGING_TIME (9 * 1000)
#define during_ip_time 15000
--
2.7.4
Hi Claudiu,
On Tue, 27 Mar 2018 11:55:52 +0300
Claudiu Beznea <[email protected]> wrote:
> On 27.03.2018 10:22, Ajay Singh wrote:
> >
> > Please let me know, in case I have to rework and resubmit this patch
> > series to make them into staging branch.
> >
>
> As I suggested in patch 6, I prefer having the same format for
> wilc_wfi_cfg_tx_vendor_spec() and wilc_wfi_cfg_parse_rx_vendor_spec(). I
> think this could be achieved. To merge them I don't think would be feasible.
>
> This series could go as is but I would like to see another future patch to
> treat the format of wilc_wfi_cfg_tx_vendor_spec().
>
Sure, I had noted the review comments received for patch#6 & patch#1. I
will include the code modification in another future patches to handle it.
Regards,
Ajay
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 <[email protected]>
> ---
> 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(&p2p_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, &buf[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(&mgmt_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(&mgmt_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(&p2p_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, &buf[i + 2], 4))
continue;
wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6],
len - (i + 6),
(subtype == P2P_INV_REQ ||
subtype == P2P_INV_RSP),
iftype);
break;
}
if (subtype != P2P_INV_REQ && subtype != P2P_INV_RSP) {
int vendor_spec_len = sizeof(p2p_vendor_spec);
memcpy(&mgmt_tx->buff[len], p2p_vendor_spec,
vendor_spec_len);
mgmt_tx->buff[len + vendor_spec_len] = p2p_local_random;
mgmt_tx->size = buf_len;
}
}
}
which is mostly in the same format as wilc_wfi_cfg_parse_rx_vendor_spec(), from
the point of view of if () sequences:
if ((subtype == GO_NEG_REQ || subtype == GO_NEG_RSP) && something) {
// ...
}
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) {
// ...
}
> +
> 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 (ieee80211_is_probe_resp(mgmt->frame_control)) {
> + if (buf[ACTION_CAT_ID] == PUB_ACTION_ATTR_ID) {
> + if (buf[ACTION_SUBTYPE_ID] != PUBLIC_ACT_VENDORSPEC ||
> + buf[P2P_PUB_ACTION_SUBTYPE] != GO_NEG_CONF) {
> wilc_set_mac_chnl_num(vif, chan->hw_value);
> curr_channel = chan->hw_value;
> - } else if (ieee80211_is_action(mgmt->frame_control)) {
> - if (buf[ACTION_CAT_ID] == PUB_ACTION_ATTR_ID) {
> - if (buf[ACTION_SUBTYPE_ID] != PUBLIC_ACT_VENDORSPEC ||
> - buf[P2P_PUB_ACTION_SUBTYPE] != GO_NEG_CONF) {
> - wilc_set_mac_chnl_num(vif,
> - chan->hw_value);
> - curr_channel = chan->hw_value;
> - }
> - switch (buf[ACTION_SUBTYPE_ID]) {
> - case GAS_INITIAL_REQ:
> - break;
> + }
> + switch (buf[ACTION_SUBTYPE_ID]) {
> + case GAS_INITIAL_REQ:
> + case GAS_INITIAL_RSP:
> + break;
>
> - case GAS_INITIAL_RSP:
> - break;
> + case PUBLIC_ACT_VENDORSPEC:
> + if (!memcmp(p2p_oui, &buf[ACTION_SUBTYPE_ID + 1], 4))
> + wilc_wfi_cfg_tx_vendor_spec(mgmt_tx, params,
> + vif->iftype,
> + buf_len);
> + else
> + netdev_dbg(vif->ndev,
> + "Not a P2P public action frame\n");
>
> - case PUBLIC_ACT_VENDORSPEC:
> - {
> - if (!memcmp(p2p_oui, &buf[ACTION_SUBTYPE_ID + 1], 4)) {
> - if ((buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP)) {
> - if (p2p_local_random == 1 && p2p_recv_random < p2p_local_random) {
> - get_random_bytes(&p2p_local_random, 1);
> - p2p_local_random++;
> - }
> - }
> -
> - if ((buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == GO_NEG_RSP ||
> - buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_RSP)) {
> - if (p2p_local_random > p2p_recv_random) {
> - for (i = P2P_PUB_ACTION_SUBTYPE + 2; i < len; i++) {
> - if (buf[i] == P2PELEM_ATTR_ID && !(memcmp(p2p_oui, &buf[i + 2], 4))) {
> - if (buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_REQ || buf[P2P_PUB_ACTION_SUBTYPE] == P2P_INV_RSP)
> - wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6], len - (i + 6), true, vif->iftype);
> - else
> - wilc_wfi_cfg_parse_tx_action(&mgmt_tx->buff[i + 6], len - (i + 6), false, vif->iftype);
> - break;
> - }
> - }
> -
> - if (buf[P2P_PUB_ACTION_SUBTYPE] != P2P_INV_REQ && buf[P2P_PUB_ACTION_SUBTYPE] != P2P_INV_RSP) {
> - memcpy(&mgmt_tx->buff[len], p2p_vendor_spec, sizeof(p2p_vendor_spec));
> - mgmt_tx->buff[len + sizeof(p2p_vendor_spec)] = p2p_local_random;
> - mgmt_tx->size = buf_len;
> - }
> - }
> - }
> -
> - } else {
> - netdev_dbg(vif->ndev, "Not a P2P public action frame\n");
> - }
> + break;
>
> - break;
> - }
> + default:
> + netdev_dbg(vif->ndev,
> + "NOT HANDLED PUBLIC ACTION FRAME TYPE:%x\n",
> + buf[ACTION_SUBTYPE_ID]);
> + break;
> + }
> + }
>
> - default:
> - {
> - netdev_dbg(vif->ndev, "NOT HANDLED PUBLIC ACTION FRAME TYPE:%x\n", buf[ACTION_SUBTYPE_ID]);
> - break;
> - }
> - }
> - }
> + wfi_drv->p2p_timeout = (jiffies + msecs_to_jiffies(wait));
>
> - wfi_drv->p2p_timeout = (jiffies + msecs_to_jiffies(wait));
> - }
> +out_txq_add_pkt:
>
> - wilc_wlan_txq_add_mgmt_pkt(wdev->netdev, mgmt_tx,
> - mgmt_tx->buff, mgmt_tx->size,
> - wilc_wfi_mgmt_tx_complete);
> - }
> - return 0;
> + wilc_wlan_txq_add_mgmt_pkt(wdev->netdev, mgmt_tx,
> + mgmt_tx->buff, mgmt_tx->size,
> + wilc_wfi_mgmt_tx_complete);
You should check return value of this function and free mgmt_tx and
mgmt_tx->buff accordingly (if not in this series maybe in a future one).
> +
> +out:
> +
> + return ret;
> }
>
> static int mgmt_tx_cancel_wait(struct wiphy *wiphy,
>
On Tue, Mar 27, 2018 at 12:52:13PM +0530, Ajay Singh wrote:
>
> Please let me know, in case I have to rework and resubmit this patch
> series to make them into staging branch.
You already sent a v2 series for this, right? Why respond to the v1
set?
confused,
greg k-h
Added changes to free the allocated memory in scan() for error condition.
Also added 'NULL' check validation before accessing allocated memory.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 62 +++++++++++++++++------
1 file changed, 46 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 9d8d5d0..b784e15 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -582,6 +582,49 @@ static int set_channel(struct wiphy *wiphy,
return result;
}
+static inline bool
+wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request *request,
+ struct hidden_network *ntwk)
+{
+ int i = 0;
+
+ ntwk->net_info = kcalloc(request->n_ssids,
+ sizeof(struct hidden_network), GFP_KERNEL);
+
+ if (!ntwk->net_info)
+ goto out;
+
+ ntwk->n_ssids = request->n_ssids;
+
+ for (i = 0; i < request->n_ssids; i++) {
+ if (request->ssids[i].ssid_len > 0) {
+ struct hidden_net_info *info = &ntwk->net_info[i];
+
+ info->ssid = kmemdup(request->ssids[i].ssid,
+ request->ssids[i].ssid_len,
+ GFP_KERNEL);
+
+ if (!info->ssid)
+ goto out_free;
+
+ info->ssid_len = request->ssids[i].ssid_len;
+ } else {
+ ntwk->n_ssids -= 1;
+ }
+ }
+ return true;
+
+out_free:
+
+ for (; i >= 0 ; i--)
+ kfree(ntwk->net_info[i].ssid);
+
+ kfree(ntwk->net_info);
+out:
+
+ return false;
+}
+
static int scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
{
struct wilc_priv *priv;
@@ -606,23 +649,10 @@ static int scan(struct wiphy *wiphy, struct cfg80211_scan_request *request)
scan_ch_list[i] = (u8)ieee80211_frequency_to_channel(request->channels[i]->center_freq);
if (request->n_ssids >= 1) {
- hidden_ntwk.net_info =
- kmalloc_array(request->n_ssids,
- sizeof(struct hidden_network),
- GFP_KERNEL);
- if (!hidden_ntwk.net_info)
+ if (!wilc_wfi_cfg_alloc_fill_ssid(request,
+ &hidden_ntwk))
return -ENOMEM;
- hidden_ntwk.n_ssids = request->n_ssids;
-
- for (i = 0; i < request->n_ssids; i++) {
- if (request->ssids[i].ssid_len != 0) {
- hidden_ntwk.net_info[i].ssid = kmalloc(request->ssids[i].ssid_len, GFP_KERNEL);
- memcpy(hidden_ntwk.net_info[i].ssid, request->ssids[i].ssid, request->ssids[i].ssid_len);
- hidden_ntwk.net_info[i].ssid_len = request->ssids[i].ssid_len;
- } else {
- hidden_ntwk.n_ssids -= 1;
- }
- }
+
ret = wilc_scan(vif, USER_SCAN, ACTIVE_SCAN,
scan_ch_list,
request->n_channels,
--
2.7.4
Remove 'line over 80 characters' issues found by checkpatch.pl script
for following functions.
disconnect()
del_pmksa()
wilc_create_wiphy()
del_pmksa()
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index b9bba86..69f1e06 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -823,7 +823,8 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
return ret;
}
-static int disconnect(struct wiphy *wiphy, struct net_device *dev, u16 reason_code)
+static int disconnect(struct wiphy *wiphy, struct net_device *dev,
+ u16 reason_code)
{
s32 ret = 0;
struct wilc_priv *priv;
@@ -1267,7 +1268,8 @@ static int del_pmksa(struct wiphy *wiphy, struct net_device *netdev,
for (i = 0; i < priv->pmkid_list.numpmkid; i++) {
if (!memcmp(pmksa->bssid, priv->pmkid_list.pmkidlist[i].bssid,
ETH_ALEN)) {
- memset(&priv->pmkid_list.pmkidlist[i], 0, sizeof(struct host_if_pmkid));
+ memset(&priv->pmkid_list.pmkidlist[i], 0,
+ sizeof(struct host_if_pmkid));
break;
}
}
@@ -1979,7 +1981,8 @@ static int add_station(struct wiphy *wiphy, struct net_device *dev,
if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
memcpy(sta_params.bssid, mac, ETH_ALEN);
- memcpy(priv->assoc_stainfo.sta_associated_bss[params->aid], mac, ETH_ALEN);
+ memcpy(priv->assoc_stainfo.sta_associated_bss[params->aid], mac,
+ ETH_ALEN);
sta_params.aid = params->aid;
sta_params.rates_len = params->supported_rates_len;
sta_params.rates = params->supported_rates;
@@ -2235,7 +2238,8 @@ static struct wireless_dev *wilc_wfi_cfg_alloc(void)
return NULL;
}
-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)
{
struct wilc_priv *priv;
struct wireless_dev *wdev;
--
2.7.4
On Tue, Mar 20, 2018 at 10:25:34PM +0530, Ajay Singh wrote:
> Added changes to free the allocated memory in scan() for error condition.
> Also added 'NULL' check validation before accessing allocated memory.
>
> Signed-off-by: Ajay Singh <[email protected]>
> ---
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 62 +++++++++++++++++------
> 1 file changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 9d8d5d0..b784e15 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -582,6 +582,49 @@ static int set_channel(struct wiphy *wiphy,
> return result;
> }
>
> +static inline bool
This shouldn't be a bool function. It should return 0 and -ENOMEM.
Bool functions should kind of have the return values built into the name
like access_ok() or IS_ERR().
> +wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request *request,
> + struct hidden_network *ntwk)
> +{
> + int i = 0;
No need to initialize "i".
> +
> + ntwk->net_info = kcalloc(request->n_ssids,
> + sizeof(struct hidden_network), GFP_KERNEL);
> +
> + if (!ntwk->net_info)
> + goto out;
Don't put a blank line between the alloc and the check. They're as
connected as can be. I hate "goto out;" but that is a personal
preference which I would never push on to other developers...
> +
> + ntwk->n_ssids = request->n_ssids;
> +
> + for (i = 0; i < request->n_ssids; i++) {
> + if (request->ssids[i].ssid_len > 0) {
> + struct hidden_net_info *info = &ntwk->net_info[i];
> +
> + info->ssid = kmemdup(request->ssids[i].ssid,
> + request->ssids[i].ssid_len,
> + GFP_KERNEL);
> +
> + if (!info->ssid)
> + goto out_free;
> +
> + info->ssid_len = request->ssids[i].ssid_len;
> + } else {
> + ntwk->n_ssids -= 1;
> + }
You didn't introduce the problem, but this loop seems kind of buggy. We
should have two iterators, one for request->ssids[i] and one for
ntwk->net_info[i]. Otherwise we're copying the array information but
we're leaving holes in the destination array. Which would be fine
except we're not saving the totaly number of elements in the destination
array, we're saving the number of elements with stuff in them.
So imagine that we have a request->n_ssids == 10 but only the last
three elements have request->ssids[i].ssid_len > 0. Then we record that
ntwk->n_ssids is 3 but wthose elements are all holes. So that can't
work. See handle_scan():
for (i = 0; i < hidden_net->n_ssids; i++)
valuesize += ((hidden_net->net_info[i].ssid_len) + 1);
"valuesize" is wrong because it's looking at holes.
> + }
> + return true;
> +
> +out_free:
> +
> + for (; i >= 0 ; i--)
> + kfree(ntwk->net_info[i].ssid);
The first kfree(ntwk->net_info[i].ssid); is a no-op. You could write
this like:
while (--i >= 0)
kfree(ntwk->net_info[i].ssid);
Or:
while (i--)
kfree(ntwk->net_info[i].ssid);
Or you could do:
for (i--; i >= 0 ; i--)
kfree(ntwk->net_info[i].ssid);
I don't care...
regards,
dan carpenter
These look good. I've reviewed them all.
Reviewed-by: Dan Carpenter <[email protected]>
I had some small process complaints but it doesn't make life easier for
me if you resend them and I have to review everything twice.... :P
regards,
dan carpenter
Fix 'line over 80 character' issue reported by checkpatch.pl script in
set_wiphy_params(). Directly used the 'wiphy' pointer received as
function argument instead of using 'priv->dev->ieee80211_ptr->wiphy'.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index cd5ad9b..9d35a08 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1192,20 +1192,20 @@ static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
if (changed & WIPHY_PARAM_RETRY_SHORT) {
cfg_param_val.flag |= RETRY_SHORT;
- cfg_param_val.short_retry_limit = priv->dev->ieee80211_ptr->wiphy->retry_short;
+ cfg_param_val.short_retry_limit = wiphy->retry_short;
}
if (changed & WIPHY_PARAM_RETRY_LONG) {
cfg_param_val.flag |= RETRY_LONG;
- cfg_param_val.long_retry_limit = priv->dev->ieee80211_ptr->wiphy->retry_long;
+ cfg_param_val.long_retry_limit = wiphy->retry_long;
}
if (changed & WIPHY_PARAM_FRAG_THRESHOLD) {
cfg_param_val.flag |= FRAG_THRESHOLD;
- cfg_param_val.frag_threshold = priv->dev->ieee80211_ptr->wiphy->frag_threshold;
+ cfg_param_val.frag_threshold = wiphy->frag_threshold;
}
if (changed & WIPHY_PARAM_RTS_THRESHOLD) {
cfg_param_val.flag |= RTS_THRESHOLD;
- cfg_param_val.rts_threshold = priv->dev->ieee80211_ptr->wiphy->rts_threshold;
+ cfg_param_val.rts_threshold = wiphy->rts_threshold;
}
ret = wilc_hif_set_cfg(vif, &cfg_param_val);
--
2.7.4
Removed the unnecessary global variables used to store gtk and ptk
information. Key data stored in the params was never access using these
global variables.
Global variables given below are removed
g_add_gtk_key_params;
g_key_gtk_params;
g_add_ptk_key_params;
g_key_ptk_params;
g_key_wep_params;
g_ptk_keys_saved;
g_gtk_keys_saved;
g_wep_keys_saved;
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 80 -----------------------
1 file changed, 80 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index b784e15..cd5ad9b 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -166,15 +166,6 @@ struct add_key_params {
u8 *mac_addr;
};
-static struct add_key_params g_add_gtk_key_params;
-static struct wilc_wfi_key g_key_gtk_params;
-static struct add_key_params g_add_ptk_key_params;
-static struct wilc_wfi_key g_key_ptk_params;
-static struct wilc_wfi_wep_key g_key_wep_params;
-static bool g_ptk_keys_saved;
-static bool g_gtk_keys_saved;
-static bool g_wep_keys_saved;
-
#define AGING_TIME (9 * 1000)
#define during_ip_time 15000
@@ -740,12 +731,6 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
priv->WILC_WFI_wep_key_len[sme->key_idx] = sme->key_len;
memcpy(priv->WILC_WFI_wep_key[sme->key_idx], sme->key, sme->key_len);
- g_key_wep_params.key_len = sme->key_len;
- g_key_wep_params.key = kmalloc(sme->key_len, GFP_KERNEL);
- memcpy(g_key_wep_params.key, sme->key, sme->key_len);
- g_key_wep_params.key_idx = sme->key_idx;
- g_wep_keys_saved = true;
-
wilc_set_wep_default_keyid(vif, sme->key_idx);
wilc_add_wep_key_bss_sta(vif, sme->key, sme->key_len,
sme->key_idx);
@@ -755,12 +740,6 @@ static int connect(struct wiphy *wiphy, struct net_device *dev,
priv->WILC_WFI_wep_key_len[sme->key_idx] = sme->key_len;
memcpy(priv->WILC_WFI_wep_key[sme->key_idx], sme->key, sme->key_len);
- g_key_wep_params.key_len = sme->key_len;
- g_key_wep_params.key = kmalloc(sme->key_len, GFP_KERNEL);
- memcpy(g_key_wep_params.key, sme->key, sme->key_len);
- g_key_wep_params.key_idx = sme->key_idx;
- g_wep_keys_saved = true;
-
wilc_set_wep_default_keyid(vif, sme->key_idx);
wilc_add_wep_key_bss_sta(vif, sme->key, sme->key_len,
sme->key_idx);
@@ -1019,27 +998,6 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
keylen = params->key_len - 16;
}
- if (!g_gtk_keys_saved && netdev == wl->vif[0]->ndev) {
- g_add_gtk_key_params.key_idx = key_index;
- g_add_gtk_key_params.pairwise = pairwise;
- if (!mac_addr) {
- g_add_gtk_key_params.mac_addr = NULL;
- } else {
- g_add_gtk_key_params.mac_addr = kmalloc(ETH_ALEN, GFP_KERNEL);
- memcpy(g_add_gtk_key_params.mac_addr, mac_addr, ETH_ALEN);
- }
- g_key_gtk_params.key_len = params->key_len;
- g_key_gtk_params.seq_len = params->seq_len;
- g_key_gtk_params.key = kmalloc(params->key_len, GFP_KERNEL);
- memcpy(g_key_gtk_params.key, params->key, params->key_len);
- if (params->seq_len > 0) {
- g_key_gtk_params.seq = kmalloc(params->seq_len, GFP_KERNEL);
- memcpy(g_key_gtk_params.seq, params->seq, params->seq_len);
- }
- g_key_gtk_params.cipher = params->cipher;
- g_gtk_keys_saved = true;
- }
-
wilc_add_rx_gtk(vif, params->key, keylen,
key_index, params->seq_len,
params->seq, rx_mic,
@@ -1052,27 +1010,6 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
keylen = params->key_len - 16;
}
- if (!g_ptk_keys_saved && netdev == wl->vif[0]->ndev) {
- g_add_ptk_key_params.key_idx = key_index;
- g_add_ptk_key_params.pairwise = pairwise;
- if (!mac_addr) {
- g_add_ptk_key_params.mac_addr = NULL;
- } else {
- g_add_ptk_key_params.mac_addr = kmalloc(ETH_ALEN, GFP_KERNEL);
- memcpy(g_add_ptk_key_params.mac_addr, mac_addr, ETH_ALEN);
- }
- g_key_ptk_params.key_len = params->key_len;
- g_key_ptk_params.seq_len = params->seq_len;
- g_key_ptk_params.key = kmalloc(params->key_len, GFP_KERNEL);
- memcpy(g_key_ptk_params.key, params->key, params->key_len);
- if (params->seq_len > 0) {
- g_key_ptk_params.seq = kmalloc(params->seq_len, GFP_KERNEL);
- memcpy(g_key_ptk_params.seq, params->seq, params->seq_len);
- }
- g_key_ptk_params.cipher = params->cipher;
- g_ptk_keys_saved = true;
- }
-
wilc_add_ptk(vif, params->key, keylen,
mac_addr, rx_mic, tx_mic,
STATION_MODE, u8mode, key_index);
@@ -1102,13 +1039,6 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev,
wl = vif->wilc;
if (netdev == wl->vif[0]->ndev) {
- g_ptk_keys_saved = false;
- g_gtk_keys_saved = false;
- g_wep_keys_saved = false;
-
- kfree(g_key_wep_params.key);
- g_key_wep_params.key = NULL;
-
if (priv->wilc_gtk[key_index] != NULL) {
kfree(priv->wilc_gtk[key_index]->key);
priv->wilc_gtk[key_index]->key = NULL;
@@ -1127,16 +1057,6 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev,
kfree(priv->wilc_ptk[key_index]);
priv->wilc_ptk[key_index] = NULL;
}
-
- kfree(g_key_ptk_params.key);
- g_key_ptk_params.key = NULL;
- kfree(g_key_ptk_params.seq);
- g_key_ptk_params.seq = NULL;
-
- kfree(g_key_gtk_params.key);
- g_key_gtk_params.key = NULL;
- kfree(g_key_gtk_params.seq);
- g_key_gtk_params.seq = NULL;
}
if (key_index >= 0 && key_index <= 3) {
--
2.7.4
Patch 6 may be reworked a bit. Other than this:
Reviewed-by: Claudiu Beznea <[email protected]>
On 21.03.2018 11:20, Ajay Singh wrote:
> Hi Dan,
>
> On Wed, 21 Mar 2018 10:51:16 +0300
> Dan Carpenter <[email protected]> wrote:
>
>> These look good. I've reviewed them all.
>>
>> Reviewed-by: Dan Carpenter <[email protected]>
>
> Thanks for reviewing all the patches.
>
>>
>> I had some small process complaints but it doesn't make life easier for
>> me if you resend them and I have to review everything twice.... :P
>>
>
> In future patches, I will take care as per your suggestions. Instead of
> resubmitting this patch series, I will include the additional changes
> required for [PATCH 01/11] in separate patch series.
>
>
> Regards,
> Ajay
>
This one would have been easier for me to review if it were broken up
slightly differently. I have a script to review when people split
functions up, but there were a bunch of other stuff so my script gets
confused.
Anyway, looks good.
regards,
dan carpenter
Remove 'line over 80 char' issue found by checkpatch.pl script.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index ebebdc3..1b6fe64 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -173,20 +173,21 @@ static void clear_shadow_scan(void)
{
int i;
- if (op_ifcs == 0) {
- del_timer_sync(&aging_timer);
+ if (op_ifcs != 0)
+ return;
- for (i = 0; i < last_scanned_cnt; i++) {
- if (last_scanned_shadow[last_scanned_cnt].ies) {
- kfree(last_scanned_shadow[i].ies);
- last_scanned_shadow[last_scanned_cnt].ies = NULL;
- }
+ del_timer_sync(&aging_timer);
- kfree(last_scanned_shadow[i].join_params);
- last_scanned_shadow[i].join_params = NULL;
+ for (i = 0; i < last_scanned_cnt; i++) {
+ if (last_scanned_shadow[last_scanned_cnt].ies) {
+ kfree(last_scanned_shadow[i].ies);
+ last_scanned_shadow[last_scanned_cnt].ies = NULL;
}
- last_scanned_cnt = 0;
+
+ kfree(last_scanned_shadow[i].join_params);
+ last_scanned_shadow[i].join_params = NULL;
}
+ last_scanned_cnt = 0;
}
static u32 get_rssi_avg(struct network_info *network_info)
--
2.7.4
Fix 'Avoid camelCase' issue found by checkpatch.pl script.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 9950ca5..ebebdc3 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -90,7 +90,7 @@ static const struct wiphy_wowlan_support wowlan_support = {
static struct network_info last_scanned_shadow[MAX_NUM_SCANNED_NETWORKS_SHADOW];
static u32 last_scanned_cnt;
struct timer_list wilc_during_ip_timer;
-static struct timer_list hAgingTimer;
+static struct timer_list aging_timer;
static u8 op_ifcs;
#define CHAN2G(_channel, _freq, _flags) { \
@@ -174,7 +174,7 @@ static void clear_shadow_scan(void)
int i;
if (op_ifcs == 0) {
- del_timer_sync(&hAgingTimer);
+ del_timer_sync(&aging_timer);
for (i = 0; i < last_scanned_cnt; i++) {
if (last_scanned_shadow[last_scanned_cnt].ies) {
@@ -276,7 +276,7 @@ static void remove_network_from_shadow(struct timer_list *unused)
}
if (last_scanned_cnt != 0) {
- mod_timer(&hAgingTimer, jiffies + msecs_to_jiffies(AGING_TIME));
+ mod_timer(&aging_timer, jiffies + msecs_to_jiffies(AGING_TIME));
}
}
@@ -291,7 +291,7 @@ static int is_network_in_shadow(struct network_info *nw_info, void *user_void)
int i;
if (last_scanned_cnt == 0) {
- mod_timer(&hAgingTimer, jiffies + msecs_to_jiffies(AGING_TIME));
+ mod_timer(&aging_timer, jiffies + msecs_to_jiffies(AGING_TIME));
state = -1;
} else {
for (i = 0; i < last_scanned_cnt; i++) {
@@ -2282,7 +2282,7 @@ int wilc_init_host_int(struct net_device *net)
priv = wdev_priv(net->ieee80211_ptr);
if (op_ifcs == 0) {
- timer_setup(&hAgingTimer, remove_network_from_shadow, 0);
+ timer_setup(&aging_timer, remove_network_from_shadow, 0);
timer_setup(&wilc_during_ip_timer, clear_duringIP, 0);
}
op_ifcs++;
--
2.7.4
Hi Dan,
Thanks for your detailed review comments.
On Tue, 20 Mar 2018 22:46:32 +0300
Dan Carpenter <[email protected]> wrote:
> On Tue, Mar 20, 2018 at 10:25:34PM +0530, Ajay Singh wrote:
> > Added changes to free the allocated memory in scan() for error condition.
> > Also added 'NULL' check validation before accessing allocated memory.
> >
> > Signed-off-by: Ajay Singh <[email protected]>
> Don't put a blank line between the alloc and the check. They're as
> connected as can be. I hate "goto out;" but that is a personal
> preference which I would never push on to other developers...
>
I will modify the code to address the review comments and will send the
updated patch.
> > +
> > + ntwk->n_ssids = request->n_ssids;
> > +
> > + for (i = 0; i < request->n_ssids; i++) {
> > + if (request->ssids[i].ssid_len > 0) {
> > + struct hidden_net_info *info = &ntwk->net_info[i];
> > +
> > + info->ssid = kmemdup(request->ssids[i].ssid,
> > + request->ssids[i].ssid_len,
> > + GFP_KERNEL);
> > +
> > + if (!info->ssid)
> > + goto out_free;
> > +
> > + info->ssid_len = request->ssids[i].ssid_len;
> > + } else {
> > + ntwk->n_ssids -= 1;
> > + }
>
> You didn't introduce the problem, but this loop seems kind of buggy. We
> should have two iterators, one for request->ssids[i] and one for
> ntwk->net_info[i]. Otherwise we're copying the array information but
> we're leaving holes in the destination array. Which would be fine
> except we're not saving the totaly number of elements in the destination
> array, we're saving the number of elements with stuff in them.
>
> So imagine that we have a request->n_ssids == 10 but only the last
> three elements have request->ssids[i].ssid_len > 0. Then we record that
> ntwk->n_ssids is 3 but wthose elements are all holes. So that can't
> work. See handle_scan():
>
> for (i = 0; i < hidden_net->n_ssids; i++)
> valuesize += ((hidden_net->net_info[i].ssid_len) + 1);
>
> "valuesize" is wrong because it's looking at holes.
While testing, I found that the last element in request->ssids the
ssid_len is zero. For in between elements the values has some valid
length. I only tested for 'connect with AP' and 'iw scan' operation.
The scenario you have mention can occur for some instance. So, I will
modify the code to not have holes in allocated array at the time of
filling the data.
I will include these changes and send updated v2 patch set.
>
> > + }
> > + return true;
> > +
> > +out_free:
> > +
> > + for (; i >= 0 ; i--)
> > + kfree(ntwk->net_info[i].ssid);
>
> The first kfree(ntwk->net_info[i].ssid); is a no-op. You could write
> this like:
>
> while (--i >= 0)
> kfree(ntwk->net_info[i].ssid);
>
I will include these changes and send the updated patch.
Regards,
Ajay
On Tue, Mar 20, 2018 at 10:25:42PM +0530, Ajay Singh wrote:
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 1b6fe64..af1b8aa 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -460,6 +460,17 @@ static void cfg_scan_result(enum scan_event scan_event,
> }
> }
>
> +static inline bool wilc_wfi_cfg_scan_time_expired(int i)
"i" is the wrong parameter to pass. The name is not useful. Probably
the right parameter is either &last_scanned_shadow[i] or
last_scanned_shadow[i].time_scan_cached.
> +{
> + unsigned long now = jiffies;
> +
> + if (time_after(now, last_scanned_shadow[i].time_scan_cached +
> + (unsigned long)(nl80211_SCAN_RESULT_EXPIRE - (1 * HZ))))
> + return true;
> + else
> + return false;
Also I think it you apply this patch and then run checkpatch.pl --strict
against the file it will complain that it should be:
if (time_after(now, last_scanned_shadow[i].time_scan_cached +
(unsigned long)(nl80211_SCAN_RESULT_EXPIRE - (1 * HZ))))
return true;
return false;
> +}
> +
> int wilc_connecting;
>
> static void cfg_connect_result(enum conn_event conn_disconn_evt,
> @@ -505,17 +516,14 @@ static void cfg_connect_result(enum conn_event conn_disconn_evt,
> bool scan_refresh = false;
> u32 i;
>
> - memcpy(priv->associated_bss, conn_info->bssid, ETH_ALEN);
> + memcpy(priv->associated_bss, conn_info->bssid,
> + ETH_ALEN);
>
It basically always helps me if the "new function" changes are in a
patch by themselves. These lines are a pure white space changes and I
have a script that reviews those for me instantly, but when it's mixed
with this patch I have to do it by hand.
regards,
dan carpenter
On Wed, Apr 04, 2018 at 10:45:25AM +0530, Ajay Singh wrote:
> Hi Greg,
>
> On Wed, 28 Mar 2018 13:31:01 +0200
> Greg KH <[email protected]> wrote:
>
> > On Tue, Mar 27, 2018 at 12:52:13PM +0530, Ajay Singh wrote:
> > >
> > > Please let me know, in case I have to rework and resubmit this
> > > patch series to make them into staging branch.
> >
> > You already sent a v2 series for this, right? Why respond to the v1
> > set?
> >
>
> Sorry for late reply.
>
> V2 series was not sent for this patchset. No code updates were
> submitted for this patch series. It's an independent patch series
> with 11 patches and had changes different from other patch series.
>
> Just for information, the v2 patch series was submitted for different
> patch series which had 9 patches.
>
> Please let me know if any other information/action is required from
> my side to include this patch series to staging.
THis patch series is long gone from my queue. Please resend it if I
have not applied it.
greg k-h
Hi Greg,
On Wed, 28 Mar 2018 13:31:01 +0200
Greg KH <[email protected]> wrote:
> On Tue, Mar 27, 2018 at 12:52:13PM +0530, Ajay Singh wrote:
> >
> > Please let me know, in case I have to rework and resubmit this
> > patch series to make them into staging branch.
>
> You already sent a v2 series for this, right? Why respond to the v1
> set?
>
Sorry for late reply.
V2 series was not sent for this patchset. No code updates were
submitted for this patch series. It's an independent patch series
with 11 patches and had changes different from other patch series.
Just for information, the v2 patch series was submitted for different
patch series which had 9 patches.
Please let me know if any other information/action is required from
my side to include this patch series to staging.
Regards,
Ajay