ath9k disconnects in 4.13 with reason=4 locally_generated=1

2017-11-02 Thread Daniel Drake 
Hi,

Endless OS recently upgraded from Linux 4.11 to Linux 4.13, and we now
have a few reports of issues with ath9k wireless becoming unusable.

In the logs we can see that it authenticates, associates and completes
the WPA 4 way handshake, before then being disconnected with:

 wlp2s0: CTRL-EVENT-DISCONNECTED bssid=74:26:ac:68:2f:c0 reason=4
locally_generated=1

The cycle then repeats with it connecting again before being swiftly
disconnected, etc.

More logs: https://gist.github.com/dsd/49f263c67c2859838ce168628ab043e0

At the same time that we upgraded the kernel, we also upgraded many
other components (e.g. NetworkManager and wpa_supplicant), however the
same problem has been reported on Arch Linux and a user there reports
that he narrowed it down to a kernel regression between 4.12 and 4.13:
https://bbs.archlinux.org/viewtopic.php?id=225199

Unfortunately we can not reproduce this in our office, so can't offer
much more info yet, but we are continuing to investigate. I have not
found any codepaths in userspace that generate disconnect reason 4, so
I think it must be something in the kernel causing the disconnection,
but I did not see any suspicious changes in recent commit history.

It would be good to hear from anyone who has heard of this or has any
ideas about causes or solutions.

Thanks
Daniel

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

2017-11-02 Thread Sebastian Gottschall 

i know. saw that later too. code should be safe

Am 02.11.2017 um 20:34 schrieb Christian Lamparter:

On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:

a additional array bounds check would be good

Ah, about that:

the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
in the following way [0]:
|   bw = info2 & 3;

the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
|   txrate.bw = ATH10K_HW_BW(peer_stats->flags);

ATH10K_HW_BW is a macro defined as [2]:
|   #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)

In both cases the bandwidth values already are limited to 0-3 by
the "and 3" operation.

[0] 


[1] 

[2] 




@@ -592,6 +592,9 @@ struct amsdu_subframe_hdr {
   
   #define GROUP_ID_IS_SU_MIMO(x) ((x) == 0 || (x) == 63)
   
+static const u8 ath10k_bw_to_mac80211[] = { RATE_INFO_BW_20, RATE_INFO_BW_40,

+   RATE_INFO_BW_80, RATE_INFO_BW_160 };
+
   static void ath10k_htt_rx_h_rates(struct ath10k *ar,
  struct ieee80211_rx_status *status,
  struct htt_rx_desc *rxd)
@@ -694,23 +697,7 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
if (sgi)
status->enc_flags |= RX_ENC_FLAG_SHORT_GI;
   
[...]

+   status->bw = ath10k_bw_to_mac80211[bw];
status->encoding = RX_ENC_VHT;
break;
default:
@@ -2297,7 +2284,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
   
   	arsta->txrate.nss = txrate.nss;

-   arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
+   arsta->txrate.bw = ath10k_bw_to_mac80211[txrate.bw];
   }
   
   static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,






--
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH] ath10k: fix recent bandwidth conversion bug

2017-11-02 Thread Christian Lamparter 
On Wednesday, November 1, 2017 9:37:53 PM CET Sebastian Gottschall wrote:
> a additional array bounds check would be good

Ah, about that:

the bw variable in ath10k_htt_rx_h_rates() is extracted from info2
in the following way [0]:
|   bw = info2 & 3;

the txrate.bw variable in ath10k_update_per_peer_tx_stats() is set by [1]:
|   txrate.bw = ATH10K_HW_BW(peer_stats->flags);

ATH10K_HW_BW is a macro defined as [2]:
|   #define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)

In both cases the bandwidth values already are limited to 0-3 by
the "and 3" operation.

[0] 


[1] 

[2] 



