[PATCH 1/5] rt2x00: rt2800usb: move additional txdone into new function

[PATCH 1/5] rt2x00: rt2800usb: move additional txdone into new function

[Stanislaw Gruszka]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 2c11137..8e1855a 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -528,15 +528,11 @@ static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
 	}
 }
 
-static void rt2800usb_work_txdone(struct work_struct *work)
+static void rt2800usb_txdone_nostatus(struct rt2x00_dev *rt2x00dev)
 {
-	struct rt2x00_dev *rt2x00dev =
-	    container_of(work, struct rt2x00_dev, txdone_work);
 	struct data_queue *queue;
 	struct queue_entry *entry;
 
-	rt2800usb_txdone(rt2x00dev);
-
 	/*
 	 * Process any trailing TX status reports for IO failures,
 	 * we loop until we find the first non-IO error entry. This
@@ -560,6 +556,16 @@ static void rt2800usb_work_txdone(struct work_struct *work)
 				break;
 		}
 	}
+}
+
+static void rt2800usb_work_txdone(struct work_struct *work)
+{
+	struct rt2x00_dev *rt2x00dev =
+	    container_of(work, struct rt2x00_dev, txdone_work);
+
+	rt2800usb_txdone(rt2x00dev);
+
+	rt2800usb_txdone_nostatus(rt2x00dev);
 
 	/*
 	 * The hw may delay sending the packet after DMA complete
-- 
1.7.1

[PATCH 2/5] rt2x00: rt2800usb: rework txdone code

[Stanislaw Gruszka]

Patch change txdone code to make it similar like txdone in rt2800pci,
process only one entry from queue matching tx status.

Before we processed all pending entries from queue until PACKEDID match,
that caused that we do not report tx statuses correctly.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   72 +++++++++++++-----------------
 1 files changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 8e1855a..4eaa628 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -438,35 +438,25 @@ static int rt2800usb_get_tx_data_len(struct queue_entry *entry)
 /*
  * TX control handlers
  */
-static bool rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
+static enum txdone_entry_desc_flags
+rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
 {
 	__le32 *txwi;
 	u32 word;
 	int wcid, ack, pid;
 	int tx_wcid, tx_ack, tx_pid;
 
-	if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
-	    !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags)) {
-		WARNING(entry->queue->rt2x00dev,
-			"Data pending for entry %u in queue %u\n",
-			entry->entry_idx, entry->queue->qid);
-		cond_resched();
-		return false;
-	}
-
-	wcid	= rt2x00_get_field32(reg, TX_STA_FIFO_WCID);
-	ack	= rt2x00_get_field32(reg, TX_STA_FIFO_TX_ACK_REQUIRED);
-	pid	= rt2x00_get_field32(reg, TX_STA_FIFO_PID_TYPE);
-
 	/*
 	 * This frames has returned with an IO error,
 	 * so the status report is not intended for this
 	 * frame.
 	 */
-	if (test_bit(ENTRY_DATA_IO_FAILED, &entry->flags)) {
-		rt2x00lib_txdone_noinfo(entry, TXDONE_FAILURE);
-		return false;
-	}
+	if (test_bit(ENTRY_DATA_IO_FAILED, &entry->flags))
+		return TXDONE_FAILURE;
+
+	wcid	= rt2x00_get_field32(reg, TX_STA_FIFO_WCID);
+	ack	= rt2x00_get_field32(reg, TX_STA_FIFO_TX_ACK_REQUIRED);
+	pid	= rt2x00_get_field32(reg, TX_STA_FIFO_PID_TYPE);
 
 	/*
 	 * Validate if this TX status report is intended for
@@ -482,12 +472,11 @@ static bool rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
 	if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) {
 		WARNING(entry->queue->rt2x00dev,
 			"TX status report missed for queue %d entry %d\n",
-		entry->queue->qid, entry->entry_idx);
-		rt2x00lib_txdone_noinfo(entry, TXDONE_UNKNOWN);
-		return false;
+			entry->queue->qid, entry->entry_idx);
+		return TXDONE_UNKNOWN;
 	}
 
-	return true;
+	return TXDONE_SUCCESS;
 }
 
 static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
@@ -496,35 +485,36 @@ static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
 	struct queue_entry *entry;
 	u32 reg;
 	u8 qid;
+	enum txdone_entry_desc_flags done_status;
 
 	while (kfifo_get(&rt2x00dev->txstatus_fifo, &reg)) {
-
-		/* TX_STA_FIFO_PID_QUEUE is a 2-bit field, thus
-		 * qid is guaranteed to be one of the TX QIDs
+		/*
+		 * TX_STA_FIFO_PID_QUEUE is a 2-bit field, thus qid is
+		 * guaranteed to be one of the TX QIDs .
 		 */
 		qid = rt2x00_get_field32(reg, TX_STA_FIFO_PID_QUEUE);
 		queue = rt2x00queue_get_tx_queue(rt2x00dev, qid);
