ath9k: Restart xmit logic in xmit watchdog.

PATCH New greearb at candelatech.com 2 participants 5 messages Jan 9, 2011 – Jan 10, 2011 Qualcomm Atheros ATH9K wireless driver

lore

Share & tools

lore

greearb at candelatech.com

author

2011-01-09 07:46 UTC #0 lore raw

PATCH 1/1 ath9k: Restart xmit logic in xmit watchdog.

From: Ben Greear <[email protected]>

The system can get into a state where the xmit queue is stopped, but there are no packets pending, so the queue will not be restarted.

Add logic to the xmit watchdog to attempt to restart the xmit logic if this situation is detected.

Signed-off-by Ben Greear <[email protected]>

---

NOTE: This is basically the same as a patch I posted a day or two ago. It doesn't address the concern of the reviewer who NACK'd it, but my system will not properly transmit packets without this patch applied. I realize this is a bit of a hack, but until we find and fix all of the other bugs, I think this patch or something similar should be applied.

Still, this patch should not be applied unless given positive ACK by the ath9k developers.

:100644 100644 93209d6... ca9d0d3... M drivers/net/wireless/ath/ath9k/ath9k.h
:100644 100644 9e009cc... b0cb792... M drivers/net/wireless/ath/ath9k/debug.c
:100644 100644 d9a4144... 1b3a62c... M drivers/net/wireless/ath/ath9k/xmit.c

 drivers/net/wireless/ath/ath9k/ath9k.h |    1 +
 drivers/net/wireless/ath/ath9k/debug.c |    4 ++-
 drivers/net/wireless/ath/ath9k/xmit.c  |   57 +++++++++++++++++++++++++++-----
 3 files changed, 52 insertions(+), 10 deletions(-)

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

index 93209d6..ca9d0d3 100644
@@ -630,6 +630,7 @@ struct ath_softc {
 	struct ath9k_debug debug;
 	spinlock_t nodes_lock;
 	struct list_head nodes; /* basically, stations */
+	unsigned int tx_complete_poll_work_seen;
 #endif
 	struct ath_beacon_config cur_beacon_conf;
 	struct delayed_work tx_complete_work;

drivers/net/wireless/ath/ath9k/debug.c +3 -1

index 9e009cc..b0cb792 100644
@@ -629,9 +629,11 @@ static ssize_t read_file_xmit(struct file *file, char __user *user_buf,
 	if (buf == NULL)
 		return -ENOMEM;
 
-	len += sprintf(buf, "Num-Tx-Queues: %i  tx-queues-setup: 0x%x\n"
+	len += sprintf(buf, "Num-Tx-Queues: %i  tx-queues-setup: 0x%x"
+		       " poll-work-seen: %u\n"
 		       "%30s %10s%10s%10s\n\n",
 		       ATH9K_NUM_TX_QUEUES, sc->tx.txqsetup,
+		       sc->tx_complete_poll_work_seen,
 		       "BE", "BK", "VI", "VO");
 
 	PR("MPDUs Queued:    ", queued);

drivers/net/wireless/ath/ath9k/xmit.c +48 -9

89 lines not on this page · raw message

Signature · 1 line

1.7.2.3

Felix Fietkau

2011-01-09 18:19 UTC #1 lore raw

PATCH 1/1 ath9k: Restart xmit logic in xmit watchdog.

Quoted 35 lines · greearb at candelatech.com

From: Ben Greear<[email protected]>
The system can get into a state where the xmit queue is stopped, but there are no packets pending, so the queue will not be restarted.
Add logic to the xmit watchdog to attempt to restart the xmit logic if this situation is detected.
Signed-off-by Ben Greear<[email protected]>
---
NOTE: This is basically the same as a patch I posted a day or two ago. It doesn't address the concern of the reviewer who NACK'd it, but my system will not properly transmit packets without this patch applied. I realize this is a bit of a hack, but until we find and fix all of the other bugs, I think this patch or something similar should be applied.
Still, this patch should not be applied unless given positive ACK by the ath9k developers.
drivers/net/wireless/ath/ath9k/xmit.c +2 -1
index d9a4144..1b3a62c 100644
@@ -1988,19 +1988,30 @@ static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts,
  	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
  }
 
-static void ath_wake_mac80211_queue(struct ath_softc *sc, int qnum)
+/* Has no locking. */
+static void __ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq)
  {

drivers/net/wireless/ath/ath9k/xmit.c static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts, · line 1998

Quoted 10 lines

drivers/net/wireless/ath/ath9k/xmit.c +2 -5

-	struct ath_txq *txq;
-
-	txq = sc->tx.txq_map[qnum];
-	spin_lock_bh(&txq->axq_lock);
  	if (txq->stopped&&  txq->pending_frames<  ATH_MAX_QDEPTH) {
-		if (ath_mac80211_start_queue(sc, qnum))
+		if (ath_mac80211_start_queue(sc, txq->axq_qnum))
  			txq->stopped = 0;
  	}
+}

This part is quite broken, I think you got confused with various types of queue numbers. txq->axq_qnum refers to the atheros hw queue index, whereas the qnum argument to this function refers to the mac80211 queue index (which is also the correct index for sc->tx.txq_map - not to be confused with the sc->tx.txq array).

Pushing the locking out to the caller side (with a wrapper) does sound like a good idea though.

- Felix

Ben Greear

author

2011-01-10 04:39 UTC #2 lore raw

PATCH 1/1 ath9k: Restart xmit logic in xmit watchdog.

Quoted 32 lines · Felix Fietkau

Quoted 31 lines · greearb at candelatech.com
From: Ben Greear<[email protected]>
The system can get into a state where the xmit queue is stopped, but there are no packets pending, so the queue will not be restarted.
Add logic to the xmit watchdog to attempt to restart the xmit logic if this situation is detected.
Signed-off-by Ben Greear<[email protected]>
---
NOTE: This is basically the same as a patch I posted a day or two ago. It doesn't address the concern of the reviewer who NACK'd it, but my system will not properly transmit packets without this patch applied. I realize this is a bit of a hack, but until we find and fix all of the other bugs, I think this patch or something similar should be applied.
Still, this patch should not be applied unless given positive ACK by the ath9k developers.
drivers/net/wireless/ath/ath9k/xmit.c +0 -0
index d9a4144..1b3a62c 100644
@@ -1988,19 +1988,30 @@ static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts,
 tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
 }
 