> > @@ -592,6 +592,9 @@ struct amsdu_subframe_hdr {
> >   
> >   #define GROUP_ID_IS_SU_MIMO(x) ((x) == 0 || (x) == 63)
> >   
> > +static const u8 ath10k_bw_to_mac80211[] = { RATE_INFO_BW_20, 
> > RATE_INFO_BW_40,
> > +   RATE_INFO_BW_80, RATE_INFO_BW_160 };
> > +
> >   static void ath10k_htt_rx_h_rates(struct ath10k *ar,
> >   struct ieee80211_rx_status *status,
> >   struct htt_rx_desc *rxd)
> > @@ -694,23 +697,7 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
> > if (sgi)
> > status->enc_flags |= RX_ENC_FLAG_SHORT_GI;
> >   
> > [...]
> > +   status->bw = ath10k_bw_to_mac80211[bw];
> > status->encoding = RX_ENC_VHT;
> > break;
> > default:
> > @@ -2297,7 +2284,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
> > arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
> >   
> > arsta->txrate.nss = txrate.nss;
> > -   arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
> > +   arsta->txrate.bw = ath10k_bw_to_mac80211[txrate.bw];
> >   }
> >   
> >   static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,

RE: [PATCH] mac80211: Update last_ack status for all except probing frames

2017-11-02 Thread Rajkumar Manoharan 
> To clarify: wouldn't it break hostapd's "inactivity timeot kick-out"
> logic in hostapd? As hostapd generates NULL frames to idle stations to reset
> their inactivity time, otherwise it will disassociate a station, after an
> ap_max_inactivity period.

Igor,

I validated hostpad inactivity monitoring with this change and Hostapd is 
sending
Nulldata periodically for inactive stations. Hostapd resets timer based on tx 
ack
status of nulldata. So retaining actual station inactive period will not break
hostapd monitoring.

Please report if you see any issue with this change. Thanks for your feedback.

-Rajkumar

Re: [1/2] rsi: move rsi_sdio_reinit_device() out of CONFIG_PM

2017-11-02 Thread Kalle Valo 
Amitkumar Karwar  wrote:

> From: Amitkumar Karwar 
> 
> This function is generic. It doesn't contain wowlan specific code.
> It should not be under CONFIG_PM. This patch resolves compilation
> errors observed when CONFIG_PM flag is disabled.
> 
> Reported-by: kbuild test robot 
> Fixes: ef71ed0608c ("rsi: sdio: Add WOWLAN support for S5 shutdown state")
> Fixes: a24e35fcee0 ("rsi: sdio: Add WOWLAN support for S4 hibernate state")
> Fixes: e1ced6422a3 ("rsi: sdio: add WOWLAN support for S3 suspend state")
> Signed-off-by: Amitkumar Karwar 

2 patches applied to wireless-drivers-next.git, thanks.

39f1332c526c rsi: move rsi_sdio_reinit_device() out of CONFIG_PM
e6b3b2ed3d27 rsi: fix kbuild reported build errors with CONFIG_PM off

-- 
https://patchwork.kernel.org/patch/10036297/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] ath10k: move pci suspend/resume functions

2017-11-02 Thread Brian Norris 
On Thu, Nov 02, 2017 at 04:40:57PM +0100, Arnd Bergmann wrote:
> On Thu, Nov 2, 2017 at 4:23 PM, Kalle Valo  wrote:
> > Brian has already fixed this, please check that:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next=20665a9076d48e9abd9a2db13d307f58f7ef6647
> >
> > But apparently I forgot to merge ath-next to wireless-drivers-next, will
> > do that soon.
> 
> Yes, Brian's version is better. I considered the same, but wasn't sure
> it was safe.

Yes, it's safe. The code is still basically dead, since the mac80211 ops
get dropped with !CONFIG_PM (so no one calls the
*_hif_{suspend,resume}() functions), but at least we have fewer
#ifdef's.

Brian

Re: [PATCH] ath10k: move pci suspend/resume functions

