[PATCH 0/5] staging: wfx: late fixes

[PATCH 0/5] staging: wfx: late fixes

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

This series fixes some issues for 5.6. The two last patches fix misuse
of RCU. They are probably the most important of the series.

Jérôme Pouiller (5):
  staging: wfx: fix warning about freeing in-use mutex during device
    unregister
  staging: wfx: fix lines ending with a comma instead of a semicolon
  staging: wfx: make warning about pending frame less scary
  staging: wfx: fix RCU usage in wfx_join_finalize()
  staging: wfx: fix RCU usage between hif_join() and
    ieee80211_bss_get_ie()

 drivers/staging/wfx/hif_tx.c     | 15 ++++++++-------
 drivers/staging/wfx/hif_tx.h     |  2 +-
 drivers/staging/wfx/hif_tx_mib.h | 15 ++++++++++-----
 drivers/staging/wfx/sta.c        | 25 +++++++++++++++----------
 4 files changed, 34 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH 0/5] staging: wfx: late fixes

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

This series fixes some issues for 5.6. The two last patches fix misuse
of RCU. They are probably the most important of the series.

Jérôme Pouiller (5):
  staging: wfx: fix warning about freeing in-use mutex during device
    unregister
  staging: wfx: fix lines ending with a comma instead of a semicolon
  staging: wfx: make warning about pending frame less scary
  staging: wfx: fix RCU usage in wfx_join_finalize()
  staging: wfx: fix RCU usage between hif_join() and
    ieee80211_bss_get_ie()

 drivers/staging/wfx/hif_tx.c     | 15 ++++++++-------
 drivers/staging/wfx/hif_tx.h     |  2 +-
 drivers/staging/wfx/hif_tx_mib.h | 15 ++++++++++-----
 drivers/staging/wfx/sta.c        | 25 +++++++++++++++----------
 4 files changed, 34 insertions(+), 23 deletions(-)

-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/5] staging: wfx: fix warning about freeing in-use mutex during device unregister

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

After hif_shutdown(), communication with the chip is no more possible.
It the only request that never reply. Therefore, hif_cmd.lock is never
unlocked. hif_shutdown() unlock itself hif_cmd.lock to avoid a potential
warning during disposal of device. hif_cmd.key_renew_lock should also
been unlocked for the same reason.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 2428363371fa..7b732c531a74 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -140,6 +140,7 @@ int hif_shutdown(struct wfx_dev *wdev)
 	else
 		control_reg_write(wdev, 0);
 	mutex_unlock(&wdev->hif_cmd.lock);
+	mutex_unlock(&wdev->hif_cmd.key_renew_lock);
 	kfree(hif);
 	return ret;
 }
-- 
2.25.1

[PATCH 1/5] staging: wfx: fix warning about freeing in-use mutex during device unregister

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

After hif_shutdown(), communication with the chip is no more possible.
It the only request that never reply. Therefore, hif_cmd.lock is never
unlocked. hif_shutdown() unlock itself hif_cmd.lock to avoid a potential
warning during disposal of device. hif_cmd.key_renew_lock should also
been unlocked for the same reason.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 2428363371fa..7b732c531a74 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -140,6 +140,7 @@ int hif_shutdown(struct wfx_dev *wdev)
 	else
 		control_reg_write(wdev, 0);
 	mutex_unlock(&wdev->hif_cmd.lock);
+	mutex_unlock(&wdev->hif_cmd.key_renew_lock);
 	kfree(hif);
 	return ret;
 }
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/5] staging: wfx: fix lines ending with a comma instead of a semicolon

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Obviously introduced by mistake.

Fixes: 09779276f1ba ("staging: wfx: simplify hif_start() usage")
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 7b732c531a74..7a56e45bcdaa 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -428,9 +428,9 @@ int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
 	struct hif_msg *hif;
 	struct hif_req_start *body = wfx_alloc_hif(sizeof(*body), &hif);
 
-	body->dtim_period = conf->dtim_period,
-	body->short_preamble = conf->use_short_preamble,
-	body->channel_number = cpu_to_le16(channel->hw_value),
+	body->dtim_period = conf->dtim_period;
+	body->short_preamble = conf->use_short_preamble;
+	body->channel_number = cpu_to_le16(channel->hw_value);
 	body->beacon_interval = cpu_to_le32(conf->beacon_int);
 	body->basic_rate_set =
 		cpu_to_le32(wfx_rate_mask_to_hw(wvif->wdev, conf->basic_rates));