-		if (unlikely(!queue)) {
-			WARNING(rt2x00dev, "Got TX status for an unavailable "
+
+		if (unlikely(rt2x00queue_empty(queue))) {
+			WARNING(rt2x00dev, "Got TX status for an empty "
 					   "queue %u, dropping\n", qid);
-			continue;
+			break;
 		}
 
-		/*
-		 * Inside each queue, we process each entry in a chronological
-		 * order. We first check that the queue is not empty.
-		 */
-		entry = NULL;
-		while (!rt2x00queue_empty(queue)) {
-			entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
-			if (rt2800usb_txdone_entry_check(entry, reg))
-				break;
-			entry = NULL;
+		entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
+
+		if (unlikely(test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
+			     !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))) {
+			WARNING(rt2x00dev, "Data pending for entry %u "
+					   "in queue %u\n", entry->entry_idx, qid);
+			break;
 		}
 
-		if (entry)
-			rt2800_txdone_entry(entry, reg,
-					    rt2800usb_get_txwi(entry));
+		done_status = rt2800usb_txdone_entry_check(entry, reg);
+		if (likely(done_status == TXDONE_SUCCESS))
+			rt2800_txdone_entry(entry, reg, rt2800usb_get_txwi(entry));
+		else
+			rt2x00lib_txdone_noinfo(entry, done_status);
 	}
 }
 
-- 
1.7.1

[PATCH 3/5] rt2x00: rt2800usb: rework txstatus code

[Stanislaw Gruszka]

Currently we read tx status register after each urb data transfer. As
callback procedure also trigger reading, that causing we have many
"threads" of reading status. To prevent that introduce TX_STATUS_READING
flags, and check if we are already in process of sequential reading
TX_STA_FIFO, before requesting new reads.

Change timer to hrtimer, that make TX_STA_FIFO overruns less possible.
Use 200 us for initial timeout, and then reschedule in 100 us period,
this values probably have to be tuned.

Make changes on txdone work. Schedule it from
rt2800usb_tx_sta_fifo_read_completed() callback when first valid status
show up. Check in callback if tx status timeout happens, and schedule
work on that condition too. That make possible to remove tx status
timeout from generic watchdog. I moved that to rt2800usb.

Loop in txdone work, that should prevent situation when we queue work,
which is already processed, and after finish work is not rescheduled
again.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c   |  120 +++++++++++++++++++++-------
 drivers/net/wireless/rt2x00/rt2x00.h      |   10 ++-
 drivers/net/wireless/rt2x00/rt2x00dev.c   |    2 +-
 drivers/net/wireless/rt2x00/rt2x00queue.h |   12 ---
 drivers/net/wireless/rt2x00/rt2x00usb.c   |   21 +-----
 5 files changed, 101 insertions(+), 64 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 4eaa628..eacf94b 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -114,45 +114,103 @@ static bool rt2800usb_txstatus_pending(struct rt2x00_dev *rt2x00dev)
 	return false;
 }
 