2017-11-02 Thread Arnd Bergmann 
On Thu, Nov 2, 2017 at 4:23 PM, Kalle Valo  wrote:
> Arnd Bergmann  writes:
>
>> The combination of two patches has led to a build failure:
>>
>> drivers/net/wireless/ath/ath10k/pci.c: In function 'ath10k_pci_pm_suspend':
>> drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of 
>> function 'ath10k_pci_suspend'; did you mean 'ath10k_pci_pm_suspend'? 
>> [-Werror=implicit-function-declaration]
>> drivers/net/wireless/ath/ath10k/pci.c: In function 'ath10k_pci_pm_resume':
>> drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of 
>> function 'ath10k_pci_resume'; did you mean 'ath10k_pci_pm_resume'? 
>> [-Werror=implicit-function-declaration]
>>
>> This moves the functions outside of the now incorrect #ifdef.
>>
>> Fixes: 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported 
>> but disabled")
>> Fixes: 6af1de2e4ec4 ("ath10k: mark PM functions as __maybe_unused")
>> Signed-off-by: Arnd Bergmann 
>
> Brian has already fixed this, please check that:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next=20665a9076d48e9abd9a2db13d307f58f7ef6647
>
> But apparently I forgot to merge ath-next to wireless-drivers-next, will
> do that soon.

Yes, Brian's version is better. I considered the same, but wasn't sure
it was safe.

  Arnd

Re: [PATCH] ath10k: move pci suspend/resume functions

2017-11-02 Thread Kalle Valo 
Arnd Bergmann  writes:

> The combination of two patches has led to a build failure:
>
> drivers/net/wireless/ath/ath10k/pci.c: In function 'ath10k_pci_pm_suspend':
> drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of 
> function 'ath10k_pci_suspend'; did you mean 'ath10k_pci_pm_suspend'? 
> [-Werror=implicit-function-declaration]
> drivers/net/wireless/ath/ath10k/pci.c: In function 'ath10k_pci_pm_resume':
> drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of 
> function 'ath10k_pci_resume'; did you mean 'ath10k_pci_pm_resume'? 
> [-Werror=implicit-function-declaration]
>
> This moves the functions outside of the now incorrect #ifdef.
>
> Fixes: 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported 
> but disabled")
> Fixes: 6af1de2e4ec4 ("ath10k: mark PM functions as __maybe_unused")
> Signed-off-by: Arnd Bergmann 

Brian has already fixed this, please check that:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next=20665a9076d48e9abd9a2db13d307f58f7ef6647

But apparently I forgot to merge ath-next to wireless-drivers-next, will
do that soon.

-- 
Kalle Valo

Re: [PATCH] rsi: sdio: fix building without CONFIG_PM

2017-11-02 Thread Arnd Bergmann 
On Thu, Nov 2, 2017 at 3:57 PM, Kalle Valo  wrote:
> Arnd Bergmann  writes:
>
>> The addition of the WoWLAN support has caused a number of new
>> build errors when CONFIG_PM is disabled, including:
>>
>> drivers/net/wireless/rsi/rsi_91x_mac80211.c: In function 
>> 'rsi_wow_map_triggers':
>> drivers/net/wireless/rsi/rsi_91x_mac80211.c:1773:19: error: 'RSI_WOW_ANY' 
>> undeclared (first use in this function); did you mean 'RSI_WEP_KEY'?
>> drivers/net/wireless/rsi/rsi_91x_mac80211.c: In function 
>> 'rsi_mac80211_attach':
>> drivers/net/wireless/rsi/rsi_91x_mac80211.c:1980:7: error: 'struct wiphy' 
>> has no member named 'wowlan'
>>
>> This adds more #ifdef CONFIG_PM guards around the code that otherwise
>> fails to build and that we know is not used without CONFIG_PM.
>>
>> Fixes: f3ac4e7394a1 ("rsi: sdio: add WOWLAN support for S3 suspend state")
>> Signed-off-by: Arnd Bergmann 
>
> Amit already submitted two patches to fix this problem:
>
> https://patchwork.kernel.org/patch/10036297/
>
> https://patchwork.kernel.org/patch/10036299/
>
> I applied them to my pending branch yesterday, and at least buildbot
> seems to be happy, so I'm planning take apply those instead. Please let
> me know if that's a problem.

Looks good: the first patch is identical to mine, the second one appears to
be something I missed.

  Arnd

Re: [PATCH] rsi: sdio: fix building without CONFIG_PM

2017-11-02 Thread Kalle Valo 
Arnd Bergmann  writes:

> The addition of the WoWLAN support has caused a number of new
> build errors when CONFIG_PM is disabled, including:
>
> drivers/net/wireless/rsi/rsi_91x_mac80211.c: In function 
> 'rsi_wow_map_triggers':
> drivers/net/wireless/rsi/rsi_91x_mac80211.c:1773:19: error: 'RSI_WOW_ANY' 
> undeclared (first use in this function); did you mean 'RSI_WEP_KEY'?
> drivers/net/wireless/rsi/rsi_91x_mac80211.c: In function 
> 'rsi_mac80211_attach':
> drivers/net/wireless/rsi/rsi_91x_mac80211.c:1980:7: error: 'struct wiphy' has 
> no member named 'wowlan'
>
> This adds more #ifdef CONFIG_PM guards around the code that otherwise
> fails to build and that we know is not used without CONFIG_PM.
>
> Fixes: f3ac4e7394a1 ("rsi: sdio: add WOWLAN support for S3 suspend state")
> Signed-off-by: Arnd Bergmann 

Amit already submitted two patches to fix this problem:

https://patchwork.kernel.org/patch/10036297/

https://patchwork.kernel.org/patch/10036299/

I applied them to my pending branch yesterday, and at least buildbot
seems to be happy, so I'm planning take apply those instead. Please let
me know if that's a problem.

-- 
Kalle Valo

[PATCH] rsi: sdio: fix building without CONFIG_PM

2017-11-02 Thread Arnd Bergmann 
The addition of the WoWLAN support has caused a number of new
build errors when CONFIG_PM is disabled, including:

drivers/net/wireless/rsi/rsi_91x_mac80211.c: In function 'rsi_wow_map_triggers':
drivers/net/wireless/rsi/rsi_91x_mac80211.c:1773:19: error: 'RSI_WOW_ANY' 
undeclared (first use in this function); did you mean 'RSI_WEP_KEY'?
drivers/net/wireless/rsi/rsi_91x_mac80211.c: In function 'rsi_mac80211_attach':
drivers/net/wireless/rsi/rsi_91x_mac80211.c:1980:7: error: 'struct wiphy' has 
no member named 'wowlan'

This adds more #ifdef CONFIG_PM guards around the code that otherwise
fails to build and that we know is not used without CONFIG_PM.

Fixes: f3ac4e7394a1 ("rsi: sdio: add WOWLAN support for S3 suspend state")
Signed-off-by: Arnd Bergmann 
---
 drivers/net/wireless/rsi/rsi_91x_mac80211.c | 4 +++-
 drivers/net/wireless/rsi/rsi_91x_mgmt.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c 
b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 36c63e953f84..ba6405c7d92b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -1752,6 +1752,7 @@ static int rsi_mac80211_cancel_roc(struct ieee80211_hw 
*hw)
return 0;
 }
 
+#ifdef CONFIG_PM
 static const struct wiphy_wowlan_support rsi_wowlan_support = {
.flags = WIPHY_WOWLAN_ANY |
 WIPHY_WOWLAN_MAGIC_PKT |
@@ -1824,7 +1825,6 @@ int rsi_config_wowlan(struct rsi_hw *adapter, struct 
cfg80211_wowlan *wowlan)
 }
 EXPORT_SYMBOL(rsi_config_wowlan);
 
-#ifdef CONFIG_PM
 static int rsi_mac80211_suspend(struct ieee80211_hw *hw,
struct cfg80211_wowlan *wowlan)
 {
@@ -1977,7 +1977,9 @@ int rsi_mac80211_attach(struct rsi_common *common)
wiphy->features |= NL80211_FEATURE_INACTIVITY_TIMER;
wiphy->reg_notifier = rsi_reg_notify;
 
+#ifdef CONFIG_PM
wiphy->wowlan = _wowlan_support;
+#endif
wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
 
/* Wi-Fi direct parameters */
diff --git a/drivers/net/wireless/rsi/rsi_91x_mgmt.c 
b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
index d38a09f15742..46c9d5470dfb 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mgmt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
@@ -1597,6 +1597,7 @@ static int rsi_send_beacon(struct rsi_common *common)
return 0;
 }
 