-- 
2.25.1

[PATCH 2/5] staging: wfx: fix lines ending with a comma instead of a semicolon

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Obviously introduced by mistake.

Fixes: 09779276f1ba ("staging: wfx: simplify hif_start() usage")
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 7b732c531a74..7a56e45bcdaa 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -428,9 +428,9 @@ int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
 	struct hif_msg *hif;
 	struct hif_req_start *body = wfx_alloc_hif(sizeof(*body), &hif);
 
-	body->dtim_period = conf->dtim_period,
-	body->short_preamble = conf->use_short_preamble,
-	body->channel_number = cpu_to_le16(channel->hw_value),
+	body->dtim_period = conf->dtim_period;
+	body->short_preamble = conf->use_short_preamble;
+	body->channel_number = cpu_to_le16(channel->hw_value);
 	body->beacon_interval = cpu_to_le32(conf->beacon_int);
 	body->basic_rate_set =
 		cpu_to_le32(wfx_rate_mask_to_hw(wvif->wdev, conf->basic_rates));
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/5] staging: wfx: make warning about pending frame less scary

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Removing station while some traffic is in progress may happen.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/sta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 03d0f224ffdb..010e13bcd33e 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
-		WARN(sta_priv->buffered[i], "release station while Tx is in progress");
+		if (sta_priv->buffered[i])
+			dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
+				 sta_priv->buffered[i], i);
 	// FIXME: see note in wfx_sta_add()
 	if (vif->type == NL80211_IFTYPE_STATION)
 		return 0;
-- 
2.25.1

[PATCH 3/5] staging: wfx: make warning about pending frame less scary

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Removing station while some traffic is in progress may happen.

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/sta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 03d0f224ffdb..010e13bcd33e 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
-		WARN(sta_priv->buffered[i], "release station while Tx is in progress");
+		if (sta_priv->buffered[i])
+			dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
+				 sta_priv->buffered[i], i);
 	// FIXME: see note in wfx_sta_add()
 	if (vif->type == NL80211_IFTYPE_STATION)
 		return 0;
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/5] staging: wfx: fix RCU usage in wfx_join_finalize()

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Access to sta->ht_cap is protected by RCU. However,
hif_set_association_mode() may sleep, so it can't be called in RCU.

This patch fix this behavior by handling sta and its RCU directly from
function hif_set_association_mode().

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
Fixes: d00149011066 ("staging: wfx: fix RCU usage")
---
 drivers/staging/wfx/hif_tx_mib.h | 15 ++++++++++-----
 drivers/staging/wfx/sta.c        |  4 ++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx_mib.h b/drivers/staging/wfx/hif_tx_mib.h