+static inline bool rt2800usb_entry_txstatus_timeout(struct queue_entry *entry)
+{
+	bool tout;
+
+	if (!test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
+		return false;
+
+	tout = time_after(jiffies, entry->last_action + msecs_to_jiffies(100));
+	if (unlikely(tout))
+		WARNING(entry->queue->rt2x00dev,
+			"TX status timeout for entry %d in queue %d\n",
+			entry->entry_idx, entry->queue->qid);
+	return tout;
+
+}
+
+static bool rt2800usb_txstatus_timeout(struct rt2x00_dev *rt2x00dev)
+{
+	struct data_queue *queue;
+	struct queue_entry *entry;
+
+	tx_queue_for_each(rt2x00dev, queue) {
+		entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
+		if (rt2800usb_entry_txstatus_timeout(entry))
+			return true;
+	}
+	return false;
+}
+
 static bool rt2800usb_tx_sta_fifo_read_completed(struct rt2x00_dev *rt2x00dev,
 						 int urb_status, u32 tx_status)
 {
+	bool valid;
+
 	if (urb_status) {
-		WARNING(rt2x00dev, "rt2x00usb_register_read_async failed: %d\n", urb_status);
-		return false;
+		WARNING(rt2x00dev, "TX status read failed %d\n", urb_status);
+
+		goto stop_reading;
 	}
 
-	/* try to read all TX_STA_FIFO entries before scheduling txdone_work */
-	if (rt2x00_get_field32(tx_status, TX_STA_FIFO_VALID)) {
-		if (!kfifo_put(&rt2x00dev->txstatus_fifo, &tx_status)) {
-			WARNING(rt2x00dev, "TX status FIFO overrun, "
-				"drop tx status report.\n");
-			queue_work(rt2x00dev->workqueue, &rt2x00dev->txdone_work);
-		} else
-			return true;
-	} else if (!kfifo_is_empty(&rt2x00dev->txstatus_fifo)) {
+	valid = rt2x00_get_field32(tx_status, TX_STA_FIFO_VALID);
+	if (valid) {
+		if (!kfifo_put(&rt2x00dev->txstatus_fifo, &tx_status))
+			WARNING(rt2x00dev, "TX status FIFO overrun\n");
+
 		queue_work(rt2x00dev->workqueue, &rt2x00dev->txdone_work);
+
+		/* Reschedule urb to read TX status again instantly */
+		return true;
 	} else if (rt2800usb_txstatus_pending(rt2x00dev)) {
-		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
+		/* Read register after 250 us */
+		hrtimer_start(&rt2x00dev->txstatus_timer, ktime_set(0, 250000),
+			      HRTIMER_MODE_REL);
+		return false;
 	}
 
-	return false;
+stop_reading:
+	clear_bit(TX_STATUS_READING, &rt2x00dev->flags);
+	/*
+	 * There is small race window above, between txstatus pending check and
+	 * clear_bit someone could do rt2x00usb_interrupt_txdone, so recheck
+	 * here again if status reading is needed.
+	 */
+	if (rt2800usb_txstatus_pending(rt2x00dev) &&
+	    test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
+		return true;
+	else
+		return false;
+}
+
+static void rt2800usb_async_read_tx_status(struct rt2x00_dev *rt2x00dev)
+{
+
+	if (test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
+		return;
+
+	/* Read TX_STA_FIFO register after 500 us */
+	hrtimer_start(&rt2x00dev->txstatus_timer, ktime_set(0, 500000),
+		      HRTIMER_MODE_REL);
 }
 
 static void rt2800usb_tx_dma_done(struct queue_entry *entry)
 {
 	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
 
-	rt2x00usb_register_read_async(rt2x00dev, TX_STA_FIFO,
-				      rt2800usb_tx_sta_fifo_read_completed);
+	rt2800usb_async_read_tx_status(rt2x00dev);
 }
 
-static void rt2800usb_tx_sta_fifo_timeout(unsigned long data)
+static enum hrtimer_restart rt2800usb_tx_sta_fifo_timeout(struct hrtimer *timer)
 {
-	struct rt2x00_dev *rt2x00dev = (struct rt2x00_dev *)data;
+	struct rt2x00_dev *rt2x00dev =
+	    container_of(timer, struct rt2x00_dev, txstatus_timer);
 
 	rt2x00usb_register_read_async(rt2x00dev, TX_STA_FIFO,
 				      rt2800usb_tx_sta_fifo_read_completed);
+
+	return HRTIMER_NORESTART;
 }
 
 /*
@@ -540,7 +598,7 @@ static void rt2800usb_txdone_nostatus(struct rt2x00_dev *rt2x00dev)
 
 			if (test_bit(ENTRY_DATA_IO_FAILED, &entry->flags))
 				rt2x00lib_txdone_noinfo(entry, TXDONE_FAILURE);
-			else if (rt2x00queue_status_timeout(entry))
+			else if (rt2800usb_entry_txstatus_timeout(entry))
 				rt2x00lib_txdone_noinfo(entry, TXDONE_UNKNOWN);
 			else
 				break;
@@ -553,17 +611,21 @@ static void rt2800usb_work_txdone(struct work_struct *work)
 	struct rt2x00_dev *rt2x00dev =
 	    container_of(work, struct rt2x00_dev, txdone_work);
 
-	rt2800usb_txdone(rt2x00dev);
+	while (!kfifo_is_empty(&rt2x00dev->txstatus_fifo) ||
+	       rt2800usb_txstatus_timeout(rt2x00dev)) {
 
-	rt2800usb_txdone_nostatus(rt2x00dev);
+		rt2800usb_txdone(rt2x00dev);
 
-	/*
-	 * The hw may delay sending the packet after DMA complete
-	 * if the medium is busy, thus the TX_STA_FIFO entry is
-	 * also delayed -> use a timer to retrieve it.
-	 */
-	if (rt2800usb_txstatus_pending(rt2x00dev))
-		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
+		rt2800usb_txdone_nostatus(rt2x00dev);
+
+		/*
+		 * The hw may delay sending the packet after DMA complete
+		 * if the medium is busy, thus the TX_STA_FIFO entry is
+		 * also delayed -> use a timer to retrieve it.
+		 */
+		if (rt2800usb_txstatus_pending(rt2x00dev))
+			rt2800usb_async_read_tx_status(rt2x00dev);
+	}
 }
 
 /*
@@ -705,9 +767,7 @@ static int rt2800usb_probe_hw(struct rt2x00_dev *rt2x00dev)
 	__set_bit(REQUIRE_TXSTATUS_FIFO, &rt2x00dev->cap_flags);
 	__set_bit(REQUIRE_PS_AUTOWAKE, &rt2x00dev->cap_flags);
 
-	setup_timer(&rt2x00dev->txstatus_timer,
-		    rt2800usb_tx_sta_fifo_timeout,
-		    (unsigned long) rt2x00dev);
+	rt2x00dev->txstatus_timer.function = rt2800usb_tx_sta_fifo_timeout,
 
 	/*
 	 * Set the rssi offset.
diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index 65275ef..471f87c 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -38,7 +38,7 @@
 #include <linux/etherdevice.h>
 #include <linux/input-polldev.h>
 #include <linux/kfifo.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 
 #include <net/mac80211.h>
 
@@ -692,6 +692,12 @@ enum rt2x00_state_flags {
 	 */
 	CONFIG_CHANNEL_HT40,
 	CONFIG_POWERSAVING,
+
+	/*
+	 * Mark we currently are sequentially reading TX_STA_FIFO register
+	 * FIXME: this is for only rt2800usb, should go to private data
+	 */
+	TX_STATUS_READING,
 };
 
 /*
@@ -974,7 +980,7 @@ struct rt2x00_dev {
 	/*
 	 * Timer to ensure tx status reports are read (rt2800usb).
 	 */
-	struct timer_list txstatus_timer;
+	struct hrtimer txstatus_timer;
 
 	/*
 	 * Tasklet for processing tx status reports (rt2800pci).
diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 61a5042..fc9901e 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -1236,7 +1236,7 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
 	cancel_delayed_work_sync(&rt2x00dev->autowakeup_work);
 	cancel_work_sync(&rt2x00dev->sleep_work);
 	if (rt2x00_is_usb(rt2x00dev)) {
-		del_timer_sync(&rt2x00dev->txstatus_timer);
+		hrtimer_cancel(&rt2x00dev->txstatus_timer);
 		cancel_work_sync(&rt2x00dev->rxdone_work);
 		cancel_work_sync(&rt2x00dev->txdone_work);
 	}
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h
index 349008d..5f1392c 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.h
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.h
@@ -636,18 +636,6 @@ static inline int rt2x00queue_threshold(struct data_queue *queue)
 {
 	return rt2x00queue_available(queue) < queue->threshold;
 }
-
-/**
- * rt2x00queue_status_timeout - Check if a timeout occurred for STATUS reports
- * @entry: Queue entry to check.
- */
-static inline int rt2x00queue_status_timeout(struct queue_entry *entry)
-{
-	if (!test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
-		return false;
-	return time_after(jiffies, entry->last_action + msecs_to_jiffies(100));
-}
-
 /**
  * rt2x00queue_dma_timeout - Check if a timeout occurred for DMA transfers
  * @entry: Queue entry to check.
diff --git a/drivers/net/wireless/rt2x00/rt2x00usb.c b/drivers/net/wireless/rt2x00/rt2x00usb.c
index 2eea386..66094eb 100644
--- a/drivers/net/wireless/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/rt2x00/rt2x00usb.c
@@ -526,22 +526,6 @@ static void rt2x00usb_watchdog_tx_dma(struct data_queue *queue)
 	rt2x00queue_flush_queue(queue, true);
 }
 
-static void rt2x00usb_watchdog_tx_status(struct data_queue *queue)
-{
-	WARNING(queue->rt2x00dev, "TX queue %d status timed out,"
-		" invoke forced tx handler\n", queue->qid);
-
-	queue_work(queue->rt2x00dev->workqueue, &queue->rt2x00dev->txdone_work);
-}
-
-static int rt2x00usb_status_timeout(struct data_queue *queue)
-{
-	struct queue_entry *entry;
-
-	entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
-	return rt2x00queue_status_timeout(entry);
-}
-
 static int rt2x00usb_dma_timeout(struct data_queue *queue)
 {
 	struct queue_entry *entry;
@@ -558,8 +542,6 @@ void rt2x00usb_watchdog(struct rt2x00_dev *rt2x00dev)
 		if (!rt2x00queue_empty(queue)) {
 			if (rt2x00usb_dma_timeout(queue))
 				rt2x00usb_watchdog_tx_dma(queue);
-			if (rt2x00usb_status_timeout(queue))
-				rt2x00usb_watchdog_tx_status(queue);
 		}
 	}
 }
@@ -829,7 +811,8 @@ int rt2x00usb_probe(struct usb_interface *usb_intf,
 
 	INIT_WORK(&rt2x00dev->rxdone_work, rt2x00usb_work_rxdone);
 	INIT_WORK(&rt2x00dev->txdone_work, rt2x00usb_work_txdone);
-	init_timer(&rt2x00dev->txstatus_timer);
+	hrtimer_init(&rt2x00dev->txstatus_timer, CLOCK_MONOTONIC,
+		     HRTIMER_MODE_REL);
 
 	retval = rt2x00usb_alloc_reg(rt2x00dev);
 	if (retval)
-- 
1.7.1

Re: [rt2x00-users] [PATCH 3/5] rt2x00: rt2800usb: rework txstatus code

[Jakub Kicinski]

Hi,

I feel a bit guilty to comment on a patch you have first posted more
than a week ago and that have already been merged but to jump in with
patches seems even worse... Let's hope none of my points are valid ;-)

On Wed, 14 Mar 2012 11:16:19 +0100
Stanislaw Gruszka <sgruszka@redhat.com> wrote:

> Currently we read tx status register after each urb data transfer. As
> callback procedure also trigger reading, that causing we have many
> "threads" of reading status. To prevent that introduce TX_STATUS_READING
> flags, and check if we are already in process of sequential reading
> TX_STA_FIFO, before requesting new reads.
> 
> Change timer to hrtimer, that make TX_STA_FIFO overruns less possible.
> Use 200 us for initial timeout, and then reschedule in 100 us period,
> this values probably have to be tuned.
> 
> Make changes on txdone work. Schedule it from
> rt2800usb_tx_sta_fifo_read_completed() callback when first valid status
> show up. Check in callback if tx status timeout happens, and schedule
> work on that condition too. That make possible to remove tx status
> timeout from generic watchdog. I moved that to rt2800usb.

Does above mean that you want to check for timeouts in
rt2800usb_tx_sta_fifo_read_completed? You don't seem to be doing so.

> Loop in txdone work, that should prevent situation when we queue work,
> which is already processed, and after finish work is not rescheduled
> again.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c   |  120 +++++++++++++++++++++-------
>  drivers/net/wireless/rt2x00/rt2x00.h      |   10 ++-
>  drivers/net/wireless/rt2x00/rt2x00dev.c   |    2 +-
>  drivers/net/wireless/rt2x00/rt2x00queue.h |   12 ---
>  drivers/net/wireless/rt2x00/rt2x00usb.c   |   21 +-----
>  5 files changed, 101 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 4eaa628..eacf94b 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -114,45 +114,103 @@ static bool rt2800usb_txstatus_pending(struct rt2x00_dev *rt2x00dev)
>  	return false;
>  }
>  
> +static inline bool rt2800usb_entry_txstatus_timeout(struct queue_entry *entry)
> +{
> +	bool tout;
> +
> +	if (!test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
> +		return false;
> +
> +	tout = time_after(jiffies, entry->last_action + msecs_to_jiffies(100));
> +	if (unlikely(tout))
> +		WARNING(entry->queue->rt2x00dev,
> +			"TX status timeout for entry %d in queue %d\n",
> +			entry->entry_idx, entry->queue->qid);
> +	return tout;
> +
> +}
> +
> +static bool rt2800usb_txstatus_timeout(struct rt2x00_dev *rt2x00dev)
> +{
> +	struct data_queue *queue;
> +	struct queue_entry *entry;
> +
> +	tx_queue_for_each(rt2x00dev, queue) {
> +		entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
> +		if (rt2800usb_entry_txstatus_timeout(entry))
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static bool rt2800usb_tx_sta_fifo_read_completed(struct rt2x00_dev *rt2x00dev,
>  						 int urb_status, u32 tx_status)
>  {
> +	bool valid;
> +
>  	if (urb_status) {
> -		WARNING(rt2x00dev, "rt2x00usb_register_read_async failed: %d\n", urb_status);
> -		return false;
> +		WARNING(rt2x00dev, "TX status read failed %d\n", urb_status);
> +
> +		goto stop_reading;
>  	}
>  
> -	/* try to read all TX_STA_FIFO entries before scheduling txdone_work */
> -	if (rt2x00_get_field32(tx_status, TX_STA_FIFO_VALID)) {
> -		if (!kfifo_put(&rt2x00dev->txstatus_fifo, &tx_status)) {
> -			WARNING(rt2x00dev, "TX status FIFO overrun, "
> -				"drop tx status report.\n");
> -			queue_work(rt2x00dev->workqueue, &rt2x00dev->txdone_work);
> -		} else
> -			return true;
> -	} else if (!kfifo_is_empty(&rt2x00dev->txstatus_fifo)) {
> +	valid = rt2x00_get_field32(tx_status, TX_STA_FIFO_VALID);
> +	if (valid) {
> +		if (!kfifo_put(&rt2x00dev->txstatus_fifo, &tx_status))
> +			WARNING(rt2x00dev, "TX status FIFO overrun\n");
> +
>  		queue_work(rt2x00dev->workqueue, &rt2x00dev->txdone_work);
> +
> +		/* Reschedule urb to read TX status again instantly */
> +		return true;
>  	} else if (rt2800usb_txstatus_pending(rt2x00dev)) {
> -		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
> +		/* Read register after 250 us */
> +		hrtimer_start(&rt2x00dev->txstatus_timer, ktime_set(0, 250000),
> +			      HRTIMER_MODE_REL);
> +		return false;
>  	}
>  
> -	return false;
> +stop_reading:
> +	clear_bit(TX_STATUS_READING, &rt2x00dev->flags);
> +	/*
> +	 * There is small race window above, between txstatus pending check and
> +	 * clear_bit someone could do rt2x00usb_interrupt_txdone, so recheck
> +	 * here again if status reading is needed.
> +	 */
> +	if (rt2800usb_txstatus_pending(rt2x00dev) &&
> +	    test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))

I would put a bang before that test_and...

> +		return true;
> +	else
> +		return false;
> +}
> +
> +static void rt2800usb_async_read_tx_status(struct rt2x00_dev *rt2x00dev)
> +{
> +
> +	if (test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
> +		return;
> +
> +	/* Read TX_STA_FIFO register after 500 us */
> +	hrtimer_start(&rt2x00dev->txstatus_timer, ktime_set(0, 500000),
> +		      HRTIMER_MODE_REL);
>  }
>  
>  static void rt2800usb_tx_dma_done(struct queue_entry *entry)
>  {
>  	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
>  
> -	rt2x00usb_register_read_async(rt2x00dev, TX_STA_FIFO,
> -				      rt2800usb_tx_sta_fifo_read_completed);
> +	rt2800usb_async_read_tx_status(rt2x00dev);
>  }
>  
> -static void rt2800usb_tx_sta_fifo_timeout(unsigned long data)
> +static enum hrtimer_restart rt2800usb_tx_sta_fifo_timeout(struct hrtimer *timer)
>  {
> -	struct rt2x00_dev *rt2x00dev = (struct rt2x00_dev *)data;
> +	struct rt2x00_dev *rt2x00dev =
> +	    container_of(timer, struct rt2x00_dev, txstatus_timer);
>  
>  	rt2x00usb_register_read_async(rt2x00dev, TX_STA_FIFO,
>  				      rt2800usb_tx_sta_fifo_read_completed);
> +
> +	return HRTIMER_NORESTART;
>  }
>  
>  /*
> @@ -540,7 +598,7 @@ static void rt2800usb_txdone_nostatus(struct rt2x00_dev *rt2x00dev)
>  
>  			if (test_bit(ENTRY_DATA_IO_FAILED, &entry->flags))
>  				rt2x00lib_txdone_noinfo(entry, TXDONE_FAILURE);
> -			else if (rt2x00queue_status_timeout(entry))
> +			else if (rt2800usb_entry_txstatus_timeout(entry))
>  				rt2x00lib_txdone_noinfo(entry, TXDONE_UNKNOWN);
>  			else
>  				break;
> @@ -553,17 +611,21 @@ static void rt2800usb_work_txdone(struct work_struct *work)
>  	struct rt2x00_dev *rt2x00dev =
>  	    container_of(work, struct rt2x00_dev, txdone_work);
>  
> -	rt2800usb_txdone(rt2x00dev);
> +	while (!kfifo_is_empty(&rt2x00dev->txstatus_fifo) ||
> +	       rt2800usb_txstatus_timeout(rt2x00dev)) {
>  
> -	rt2800usb_txdone_nostatus(rt2x00dev);
> +		rt2800usb_txdone(rt2x00dev);
>  
> -	/*
> -	 * The hw may delay sending the packet after DMA complete
> -	 * if the medium is busy, thus the TX_STA_FIFO entry is
> -	 * also delayed -> use a timer to retrieve it.
> -	 */
> -	if (rt2800usb_txstatus_pending(rt2x00dev))
> -		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
> +		rt2800usb_txdone_nostatus(rt2x00dev);
> +
> +		/*
> +		 * The hw may delay sending the packet after DMA complete
> +		 * if the medium is busy, thus the TX_STA_FIFO entry is
> +		 * also delayed -> use a timer to retrieve it.
> +		 */
> +		if (rt2800usb_txstatus_pending(rt2x00dev))
> +			rt2800usb_async_read_tx_status(rt2x00dev);

How is it possible that this call will ever start the timer? The
reading "thread" won't exit if rt2800usb_txstatus_pending returns true
and every dma_done will schedule reading itself.

  -- Kuba

Re: [rt2x00-users] [PATCH 3/5] rt2x00: rt2800usb: rework txstatus code

[Stanislaw Gruszka]

On Sat, Mar 17, 2012 at 05:53:11PM +0100, Jakub Kicinski wrote:
> I feel a bit guilty to comment on a patch you have first posted more
> than a week ago and that have already been merged but to jump in with
No problems, it's never too late for code review :-)

> > Make changes on txdone work. Schedule it from
> > rt2800usb_tx_sta_fifo_read_completed() callback when first valid status
> > show up. Check in callback if tx status timeout happens, and schedule
> > work on that condition too. That make possible to remove tx status
> > timeout from generic watchdog. I moved that to rt2800usb.
> 
> Does above mean that you want to check for timeouts in
> rt2800usb_tx_sta_fifo_read_completed? You don't seem to be doing so.
Good catch, I'll post fix shortly.

> > +	if (rt2800usb_txstatus_pending(rt2x00dev) &&
> > +	    test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
> 
> I would put a bang before that test_and...
I don't understand what you mean, perhaps you could post a patch
or provide code snipset here, so I could comment.
 
> > +	while (!kfifo_is_empty(&rt2x00dev->txstatus_fifo) ||
> > +	       rt2800usb_txstatus_timeout(rt2x00dev)) {
> >  
> > -	rt2800usb_txdone_nostatus(rt2x00dev);
> > +		rt2800usb_txdone(rt2x00dev);
> >  
> > -	/*
> > -	 * The hw may delay sending the packet after DMA complete
> > -	 * if the medium is busy, thus the TX_STA_FIFO entry is
> > -	 * also delayed -> use a timer to retrieve it.
> > -	 */
> > -	if (rt2800usb_txstatus_pending(rt2x00dev))
> > -		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
> > +		rt2800usb_txdone_nostatus(rt2x00dev);
> > +
> > +		/*
> > +		 * The hw may delay sending the packet after DMA complete
> > +		 * if the medium is busy, thus the TX_STA_FIFO entry is
> > +		 * also delayed -> use a timer to retrieve it.
> > +		 */
> > +		if (rt2800usb_txstatus_pending(rt2x00dev))
> > +			rt2800usb_async_read_tx_status(rt2x00dev);
> 
> How is it possible that this call will ever start the timer? The
> reading "thread" won't exit if rt2800usb_txstatus_pending returns true
> and every dma_done will schedule reading itself.

I do not understand your objection here too. If rt2800usb_txstatus_pending()
will return true and if TX_STATUS_READING bit is not set, we will run hrtimer
to read status after 500 micro seconds. We exit the loop if kfifo is empty
and no entry timed out waiting to get corresponding TX status.

Thanks
Stanislaw

Re: [rt2x00-users] [PATCH 3/5] rt2x00: rt2800usb: rework txstatus code

[Jakub Kicinski]

Hi!

On Mon, 19 Mar 2012 08:52:24 +0100
Stanislaw Gruszka <sgruszka@redhat.com> wrote:

> > > +	if (rt2800usb_txstatus_pending(rt2x00dev) &&
> > > +	    test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
> > 
> > I would put a bang before that test_and...
> I don't understand what you mean, perhaps you could post a patch
> or provide code snipset here, so I could comment.

If I understand correctly, status should be read again if there are
pending entries and no one else has set TX_STATUS_READING yet. In that
case return value of test_and_set_bit should be negated. I might be
missing something though.

> > > +	while (!kfifo_is_empty(&rt2x00dev->txstatus_fifo) ||
> > > +	       rt2800usb_txstatus_timeout(rt2x00dev)) {
> > >  
> > > -	rt2800usb_txdone_nostatus(rt2x00dev);
> > > +		rt2800usb_txdone(rt2x00dev);
> > >  
> > > -	/*
> > > -	 * The hw may delay sending the packet after DMA complete
> > > -	 * if the medium is busy, thus the TX_STA_FIFO entry is
> > > -	 * also delayed -> use a timer to retrieve it.
> > > -	 */
> > > -	if (rt2800usb_txstatus_pending(rt2x00dev))
> > > -		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
> > > +		rt2800usb_txdone_nostatus(rt2x00dev);
> > > +
> > > +		/*
> > > +		 * The hw may delay sending the packet after DMA complete
> > > +		 * if the medium is busy, thus the TX_STA_FIFO entry is
> > > +		 * also delayed -> use a timer to retrieve it.
> > > +		 */
> > > +		if (rt2800usb_txstatus_pending(rt2x00dev))
> > > +			rt2800usb_async_read_tx_status(rt2x00dev);
> > 
> > How is it possible that this call will ever start the timer? The
> > reading "thread" won't exit if rt2800usb_txstatus_pending returns true
> > and every dma_done will schedule reading itself.
> 
> I do not understand your objection here too. If rt2800usb_txstatus_pending()
> will return true and if TX_STATUS_READING bit is not set, we will run hrtimer
> to read status after 500 micro seconds. We exit the loop if kfifo is empty
> and no entry timed out waiting to get corresponding TX status.

Yes, I don't mean that this code is wrong. I just think that
rt2800usb_async_read_tx_status have no chance of actually going past
TX_STATUS_READING check. If every dma_done schedules reading and
reading stops only when all pending entries have their statuses then
call to rt2800usb_async_read_tx_status after we processed statuses is
excessive.

All that said, I haven't tested this hypothesis and may be completely
wrong (again). Also I _don't_ mean that this call should be removed,
just wanted to me sure I understand everything correctly ;-)

  -- Kuba

Re: [rt2x00-users] [PATCH 3/5] rt2x00: rt2800usb: rework txstatus code

[Stanislaw Gruszka]

On Mon, Mar 19, 2012 at 02:13:39PM +0100, Jakub Kicinski wrote:
> > > > +	if (rt2800usb_txstatus_pending(rt2x00dev) &&
> > > > +	    test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
> > > 
> > > I would put a bang before that test_and...
> > I don't understand what you mean, perhaps you could post a patch
> > or provide code snipset here, so I could comment.
> 
> If I understand correctly, status should be read again if there are
> pending entries and no one else has set TX_STATUS_READING yet. In that
> case return value of test_and_set_bit should be negated. I might be
> missing something though.

You are correct, this is another good catch! Fix is on the go.

> > I do not understand your objection here too. If rt2800usb_txstatus_pending()
> > will return true and if TX_STATUS_READING bit is not set, we will run hrtimer
> > to read status after 500 micro seconds. We exit the loop if kfifo is empty
> > and no entry timed out waiting to get corresponding TX status.
> 
> Yes, I don't mean that this code is wrong. I just think that
> rt2800usb_async_read_tx_status have no chance of actually going past
> TX_STATUS_READING check. If every dma_done schedules reading and
> reading stops only when all pending entries have their statuses then
> call to rt2800usb_async_read_tx_status after we processed statuses is
> excessive.
> 
> All that said, I haven't tested this hypothesis and may be completely
> wrong (again). Also I _don't_ mean that this call should be removed,
> just wanted to me sure I understand everything correctly ;-)

I think you have right. I'll review that carefully and remove those
lines if they are useless.

Thanks
Stanislaw

[PATCH 4/5] rt2x00: rt2800usb: do not check packedid for aggregated frames

[Stanislaw Gruszka]

Tx statuses of aggregated subframes contain packetid of first subframe
in the AMPDU. We can not identify AMPDU subframes based on packedid, so
simply assume that status match first pending frame in the queue. Thats
mostly the same what 2800pci do.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>
Acked-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index eacf94b..8c1d2c0 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -502,7 +502,7 @@ rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
 	__le32 *txwi;
 	u32 word;
 	int wcid, ack, pid;
-	int tx_wcid, tx_ack, tx_pid;
+	int tx_wcid, tx_ack, tx_pid, is_agg;
 
 	/*
 	 * This frames has returned with an IO error,
@@ -515,6 +515,7 @@ rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
 	wcid	= rt2x00_get_field32(reg, TX_STA_FIFO_WCID);
 	ack	= rt2x00_get_field32(reg, TX_STA_FIFO_TX_ACK_REQUIRED);
 	pid	= rt2x00_get_field32(reg, TX_STA_FIFO_PID_TYPE);
+	is_agg	= rt2x00_get_field32(reg, TX_STA_FIFO_TX_AGGRE);
 
 	/*
 	 * Validate if this TX status report is intended for
@@ -527,7 +528,7 @@ rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
 	tx_ack  = rt2x00_get_field32(word, TXWI_W1_ACK);
 	tx_pid  = rt2x00_get_field32(word, TXWI_W1_PACKETID);
 
-	if ((wcid != tx_wcid) || (ack != tx_ack) || (pid != tx_pid)) {
+	if (wcid != tx_wcid || ack != tx_ack || (!is_agg && pid != tx_pid)) {
 		WARNING(entry->queue->rt2x00dev,
 			"TX status report missed for queue %d entry %d\n",
 			entry->queue->qid, entry->entry_idx);
-- 
1.7.1

[PATCH 5/5] rt2x00: rt2800usb: limit tx queues length

[Stanislaw Gruszka]

TX status fifo is limited to 16 elements. When we send more frames than
that, we can easily loose status, what is not good for rate scaling
algorithm.

On my testing the change does not degrade performance, actually make
is slightly better. Additionally with the patch I can see much less
various rt2x00 warnings in dmesg.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>
Acked-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 8c1d2c0..cd490ab 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -870,7 +870,7 @@ static const struct data_queue_desc rt2800usb_queue_rx = {
 };
 
 static const struct data_queue_desc rt2800usb_queue_tx = {
-	.entry_num		= 64,
+	.entry_num		= 16,
 	.data_size		= AGGREGATION_SIZE,
 	.desc_size		= TXINFO_DESC_SIZE + TXWI_DESC_SIZE,
 	.priv_size		= sizeof(struct queue_entry_priv_usb),
-- 
1.7.1