+#ifdef CONFIG_PM
 int rsi_send_wowlan_request(struct rsi_common *common, u16 flags,
u16 sleep_status)
 {
@@ -1630,6 +1631,7 @@ int rsi_send_wowlan_request(struct rsi_common *common, 
u16 flags,
 
return rsi_send_internal_mgmt_frame(common, skb);
 }
+#endif
 
 /**
  * rsi_handle_ta_confirm_type() - This function handles the confirm frames.
-- 
2.9.0

[PATCH] IW: Zero or Uninitialized value of keylen passing

2017-11-02 Thread Amit Khatri 
>From b755c8ee282abbd0008e9e7241c457662c90f2c3 Mon Sep 17 00:00:00 2001
From: Amit Khatri 
Date: Thu, 2 Nov 2017 15:55:16 +0530
Subject: [PATCH] IW: Zero or Uninitialized value of keylen passing

In case of hexadeciaml keydata, keylen is not gettig updated
and passing in NLA_PUT(msg, NL80211_KEY_DATA, keylen, keydata)
as zero (becasue of local variable).

This patch initilalize keylen variable in case of hexkey data.

Signed-off-by: Amit Khatri 
---
 util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util.c b/util.c
index 25d909a..1ec0791 100644
--- a/util.c
+++ b/util.c
@@ -427,12 +427,14 @@ int parse_keys(struct nl_msg *msg, char **argv, int argc)
switch (strlen(keydata)) {
case 10:
keydata = hex2bin(keydata, keybuf);
+   keylen = 5;
case 5:
NLA_PUT_U32(msg, NL80211_KEY_CIPHER, 0x000FAC01);
keylen = 5;
break;
case 26:
keydata = hex2bin(keydata, keybuf);
+   keylen = 13;
case 13:
NLA_PUT_U32(msg, NL80211_KEY_CIPHER, 0x000FAC05);
keylen = 13;
-- 
2.7.4

[PATCH] ath10k: move pci suspend/resume functions

2017-11-02 Thread Arnd Bergmann 
The combination of two patches has led to a build failure:

drivers/net/wireless/ath/ath10k/pci.c: In function 'ath10k_pci_pm_suspend':
drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of 
function 'ath10k_pci_suspend'; did you mean 'ath10k_pci_pm_suspend'? 
[-Werror=implicit-function-declaration]
drivers/net/wireless/ath/ath10k/pci.c: In function 'ath10k_pci_pm_resume':
drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of 
function 'ath10k_pci_resume'; did you mean 'ath10k_pci_pm_resume'? 
[-Werror=implicit-function-declaration]

This moves the functions outside of the now incorrect #ifdef.

Fixes: 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported but 
disabled")
Fixes: 6af1de2e4ec4 ("ath10k: mark PM functions as __maybe_unused")
Signed-off-by: Arnd Bergmann 
---
 drivers/net/wireless/ath/ath10k/pci.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
b/drivers/net/wireless/ath/ath10k/pci.c
index b18a9b690df4..6513edbd86e6 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2585,6 +2585,13 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
return 0;
 }
 
+static int ath10k_pci_hif_resume(struct ath10k *ar)
+{
+   /* Nothing to do; the important stuff is in the driver resume. */
+   return 0;
+}
+#endif
+
 static int ath10k_pci_suspend(struct ath10k *ar)
 {
/* The grace timer can still be counting down and ar->ps_awake be true.
@@ -2597,12 +2604,6 @@ static int ath10k_pci_suspend(struct ath10k *ar)
return 0;
 }
 
-static int ath10k_pci_hif_resume(struct ath10k *ar)
-{
-   /* Nothing to do; the important stuff is in the driver resume. */
-   return 0;
-}
-
 static int ath10k_pci_resume(struct ath10k *ar)
 {
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
@@ -2627,7 +2628,6 @@ static int ath10k_pci_resume(struct ath10k *ar)
 
return ret;
 }
-#endif
 
 static bool ath10k_pci_validate_cal(void *data, size_t size)
 {
-- 
2.9.0

Re: [PATCH V2 9/9] qtnfmac: pass all CONNECT cmd params to wireless card for processing

2017-11-02 Thread Sergey Matyukevich 
> From: Igor Mitsyanko 
> 
> Specifically, following parameters are needed for wireless device
> configuration but were not available to it before:
> - HT/VHT capabilities and capabilities masks.
> - full channel info (not just IEEE number)
> - BSSID hint
> - previous BSSID for reassoc request
> 
> Move Management Frame Protection setting from common encr info
> structure into STA-specific .connect command parameters.
> 
> Make sure that all new qlink structure definitions are alignment-safe.
> 
> Signed-off-by: Igor Mitsyanko 

Reviewed-by: Sergey Matyukevich 

for the whole patch set...

Regards,
Sergey

RE: [EXT] Re: [PATCH 1/3] mwifiex: cleanup rx_pkt_lock usage in 11n_rxreorder.c

2017-11-02 Thread Ganapathi Bhat 
Hi Brian,

> On Tue, Oct 31, 2017 at 03:12:45PM +0530, Ganapathi Bhat wrote:
> > From: Karthik Ananthapadmanabha 
> >
> > Fix the incorrect usage of rx_pkt_lock spinlock. Realsing the spinlock
> > while the element is still in process is unsafe. The lock must be
> > released only after the element processing is complete.
> >
> > Signed-off-by: Karthik Ananthapadmanabha 
> > Signed-off-by: Ganapathi Bhat 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> > b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> > index 1edcdda..0149c4a 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> > @@ -124,9 +124,10 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> > rx_tmp_ptr = tbl->rx_reorder_ptr[i];
> > tbl->rx_reorder_ptr[i] = NULL;
> > }
> > -   spin_unlock_irqrestore(>rx_pkt_lock, flags);
>
> So, what is this lock protecting? Is it for protecting the packet itself, or 
> for
> protecting the ring buffer?

This lock is protecting the ring buffer but with the current change, we are 
trying also to protect the packet too.

> As I read it, it's just the latter, since once we've
> removed it from the ring buffer (and are about to dispatch it up toward the
> networking layer), it's handled only by a single context (or else, protected 
> via
> some other common lock(s)).
>
> If I'm correct above, then I think the current code here is actually safe, 
> since
> it holds the lock while removing from the ring buffer -- after it's removed
> from the ring, there's no way another thread can find this payload, and so we
> can release the lock.

There are cases where the ring buffer is iterated by cfg80211 thread:
mwifiex_cfg80211_tdls_oper => mwifiex_tdls_oper => 
mwifiex_tdls_process_disable_link => mwifiex_11n_cleanup_reorder_tbl => 
mwifiex_del_rx_reorder_entry => mwifiex_11n_dispatch_pkt_until_start_win => 
{iterates the ring buffer}
So, at worst case we can have two threads (main & cfg80211) running 
mwifiex_11n_dispatch_pkt_until_start_win(), iterating the ring buffer.

>
> On a related note: I don't actually see the ring buffer *insertion* being
> protected by this rx_pkt_lock, so is it really useful? See
> mwifiex_11n_rx_reorder_pkt(). Perhaps the correct lock is actually
> rx_reorder_tbl_lock()? Except that serves multiple purposes it seems...

Right, ring buffer insertion is not protected.  I think we should be using both 
rx_reorder_tbl_lock(which is already present) and rx_pkt_lock(we need to add) 
during insertion (mwifiex_11n_rx_reorder_pkt()).

Also, we should be using rx_pkt_lock instead of rx_reorder_tbl_lock in 
mwifiex_11n_find_last_seq_num().
>
> Another thought: from what I can tell, all of these operations are
> *only* performed on the main thread, so there's actually no concurrency
> involved at all. So we also could probably drop this lock entirely...

Let us know your inputs on above observations.
>
> I guess the real point is: can you explain better what you intend this lock to
> do? Then we can review whether you're accomplishing your intention, or
> whether we should improve the usage of this lock in some other way, or
> perhaps even kill it entirely.
>
> Thanks,
> Brian
>
> > if (rx_tmp_ptr)
> > mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> > +
> > +   spin_unlock_irqrestore(>rx_pkt_lock, flags);
> > }
> >
> > spin_lock_irqsave(>rx_pkt_lock, flags); @@ -167,8 +168,8 @@
> > static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void
> *payload)
> > }
> > rx_tmp_ptr = tbl->rx_reorder_ptr[i];
> > tbl->rx_reorder_ptr[i] = NULL;
> > -   spin_unlock_irqrestore(>rx_pkt_lock, flags);
> > mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> > +   spin_unlock_irqrestore(>rx_pkt_lock, flags);
> > }
> >
> > spin_lock_irqsave(>rx_pkt_lock, flags);
> > --
> > 1.9.1
> >

