Message-Id: <20200401110405.80282-29-Jerome.Pouiller@silabs.com>
Date:   Wed,  1 Apr 2020 13:04:01 +0200
From:   Jerome Pouiller <Jerome.Pouiller@...abs.com>
To:     devel@...verdev.osuosl.org, linux-wireless@...r.kernel.org
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jérôme Pouiller 
        <jerome.pouiller@...abs.com>
Subject: [PATCH 28/32] staging: wfx: repair wfx_flush()

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

Until now, wfx_flush() flushed queue for while device instead of only
the queue of the intended vif. It sometime failed with a timeout, but
this error was not reported.

Moreover, if the device was frozen, wfx_flush didn't do anything and it
results a potential warning (and maybe a resource leak) when the frozen
device was unregistered.

We can also notice that wfx_tx_queues_wait_empty_vif() did only exist to
work around the broken feature of wfx_flush().

This patch repair wfx_flush() and therefore drop
wfx_tx_queues_wait_empty_vif().

Signed-off-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
---
 drivers/staging/wfx/data_tx.c |  34 ++++++-
 drivers/staging/wfx/data_tx.h |   3 +-
 drivers/staging/wfx/main.c    |   1 -
 drivers/staging/wfx/queue.c   | 163 ++++++++++++++++------------------
 drivers/staging/wfx/queue.h   |  10 ++-
 drivers/staging/wfx/sta.c     |  36 +-------
 drivers/staging/wfx/sta.h     |   2 -
 7 files changed, 120 insertions(+), 129 deletions(-)

diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index ec95518c9167..1d9a8089f3d3 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -524,7 +524,7 @@ static void wfx_notify_buffered_tx(struct wfx_vif *wvif, struct sk_buff *skb)
 	rcu_read_unlock();
 }
 
-void wfx_skb_dtor(struct wfx_dev *wdev, struct sk_buff *skb)
+static void wfx_skb_dtor(struct wfx_dev *wdev, struct sk_buff *skb)
 {
 	struct hif_msg *hif = (struct hif_msg *)skb->data;
 	struct hif_req_tx *req = (struct hif_req_tx *)hif->body;
@@ -626,4 +626,36 @@ void wfx_tx_confirm_cb(struct wfx_vif *wvif, const struct hif_cnf_tx *arg)
 	wfx_skb_dtor(wvif->wdev, skb);
 }
 
+void wfx_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+	       u32 queues, bool drop)
+{
+	struct wfx_dev *wdev = hw->priv;
+	struct sk_buff_head dropped;
+	struct wfx_queue *queue;
+	struct sk_buff *skb;
+	int vif_id = -1;
+	int i;
+
+	if (vif)
+		vif_id = ((struct wfx_vif *)vif->drv_priv)->id;
+	skb_queue_head_init(&dropped);
+	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
+		if (!(BIT(i) & queues))
+			continue;
+		queue = &wdev->tx_queue[i];
+		if (drop)
+			wfx_tx_queue_drop(wdev, queue, vif_id, &dropped);
+		if (wdev->chip_frozen)
+			continue;
+		if (wait_event_timeout(wdev->tx_dequeue,
+				       wfx_tx_queue_empty(wdev, queue, vif_id),
+				       msecs_to_jiffies(1000)) <= 0)
+			dev_warn(wdev->dev, "frames queued while flushing tx queues?");
+	}
+	wfx_tx_flush(wdev);
+	if (wdev->chip_frozen)
+		wfx_pending_drop(wdev, &dropped);
+	while ((skb = skb_dequeue(&dropped)) != NULL)
+		wfx_skb_dtor(wdev, skb);
+}
 
diff --git a/drivers/staging/wfx/data_tx.h b/drivers/staging/wfx/data_tx.h
index 03fe3e319ba1..7f201f626410 100644
--- a/drivers/staging/wfx/data_tx.h
+++ b/drivers/staging/wfx/data_tx.h
@@ -44,7 +44,8 @@ void wfx_tx_policy_upload_work(struct work_struct *work);
 void wfx_tx(struct ieee80211_hw *hw, struct ieee80211_tx_control *control,
 	    struct sk_buff *skb);
 void wfx_tx_confirm_cb(struct wfx_vif *wvif, const struct hif_cnf_tx *arg);
-void wfx_skb_dtor(struct wfx_dev *wdev, struct sk_buff *skb);
+void wfx_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+	       u32 queues, bool drop);
 
 static inline struct wfx_tx_priv *wfx_skb_tx_priv(struct sk_buff *skb)
 {
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index 5e1a7a932b53..738016d45d63 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -267,7 +267,6 @@ static void wfx_free_common(void *data)
 
 	mutex_destroy(&wdev->rx_stats_lock);
 	mutex_destroy(&wdev->conf_mutex);
-	wfx_tx_queues_deinit(wdev);
 	ieee80211_free_hw(wdev->hw);
 }
 
diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
index a1a2f7756a27..d4302a30dc41 100644
--- a/drivers/staging/wfx/queue.c
+++ b/drivers/staging/wfx/queue.c
@@ -62,92 +62,79 @@ void wfx_tx_lock_flush(struct wfx_dev *wdev)
 	wfx_tx_flush(wdev);
 }
 
-/* If successful, LOCKS the TX queue! */
-void wfx_tx_queues_wait_empty_vif(struct wfx_vif *wvif)
-{
-	int i;
-	bool done;
-	struct wfx_queue *queue;
-	struct sk_buff *item;
-	struct wfx_dev *wdev = wvif->wdev;
-	struct hif_msg *hif;
-
-	if (wvif->wdev->chip_frozen) {
-		wfx_tx_lock_flush(wdev);
-		wfx_tx_queues_clear(wdev);
-		return;
-	}
-
-	do {
-		done = true;
-		wfx_tx_lock_flush(wdev);
-		for (i = 0; i < IEEE80211_NUM_ACS && done; ++i) {
-			queue = &wdev->tx_queue[i];
-			spin_lock_bh(&queue->normal.lock);
-			skb_queue_walk(&queue->normal, item) {
-				hif = (struct hif_msg *)item->data;
-				if (hif->interface == wvif->id)
-					done = false;
-			}
-			spin_unlock_bh(&queue->normal.lock);
-			spin_lock_bh(&queue->cab.lock);
-			skb_queue_walk(&queue->cab, item) {
-				hif = (struct hif_msg *)item->data;
-				if (hif->interface == wvif->id)
-					done = false;
-			}
-			spin_unlock_bh(&queue->cab.lock);
-		}
-		if (!done) {
-			wfx_tx_unlock(wdev);
-			msleep(20);
-		}
-	} while (!done);
-}
-
-static void wfx_tx_queue_clear(struct wfx_dev *wdev, struct wfx_queue *queue,
-			       struct sk_buff_head *gc_list)
-{
-	struct sk_buff *item;
-
-	while ((item = skb_dequeue(&queue->normal)) != NULL)
-		skb_queue_head(gc_list, item);
-	while ((item = skb_dequeue(&queue->cab)) != NULL)
-		skb_queue_head(gc_list, item);
-}
-
-void wfx_tx_queues_clear(struct wfx_dev *wdev)
-{
-	int i;
-	struct sk_buff *item;
-	struct sk_buff_head gc_list;
-
-	skb_queue_head_init(&gc_list);
-	for (i = 0; i < IEEE80211_NUM_ACS; ++i)
-		wfx_tx_queue_clear(wdev, &wdev->tx_queue[i], &gc_list);
-	wake_up(&wdev->tx_dequeue);
-	while ((item = skb_dequeue(&gc_list)) != NULL)
-		wfx_skb_dtor(wdev, item);
-}
-
 void wfx_tx_queues_init(struct wfx_dev *wdev)
 {
 	int i;
 
-	memset(wdev->tx_queue, 0, sizeof(wdev->tx_queue));
 	skb_queue_head_init(&wdev->tx_pending);
 	init_waitqueue_head(&wdev->tx_dequeue);
-
 	for (i = 0; i < IEEE80211_NUM_ACS; ++i) {
 		skb_queue_head_init(&wdev->tx_queue[i].normal);
 		skb_queue_head_init(&wdev->tx_queue[i].cab);
 	}
 }
 