Quoted 16 lines

Quoted 4 lines
drivers/net/wireless/ath/ath9k/xmit.c +2 -1
-static void ath_wake_mac80211_queue(struct ath_softc *sc, int qnum)
+/* Has no locking. */
+static void __ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq)
 {
drivers/net/wireless/ath/ath9k/xmit.c static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts, · line 1998
Quoted 10 lines
drivers/net/wireless/ath/ath9k/xmit.c +2 -5
- struct ath_txq *txq;
-
- txq = sc->tx.txq_map[qnum];
- spin_lock_bh(&txq->axq_lock);
 if (txq->stopped&& txq->pending_frames< ATH_MAX_QDEPTH) {
- if (ath_mac80211_start_queue(sc, qnum))
+ if (ath_mac80211_start_queue(sc, txq->axq_qnum))
 txq->stopped = 0;
 }
+}
This part is quite broken, I think you got confused with various types of queue numbers. txq->axq_qnum refers to the atheros hw queue index, whereas the qnum argument to this function refers to the mac80211 queue index (which is also the correct index for sc->tx.txq_map - not to be confused with the sc->tx.txq array).

Yeah, I am confused on all of this. Looks like I should add a member to the txq struct to record it's mac80211 index and use that instead?

In the upstream code, is this correct? It seems to me that it should always be waking 'txq' since it just completed a packet. Why the check against txq_map?

		if (txq == sc->tx.txq_map[qnum])
			ath_wake_mac80211_queue(sc, qnum);

Quoted 1 line

Pushing the locking out to the caller side (with a wrapper) does sound like a good idea though.

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Felix Fietkau

2011-01-10 05:02 UTC #3 lore raw

PATCH 1/1 ath9k: Restart xmit logic in xmit watchdog.

Quoted 24 lines · Ben Greear