Regards,
Ganapathi

[PATCH 3/3] staging: r8188eu: Revert 4 commits breaking ARP

2017-11-02 Thread Hans de Goede 
Commit 2ba8444c97b1 ("staging:r8188eu: move IV/ICV trimming into
decrypt() and also place it after rtl88eu_mon_recv_hook()") breaks ARP.

After this commit ssh-ing to a laptop with r8188eu wifi no longer works
if the machine connecting has never communicated with the laptop before.
This is 100% reproducable using "arp -d  && ssh " to ssh to
a laptop with r8188eu wifi.

This commit reverts 4 commits in total:

1. Commit 79650ffde38e ("staging:r8188eu: trim IV/ICV fields in
   validate_recv_data_frame()")
This commit depends on 2 of the other commits being reverted.

2. Commit 02b19b4c4920 ("staging:r8188eu: inline unprotect_frame() in
   mon_recv_decrypted_recv()")
The inline code is wrong the un-inlined version contains:
if (skb->len < hdr_len + iv_len + icv_len)
return;
...
Where as the inline-ed code introduced by this commit does:
if (skb->len < hdr_len + iv_len + icv_len) {
...
Note the same check, but now to actually continue doing ... instead
of to not do it, so this commit is no good.

3. Commit d86e16da6a5d ("staging:r8188eu: use different mon_recv_decrypted()
   inside rtl88eu_mon_recv_hook() and rtl88eu_mon_xmit_hook().")
This commit introduced a 1:1 copy of a function so that one of the
2 copies can be modified in the 2 commits we're already reverting.

4. Commit 2ba8444c97b1 ("staging:r8188eu: move IV/ICV trimming into
   decrypt() and also place it after rtl88eu_mon_recv_hook()")
This is the commit actually breaking ARP.

Note this commit is a straight-forward squash of the revert of these
4 commits, without any changes.

Cc: sta...@vger.kernel.org
Cc: Ivan Safonov 
Signed-off-by: Hans de Goede 
---
 drivers/staging/rtl8188eu/core/rtw_recv.c | 83 +++
 drivers/staging/rtl8188eu/os_dep/mon.c| 34 ++---
 2 files changed, 55 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c 
b/drivers/staging/rtl8188eu/core/rtw_recv.c
index 2f0341689e2f..6506a1587df0 100644
--- a/drivers/staging/rtl8188eu/core/rtw_recv.c
+++ b/drivers/staging/rtl8188eu/core/rtw_recv.c
@@ -264,10 +264,12 @@ static int recvframe_chkmic(struct adapter *adapter,
}
 
/* icv_len included the mic code */
-   datalen = precvframe->pkt->len-prxattrib->hdrlen - 8;
+   datalen = precvframe->pkt->len-prxattrib->hdrlen -
+ prxattrib->iv_len-prxattrib->icv_len-8;
pframe = precvframe->pkt->data;
-   payload = pframe+prxattrib->hdrlen;
+   payload = pframe+prxattrib->hdrlen+prxattrib->iv_len;
 
+   RT_TRACE(_module_rtl871x_recv_c_, _drv_info_, ("\n 
prxattrib->iv_len=%d prxattrib->icv_len=%d\n", prxattrib->iv_len, 
prxattrib->icv_len));
rtw_seccalctkipmic(mickey, pframe, payload, datalen, 
[0],
   (unsigned char)prxattrib->priority); 
/* care the length of the data */
 
@@ -413,15 +415,9 @@ static struct recv_frame *decryptor(struct adapter 
*padapter,
default:
break;
}
-   if (res != _FAIL) {
-   memmove(precv_frame->pkt->data + 
precv_frame->attrib.iv_len, precv_frame->pkt->data, precv_frame->attrib.hdrlen);
-   skb_pull(precv_frame->pkt, precv_frame->attrib.iv_len);
-   skb_trim(precv_frame->pkt, precv_frame->pkt->len - 
precv_frame->attrib.icv_len);
-   }
} else if (prxattrib->bdecrypted == 1 && prxattrib->encrypt > 0 &&
-  (psecuritypriv->busetkipkey == 1 || prxattrib->encrypt != 
_TKIP_)) {
-   psecuritypriv->hw_decrypted = true;
-   }
+  (psecuritypriv->busetkipkey == 1 || prxattrib->encrypt != 
_TKIP_))
+   psecuritypriv->hw_decrypted = true;
 
if (res == _FAIL) {
rtw_free_recvframe(return_packet, 
>recvpriv.free_recv_queue);
@@ -462,7 +458,7 @@ static struct recv_frame *portctrl(struct adapter *adapter,
 
if (auth_alg == 2) {
/* get ether_type */
-   ptr = ptr + pfhdr->attrib.hdrlen + LLC_HEADER_SIZE;
+   ptr = ptr + pfhdr->attrib.hdrlen + LLC_HEADER_SIZE + 
pfhdr->attrib.iv_len;
memcpy(_tmp, ptr, 2);
ether_type = ntohs(be_tmp);
 
@@ -1145,8 +1141,6 @@ static int validate_recv_data_frame(struct adapter 
*adapter,
}
 
if (pattrib->privacy) {
-   struct sk_buff *skb = precv_frame->pkt;
-
RT_TRACE(_module_rtl871x_recv_c_, _drv_info_, 
("validate_recv_data_frame:pattrib->privacy=%x\n", pattrib->privacy));
RT_TRACE(_module_rtl871x_recv_c_, _drv_info_, ("\n 
^^^IS_MCAST(pattrib->ra(0x%02x))=%d^^^6\n", pattrib->ra[0],

[PATCH 1/3] staging: rtl8188eu: Revert part of "staging: rtl8188eu: fix comments with lines over 80 characters"

2017-11-02 Thread Hans de Goede 
Commit 74e1e498e84e ("staging: rtl8188eu: fix comments with lines over 80
characters") not only changed comments but also changed an if check:

-if (pmlmepriv->cur_network.join_res != true) {
+if (!(pmlmepriv->cur_network.join_res)) {

This is not equivalent as join_res is an int and can have values such
as -2 and -3.

Note for the next time, please only make one type of changes in a single
clean-up commit.

Fixes: 74e1e498e84e ("staging: rtl8188eu: fix comments with lines over 80 ...")
Cc: Juliana Rodrigues 
Signed-off-by: Hans de Goede 
---
 drivers/staging/rtl8188eu/core/rtw_ap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c 
b/drivers/staging/rtl8188eu/core/rtw_ap.c
index 32a483769975..fa611455109a 100644
--- a/drivers/staging/rtl8188eu/core/rtw_ap.c
+++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
@@ -754,7 +754,7 @@ static void start_bss_network(struct adapter *padapter, u8 
*pbuf)
}
 
/* setting only at  first time */
-   if (!(pmlmepriv->cur_network.join_res)) {
+   if (pmlmepriv->cur_network.join_res != true) {
/* WEP Key will be set before this function, do not
 * clear CAM.
 */
-- 
2.14.2

[PATCH 2/3] staging: rtl8188eu: Fix bug introduced by convert timers to use timer_setup()

2017-11-02 Thread Hans de Goede 
Commit b7749656e946 ("staging: rtl8188eu: Convert timers to use
timer_setup()") introduces a copy and paste error which causes the
rtl8188eu driver to no longer function. This commit fixes this.

Fixes: b7749656e946 ("staging: rtl8188eu: Convert timers to use timer_setup()")
Cc: Kees Cook 
Signed-off-by: Hans de Goede 
---
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index d717046a6c2f..d73e9bdc80cc 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -4806,7 +4806,7 @@ void linked_status_chk(struct adapter *padapter)
 void survey_timer_hdl(struct timer_list *t)
 {
struct adapter *padapter = from_timer(padapter, t,
-   mlmeextpriv.link_timer);
+ mlmeextpriv.survey_timer);
struct cmd_obj  *ph2c;
struct sitesurvey_parm  *psurveyPara;
struct cmd_priv *pcmdpriv = 
>cmdpriv;
-- 
2.14.2