-void wfx_tx_queues_deinit(struct wfx_dev *wdev)
+void wfx_tx_queues_check_empty(struct wfx_dev *wdev)
 {
-	WARN_ON(!skb_queue_empty(&wdev->tx_pending));
-	wfx_tx_queues_clear(wdev);
+	int i;
+
+	WARN_ON(!skb_queue_empty_lockless(&wdev->tx_pending));
+	for (i = 0; i < IEEE80211_NUM_ACS; ++i) {
+		WARN_ON(atomic_read(&wdev->tx_queue[i].pending_frames));
+		WARN_ON(!skb_queue_empty_lockless(&wdev->tx_queue[i].normal));
+		WARN_ON(!skb_queue_empty_lockless(&wdev->tx_queue[i].cab));
+	}
+}
+
+static bool __wfx_tx_queue_empty(struct wfx_dev *wdev,
+				 struct sk_buff_head *skb_queue, int vif_id)
+{
+	struct hif_msg *hif_msg;
+	struct sk_buff *skb;
+
+	spin_lock_bh(&skb_queue->lock);
+	skb_queue_walk(skb_queue, skb) {
+		hif_msg = (struct hif_msg *)skb->data;
+		if (vif_id < 0 || hif_msg->interface == vif_id) {
+			spin_unlock_bh(&skb_queue->lock);
+			return false;
+		}
+	}
+	spin_unlock_bh(&skb_queue->lock);
+	return true;
+}
+
+bool wfx_tx_queue_empty(struct wfx_dev *wdev,
+			struct wfx_queue *queue, int vif_id)
+{
+	return __wfx_tx_queue_empty(wdev, &queue->normal, vif_id) &&
+	       __wfx_tx_queue_empty(wdev, &queue->cab, vif_id);
+}
+
+static void __wfx_tx_queue_drop(struct wfx_dev *wdev,
+				struct sk_buff_head *skb_queue, int vif_id,
+				struct sk_buff_head *dropped)
+{
+	struct sk_buff *skb, *tmp;
+	struct hif_msg *hif_msg;
+
+	spin_lock_bh(&skb_queue->lock);
+	skb_queue_walk_safe(skb_queue, skb, tmp) {
+		hif_msg = (struct hif_msg *)skb->data;
+		if (vif_id < 0 || hif_msg->interface == vif_id) {
+			__skb_unlink(skb, skb_queue);
+			skb_queue_head(dropped, skb);
+		}
+	}
+	spin_unlock_bh(&skb_queue->lock);
+}
+
+void wfx_tx_queue_drop(struct wfx_dev *wdev, struct wfx_queue *queue,
+		       int vif_id, struct sk_buff_head *dropped)
+{
+	__wfx_tx_queue_drop(wdev, &queue->cab, vif_id, dropped);
+	__wfx_tx_queue_drop(wdev, &queue->normal, vif_id, dropped);
+	wake_up(&wdev->tx_dequeue);
 }
 
 void wfx_tx_queues_put(struct wfx_dev *wdev, struct sk_buff *skb)
@@ -174,6 +161,22 @@ int wfx_pending_requeue(struct wfx_dev *wdev, struct sk_buff *skb)
 	return 0;
 }
 
+void wfx_pending_drop(struct wfx_dev *wdev, struct sk_buff_head *dropped)
+{
+	struct wfx_queue *queue;
+	struct sk_buff *skb;
+
+	WARN(!wdev->chip_frozen, "%s should only be used to recover a frozen device",
+	     __func__);
+	while ((skb = skb_dequeue(&wdev->tx_pending)) != NULL) {
+		queue = &wdev->tx_queue[skb_get_queue_mapping(skb)];
+		WARN_ON(skb_get_queue_mapping(skb) > 3);
+		WARN_ON(!atomic_read(&queue->pending_frames));
+		atomic_dec(&queue->pending_frames);
+		skb_queue_head(dropped, skb);
+	}
+}
+
 struct sk_buff *wfx_pending_get(struct wfx_dev *wdev, u32 packet_id)
 {
 	struct wfx_queue *queue;
@@ -249,17 +252,6 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
 	return false;
 }
 