Quoted 23 lines · Felix Fietkau
Quoted 22 lines · greearb at candelatech.com
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c index d9a4144..1b3a62c 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1988,19 +1988,30 @@ static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts, tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
}
-static void ath_wake_mac80211_queue(struct ath_softc *sc, int qnum) +/* Has no locking. */
+static void __ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq) {
- struct ath_txq *txq;
-
- txq = sc->tx.txq_map[qnum];
- spin_lock_bh(&txq->axq_lock);
if (txq->stopped&& txq->pending_frames< ATH_MAX_QDEPTH) {
- if (ath_mac80211_start_queue(sc, qnum))
+ if (ath_mac80211_start_queue(sc, txq->axq_qnum))
txq->stopped = 0;
}
+}

Quoted 5 lines

Quoted 2 lines
This part is quite broken, I think you got confused with various types of queue numbers. txq->axq_qnum refers to the atheros hw queue index, whereas the qnum argument to this function refers to the mac80211 queue index (which is also the correct index for sc->tx.txq_map - not to be confused with the sc->tx.txq array).
Yeah, I am confused on all of this. Looks like I should add a member to the txq struct to record it's mac80211 index and use that instead?

How about just passing the proper qnum? You can get it from the skb queue mapping anyway.

Quoted 6 lines

In the upstream code, is this correct? It seems to me that it should always be waking 'txq' since it just completed a packet. Why the check against txq_map?
		if (txq == sc->tx.txq_map[qnum])
			ath_wake_mac80211_queue(sc, qnum);

Things like CAB (or maybe UAPSD at some point), where a frame might go out through a queue other than the 4 WMM data queues.

- Felix

Ben Greear

author

2011-01-10 05:08 UTC #4 lore raw

PATCH 1/1 ath9k: Restart xmit logic in xmit watchdog.

Quoted 29 lines · Felix Fietkau

Quoted 28 lines · Ben Greear
Quoted 26 lines · Felix Fietkau
Quoted 12 lines · greearb at candelatech.com
drivers/net/wireless/ath/ath9k/xmit.c +2 -1
index d9a4144..1b3a62c 100644
@@ -1988,19 +1988,30 @@ static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts,
 tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
 }
 
-static void ath_wake_mac80211_queue(struct ath_softc *sc, int qnum)
+/* Has no locking. */
+static void __ath_wake_mac80211_queue(struct ath_softc *sc, struct ath_txq *txq)
 {
drivers/net/wireless/ath/ath9k/xmit.c static void ath_tx_rc_status(struct ath_buf *bf, struct ath_tx_status *ts, · line 1998
Quoted 10 lines
drivers/net/wireless/ath/ath9k/xmit.c +2 -5
- struct ath_txq *txq;
-
- txq = sc->tx.txq_map[qnum];
- spin_lock_bh(&txq->axq_lock);
 if (txq->stopped&& txq->pending_frames< ATH_MAX_QDEPTH) {
- if (ath_mac80211_start_queue(sc, qnum))
+ if (ath_mac80211_start_queue(sc, txq->axq_qnum))
 txq->stopped = 0;
 }
+}
This part is quite broken, I think you got confused with various types of queue numbers. txq->axq_qnum refers to the atheros hw queue index, whereas the qnum argument to this function refers to the mac80211 queue index (which is also the correct index for sc->tx.txq_map - not to be confused with the sc->tx.txq array).

Quoted 3 lines

Quoted 2 lines
Yeah, I am confused on all of this. Looks like I should add a member to the txq struct to record it's mac80211 index and use that instead?
How about just passing the proper qnum? You can get it from the skb queue mapping anyway.

That seems too indirect to me, though that's probably just paranoia at this point.

Quoted 7 lines

Quoted 6 lines
In the upstream code, is this correct? It seems to me that it should always be waking 'txq' since it just completed a packet. Why the check against txq_map?
if (txq == sc->tx.txq_map[qnum])
ath_wake_mac80211_queue(sc, qnum);
Things like CAB (or maybe UAPSD at some point), where a frame might go out through a queue other than the 4 WMM data queues.

Ok, but that code is very subtle and difficult to understand in my opinion.

I'll re-post my patch with the qnums a bit more explicit..but if you still think it should be more like it is upstream, I'll re-do the patch.

Thanks,
Ben

Quoted 2 lines

- Felix

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com