Re: Bayesian rate control

2016-10-25 Thread Johannes Berg 

> The intel 7260 and later parts also allow user controllable rate
> control and provide transmit completion feedback, but I don't know
> whether it's enough for your needs.

Perhaps. However, existing rate control is *very* tightly coupled to
the driver, and it'd be fairly pointless to disentangle just for the
sake of playing with a rate control algorithm.

Also, the device doesn't support per-frame control nor any kind of
sampling-with-table-fallback, only the rate table that you give to the
device and update.

Btw, mac80211_hwsim with wmediumd doing some medium simulation might
also be something to look at for just extending to VHT.

And come to think of it, there's this new driver Felix et al have been
working on, mt7601u, which also should support proper rate control
APIs.

johannes

Re: [PATCH 7/8] mac80211: FILS AEAD protection for station mode association frames

2016-10-25 Thread Johannes Berg 

> +static u8 *fils_find_session(u8 *pos, u8 *end)
> +{
> + while (end - pos > 2 && end - pos >= 2 + pos[1]) {
> + if (pos[0] == 255 && pos[1] == 1 + 8 && pos[2] == 4)
> + return pos;
> + pos += 2 + pos[1];
> + }
> +
> + return NULL;
> +}

Hmm. I think we should try to write this in terms of cfg80211_find_ie,
or perhaps cfg80211_find_ie_match, maybe we need to extend those but
this won't be the only one using the 255 escape.

Perhaps just making the eid passed there be a u16, and extending the
EID definitions appropriately?

The code would probably be shorter as is for now, but still ...

> + if (!session) {
> + sdata_info(sdata,
> +    "No FILS Session element in
> (Re)Association Response frame from %pM",
> +    mgmt->sa);
> + return -EINVAL;

Should we really print the message this prominently? It seems like an
error case that we may not want to log at all, in case somebody tries
to flood us with such frames?

> + crypt_len = frame + *frame_len - encr;
> + if (crypt_len < AES_BLOCK_SIZE) {
> + sdata_info(sdata,
> +    "Not enough room for AES-SIV data after
> FILS Session element in (Re)Association Response frame from %pM",
> +    mgmt->sa);
> + return -EINVAL;
> + }
> + res = aes_siv_decrypt(assoc_data->fils_kek, assoc_data-
> >fils_kek_len,
> +   encr, crypt_len, 5, addr, len, encr);
> + if (res != 0) {
> + sdata_info(sdata,
> +    "AES-SIV decryption of (Re)Association
> Response frame from %pM failed",
> +    mgmt->sa);
> + return res;
> + }

Likewise here. Perhaps make these mlme_dbg() or so instead?

> + if (req->fils_nonces)
> + assoc_data_len += 2 * FILS_NONCE_LEN;

Is this really correct? It seems like a bit of an artifact of initially
having had the nonces in a variable part of the struct?

Or perhaps they have to go into the frame in some place that I missed?
Please add a comment if so.

Also, you're never checking req->fils_nonces_set, so you can get rid of
that.

johannes

Re: [PATCH 5/8] cfg80211: Add KEK/nonces for FILS association frames

2016-10-25 Thread Johannes Berg 

> +++ b/net/wireless/nl80211.c
> @@ -414,6 +414,10 @@ enum nl80211_multicast_groups {
>   [NL80211_ATTR_NAN_MASTER_PREF] = { .type = NLA_U8 },
>   [NL80211_ATTR_NAN_DUAL] = { .type = NLA_U8 },
>   [NL80211_ATTR_NAN_FUNC] = { .type = NLA_NESTED },
> + [NL80211_ATTR_FILS_KEK] = { .type = NLA_BINARY,
> + .len = FILS_MAX_KEK_LEN },
> + [NL80211_ATTR_FILS_NONCES] = { .type = NLA_BINARY,
> +    .len = 2 * FILS_NONCE_LEN },
>  };

If you remove the type = NLA_BINARY and just leave the type zero, then
you'll get *minimum* length validation, rather than limiting the
maximum length. That seems more appropriate for the nonces?

> + if (info->attrs[NL80211_ATTR_FILS_NONCES]) {
> + if (nla_len(info->attrs[NL80211_ATTR_FILS_NONCES])
> !=
> + 2 * FILS_NONCE_LEN)
> + return -EINVAL;

You're validating the *exact* length here, which unfortunately nlattr
doesn't support right now, but perhaps we can live with checking that
it's at least that many bytes, and using only 2*nonces? We do that for
most other attributes (like MAC addresses).

Or do we expect to extend this to more than 2 nonces in the future, at
which point we'll need the length?

johannes

Re: [PATCH 2/8] mac80211: Allow AUTH_DATA to be used for FILS

2016-10-25 Thread Johannes Berg 

>   if (req->auth_data_len >= 4) {
> - __le16 *pos = (__le16 *) req->auth_data;
> - auth_data->sae_trans = le16_to_cpu(pos[0]);
> - auth_data->sae_status = le16_to_cpu(pos[1]);
> + if (req->auth_type == NL80211_AUTHTYPE_SAE) {
> + __le16 *pos = (__le16 *) req->auth_data;
> + auth_data->sae_trans = le16_to_cpu(pos[0]);
> + auth_data->sae_status = le16_to_cpu(pos[1]);
> + }
>   memcpy(auth_data->data, req->auth_data + 4,
>      req->auth_data_len - 4);
>   auth_data->data_len += req->auth_data_len - 4;

Hmm. Do we really want to still skip the first four bytes of the data
userspace passed? That seems a bit strange to me. The docs in nl80211.h
do say it that way now, but should we really include a dummy
Authentication transaction sequence number field?

johannes

lening bieden 3%

2016-10-25 Thread Lloyds TSB Bank PLC 
Goede dag,

 Dit is Lloyd's TSB Bank plc leningen aan te bieden.

   Lloyds TSB biedt flexibele en betaalbare leningen voor welk doel u te helpen 
uw doelen te bereiken. we lening tegen lage rente van 3%. Hier zijn een aantal 
belangrijke kenmerken van de persoonlijke lening aangeboden door Lloyd's TSB. 
Hier zijn de Loan Factoren we werken met de toonaangevende Britse makelaars die 
toegang hebben tot de top kredietverstrekkers hebben en in staat zijn om de 
beste financiële oplossing tegen een betaalbare price.Please vinden als u 
geïnteresseerd bent vriendelijk contact met ons op via deze e-mail: 
lloyds26...@gmail.com


Na de reactie, zal u een aanvraag voor een lening te vullen ontvangen. Geen 
sociale zekerheid en geen credit check, 100% gegarandeerd.

Het zal ons een eer zijn als u ons toelaten om u van dienst zijn.


INFORMATIE NODIG

Jullie namen:
Adres: ...
Telefoon: ...
Benodigd 
Duur: ...
Bezetting: ...
Maandelijks Inkomen Level: 
Geslacht: ...
Geboortedatum: 
Staat: ..
Land: ..
Doel: .

Ontmoeting uw financiële behoeften is onze trots.


Dr.John Mahama.

Re: Bayesian rate control

2016-10-25 Thread Adrian Chadd 
hiya,

there's a tplink RTL81xx 11ac NIC that's growing linux and freebsd
support. I believe it's software rate controllable.

The intel 7260 and later parts also allow user controllable rate
control and provide transmit completion feedback, but I don't know
whether it's enough for your needs.

Unfortunately the ath10k firmware .. doesn't :( Not by default, anyway.


-adrian

[PATCH 3/8] cfg80211: Add feature flag for Fast Initial Link Setup (FILS)

2016-10-25 Thread Jouni Malinen 
This defines a feature flag that drivers can use to indicate that they
support (IEEE 802.11ai) when using user space SME
(NL80211_CMD_AUTHENTICATE) in station mode.

Signed-off-by: Jouni Malinen 
---
 include/uapi/linux/nl80211.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 528aa48..39e1647 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4641,6 +4641,8 @@ enum nl80211_feature_flags {
  * configuration (AP/mesh) with HT rates.
  * @NL80211_EXT_FEATURE_BEACON_RATE_VHT: Driver supports beacon rate
  * configuration (AP/mesh) with VHT rates.
+ * @NL80211_EXT_FEATURE_FILS: This driver supports Fast Initial Link Setup with
+ * user space SME (NL80211_CMD_AUTHENTICATE) in station mode.
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -4655,6 +4657,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_BEACON_RATE_LEGACY,
NL80211_EXT_FEATURE_BEACON_RATE_HT,
NL80211_EXT_FEATURE_BEACON_RATE_VHT,
+   NL80211_EXT_FEATURE_FILS,
 
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
-- 
1.9.1

[PATCH 8/8] mac80211: Claim Fast Initial Link Setup (FILS) support

2016-10-25 Thread Jouni Malinen 
With the previous commits, initial FILS support is now functional in
mac80211-based drivers for both AP and stations roles.

Signed-off-by: Jouni Malinen 
---
 net/mac80211/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 0d9163c..3e9ca80 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -549,6 +549,7 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t 
priv_data_len,
   NL80211_FEATURE_MAC_ON_CREATE |
   NL80211_FEATURE_USERSPACE_MPM |
   NL80211_FEATURE_FULL_AP_CLIENT_STATE;
+   wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_FILS);
 
if (!ops->hw_scan)
wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN |
-- 
1.9.1

[PATCH 5/8] cfg80211: Add KEK/nonces for FILS association frames

2016-10-25 Thread Jouni Malinen 
The new nl80211 attributes can be used to provide KEK and nonces to
allow the driver to encrypt and decrypt FILS (Re)Association
Request/Response frames in station mode.

Signed-off-by: Jouni Malinen 
---
 include/linux/ieee80211.h|  3 +++
 include/net/cfg80211.h   |  9 +
 include/uapi/linux/nl80211.h |  8 
 net/wireless/nl80211.c   | 17 +
 4 files changed, 37 insertions(+)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 9a523d9..a39beb9 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -2076,6 +2076,9 @@ enum ieee80211_key_len {
 #define IEEE80211_GCMP_MIC_LEN 16
 #define IEEE80211_GCMP_PN_LEN  6
 
+#define FILS_NONCE_LEN 16
+#define FILS_MAX_KEK_LEN   64
+
 /* Public action codes */
 enum ieee80211_pub_actioncode {
WLAN_PUB_ACTION_EXT_CHANSW_ANN = 4,
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2667917..a3045f1 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1842,6 +1842,12 @@ enum cfg80211_assoc_req_flags {
  * @ht_capa_mask:  The bits of ht_capa which are to be used.
  * @vht_capa: VHT capability override
  * @vht_capa_mask: VHT capability mask indicating which fields to use
+ * @fils_kek: FILS KEK for protecting (Re)Association Request/Response frame or
+ * %NULL if FILS is not used.
+ * @fils_kek_len: Length of fils_kek in octets
+ * @fils_nonces: FILS nonces (part of AAD) for protecting (Re)Association
+ * Request/Response frame or %NULL if FILS is not used. This field starts
+ * with 16 octets of STA Nonce followed by 16 octets of AP Nonce.
  */
 struct cfg80211_assoc_request {
struct cfg80211_bss *bss;
@@ -1853,6 +1859,9 @@ struct cfg80211_assoc_request {
struct ieee80211_ht_cap ht_capa;
struct ieee80211_ht_cap ht_capa_mask;
struct ieee80211_vht_cap vht_capa, vht_capa_mask;
+   const u8 *fils_kek;
+   size_t fils_kek_len;
+   const u8 *fils_nonces;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 5d36e83..b468a89 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1938,6 +1938,11 @@ enum nl80211_commands {
  * attribute.
  * @NL80211_ATTR_NAN_MATCH: used to report a match. This is a nested attribute.
  * See  nl80211_nan_match_attributes.
+ * @NL80211_ATTR_FILS_KEK: KEK for FILS (Re)Association Request/Response frame
+ * protection.
+ * @NL80211_ATTR_FILS_NONCES: Nonces (part of AAD) for FILS (Re)Association
+ * Request/Response frame protection. This attribute contains the 16 octet
+ * STA Nonce followed by 16 octets of AP Nonce.
  *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
@@ -2338,6 +2343,9 @@ enum nl80211_attrs {
NL80211_ATTR_NAN_FUNC,
NL80211_ATTR_NAN_MATCH,
 
+   NL80211_ATTR_FILS_KEK,
+   NL80211_ATTR_FILS_NONCES,
+
/* add attributes here, update the policy in nl80211.c */
 
__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ebd7eaf..a06145c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -414,6 +414,10 @@ enum nl80211_multicast_groups {
[NL80211_ATTR_NAN_MASTER_PREF] = { .type = NLA_U8 },
[NL80211_ATTR_NAN_DUAL] = { .type = NLA_U8 },
[NL80211_ATTR_NAN_FUNC] = { .type = NLA_NESTED },
+   [NL80211_ATTR_FILS_KEK] = { .type = NLA_BINARY,
+   .len = FILS_MAX_KEK_LEN },
+   [NL80211_ATTR_FILS_NONCES] = { .type = NLA_BINARY,
+  .len = 2 * FILS_NONCE_LEN },
 };
 
 /* policy for the key attributes */
@@ -8019,6 +8023,19 @@ static int nl80211_associate(struct sk_buff *skb, struct 
genl_info *info)
req.flags |= ASSOC_REQ_USE_RRM;
}
 
+   if (info->attrs[NL80211_ATTR_FILS_KEK]) {
+   req.fils_kek = nla_data(info->attrs[NL80211_ATTR_FILS_KEK]);
+   req.fils_kek_len = nla_len(info->attrs[NL80211_ATTR_FILS_KEK]);
+   }
+
+   if (info->attrs[NL80211_ATTR_FILS_NONCES]) {
+   if (nla_len(info->attrs[NL80211_ATTR_FILS_NONCES]) !=
+   2 * FILS_NONCE_LEN)
+   return -EINVAL;
+   req.fils_nonces =
+   nla_data(info->attrs[NL80211_ATTR_FILS_NONCES]);
+   }
+
err = nl80211_crypto_settings(rdev, info, , 1);
if (!err) {
wdev_lock(dev->ieee80211_ptr);
-- 
1.9.1

[PATCH 7/8] mac80211: FILS AEAD protection for station mode association frames

2016-10-25 Thread Jouni Malinen 
This adds support for encrypting (Re)Association Request frame and
decryption (Re)Association Response frame when using FILS in station
mode.

Signed-off-by: Jouni Malinen 
---
 net/mac80211/Makefile  |   1 +
 net/mac80211/aes_cmac.c|   8 +-
 net/mac80211/aes_cmac.h|   4 +
 net/mac80211/fils_aead.c   | 354 +
 net/mac80211/fils_aead.h   |  19 +++
 net/mac80211/ieee80211_i.h |   5 +
 net/mac80211/mlme.c|  34 -
 7 files changed, 420 insertions(+), 5 deletions(-)
 create mode 100644 net/mac80211/fils_aead.c
 create mode 100644 net/mac80211/fils_aead.h

diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index f9137a8..0b202b3 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -19,6 +19,7 @@ mac80211-y := \
aes_gcm.o \
aes_cmac.o \
aes_gmac.o \
+   fils_aead.o \
cfg.o \
ethtool.o \
rx.o \
diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
index bdf0790..d0bd5ff 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -23,7 +23,7 @@
 #define AAD_LEN 20
 
 
-static void gf_mulx(u8 *pad)
+void gf_mulx(u8 *pad)
 {
int i, carry;
 
@@ -35,9 +35,9 @@ static void gf_mulx(u8 *pad)
pad[AES_BLOCK_SIZE - 1] ^= 0x87;
 }
 
-static void aes_cmac_vector(struct crypto_cipher *tfm, size_t num_elem,
-   const u8 *addr[], const size_t *len, u8 *mac,
-   size_t mac_len)
+void aes_cmac_vector(struct crypto_cipher *tfm, size_t num_elem,
+const u8 *addr[], const size_t *len, u8 *mac,
+size_t mac_len)
 {
u8 cbc[AES_BLOCK_SIZE], pad[AES_BLOCK_SIZE];
const u8 *pos, *end;
diff --git a/net/mac80211/aes_cmac.h b/net/mac80211/aes_cmac.h
index 3702041..c827e1d 100644
--- a/net/mac80211/aes_cmac.h
+++ b/net/mac80211/aes_cmac.h
@@ -11,6 +11,10 @@
 
 #include 
 
+void gf_mulx(u8 *pad);
+void aes_cmac_vector(struct crypto_cipher *tfm, size_t num_elem,
+const u8 *addr[], const size_t *len, u8 *mac,
+size_t mac_len);
 struct crypto_cipher *ieee80211_aes_cmac_key_setup(const u8 key[],
   size_t key_len);
 void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
diff --git a/net/mac80211/fils_aead.c b/net/mac80211/fils_aead.c
new file mode 100644
index 000..358c2cb
--- /dev/null
+++ b/net/mac80211/fils_aead.c
@@ -0,0 +1,354 @@
+/*
+ * FILS AEAD for (Re)Association Request/Response frames
+ * Copyright 2016, Qualcomm Atheros, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+
+#include "ieee80211_i.h"
+#include "aes_cmac.h"
+#include "fils_aead.h"
+
+static int aes_s2v(struct crypto_cipher *tfm,
+  size_t num_elem, const u8 *addr[], size_t len[], u8 *v)
+{
+   u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
+   size_t i;
+   const u8 *data[2];
+   size_t data_len[2], data_elems;
+
+   /* D = AES-CMAC(K, ) */
+   memset(tmp, 0, AES_BLOCK_SIZE);
+   data[0] = tmp;
+   data_len[0] = AES_BLOCK_SIZE;
+   aes_cmac_vector(tfm, 1, data, data_len, d, AES_BLOCK_SIZE);
+
+   for (i = 0; i < num_elem - 1; i++) {
+   /* D = dbl(D) xor AES_CMAC(K, Si) */
+   gf_mulx(d); /* dbl */
+   aes_cmac_vector(tfm, 1, [i], [i], tmp,
+   AES_BLOCK_SIZE);
+   crypto_xor(d, tmp, AES_BLOCK_SIZE);
+   }
+
+   if (len[i] >= AES_BLOCK_SIZE) {
+   /* len(Sn) >= 128 */
+   size_t j;
+   const u8 *pos;
+
+   /* T = Sn xorend D */
+
+   /* Use a temporary buffer to perform xorend on Sn (addr[i]) to
+* avoid modifying the const input argument.
+*/
+   data[0] = addr[i];
+   data_len[0] = len[i] - AES_BLOCK_SIZE;
+   pos = addr[i] + data_len[0];
+   for (j = 0; j < AES_BLOCK_SIZE; j++)
+   tmp[j] = pos[j] ^ d[j];
+   data[1] = tmp;
+   data_len[1] = AES_BLOCK_SIZE;
+   data_elems = 2;
+   } else {
+   /* len(Sn) < 128 */
+   /* T = dbl(D) xor pad(Sn) */
+   gf_mulx(d); /* dbl */
+   memset(tmp, 0, AES_BLOCK_SIZE);
+   memcpy(tmp, addr[i], len[i]);
+   tmp[len[i]] = 0x80;
+   crypto_xor(d, tmp, AES_BLOCK_SIZE);
+   data[0] = d;
+   data_len[0] = sizeof(d);
+   data_elems = 1;
+   }
+   /* V = AES-CMAC(K, T) */
+   aes_cmac_vector(tfm, data_elems, data, data_len, v, AES_BLOCK_SIZE);
+
+   return 0;
+}
+
+/* Note: addr[] and len[]

[PATCH 4/8] cfg80211: Add Fast Initial Link Setup (FILS) auth algs

2016-10-25 Thread Jouni Malinen 
This defines authentication algorithms for FILS (IEEE 802.11ai).

Signed-off-by: Jouni Malinen 
---
 include/linux/ieee80211.h|  3 +++
 include/uapi/linux/nl80211.h |  6 ++
 net/wireless/nl80211.c   | 21 +++--
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index a80516f..9a523d9 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -1576,6 +1576,9 @@ struct ieee80211_vht_operation {
 #define WLAN_AUTH_SHARED_KEY 1
 #define WLAN_AUTH_FT 2
 #define WLAN_AUTH_SAE 3
+#define WLAN_AUTH_FILS_SK 4
+#define WLAN_AUTH_FILS_SK_PFS 5
+#define WLAN_AUTH_FILS_PK 6
 #define WLAN_AUTH_LEAP 128
 
 #define WLAN_AUTH_CHALLENGE_LEN 128
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 39e1647..5d36e83 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3663,6 +3663,9 @@ enum nl80211_bss_status {
  * @NL80211_AUTHTYPE_FT: Fast BSS Transition (IEEE 802.11r)
  * @NL80211_AUTHTYPE_NETWORK_EAP: Network EAP (some Cisco APs and mainly LEAP)
  * @NL80211_AUTHTYPE_SAE: Simultaneous authentication of equals
+ * @NL80211_AUTHTYPE_FILS_SK: Fast Initial Link Setup shared key
+ * @NL80211_AUTHTYPE_FILS_SK_PFS: Fast Initial Link Setup shared key with PFS
+ * @NL80211_AUTHTYPE_FILS_PK: Fast Initial Link Setup public key
  * @__NL80211_AUTHTYPE_NUM: internal
  * @NL80211_AUTHTYPE_MAX: maximum valid auth algorithm
  * @NL80211_AUTHTYPE_AUTOMATIC: determine automatically (if necessary by
@@ -3675,6 +3678,9 @@ enum nl80211_auth_type {
NL80211_AUTHTYPE_FT,
NL80211_AUTHTYPE_NETWORK_EAP,
NL80211_AUTHTYPE_SAE,
+   NL80211_AUTHTYPE_FILS_SK,
+   NL80211_AUTHTYPE_FILS_SK_PFS,
+   NL80211_AUTHTYPE_FILS_PK,
 
/* keep last */
__NL80211_AUTHTYPE_NUM,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 36dd300..ebd7eaf 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3762,12 +3762,23 @@ static bool nl80211_valid_auth_type(struct 
cfg80211_registered_device *rdev,
if (!(rdev->wiphy.features & NL80211_FEATURE_SAE) &&
auth_type == NL80211_AUTHTYPE_SAE)
return false;
+   if (!wiphy_ext_feature_isset(>wiphy,
+NL80211_EXT_FEATURE_FILS) &&
+   (auth_type == NL80211_AUTHTYPE_FILS_SK ||
+auth_type == NL80211_AUTHTYPE_FILS_SK_PFS ||
+auth_type == NL80211_AUTHTYPE_FILS_PK))
+   return false;
return true;
case NL80211_CMD_CONNECT:
case NL80211_CMD_START_AP:
/* SAE not supported yet */
if (auth_type == NL80211_AUTHTYPE_SAE)
return false;
+   /* FILS not supported yet */
+   if (auth_type == NL80211_AUTHTYPE_FILS_SK ||
+   auth_type == NL80211_AUTHTYPE_FILS_SK_PFS ||
+   auth_type == NL80211_AUTHTYPE_FILS_PK)
+   return false;
return true;
default:
return false;
@@ -7796,12 +7807,18 @@ static int nl80211_authenticate(struct sk_buff *skb, 
struct genl_info *info)
if (!nl80211_valid_auth_type(rdev, auth_type, NL80211_CMD_AUTHENTICATE))
return -EINVAL;
 
-   if (auth_type == NL80211_AUTHTYPE_SAE &&
+   if ((auth_type == NL80211_AUTHTYPE_SAE ||
+auth_type == NL80211_AUTHTYPE_FILS_SK ||
+auth_type == NL80211_AUTHTYPE_FILS_SK_PFS ||
+auth_type == NL80211_AUTHTYPE_FILS_PK) &&
!info->attrs[NL80211_ATTR_AUTH_DATA])
return -EINVAL;
 
if (info->attrs[NL80211_ATTR_AUTH_DATA]) {
-   if (auth_type != NL80211_AUTHTYPE_SAE)
+   if (auth_type != NL80211_AUTHTYPE_SAE &&
+   auth_type != NL80211_AUTHTYPE_FILS_SK &&
+   auth_type != NL80211_AUTHTYPE_FILS_SK_PFS &&
+   auth_type != NL80211_AUTHTYPE_FILS_PK)
return -EINVAL;
auth_data = nla_data(info->attrs[NL80211_ATTR_AUTH_DATA]);
auth_data_len = nla_len(info->attrs[NL80211_ATTR_AUTH_DATA]);
-- 
1.9.1

[PATCH 6/8] mac80211: Add FILS auth alg mapping

2016-10-25 Thread Jouni Malinen 
Signed-off-by: Jouni Malinen 
---
 net/mac80211/mlme.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index b6222f2..b815f2d 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2618,6 +2618,9 @@ static void ieee80211_rx_mgmt_auth(struct 
ieee80211_sub_if_data *sdata,
case WLAN_AUTH_LEAP:
case WLAN_AUTH_FT:
case WLAN_AUTH_SAE:
+   case WLAN_AUTH_FILS_SK:
+   case WLAN_AUTH_FILS_SK_PFS:
+   case WLAN_AUTH_FILS_PK:
break;
case WLAN_AUTH_SHARED_KEY:
if (ifmgd->auth_data->expected_transaction != 4) {
@@ -4479,6 +4482,15 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data 
*sdata,
case NL80211_AUTHTYPE_SAE:
auth_alg = WLAN_AUTH_SAE;
break;
+   case NL80211_AUTHTYPE_FILS_SK:
+   auth_alg = WLAN_AUTH_FILS_SK;
+   break;
+   case NL80211_AUTHTYPE_FILS_SK_PFS:
+   auth_alg = WLAN_AUTH_FILS_SK_PFS;
+   break;
+   case NL80211_AUTHTYPE_FILS_PK:
+   auth_alg = WLAN_AUTH_FILS_PK;
+   break;
default:
return -EOPNOTSUPP;
}
-- 
1.9.1

[PATCH 2/8] mac80211: Allow AUTH_DATA to be used for FILS

2016-10-25 Thread Jouni Malinen 
The special SAE case should be limited only for SAE since the more
generic AUTH_DATA can now be used with other authentication algorithms
as well.

Signed-off-by: Jouni Malinen 
---
 net/mac80211/mlme.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 32fd295..b6222f2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4491,9 +4491,11 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data 
*sdata,
auth_data->bss = req->bss;
 
if (req->auth_data_len >= 4) {
-   __le16 *pos = (__le16 *) req->auth_data;
-   auth_data->sae_trans = le16_to_cpu(pos[0]);
-   auth_data->sae_status = le16_to_cpu(pos[1]);
+   if (req->auth_type == NL80211_AUTHTYPE_SAE) {
+   __le16 *pos = (__le16 *) req->auth_data;
+   auth_data->sae_trans = le16_to_cpu(pos[0]);
+   auth_data->sae_status = le16_to_cpu(pos[1]);
+   }
memcpy(auth_data->data, req->auth_data + 4,
   req->auth_data_len - 4);
auth_data->data_len += req->auth_data_len - 4;
-- 
1.9.1

[PATCH 1/8] cfg80211: Rename SAE_DATA to more generic AUTH_DATA

2016-10-25 Thread Jouni Malinen 
This adds defines and nl80211 extensions to allow FILS Authentication to
be implemented similarly to SAE. FILS does not need the special rules
for Authentication transaction number, but it does need to add non-IE
fields. The previously used NL80211_ATTR_SAE_DATA can be reused for this
to avoid having to duplicate that implementation. Rename that attribute
to more generic NL80211_ATTR_AUTH_DATA (with backwards compatibility
define for NL80211_SAE_DATA).

Signed-off-by: Jouni Malinen 
---
 include/net/cfg80211.h   | 10 +-
 include/uapi/linux/nl80211.h |  9 ++---
 net/mac80211/mlme.c  | 12 ++--
 net/wireless/core.h  |  2 +-
 net/wireless/mlme.c  |  6 +++---
 net/wireless/nl80211.c   | 18 +-
 6 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ec39f89..2667917 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1789,9 +1789,9 @@ struct cfg80211_bss {
  * @key_len: length of WEP key for shared key authentication
  * @key_idx: index of WEP key for shared key authentication
  * @key: WEP key for shared key authentication
- * @sae_data: Non-IE data to use with SAE or %NULL. This starts with
- * Authentication transaction sequence number field.
- * @sae_data_len: Length of sae_data buffer in octets
+ * @auth_data: IE and non-IE data to use with SAE/FILS or %NULL. This starts
+ * with Authentication transaction sequence number field.
+ * @auth_data_len: Length of auth_data buffer in octets
  */
 struct cfg80211_auth_request {
struct cfg80211_bss *bss;
@@ -1800,8 +1800,8 @@ struct cfg80211_auth_request {
enum nl80211_auth_type auth_type;
const u8 *key;
u8 key_len, key_idx;
-   const u8 *sae_data;
-   size_t sae_data_len;
+   const u8 *auth_data;
+   size_t auth_data_len;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 1362d24..528aa48 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1638,8 +1638,10 @@ enum nl80211_commands {
  * the connection request from a station. nl80211_connect_failed_reason
  * enum has different reasons of connection failure.
  *
- * @NL80211_ATTR_SAE_DATA: SAE elements in Authentication frames. This starts
- * with the Authentication transaction sequence number field.
+ * @NL80211_ATTR_AUTH_DATA: Fields and elements in Authentication frames. This
+ * starts with the Authentication transaction sequence number field and is
+ * used with authentication algorithms that need special fields to be added
+ * into the frames (SAE and FILS).
  *
  * @NL80211_ATTR_VHT_CAPABILITY: VHT Capability information element (from
  * association request when used with NL80211_CMD_NEW_STATION)
@@ -2195,7 +2197,7 @@ enum nl80211_attrs {
 
NL80211_ATTR_CONN_FAILED_REASON,
 
-   NL80211_ATTR_SAE_DATA,
+   NL80211_ATTR_AUTH_DATA,
 
NL80211_ATTR_VHT_CAPABILITY,
 
@@ -2347,6 +2349,7 @@ enum nl80211_attrs {
 #define NL80211_ATTR_SCAN_GENERATION NL80211_ATTR_GENERATION
 #defineNL80211_ATTR_MESH_PARAMS NL80211_ATTR_MESH_CONFIG
 #define NL80211_ATTR_IFACE_SOCKET_OWNER NL80211_ATTR_SOCKET_OWNER
+#define NL80211_ATTR_SAE_DATA NL80211_ATTR_AUTH_DATA
 
 /*
  * Allow user space programs to use #ifdef on new attributes by defining them
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index c8d3a9b..32fd295 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4483,20 +4483,20 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data 
*sdata,
return -EOPNOTSUPP;
}
 
-   auth_data = kzalloc(sizeof(*auth_data) + req->sae_data_len +
+   auth_data = kzalloc(sizeof(*auth_data) + req->auth_data_len +
req->ie_len, GFP_KERNEL);
if (!auth_data)
return -ENOMEM;
 
auth_data->bss = req->bss;
 
-   if (req->sae_data_len >= 4) {
-   __le16 *pos = (__le16 *) req->sae_data;
+   if (req->auth_data_len >= 4) {
+   __le16 *pos = (__le16 *) req->auth_data;
auth_data->sae_trans = le16_to_cpu(pos[0]);
auth_data->sae_status = le16_to_cpu(pos[1]);
-   memcpy(auth_data->data, req->sae_data + 4,
-  req->sae_data_len - 4);
-   auth_data->data_len += req->sae_data_len - 4;
+   memcpy(auth_data->data, req->auth_data + 4,
+  req->auth_data_len - 4);
+   auth_data->data_len += req->auth_data_len - 4;
}
 
if (req->ie && req->ie_len) {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 21e3188..fb2fcd5 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -345,7 +345,7 @@ int cfg80211_mlme_auth(struct cfg80211_registered_device 
*rdev,
   const u8 *ssid, int ssid_len,
   const u8

[PATCH 0/8] cfg80211/mac80211: Fast Initial Link Setup (IEEE 802.11ai)

2016-10-25 Thread Jouni Malinen 
This series adds support for using mac80211-based drivers with Fast
Initial Link Setup as defined in IEEE 802.11ai (to be published early
next year; no more technical changes are expected at this point). The
fils branch in git://w1.fi/hostap.git includes matching commits for
hostapd and wpa_supplicant to use this functionality and initial set of
mac80211_hwsim test cases for the functionality. Actually, most of the
commits are already in the master branch, i.e., only the changes
depending on the nl80211.h changes from this kernel patchset are waiting
in the fils branch.

This series covers only the FILS authentication/association
functionality from IEEE 802.11ai, i.e., the other changes like scanning
optimizations are not included.

Jouni Malinen (8):
  cfg80211: Rename SAE_DATA to more generic AUTH_DATA
  mac80211: Allow AUTH_DATA to be used for FILS
  cfg80211: Add feature flag for Fast Initial Link Setup (FILS)
  cfg80211: Add Fast Initial Link Setup (FILS) auth algs
  cfg80211: Add KEK/nonces for FILS association frames
  mac80211: Add FILS auth alg mapping
  mac80211: FILS AEAD protection for station mode association frames
  mac80211: Claim Fast Initial Link Setup (FILS) support

 include/linux/ieee80211.h|   6 +
 include/net/cfg80211.h   |  19 ++-
 include/uapi/linux/nl80211.h |  26 +++-
 net/mac80211/Makefile|   1 +
 net/mac80211/aes_cmac.c  |   8 +-
 net/mac80211/aes_cmac.h  |   4 +
 net/mac80211/fils_aead.c | 354 +++
 net/mac80211/fils_aead.h |  19 +++
 net/mac80211/ieee80211_i.h   |   5 +
 net/mac80211/main.c  |   1 +
 net/mac80211/mlme.c  |  64 ++--
 net/wireless/core.h  |   2 +-
 net/wireless/mlme.c  |   6 +-
 net/wireless/nl80211.c   |  56 +--
 14 files changed, 535 insertions(+), 36 deletions(-)
 create mode 100644 net/mac80211/fils_aead.c
 create mode 100644 net/mac80211/fils_aead.h

-- 
1.9.1

[PATCH] rtl8xxxu: Add D-Link DWA-131 rev E1

2016-10-25 Thread Barry Day 
This is a rtl8192eu dongle and has been tested

Signed-off-by: Barry Day 
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 04141e5..d426836 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -6009,7 +6009,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
untested = 0;
break;
case 0x2001:
-   if (id->idProduct == 0x3308)
+   if (id->idProduct == 0x3308 || id->idProduct == 0x3319)
untested = 0;
break;
case 0x2357:
@@ -6188,6 +6188,8 @@ static struct usb_device_id dev_table[] = {
.driver_info = (unsigned long)_fops},
 {USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, 0x818b, 0xff, 0xff, 
0xff),
.driver_info = (unsigned long)_fops},
+{USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x3319, 0xff, 0xff, 0xff),
+   .driver_info = (unsigned long)_fops},
 /* Tested by Myckel Habets */
 {USB_DEVICE_AND_INTERFACE_INFO(0x2357, 0x0109, 0xff, 0xff, 0xff),
.driver_info = (unsigned long)_fops},
-- 
2.9.2

[PATCH v2] cw1200: fix bogus maybe-uninitialized warning

2016-10-25 Thread Arnd Bergmann 
On x86, the cw1200 driver produces a rather silly warning about the
possible use of the 'ret' variable without an initialization
presumably after being confused by the architecture specific definition
of WARN_ON:

drivers/net/wireless/st/cw1200/wsm.c: In function ‘wsm_handle_rx’:
drivers/net/wireless/st/cw1200/wsm.c:1457:9: error: ‘ret’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]

We have already checked that 'count' is larger than 0 here, so
we know that 'ret' is initialized. Changing the 'for' loop
into do/while also makes this clear to the compiler.

Suggested-by: David Laight 
Signed-off-by: Arnd Bergmann 
---
 drivers/net/wireless/st/cw1200/wsm.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

v2: rewrite based on David Laight's suggestion, the first version
was completely wrong.

diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
b/drivers/net/wireless/st/cw1200/wsm.c
index 680d60eabc75..ed93bf3474ec 100644
--- a/drivers/net/wireless/st/cw1200/wsm.c
+++ b/drivers/net/wireless/st/cw1200/wsm.c
@@ -379,7 +379,6 @@ static int wsm_multi_tx_confirm(struct cw1200_common *priv,
 {
int ret;
int count;
-   int i;
 
count = WSM_GET32(buf);
if (WARN_ON(count <= 0))
@@ -395,11 +394,10 @@ static int wsm_multi_tx_confirm(struct cw1200_common 
*priv,
}
 
cw1200_debug_txed_multi(priv, count);
-   for (i = 0; i < count; ++i) {
+   do {
ret = wsm_tx_confirm(priv, buf, link_id);
-   if (ret)
-   return ret;
-   }
+   } while (!ret && --count);
+
return ret;
 
 underflow:
-- 
2.9.0

Re: [PATCH] cw1200: fix bogus maybe-uninitialized warning

2016-10-25 Thread Arnd Bergmann 
On Tuesday, October 25, 2016 1:24:55 PM CEST David Laight wrote:
> > diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
> > b/drivers/net/wireless/st/cw1200/wsm.c
> > index 680d60eabc75..094e6637ade2 100644
> > --- a/drivers/net/wireless/st/cw1200/wsm.c
> > +++ b/drivers/net/wireless/st/cw1200/wsm.c
> > @@ -385,14 +385,13 @@ static int wsm_multi_tx_confirm(struct cw1200_common 
> > *priv,
> >   if (WARN_ON(count <= 0))
> >   return -EINVAL;
> > 
> > - if (count > 1) {
> > - /* We already released one buffer, now for the rest */
> > - ret = wsm_release_tx_buffer(priv, count - 1);
> > - if (ret < 0)
> > - return ret;
> > - else if (ret > 0)
> > - cw1200_bh_wakeup(priv);
> > - }
> > + /* We already released one buffer, now for the rest */
> > + ret = wsm_release_tx_buffer(priv, count - 1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ret > 0)
> > + cw1200_bh_wakeup(priv);
> 
> That doesn't look equivalent to me (when count == 1).

Ah, that's what I missed, thanks for pointing that out!

> > 
> >   cw1200_debug_txed_multi(priv, count);
> >   for (i = 0; i < count; ++i) {
> 
> Convert this loop into a do ... while so the body executes at least once.

Good idea. Version 2 coming now.

Arnd

Re: [PATCH] cw1200: fix bogus maybe-uninitialized warning

2016-10-25 Thread Solomon Peachy 
On Tue, Oct 25, 2016 at 01:24:55PM +, David Laight wrote:
> > -   if (count > 1) {
> > -   /* We already released one buffer, now for the rest */
> > -   ret = wsm_release_tx_buffer(priv, count - 1);
> > -   if (ret < 0)
> > -   return ret;
> > -   else if (ret > 0)
> > -   cw1200_bh_wakeup(priv);
> > -   }
> > +   /* We already released one buffer, now for the rest */
> > +   ret = wsm_release_tx_buffer(priv, count - 1);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   if (ret > 0)
> > +   cw1200_bh_wakeup(priv);
> 
> That doesn't look equivalent to me (when count == 1).

I concur, this patch should not be applied in its current form.

 - Solomon
-- 
Solomon Peachy pizza at shaftnet dot org
Delray Beach, FL  ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.


signature.asc
Description: PGP signature

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-25 Thread Dmitry Torokhov 
On Tue, Oct 25, 2016 at 03:12:44PM +, Amitkumar Karwar wrote:
> Hi Brian,
> 
> Thanks for review.
> 
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: Tuesday, October 25, 2016 6:22 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> > unregister
> > 
> > Hi Amit,
> > 
> > On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > > To: Amitkumar Karwar
> > > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry
> > > > Torokhov
> > > > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during
> > > > device unregister
> > > >
> > > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > > From: Xinming Hu 
> > > > > >
> > > > > > card->adapter gets initialized during device registration.
> > > > > > As it's not cleared, we may end up accessing invalid memory in
> > > > > > some corner cases. This patch fixes the problem.
> > > > > >
> > > > > > Signed-off-by: Xinming Hu 
> > > > > > Signed-off-by: Amitkumar Karwar 
> > > > > > ---
> > > > > > v4: Same as v1, v2, v3
> > > > > > ---
> > > > > >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > > > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > index f1eeb73..ba9e068 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > > mwifiex_adapter *adapter)
> > > > > > pci_disable_msi(pdev);
> > > > > >}
> > > > > > }
> > > > > > +   card->adapter = NULL;
> > > > > >  }
> > > > > >
> > > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > > rings, etc.
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > index 8718950..4cad1c2 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > > mwifiex_adapter
> > > > *adapter)
> > > > > > struct sdio_mmc_card *card = adapter->card;
> > > > > >
> > > > > > if (adapter->card) {
> > > > > > +   card->adapter = NULL;
> > > > > > sdio_claim_host(card->func);
> > > > > > sdio_disable_func(card->func);
> > > > > > sdio_release_host(card->func);
> > > > >
> > > > > As discussed on v1, I had qualms about the raciness between
> > > > > reads/writes of card->adapter, but I believe we:
> > > > > (a) can't have any command activity while writing the ->adapter
> > > > > field (either we're just init'ing the device, or we've disabled
> > > > > interrupts and are tearing it down) and
> > > > > (b) can't have a race between suspend()/resume() and
> > > > > unregister_dev(), since unregister_dev() is called from device
> > > > > remove() (which should not be concurrent with suspend()).
> > > > >
> > > > > Also, I thought you had the same problem in usb.c, but in fact,
> > > > > you fixed that ages ago here:
> > > > >
> > > > > 353d2a69ea26 mwifiex: fix issues in driver unload path for USB
> > > > > chipsets
> > > > >
> > > > > Would be nice if fixes were bettery synchronized across the three
> > > > > interface drivers you support. We seem to be discovering
> > > > > unnecessary divergence on a few points recently.
> > > > >
> > > > > At any rate:
> > > > >
> > > > > Reviewed-by: Brian Norris 
> > > > > Tested-by: Brian Norris 
> > > >
> > > > Dmitry helped me re-realize my original qualms:
> > > >
> > > > mwifiex_unregister_dev() is called in the failure path for your
> > > > async FW request, and so it may race with suspend(). So I retract my
> > Reviewed-by.
> > > > Sorry.
> > >
> > > Thanks for your comments.
> > >
> > > Actually description for this patch was ambiguous and incorrect. Sorry
> > > for that. This patch doesn't fix any race. In fact, we don't have a
> > > race between init and remove threads due to semaphore usage as per
> > > design. This patch just adds missing "card->adapter=NULL" so that when
> > > teardown/remove thread starts after init failure, it won't try freeing
> > > already freed

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-25 Thread Brian Norris 
Hi Amit,

On Tue, Oct 25, 2016 at 03:12:44PM +, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: Tuesday, October 25, 2016 6:22 AM
> > To: Amitkumar Karwar
> > 
> > On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > > To: Amitkumar Karwar
> > > >
> > > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > index f1eeb73..ba9e068 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > > mwifiex_adapter *adapter)
> > > > > > pci_disable_msi(pdev);
> > > > > >}
> > > > > > }
> > > > > > +   card->adapter = NULL;
> > > > > >  }
> > > > > >
> > > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > > rings, etc.
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > index 8718950..4cad1c2 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > > mwifiex_adapter
> > > > *adapter)
> > > > > > struct sdio_mmc_card *card = adapter->card;
> > > > > >
> > > > > > if (adapter->card) {
> > > > > > +   card->adapter = NULL;
> > > > > > sdio_claim_host(card->func);
> > > > > > sdio_disable_func(card->func);
> > > > > > sdio_release_host(card->func);
> > > > >

[...]

> > > Thanks for your comments.
> > >
> > > Actually description for this patch was ambiguous and incorrect. Sorry
> > > for that. This patch doesn't fix any race. In fact, we don't have a
> > > race between init and remove threads due to semaphore usage as per
> > > design. This patch just adds missing "card->adapter=NULL" so that when
> > > teardown/remove thread starts after init failure, it won't try freeing
> > > already freed things.
> > 
> > So to be clear, you'r talking about mwifiex_fw_dpc(), which in the error
> > path
> > has:
> > 
> >if (adapter->if_ops.unregister_dev)
> > adapter->if_ops.unregister_dev(adapter); <--- POINT A:
> > This is where you want to set ->adapter = NULL ...
> > if (init_failed)
> > mwifiex_free_adapter(adapter);
> > up(sem); <--- POINT B: This is where you release the semaphore,
> > which is supposed to guarantee that remove() isn't happening
> > return;
> > }
> > 
> > But you *do* have a race between the above code and the remove code in
> > some cases. Particularly, see this:
> > 
> > static void mwifiex_pcie_remove(struct pci_dev *pdev) {
> > struct pcie_service_card *card;
> > struct mwifiex_adapter *adapter;
> > struct mwifiex_private *priv;
> > 
> > card = pci_get_drvdata(pdev);
> > if (!card)
> > return;
> > 
> > adapter = card->adapter; <--- POINT C: This can execute at the
> > same time as unregister_dev()
> > if (!adapter || !adapter->priv_num)
> > return;
> > 
> > if (user_rmmod && !adapter->mfg_mode) { #ifdef CONFIG_PM_SLEEP
> > if (adapter->is_suspended)
> > mwifiex_pcie_resume(>dev); #endif
> > 
> > mwifiex_deauthenticate_all(adapter);
> > 
> > priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> > 
> > mwifiex_disable_auto_ds(priv);
> > 
> > mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
> > }
> > 
> > mwifiex_remove_card(card->adapter, _remove_card_sem); <---
> > POINT D: You only grab the semaphore here }
> > 
> > So IIUC, you have a race **even here, where you claim the semaphore
> > should protect you** such that the adapter might be freed, but you still
> > can access it anywhere between C and D. i.e., you can see this:
> > 
> > Thread 1  Thread 2
> >   (1) POINT C (retrieve adapter !=
> > NULL)
> > (2) POINT A (set adapter NULL)
> > (3) POINT B (adapter has been freed)
> >   (3) Keep accessing freed
> > adapter structure
> >   (4) POINT D - acquire semaphore,
> > but we're too late
> > 
> > Step 3 is an error, and AFAICT, that's exactly what you're trying to
> > solve in this patch. It essentially comes down to the same fact: you're
> > getting a reference to the adapter structure

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

2016-10-25 Thread Dmitry Torokhov 
On Tue, Oct 25, 2016 at 04:11:14PM +, Amitkumar Karwar wrote:
> Hi Dmitry,
> 
> > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> > Sent: Tuesday, October 25, 2016 5:28 AM
> > To: Brian Norris
> > Cc: Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> > Sarmukadam
> > Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> > in shutdown_drv
> > 
> > On Mon, Oct 24, 2016 at 12:19:15PM -0700, Brian Norris wrote:
> > > On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote:
> > > > This variable is guarded by spinlock at all other places. This patch
> > > > takes care of missing spinlock usage in mwifiex_shutdown_drv().
> > > >
> > > > Signed-off-by: Amitkumar Karwar 
> > > > ---
> > > >  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > > > b/drivers/net/wireless/marvell/mwifiex/init.c
> > > > index 82839d9..8e5e424 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > > > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter
> > > > *adapter)
> > > >
> > > > adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
> > > > /* wait for mwifiex_process to complete */
> > > > +   spin_lock_irqsave(>main_proc_lock, flags);
> > > > if (adapter->mwifiex_processing) {
> > >
> > > I'm not quite sure why we have this check in the first place; if the
> > > main process is still running, then don't we have bigger problems here
> > > anyway? But this is definitely the "right" way to check this flag, so:
> > 
> > No, this is definitely not right way to check it. What exactly does this
> > spinlock protect? It seems that the intent is to make sure we do not
> > call mwifiex_shutdown_drv() while mwifiex_main_process() is executing.
> > It even says above that we are "waiting" for it (but we do not, we may
> > bail out or we may not, depends on luck).
> > 
> > To implement this properly you not only need to take a lock and check
> > the flag, but keep the lock until mwifiex_shutdown_drv() is done, or use
> > some other way to let mwifiex_main_process() know it should not access
> > the adapter while mwifiex_shutdown_drv() is running.
> > 
> > NACK.
> > 
> 
> As I explained in other email, here is the sequence.
> 1) We find mwifiex_processing is true in mwifiex_shitdown_drv(). 
> "-EINPROGRESS" is returned by the function and hw_status state is changed to 
> MWIFIEX_HW_STATUS_CLOSING.
> 2) We wait until main_process is completed.
> 
> if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
> wait_event_interruptible(adapter->init_wait_q,
>  adapter->init_wait_q_woken);
> 
> 3) We have following check at the end of main_process()
> 
> exit_main_proc:
> if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
> mwifiex_shutdown_drv(adapter);
> 
> 4) Here at the end of mwifiex_shutdown_drv(), we are setting 
> "adapter->init_wait_q_woken" and waking up the thread in point (2)

1. We do not find mwifiex_processing to be true
2. We proceed to try and shut down the driver
3. Interrupt is raised in the meantime, kicking work item
4. mwifiex_main_process() sees that adapter->hw_status is
MWIFIEX_HW_STATUS_CLOSING and jumps to exit_main_proc
5. mwifiex_main_process() calls into mwifiex_shutdown_drv() that is now
racing with another copy of the same.

It seems to me that mwifiex_main_process() is [almost] always used from
a workqueue. Can you instead of sprinkling spinlocks ensure that
mwifiex_shutdown_drv():

1. Inhibits scheduling of mwifiex_main_process()
2. Does cancel_work_sync(...) to ensure that mwifiex_main_process() does
not run
3. Continues shutting down the driver.

Alternatively, do these have to be spinlocks? Can you make them mutexes
and take them for the duration of mwifiex_main_process() and
mwifiex_shutdown_drv() and others, as needed?

Thanks.

-- 
Dmitry

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

2016-10-25 Thread Amitkumar Karwar 
Hi Brian,

> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Tuesday, October 25, 2016 1:54 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> dmitry.torok...@gmail.com; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu 
> >
> > This patch ensures to wait for firmware dump completion in
> > mwifiex_remove_card().
> >
> > For sdio interface, reset_trigger variable is used to identify if
> > mwifiex_sdio_remove() is called by sdio_work during reset or the call
> > is from sdio subsystem.
> >
> > This patch fixes a kernel crash observed during reboot when firmware
> > dump operation is in process.
> >
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 986bf07..4512e86 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> *pdev)
> > if (!adapter || !adapter->priv_num)
> > return;
> >
> > +   cancel_work_sync(_work);
> 
> Is there a good reason you have to cancel the work? What if you were
> just to finish it (i.e., flush_work())?

I assume if work is running, cancel_work_sync() will wait until completion. 
Otherwise it will cancel queued work. Firmware dump takes 20-30 seconds to 
complete. I think, it's ok to cancel it if work is just queued and not running 
yet. Reboot taking significant time due to our wait in remove handler may not 
be a good experience from end user point of view.

If you prefer flush_work(), I can use the same.

> 
> In any case, I think this is fine, so for the PCIe bits:
> 
> Reviewed-by: Brian Norris 
> 
> > +
> > if (user_rmmod && !adapter->mfg_mode) {  #ifdef CONFIG_PM_SLEEP
> > if (adapter->is_suspended)
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 4cad1c2..f974260 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -46,6 +46,15 @@
> >   */
> >  static u8 user_rmmod;
> >
> > +/* reset_trigger variable is used to identify if
> > +mwifiex_sdio_remove()
> > + * is called by sdio_work during reset or the call is from sdio
> subsystem.
> > + * We will cancel sdio_work only if the call is from sdio subsystem.
> > + */
> > +static u8 reset_triggered;
> > +
> > +static void mwifiex_sdio_work(struct work_struct *work); static
> > +DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> > +
> >  static struct mwifiex_if_ops sdio_ops;  static unsigned long
> > iface_work_flags;
> >
> > @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
> > if (!adapter || !adapter->priv_num)
> > return;
> >
> > +   if (!reset_triggered)
> > +   cancel_work_sync(_work);
> > +
> > mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
> >
> > if (user_rmmod && !adapter->mfg_mode) { @@ -2290,7 +2302,9 @@
> static
> > void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
> >  * discovered and initializes them from scratch.
> >  */
> >
> > +   reset_triggered = 1;
> > mwifiex_sdio_remove(func);
> > +   reset_triggered = 0;
> 
> Boy that's ugly! Couldn't you just create something like
> __mwifiex_sdio_remove(), which does everything except the
> cancel_work_sync()? Then you'd do this for the .remove() callback:
> 
>   cancel_work_sync(_work);
>   __mwifiex_sdio_remove(func);
> 
> and for mwifiex_recreate_adapter() you'd just call
> __mwifiex_sdio_remove()? The static variable that simply serves as a
> (non-reentrant) function call parameter seems like a really poor
> alternative to this simple refactoring.

Thanks a lot for the suggestion.
I will use this approach in updated version.

Regards,
Amitkumar

RE: [PATCH 3/5] mwifiex: do not free firmware dump memory in shutdown_drv

2016-10-25 Thread Amitkumar Karwar 
Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, October 25, 2016 5:47 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> briannor...@google.com; Xinming Hu
> Subject: Re: [PATCH 3/5] mwifiex: do not free firmware dump memory in
> shutdown_drv
> 
> On Mon, Oct 24, 2016 at 07:51:30PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu 
> >
> > mwifiex_upload_device_dump() already takes care of freeing firmware
> > dump memory. Doing the same thing in mwifiex_shutdown_drv() is
> redundant.
> >
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/init.c | 19 ---
> >  1 file changed, 19 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > b/drivers/net/wireless/marvell/mwifiex/init.c
> > index 8e5e424..365efb8 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > @@ -407,8 +407,6 @@ static void mwifiex_free_lock_list(struct
> > mwifiex_adapter *adapter)  static void  mwifiex_adapter_cleanup(struct
> > mwifiex_adapter *adapter)  {
> > -   int idx;
> > -
> > if (!adapter) {
> > pr_err("%s: adapter is NULL\n", __func__);
> > return;
> > @@ -426,23 +424,6 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter
> *adapter)
> > mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
> > mwifiex_free_cmd_buffer(adapter);
> >
> > -   for (idx = 0; idx < adapter->num_mem_types; idx++) {
> > -   struct memory_type_mapping *entry =
> > -   >mem_type_mapping_tbl[idx];
> > -
> > -   if (entry->mem_ptr) {
> > -   vfree(entry->mem_ptr);
> > -   entry->mem_ptr = NULL;
> > -   }
> > -   entry->mem_size = 0;
> > -   }
> > -
> > -   if (adapter->drv_info_dump) {
> > -   vfree(adapter->drv_info_dump);
> > -   adapter->drv_info_dump = NULL;
> > -   adapter->drv_info_size = 0;
> > -   }
> 
> Why do you even keep the pointer to dump memory in the adapter
> structure? You allocate it in mwifiex_drv_info_dump() and immediately
> use it in mwifiex_upload_device_dump(). Why not simply pass the pointer
> between the functions?
> 

Thanks. This makes sense. I will pass the pointer and get rid of adapter 
variable in updated version.

Regards,
Amitkumar

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

2016-10-25 Thread Amitkumar Karwar 
Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, October 25, 2016 5:44 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> briannor...@google.com; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu 
> >
> > This patch ensures to wait for firmware dump completion in
> > mwifiex_remove_card().
> >
> > For sdio interface, reset_trigger variable is used to identify if
> > mwifiex_sdio_remove() is called by sdio_work during reset or the call
> > is from sdio subsystem.
> >
> > This patch fixes a kernel crash observed during reboot when firmware
> > dump operation is in process.
> >
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 986bf07..4512e86 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> *pdev)
> > if (!adapter || !adapter->priv_num)
> > return;
> >
> > +   cancel_work_sync(_work);
> > +
> > if (user_rmmod && !adapter->mfg_mode) {  #ifdef CONFIG_PM_SLEEP
> > if (adapter->is_suspended)
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 4cad1c2..f974260 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -46,6 +46,15 @@
> >   */
> >  static u8 user_rmmod;
> >
> > +/* reset_trigger variable is used to identify if
> > +mwifiex_sdio_remove()
> > + * is called by sdio_work during reset or the call is from sdio
> subsystem.
> > + * We will cancel sdio_work only if the call is from sdio subsystem.
> > + */
> > +static u8 reset_triggered;
> 
> It would be really great if the driver supported multiple devices. IOW
> please avoid module-globals.

You are right. As Brian pointed out in other email, I will refactor the code 
and get rid of global variable.

> 
> > +
> > +static void mwifiex_sdio_work(struct work_struct *work); static
> > +DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> > +
> >  static struct mwifiex_if_ops sdio_ops;  static unsigned long
> > iface_work_flags;
> >
> > @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
> > if (!adapter || !adapter->priv_num)
> > return;
> >
> > +   if (!reset_triggered)
> > +   cancel_work_sync(_work);
> > +
> > mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
> >
> > if (user_rmmod && !adapter->mfg_mode) { @@ -2290,7 +2302,9 @@
> static
> > void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
> >  * discovered and initializes them from scratch.
> >  */
> >
> > +   reset_triggered = 1;
> > mwifiex_sdio_remove(func);
> > +   reset_triggered = 0;
> 
> What exactly happens if I trigger mwifiex_sdio_remove() from sysfs at
> the same time this code is running?

Yes. There would be a race. It will be avoided with Brian's code refactoring 
approach.

Regards,
Amitkumar

RE: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

2016-10-25 Thread Amitkumar Karwar 
Hi Brian/Dmitry,

> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Tuesday, October 25, 2016 5:26 AM
> To: Dmitry Torokhov
> Cc: Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> Sarmukadam; briannor...@google.com
> Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> in shutdown_drv
> 
> Hi,
> 
> On Mon, Oct 24, 2016 at 04:47:20PM -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote:
> > > This variable is guarded by spinlock at all other places. This patch
> > > takes care of missing spinlock usage in mwifiex_shutdown_drv().
> > >
> > > Signed-off-by: Amitkumar Karwar 
> > > ---
> > >  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > > b/drivers/net/wireless/marvell/mwifiex/init.c
> > > index 82839d9..8e5e424 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter
> > > *adapter)
> > >
> > >   adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
> > >   /* wait for mwifiex_process to complete */
> > > + spin_lock_irqsave(>main_proc_lock, flags);
> > >   if (adapter->mwifiex_processing) {
> > > + spin_unlock_irqrestore(>main_proc_lock, flags);
> > >   mwifiex_dbg(adapter, WARN,
> > >   "main process is still running\n");
> > >   return ret;
> > >   }
> > > + spin_unlock_irqrestore(>main_proc_lock, flags);
> >
> > What happens if adapter->mwifiex_processing will become true here?
> 
> That's why I commented:
> 
> "I'm not quite sure why we have this check in the first place; if the
> main process is still running, then don't we have bigger problems here
> anyway?"

If mwifiex_processing is true here, it means main_process is still running. We 
have set "adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING" in 
mwifiex_shutdown_drv().
Now we will wait until main_process() gets completed.

-
if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
 wait_event_interruptible(adapter->init_wait_q,
  adapter->init_wait_q_woken);
--

We have following logic at the end of main_process()
---
exit_main_proc:
if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
mwifiex_shutdown_drv(adapter);


Regards,
Amitkumar Karwar

RE: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

2016-10-25 Thread Amitkumar Karwar 
Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, October 25, 2016 5:28 AM
> To: Brian Norris
> Cc: Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> Sarmukadam
> Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> in shutdown_drv
> 
> On Mon, Oct 24, 2016 at 12:19:15PM -0700, Brian Norris wrote:
> > On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote:
> > > This variable is guarded by spinlock at all other places. This patch
> > > takes care of missing spinlock usage in mwifiex_shutdown_drv().
> > >
> > > Signed-off-by: Amitkumar Karwar 
> > > ---
> > >  drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > > b/drivers/net/wireless/marvell/mwifiex/init.c
> > > index 82839d9..8e5e424 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter
> > > *adapter)
> > >
> > >   adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
> > >   /* wait for mwifiex_process to complete */
> > > + spin_lock_irqsave(>main_proc_lock, flags);
> > >   if (adapter->mwifiex_processing) {
> >
> > I'm not quite sure why we have this check in the first place; if the
> > main process is still running, then don't we have bigger problems here
> > anyway? But this is definitely the "right" way to check this flag, so:
> 
> No, this is definitely not right way to check it. What exactly does this
> spinlock protect? It seems that the intent is to make sure we do not
> call mwifiex_shutdown_drv() while mwifiex_main_process() is executing.
> It even says above that we are "waiting" for it (but we do not, we may
> bail out or we may not, depends on luck).
> 
> To implement this properly you not only need to take a lock and check
> the flag, but keep the lock until mwifiex_shutdown_drv() is done, or use
> some other way to let mwifiex_main_process() know it should not access
> the adapter while mwifiex_shutdown_drv() is running.
> 
> NACK.
> 

As I explained in other email, here is the sequence.
1) We find mwifiex_processing is true in mwifiex_shitdown_drv(). "-EINPROGRESS" 
is returned by the function and hw_status state is changed to 
MWIFIEX_HW_STATUS_CLOSING.
2) We wait until main_process is completed.

if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
wait_event_interruptible(adapter->init_wait_q,
 adapter->init_wait_q_woken);

3) We have following check at the end of main_process()

exit_main_proc:
if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
mwifiex_shutdown_drv(adapter);

4) Here at the end of mwifiex_shutdown_drv(), we are setting 
"adapter->init_wait_q_woken" and waking up the thread in point (2)

Regards,
Amitkumar

RE: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-25 Thread Amitkumar Karwar 
Hi Brian,

Thanks for review.

> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Tuesday, October 25, 2016 6:22 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> unregister
> 
> Hi Amit,
> 
> On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > To: Amitkumar Karwar
> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry
> > > Torokhov
> > > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during
> > > device unregister
> > >
> > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > From: Xinming Hu 
> > > > >
> > > > > card->adapter gets initialized during device registration.
> > > > > As it's not cleared, we may end up accessing invalid memory in
> > > > > some corner cases. This patch fixes the problem.
> > > > >
> > > > > Signed-off-by: Xinming Hu 
> > > > > Signed-off-by: Amitkumar Karwar 
> > > > > ---
> > > > > v4: Same as v1, v2, v3
> > > > > ---
> > > > >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > > > >  2 files changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > index f1eeb73..ba9e068 100644
> > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > mwifiex_adapter *adapter)
> > > > >   pci_disable_msi(pdev);
> > > > >  }
> > > > >   }
> > > > > + card->adapter = NULL;
> > > > >  }
> > > > >
> > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > rings, etc.
> > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > index 8718950..4cad1c2 100644
> > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > mwifiex_adapter
> > > *adapter)
> > > > >   struct sdio_mmc_card *card = adapter->card;
> > > > >
> > > > >   if (adapter->card) {
> > > > > + card->adapter = NULL;
> > > > >   sdio_claim_host(card->func);
> > > > >   sdio_disable_func(card->func);
> > > > >   sdio_release_host(card->func);
> > > >
> > > > As discussed on v1, I had qualms about the raciness between
> > > > reads/writes of card->adapter, but I believe we:
> > > > (a) can't have any command activity while writing the ->adapter
> > > > field (either we're just init'ing the device, or we've disabled
> > > > interrupts and are tearing it down) and
> > > > (b) can't have a race between suspend()/resume() and
> > > > unregister_dev(), since unregister_dev() is called from device
> > > > remove() (which should not be concurrent with suspend()).
> > > >
> > > > Also, I thought you had the same problem in usb.c, but in fact,
> > > > you fixed that ages ago here:
> > > >
> > > > 353d2a69ea26 mwifiex: fix issues in driver unload path for USB
> > > > chipsets
> > > >
> > > > Would be nice if fixes were bettery synchronized across the three
> > > > interface drivers you support. We seem to be discovering
> > > > unnecessary divergence on a few points recently.
> > > >
> > > > At any rate:
> > > >
> > > > Reviewed-by: Brian Norris 
> > > > Tested-by: Brian Norris 
> > >
> > > Dmitry helped me re-realize my original qualms:
> > >
> > > mwifiex_unregister_dev() is called in the failure path for your
> > > async FW request, and so it may race with suspend(). So I retract my
> Reviewed-by.
> > > Sorry.
> >
> > Thanks for your comments.
> >
> > Actually description for this patch was ambiguous and incorrect. Sorry
> > for that. This patch doesn't fix any race. In fact, we don't have a
> > race between init and remove threads due to semaphore usage as per
> > design. This patch just adds missing "card->adapter=NULL" so that when
> > teardown/remove thread starts after init failure, it won't try freeing
> > already freed things.
> 
> So to be clear, you'r talking about mwifiex_fw_dpc(), which in the error
> path
> has:
> 
>if (adapter->if_ops.unregister_dev)
> adapter->if_ops.unregister_dev(adapter); <--- POINT A:
> This is where you want to set ->adapter = NULL

RE: [PATCH] cw1200: fix bogus maybe-uninitialized warning

2016-10-25 Thread David Laight 
From: Of Arnd Bergmann
> Sent: 24 October 2016 16:42
 
> On x86, the cw1200 driver produces a rather silly warning about the
> possible use of the 'ret' variable without an initialization
> presumably after being confused by the architecture specific definition
> of WARN_ON:
> 
> drivers/net/wireless/st/cw1200/wsm.c: In function wsm_handle_rx:
> drivers/net/wireless/st/cw1200/wsm.c:1457:9: error: ret may be used 
> uninitialized in this function [-
> Werror=maybe-uninitialized]
> 
> As the driver just checks the same variable twice here, we can simplify
> it by removing the second condition, which makes it more readable and
> avoids the warning.
> 
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/net/wireless/st/cw1200/wsm.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
> b/drivers/net/wireless/st/cw1200/wsm.c
> index 680d60eabc75..094e6637ade2 100644
> --- a/drivers/net/wireless/st/cw1200/wsm.c
> +++ b/drivers/net/wireless/st/cw1200/wsm.c
> @@ -385,14 +385,13 @@ static int wsm_multi_tx_confirm(struct cw1200_common 
> *priv,
>   if (WARN_ON(count <= 0))
>   return -EINVAL;
> 
> - if (count > 1) {
> - /* We already released one buffer, now for the rest */
> - ret = wsm_release_tx_buffer(priv, count - 1);
> - if (ret < 0)
> - return ret;
> - else if (ret > 0)
> - cw1200_bh_wakeup(priv);
> - }
> + /* We already released one buffer, now for the rest */
> + ret = wsm_release_tx_buffer(priv, count - 1);
> + if (ret < 0)
> + return ret;
> +
> + if (ret > 0)
> + cw1200_bh_wakeup(priv);

That doesn't look equivalent to me (when count == 1).

> 
>   cw1200_debug_txed_multi(priv, count);
>   for (i = 0; i < count; ++i) {

Convert this loop into a do ... while so the body executes at least once.

David

pull-request: iwlwifi-next 2016-10-25

2016-10-25 Thread Luca Coelho 
Hi Kalle,

Here's my first pull-request intended for 4.10.  It contains the usual
improvements and bugfixes, with the major thing being that we have
finally enabled dynamic queue allocation.  Some more details in the tag
description.

These patches were sent out for review on 2016-10-19, and pushed to a
pending branch.  Kbuildbot didn't find any issues.

Let me know if everything's fine (or not). :)

Luca.


The following changes since commit 15b95a15950238eff4d7f24be1716086eea67835:

  Merge ath-next from 
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git (2016-09-28 
16:37:33 +0300)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git 
tags/iwlwifi-next-for-kalle-2016-10-25

for you to fetch changes up to a048dcc71e701cb1771b2e0aca23181a45658023:

  iwlwifi: mvm: use dev_coredumpsg() (2016-10-19 12:46:37 +0300)


* Finalize and enable dynamic queue allocation;
* Use dev_coredumpmsg() to prevent locking the driver;
* Small fix to pass the AID to the FW;
* Use FW PS decisions with multi-queue;


Aviya Erenfeld (1):
  iwlwifi: mvm: use dev_coredumpsg()

Emmanuel Grumbach (1):
  iwlwifi: mvm: tell the firmware about the AID of the peer

Johannes Berg (1):
  iwlwifi: mvm: use firmware station PM notification for AP_LINK_PS

Liad Kaufman (5):
  iwlwifi: mvm: update txq metadata to current owner
  iwlwifi: mvm: fix reserved txq freeing
  iwlwifi: mvm: support MONITOR vif in DQA mode
  iwlwifi: mvm: fix dqa deferred frames marking
  iwlwifi: mvm: operate in dqa mode

Sara Sharon (1):
  iwlwifi: mvm: assign cab queue to the correct station

Sharon Dvir (1):
  iwlwifi: pcie: give a meaningful name to interrupt request

 drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h|   2 ++
 drivers/net/wireless/intel/iwlwifi/mvm/fw-api-rx.h  |  26 
 drivers/net/wireless/intel/iwlwifi/mvm/fw-api-sta.h |   9 ---
 drivers/net/wireless/intel/iwlwifi/mvm/fw-api.h |   1 +
 drivers/net/wireless/intel/iwlwifi/mvm/fw-dbg.c | 100 
+--
 drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c   |  24 +-
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c   |  86 
+---
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h|   6 ++---
 drivers/net/wireless/intel/iwlwifi/mvm/ops.c|   3 +++
 drivers/net/wireless/intel/iwlwifi/mvm/sta.c|  37 
++--
 drivers/net/wireless/intel/iwlwifi/mvm/sta.h|   1 +
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c |  29 
+-
 12 files changed, 249 insertions(+), 75 deletions(-)

signature.asc
Description: This is a digitally signed message part

pull-request: iwlwifi 2016-10-25

2016-10-25 Thread Luca Coelho 
Hi Kalle,

Here are 7 patches intended for 4.9.  They fix some small issues, as
described in the tag itself.  I sent them out for review on 2016-10-19, 
and pushed to a pending branch.  Kbuildbot didn't find any issues.

Let me know if everything's fine (or not). :)

Luca.


The following changes since commit 1ea2643961b0d1b8d0e4a11af5aa69b0f92d0533:

  ath6kl: add Dell OEM SDIO I/O for the Venue 8 Pro (2016-10-13 14:16:33 +0300)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes.git 
tags/iwlwifi-for-kalle-2016-10-25

for you to fetch changes up to 5a143db8c4a28dab6423cb6197e9f1389da375f2:

  iwlwifi: mvm: fix netdetect starting/stopping for unified images (2016-10-19 
09:54:45 +0300)


* some fixes for suspend/resume with unified FW images;
* a fix for a false-positive lockdep report;
* a fix for multi-queue that caused an unnecessary 1 second latency;
* a fix for an ACPI parsing bug that caused a misleading error message;


Haim Dreyfuss (1):
  iwlwifi: mvm: comply with fw_restart mod param on suspend

Johannes Berg (1):
  iwlwifi: pcie: mark command queue lock with separate lockdep class

Luca Coelho (4):
  iwlwifi: mvm: use ssize_t for len in iwl_debugfs_mem_read()
  iwlwifi: mvm: fix d3_test with unified D0/D3 images
  iwlwifi: pcie: fix SPLC structure parsing
  iwlwifi: mvm: fix netdetect starting/stopping for unified images

Sara Sharon (1):
  iwlwifi: mvm: wake the wait queue when the RX sync counter is zero

 drivers/net/wireless/intel/iwlwifi/mvm/d3.c   | 49 
+---
 drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c  |  4 ++--
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c |  3 +--
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h  |  1 +
 drivers/net/wireless/intel/iwlwifi/mvm/ops.c  |  1 +
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c |  3 ++-
 drivers/net/wireless/intel/iwlwifi/mvm/scan.c | 33 
+++--
 drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 79 
+++---
 drivers/net/wireless/intel/iwlwifi/pcie/tx.c  |  8 
 9 files changed, 128 insertions(+), 53 deletions(-)

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] staging: rtl8712: Free memory and return failure when kmalloc fails

2016-10-25 Thread Greg KH 
On Thu, Oct 20, 2016 at 12:29:33PM +0530, Souptick Joarder wrote:
> This patch is added to free memory and return failure when kmalloc fails

I'm sorry, but I can not parse that sentance.  Can you rephrase this a
bit better?  What exactly are you doing here?

> 
> Signed-off-by: Souptick joarder 
> ---
>  drivers/staging/rtl8712/os_intfs.c | 3 ++-
>  drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
>  drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
>  3 files changed, 10 insertions(+), 3 deletions(-)

Any reason why you didn't cc: the driverdevel mailing list?  I doubt
linux-wireless cares about staging drivers :(

> 
> diff --git a/drivers/staging/rtl8712/os_intfs.c 
> b/drivers/staging/rtl8712/os_intfs.c
> index cbe4de0..aab3141 100644
> --- a/drivers/staging/rtl8712/os_intfs.c
> +++ b/drivers/staging/rtl8712/os_intfs.c
> @@ -313,7 +313,8 @@ u8 r8712_init_drv_sw(struct _adapter *padapter)
>   return _FAIL;
>   if (r8712_init_mlme_priv(padapter) == _FAIL)
>   return _FAIL;
> - _r8712_init_xmit_priv(>xmitpriv, padapter);
> + if ((_r8712_init_xmit_priv(>xmitpriv, padapter)) != _SUCCESS)
> + return _FAIL;

You don't have to unwind anything that r8712_init_mlme_priv() did?

>   _r8712_init_recv_priv(>recvpriv, padapter);
>   memset((unsigned char *)>securitypriv, 0,
>  sizeof(struct security_priv));

thanks,

greg k-h

Re: Bayesian rate control

2016-10-25 Thread Johannes Berg 
On Mon, 2016-10-24 at 22:17 +0200, Björn Smedman wrote:
> 
> Are there any cards out there that support VHT and use s/w rate
> control (Minstrel)? Just in case I run out of search space. :P

Maybe something that's usable with brcmsmac? Not sure I'd recommend
buying Broadcom NICs though at this point, with them having been bought
out and all (and their upstream support is somewhat spotty, so you
might have a hard time finding the right NIC)

johannes