-bool wfx_tx_queues_empty(struct wfx_dev *wdev)
-{
-	int i;
-
-	for (i = 0; i < IEEE80211_NUM_ACS; i++)
-		if (!skb_queue_empty_lockless(&wdev->tx_queue[i].normal) ||
-		    !skb_queue_empty_lockless(&wdev->tx_queue[i].cab))
-			return false;
-	return true;
-}
-
 static bool wfx_handle_tx_data(struct wfx_dev *wdev, struct sk_buff *skb)
 {
 	struct hif_req_tx *req = wfx_skb_txreq(skb);
@@ -364,8 +356,7 @@ struct hif_msg *wfx_tx_queues_get(struct wfx_dev *wdev)
 		if (!skb)
 			return NULL;
 		skb_queue_tail(&wdev->tx_pending, skb);
-		if (wfx_tx_queues_empty(wdev))
-			wake_up(&wdev->tx_dequeue);
+		wake_up(&wdev->tx_dequeue);
 		// FIXME: is it useful?
 		if (wfx_handle_tx_data(wdev, skb))
 			continue;
diff --git a/drivers/staging/wfx/queue.h b/drivers/staging/wfx/queue.h
index 9bc1a5200e64..ab45e32cbfbc 100644
--- a/drivers/staging/wfx/queue.h
+++ b/drivers/staging/wfx/queue.h
@@ -31,16 +31,18 @@ void wfx_tx_flush(struct wfx_dev *wdev);
 void wfx_tx_lock_flush(struct wfx_dev *wdev);
 
 void wfx_tx_queues_init(struct wfx_dev *wdev);
-void wfx_tx_queues_deinit(struct wfx_dev *wdev);
-void wfx_tx_queues_clear(struct wfx_dev *wdev);
-bool wfx_tx_queues_empty(struct wfx_dev *wdev);
+void wfx_tx_queues_check_empty(struct wfx_dev *wdev);
 bool wfx_tx_queues_has_cab(struct wfx_vif *wvif);
-void wfx_tx_queues_wait_empty_vif(struct wfx_vif *wvif);
 void wfx_tx_queues_put(struct wfx_dev *wdev, struct sk_buff *skb);
 struct hif_msg *wfx_tx_queues_get(struct wfx_dev *wdev);
 
+bool wfx_tx_queue_empty(struct wfx_dev *wdev, struct wfx_queue *queue,
+			int vif_id);
+void wfx_tx_queue_drop(struct wfx_dev *wdev, struct wfx_queue *queue,
+		       int vif_id, struct sk_buff_head *dropped);
 
 struct sk_buff *wfx_pending_get(struct wfx_dev *wdev, u32 packet_id);
+void wfx_pending_drop(struct wfx_dev *wdev, struct sk_buff_head *dropped);
 int wfx_pending_requeue(struct wfx_dev *wdev, struct sk_buff *skb);
 unsigned int wfx_pending_get_pkt_us_delay(struct wfx_dev *wdev,
 					  struct sk_buff *skb);
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 340e09bb639d..b1ee02d2f515 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -318,29 +318,6 @@ int wfx_set_rts_threshold(struct ieee80211_hw *hw, u32 value)
 	return 0;
 }
 
-static int __wfx_flush(struct wfx_dev *wdev, bool drop)
-{
-	for (;;) {
-		if (drop)
-			wfx_tx_queues_clear(wdev);
-		if (wait_event_timeout(wdev->tx_dequeue,
-				       wfx_tx_queues_empty(wdev),
-				       2 * HZ) <= 0)
-			return -ETIMEDOUT;
-		wfx_tx_flush(wdev);
-		if (wfx_tx_queues_empty(wdev))
-			return 0;
-		dev_warn(wdev->dev, "frames queued while flushing tx queues");
-	}
-}
-
-void wfx_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
-		  u32 queues, bool drop)
-{
-	// FIXME: only flush requested vif and queues
-	__wfx_flush(hw->priv, drop);
-}
-
 /* WSM callbacks */
 
 static void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi)
@@ -843,10 +820,8 @@ static int wfx_update_tim(struct wfx_vif *wvif)
 
 	skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif,
 				       &tim_offset, &tim_length);
-	if (!skb) {
-		__wfx_flush(wvif->wdev, true);
+	if (!skb)
 		return -ENOENT;
-	}
 	tim_ptr = skb->data + tim_offset;
 
 	if (tim_offset && tim_length >= 6) {
@@ -1062,8 +1037,6 @@ void wfx_remove_interface(struct ieee80211_hw *hw,
 	}
 
 	wvif->state = WFX_STATE_PASSIVE;
-	wfx_tx_queues_wait_empty_vif(wvif);
-	wfx_tx_unlock(wdev);
 
 	/* FIXME: In add to reset MAC address, try to reset interface */
 	hif_set_macaddr(wvif, NULL);
@@ -1097,10 +1070,5 @@ void wfx_stop(struct ieee80211_hw *hw)
 {
 	struct wfx_dev *wdev = hw->priv;
 
-	wfx_tx_lock_flush(wdev);
-	mutex_lock(&wdev->conf_mutex);
-	wfx_tx_queues_clear(wdev);
-	mutex_unlock(&wdev->conf_mutex);
-	wfx_tx_unlock(wdev);
-	WARN(atomic_read(&wdev->tx_lock), "tx_lock is locked");
+	wfx_tx_queues_check_empty(wdev);
 }
diff --git a/drivers/staging/wfx/sta.h b/drivers/staging/wfx/sta.h
index cf99a8a74a81..a0c5153e5272 100644
--- a/drivers/staging/wfx/sta.h
+++ b/drivers/staging/wfx/sta.h
@@ -54,8 +54,6 @@ void wfx_configure_filter(struct ieee80211_hw *hw, unsigned int changed_flags,
 
 int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
 void wfx_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
-void wfx_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
-	       u32 queues, bool drop);
 int wfx_conf_tx(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		u16 queue, const struct ieee80211_tx_queue_params *params);
 void wfx_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
-- 
2.25.1

Powered by blists - more mailing lists