index bf3769c2a9b6..26b1406f9f6c 100644
--- a/drivers/staging/wfx/hif_tx_mib.h
+++ b/drivers/staging/wfx/hif_tx_mib.h
@@ -191,10 +191,10 @@ static inline int hif_set_block_ack_policy(struct wfx_vif *wvif,
 }
 
 static inline int hif_set_association_mode(struct wfx_vif *wvif,
-					   struct ieee80211_bss_conf *info,
-					   struct ieee80211_sta_ht_cap *ht_cap)
+					   struct ieee80211_bss_conf *info)
 {
 	int basic_rates = wfx_rate_mask_to_hw(wvif->wdev, info->basic_rates);
+	struct ieee80211_sta *sta = NULL;
 	struct hif_mib_set_association_mode val = {
 		.preambtype_use = 1,
 		.mode = 1,
@@ -204,12 +204,17 @@ static inline int hif_set_association_mode(struct wfx_vif *wvif,
 		.basic_rate_set = cpu_to_le32(basic_rates)
 	};
 
+	rcu_read_lock(); // protect sta
+	if (info->bssid && !info->ibss_joined)
+		sta = ieee80211_find_sta(wvif->vif, info->bssid);
+
 	// FIXME: it is strange to not retrieve all information from bss_info
-	if (ht_cap && ht_cap->ht_supported) {
-		val.mpdu_start_spacing = ht_cap->ampdu_density;
+	if (sta && sta->ht_cap.ht_supported) {
+		val.mpdu_start_spacing = sta->ht_cap.ampdu_density;
 		if (!(info->ht_operation_mode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT))
-			val.greenfield = !!(ht_cap->cap & IEEE80211_HT_CAP_GRN_FLD);
+			val.greenfield = !!(sta->ht_cap.cap & IEEE80211_HT_CAP_GRN_FLD);
 	}
+	rcu_read_unlock();
 
 	return hif_write_mib(wvif->wdev, wvif->id,
 			     HIF_MIB_ID_SET_ASSOCIATION_MODE, &val, sizeof(val));
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 010e13bcd33e..ed16475c207c 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -691,6 +691,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif,
 			wfx_rate_mask_to_hw(wvif->wdev, sta->supp_rates[wvif->channel->band]);
 	else
 		wvif->bss_params.operational_rate_set = -1;
+	rcu_read_unlock();
 	if (sta &&
 	    info->ht_operation_mode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT)
 		hif_dual_cts_protection(wvif, true);
@@ -703,8 +704,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif,
 	wvif->bss_params.beacon_lost_count = 20;
 	wvif->bss_params.aid = info->aid;
 
-	hif_set_association_mode(wvif, info, sta ? &sta->ht_cap : NULL);
-	rcu_read_unlock();
+	hif_set_association_mode(wvif, info);
 
 	if (!info->ibss_joined) {
 		hif_keep_alive_period(wvif, 30 /* sec */);
-- 
2.25.1

[PATCH 4/5] staging: wfx: fix RCU usage in wfx_join_finalize()

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Access to sta->ht_cap is protected by RCU. However,
hif_set_association_mode() may sleep, so it can't be called in RCU.

This patch fix this behavior by handling sta and its RCU directly from
function hif_set_association_mode().

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
Fixes: d00149011066 ("staging: wfx: fix RCU usage")
---
 drivers/staging/wfx/hif_tx_mib.h | 15 ++++++++++-----
 drivers/staging/wfx/sta.c        |  4 ++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx_mib.h b/drivers/staging/wfx/hif_tx_mib.h
index bf3769c2a9b6..26b1406f9f6c 100644
--- a/drivers/staging/wfx/hif_tx_mib.h
+++ b/drivers/staging/wfx/hif_tx_mib.h
@@ -191,10 +191,10 @@ static inline int hif_set_block_ack_policy(struct wfx_vif *wvif,
 }
 
 static inline int hif_set_association_mode(struct wfx_vif *wvif,
-					   struct ieee80211_bss_conf *info,
-					   struct ieee80211_sta_ht_cap *ht_cap)
+					   struct ieee80211_bss_conf *info)
 {
 	int basic_rates = wfx_rate_mask_to_hw(wvif->wdev, info->basic_rates);
+	struct ieee80211_sta *sta = NULL;
 	struct hif_mib_set_association_mode val = {
 		.preambtype_use = 1,
 		.mode = 1,
@@ -204,12 +204,17 @@ static inline int hif_set_association_mode(struct wfx_vif *wvif,
 		.basic_rate_set = cpu_to_le32(basic_rates)
 	};
 
+	rcu_read_lock(); // protect sta
+	if (info->bssid && !info->ibss_joined)
+		sta = ieee80211_find_sta(wvif->vif, info->bssid);
+
 	// FIXME: it is strange to not retrieve all information from bss_info
-	if (ht_cap && ht_cap->ht_supported) {
-		val.mpdu_start_spacing = ht_cap->ampdu_density;
+	if (sta && sta->ht_cap.ht_supported) {
+		val.mpdu_start_spacing = sta->ht_cap.ampdu_density;
 		if (!(info->ht_operation_mode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT))
-			val.greenfield = !!(ht_cap->cap & IEEE80211_HT_CAP_GRN_FLD);
+			val.greenfield = !!(sta->ht_cap.cap & IEEE80211_HT_CAP_GRN_FLD);
 	}
+	rcu_read_unlock();
 
 	return hif_write_mib(wvif->wdev, wvif->id,
 			     HIF_MIB_ID_SET_ASSOCIATION_MODE, &val, sizeof(val));
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 010e13bcd33e..ed16475c207c 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -691,6 +691,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif,
 			wfx_rate_mask_to_hw(wvif->wdev, sta->supp_rates[wvif->channel->band]);
 	else
 		wvif->bss_params.operational_rate_set = -1;
+	rcu_read_unlock();
 	if (sta &&
 	    info->ht_operation_mode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT)
 		hif_dual_cts_protection(wvif, true);
@@ -703,8 +704,7 @@ static void wfx_join_finalize(struct wfx_vif *wvif,
 	wvif->bss_params.beacon_lost_count = 20;
 	wvif->bss_params.aid = info->aid;
 
-	hif_set_association_mode(wvif, info, sta ? &sta->ht_cap : NULL);
-	rcu_read_unlock();
+	hif_set_association_mode(wvif, info);
 
 	if (!info->ibss_joined) {
 		hif_keep_alive_period(wvif, 30 /* sec */);
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/5] staging: wfx: fix RCU usage between hif_join() and ieee80211_bss_get_ie()

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Access to result of ieee80211_bss_get_ie() is protected by RCU. In other
hand, function hif_join() can sleep and cannot be called with RCU
locked.

Provide a copy of "ssidie" to hif_join() to solve this behavior.

Fixes: 9ced9b593741 ("staging: wfx: simplify hif_join()")
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c |  8 ++++----
 drivers/staging/wfx/hif_tx.h |  2 +-
 drivers/staging/wfx/sta.c    | 17 ++++++++++-------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 7a56e45bcdaa..77bca43aca42 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -290,7 +290,7 @@ int hif_stop_scan(struct wfx_vif *wvif)
 }
 
 int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
-	     const struct ieee80211_channel *channel, const u8 *ssidie)
+	     struct ieee80211_channel *channel, const u8 *ssid, int ssidlen)
 {
 	int ret;
 	struct hif_msg *hif;
@@ -308,9 +308,9 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
 	body->basic_rate_set =
 		cpu_to_le32(wfx_rate_mask_to_hw(wvif->wdev, conf->basic_rates));
 	memcpy(body->bssid, conf->bssid, sizeof(body->bssid));
-	if (!conf->ibss_joined && ssidie) {
-		body->ssid_length = cpu_to_le32(ssidie[1]);
-		memcpy(body->ssid, &ssidie[2], ssidie[1]);
+	if (!conf->ibss_joined && ssid) {
+		body->ssid_length = cpu_to_le32(ssidlen);
+		memcpy(body->ssid, ssid, ssidlen);
 	}
 	wfx_fill_header(hif, wvif->id, HIF_REQ_ID_JOIN, sizeof(*body));
 	ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
diff --git a/drivers/staging/wfx/hif_tx.h b/drivers/staging/wfx/hif_tx.h
index 20977e461718..f8520a14c14c 100644
--- a/drivers/staging/wfx/hif_tx.h
+++ b/drivers/staging/wfx/hif_tx.h
@@ -46,7 +46,7 @@ int hif_scan(struct wfx_vif *wvif, struct cfg80211_scan_request *req80211,
 	     int chan_start, int chan_num);
 int hif_stop_scan(struct wfx_vif *wvif);
 int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
-	     const struct ieee80211_channel *channel, const u8 *ssidie);
+	     struct ieee80211_channel *channel, const u8 *ssid, int ssidlen);
 int hif_set_pm(struct wfx_vif *wvif, bool ps, int dynamic_ps_timeout);
 int hif_set_bss_params(struct wfx_vif *wvif,
 		       const struct hif_req_set_bss_params *arg);
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index ed16475c207c..af4f4bbd0572 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -491,9 +491,11 @@ static void wfx_set_mfp(struct wfx_vif *wvif,
 static void wfx_do_join(struct wfx_vif *wvif)
 {
 	int ret;
-	const u8 *ssidie;
 	struct ieee80211_bss_conf *conf = &wvif->vif->bss_conf;
 	struct cfg80211_bss *bss = NULL;
+	u8 ssid[IEEE80211_MAX_SSID_LEN];
+	const u8 *ssidie = NULL;
+	int ssidlen = 0;
 
 	wfx_tx_lock_flush(wvif->wdev);
 
@@ -514,11 +516,14 @@ static void wfx_do_join(struct wfx_vif *wvif)
 	if (!wvif->beacon_int)
 		wvif->beacon_int = 1;
 
-	rcu_read_lock();
+	rcu_read_lock(); // protect ssidie
 	if (!conf->ibss_joined)
 		ssidie = ieee80211_bss_get_ie(bss, WLAN_EID_SSID);
-	else
-		ssidie = NULL;
+	if (ssidie) {
+		ssidlen = ssidie[1];
+		memcpy(ssid, &ssidie[2], ssidie[1]);
+	}
+	rcu_read_unlock();
 
 	wfx_tx_flush(wvif->wdev);
 
@@ -527,10 +532,8 @@ static void wfx_do_join(struct wfx_vif *wvif)
 
 	wfx_set_mfp(wvif, bss);
 
-	/* Perform actual join */
 	wvif->wdev->tx_burst_idx = -1;
-	ret = hif_join(wvif, conf, wvif->channel, ssidie);
-	rcu_read_unlock();
+	ret = hif_join(wvif, conf, wvif->channel, ssid, ssidlen);
 	if (ret) {
 		ieee80211_connection_loss(wvif->vif);
 		wvif->join_complete_status = -1;
-- 
2.25.1

[PATCH 5/5] staging: wfx: fix RCU usage between hif_join() and ieee80211_bss_get_ie()

[Jerome Pouiller]

From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Access to result of ieee80211_bss_get_ie() is protected by RCU. In other
hand, function hif_join() can sleep and cannot be called with RCU
locked.

Provide a copy of "ssidie" to hif_join() to solve this behavior.

Fixes: 9ced9b593741 ("staging: wfx: simplify hif_join()")
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/hif_tx.c |  8 ++++----
 drivers/staging/wfx/hif_tx.h |  2 +-
 drivers/staging/wfx/sta.c    | 17 ++++++++++-------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 7a56e45bcdaa..77bca43aca42 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -290,7 +290,7 @@ int hif_stop_scan(struct wfx_vif *wvif)
 }
 
 int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
-	     const struct ieee80211_channel *channel, const u8 *ssidie)
+	     struct ieee80211_channel *channel, const u8 *ssid, int ssidlen)
 {
 	int ret;
 	struct hif_msg *hif;
@@ -308,9 +308,9 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
 	body->basic_rate_set =
 		cpu_to_le32(wfx_rate_mask_to_hw(wvif->wdev, conf->basic_rates));
 	memcpy(body->bssid, conf->bssid, sizeof(body->bssid));
-	if (!conf->ibss_joined && ssidie) {
-		body->ssid_length = cpu_to_le32(ssidie[1]);
-		memcpy(body->ssid, &ssidie[2], ssidie[1]);
+	if (!conf->ibss_joined && ssid) {
+		body->ssid_length = cpu_to_le32(ssidlen);
+		memcpy(body->ssid, ssid, ssidlen);
 	}
 	wfx_fill_header(hif, wvif->id, HIF_REQ_ID_JOIN, sizeof(*body));
 	ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
diff --git a/drivers/staging/wfx/hif_tx.h b/drivers/staging/wfx/hif_tx.h
index 20977e461718..f8520a14c14c 100644
--- a/drivers/staging/wfx/hif_tx.h
+++ b/drivers/staging/wfx/hif_tx.h
@@ -46,7 +46,7 @@ int hif_scan(struct wfx_vif *wvif, struct cfg80211_scan_request *req80211,
 	     int chan_start, int chan_num);
 int hif_stop_scan(struct wfx_vif *wvif);
 int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
-	     const struct ieee80211_channel *channel, const u8 *ssidie);
+	     struct ieee80211_channel *channel, const u8 *ssid, int ssidlen);
 int hif_set_pm(struct wfx_vif *wvif, bool ps, int dynamic_ps_timeout);
 int hif_set_bss_params(struct wfx_vif *wvif,
 		       const struct hif_req_set_bss_params *arg);
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index ed16475c207c..af4f4bbd0572 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -491,9 +491,11 @@ static void wfx_set_mfp(struct wfx_vif *wvif,
 static void wfx_do_join(struct wfx_vif *wvif)
 {
 	int ret;
-	const u8 *ssidie;
 	struct ieee80211_bss_conf *conf = &wvif->vif->bss_conf;
 	struct cfg80211_bss *bss = NULL;
+	u8 ssid[IEEE80211_MAX_SSID_LEN];
+	const u8 *ssidie = NULL;
+	int ssidlen = 0;
 
 	wfx_tx_lock_flush(wvif->wdev);
 
@@ -514,11 +516,14 @@ static void wfx_do_join(struct wfx_vif *wvif)
 	if (!wvif->beacon_int)
 		wvif->beacon_int = 1;
 
-	rcu_read_lock();
+	rcu_read_lock(); // protect ssidie
 	if (!conf->ibss_joined)
 		ssidie = ieee80211_bss_get_ie(bss, WLAN_EID_SSID);
-	else
-		ssidie = NULL;
+	if (ssidie) {
+		ssidlen = ssidie[1];
+		memcpy(ssid, &ssidie[2], ssidie[1]);
+	}
+	rcu_read_unlock();
 
 	wfx_tx_flush(wvif->wdev);
 
@@ -527,10 +532,8 @@ static void wfx_do_join(struct wfx_vif *wvif)
 
 	wfx_set_mfp(wvif, bss);
 
-	/* Perform actual join */
 	wvif->wdev->tx_burst_idx = -1;
-	ret = hif_join(wvif, conf, wvif->channel, ssidie);
-	rcu_read_unlock();
+	ret = hif_join(wvif, conf, wvif->channel, ssid, ssidlen);
 	if (ret) {
 		ieee80211_connection_loss(wvif->vif);
 		wvif->join_complete_status = -1;
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/5] staging: wfx: fix lines ending with a comma instead of a semicolon

[Dan Carpenter]

On Tue, Mar 10, 2020 at 11:13:53AM +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> Obviously introduced by mistake.
> 

I have a Smatch check for when people use a comma instead of semi-colon,
but I have never published it because it seems totally harmless.  I
can't think of a reason why we should use semi-colons instead of commas.

regards,
dan carpenter

Re: [PATCH 2/5] staging: wfx: fix lines ending with a comma instead of a semicolon

[Dan Carpenter]

On Tue, Mar 10, 2020 at 11:13:53AM +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> Obviously introduced by mistake.
> 

I have a Smatch check for when people use a comma instead of semi-colon,
but I have never published it because it seems totally harmless.  I
can't think of a reason why we should use semi-colons instead of commas.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: wfx: make warning about pending frame less scary

[Dan Carpenter]

On Tue, Mar 10, 2020 at 11:13:54AM +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> Removing station while some traffic is in progress may happen.
> 

You're doing this in every commit where you start the commit message in
the subject and then just keep writing.  Take a look at your patch in
this URL.  Try to find the subject.

https://marc.info/?l=linux-driver-devel&m=158383526527951&w=2

The subject is far separated from the body of the commit message.  I
normally read the patch first, then I read the commit message and I
don't read the subject at all.  Or sometimes I only read the subject.

https://www.designershumor.com/2019/09/30/you-will-read-this-first-meme/

So it really helps me if the commit message restates the subject.  The
truth is that I don't really even like the advice that Josh wrote in
the howto about patch descriptions.  I normally start by explaining the
problem then how I solved it.  But I try not to be a pedant, so long as
I can understand the problem and the patch that's fine.  So how I would
write this commit message is:

    The warning message about releasing a station while Tx is in
    progress will trigger a stack trace, possibly a reboot depending
    on the configuration, and a syzbot email.  It's not necessarily
    a big deal that transmission is still in process so let's make the
    warning less scary.

> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  drivers/staging/wfx/sta.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 03d0f224ffdb..010e13bcd33e 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
> -		WARN(sta_priv->buffered[i], "release station while Tx is in progress");
> +		if (sta_priv->buffered[i])
> +			dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
> +				 sta_priv->buffered[i], i);

Why print a warning message at all if this is a normal situation?  Just
delete the whole thing.

regards,
dan carpenter

Re: [PATCH 3/5] staging: wfx: make warning about pending frame less scary

[Dan Carpenter]

On Tue, Mar 10, 2020 at 11:13:54AM +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> Removing station while some traffic is in progress may happen.
> 

You're doing this in every commit where you start the commit message in
the subject and then just keep writing.  Take a look at your patch in
this URL.  Try to find the subject.

https://marc.info/?l=linux-driver-devel&m=158383526527951&w=2

The subject is far separated from the body of the commit message.  I
normally read the patch first, then I read the commit message and I
don't read the subject at all.  Or sometimes I only read the subject.

https://www.designershumor.com/2019/09/30/you-will-read-this-first-meme/

So it really helps me if the commit message restates the subject.  The
truth is that I don't really even like the advice that Josh wrote in
the howto about patch descriptions.  I normally start by explaining the
problem then how I solved it.  But I try not to be a pedant, so long as
I can understand the problem and the patch that's fine.  So how I would
write this commit message is:

    The warning message about releasing a station while Tx is in
    progress will trigger a stack trace, possibly a reboot depending
    on the configuration, and a syzbot email.  It's not necessarily
    a big deal that transmission is still in process so let's make the
    warning less scary.

> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  drivers/staging/wfx/sta.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 03d0f224ffdb..010e13bcd33e 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
> -		WARN(sta_priv->buffered[i], "release station while Tx is in progress");
> +		if (sta_priv->buffered[i])
> +			dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
> +				 sta_priv->buffered[i], i);

Why print a warning message at all if this is a normal situation?  Just
delete the whole thing.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: wfx: make warning about pending frame less scary

[Jérôme Pouiller]

On Thursday 12 March 2020 15:30:19 CET Dan Carpenter wrote:
> On Tue, Mar 10, 2020 at 11:13:54AM +0100, Jerome Pouiller wrote:
[...]
> So it really helps me if the commit message restates the subject.  The
> truth is that I don't really even like the advice that Josh wrote in
> the howto about patch descriptions.  I normally start by explaining the
> problem then how I solved it.  But I try not to be a pedant, so long as
> I can understand the problem and the patch that's fine.  So how I would
> write this commit message is:
> 
>     The warning message about releasing a station while Tx is in
>     progress will trigger a stack trace, possibly a reboot depending
>     on the configuration, and a syzbot email.  It's not necessarily
>     a big deal that transmission is still in process so let's make the
>     warning less scary.

Indeed, my idea was the reviewers start by reading subjects and then read
the body of the commit. I will care now.


> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> >  drivers/staging/wfx/sta.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index 03d0f224ffdb..010e13bcd33e 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >       int i;
> >
> >       for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
> > -             WARN(sta_priv->buffered[i], "release station while Tx is in progress");
> > +             if (sta_priv->buffered[i])
> > +                     dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
> > +                              sta_priv->buffered[i], i);
> 
> Why print a warning message at all if this is a normal situation?  Just
> delete the whole thing.

I saw cases where it happened and it seems harmless. In add, this code
is going to be released with 5.6. So, the WARN have to be removed.

However, I think it is not normal. Even if it is harmless, it is the
symptom of something unclean.

So, I think that dev_warn() is the correct level of notification.

(I should have included that in the commit log)

-- 
Jérôme Pouiller

Re: [PATCH 3/5] staging: wfx: make warning about pending frame less scary

[Jérôme Pouiller]

On Thursday 12 March 2020 15:30:19 CET Dan Carpenter wrote:
> On Tue, Mar 10, 2020 at 11:13:54AM +0100, Jerome Pouiller wrote:
[...]
> So it really helps me if the commit message restates the subject.  The
> truth is that I don't really even like the advice that Josh wrote in
> the howto about patch descriptions.  I normally start by explaining the
> problem then how I solved it.  But I try not to be a pedant, so long as
> I can understand the problem and the patch that's fine.  So how I would
> write this commit message is:
> 
>     The warning message about releasing a station while Tx is in
>     progress will trigger a stack trace, possibly a reboot depending
>     on the configuration, and a syzbot email.  It's not necessarily
>     a big deal that transmission is still in process so let's make the
>     warning less scary.

Indeed, my idea was the reviewers start by reading subjects and then read
the body of the commit. I will care now.


> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> >  drivers/staging/wfx/sta.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index 03d0f224ffdb..010e13bcd33e 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -605,7 +605,9 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> >       int i;
> >
> >       for (i = 0; i < ARRAY_SIZE(sta_priv->buffered); i++)
> > -             WARN(sta_priv->buffered[i], "release station while Tx is in progress");
> > +             if (sta_priv->buffered[i])
> > +                     dev_warn(wvif->wdev->dev, "release station while %d pending frame on queue %d",
> > +                              sta_priv->buffered[i], i);
> 
> Why print a warning message at all if this is a normal situation?  Just
> delete the whole thing.

I saw cases where it happened and it seems harmless. In add, this code
is going to be released with 5.6. So, the WARN have to be removed.

However, I think it is not normal. Even if it is harmless, it is the
symptom of something unclean.

So, I think that dev_warn() is the correct level of notification.

(I should have included that in the commit log)

-- 
Jérôme Pouiller

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel