[rt2x00-users] [RFC 2/2] rt2x00: Fix tx status reporting for reordered frames in rt2800pci

[Gertjan van Wingerde]

On 03/29/11 14:02, Helmut Schaa wrote:
> rt2800 hardware sometimes reorders tx frames when transmitting to
> multiple BA enabled STAs concurrently.
>
> For example a tx queue
> 	[ STA1 | STA2 | STA1 | STA2 ]
> can result in the tx status reports
> 	[ STA1 | STA1 | STA2 | STA2 ]
> when the hw decides to put the frames for STA1 in one AMPDU.
>
> To mitigate this effect associate the currently processed tx status
> to the first frame in the tx queue with a matching wcid.
>
> rt2800usb might suffer from the same issue but since the tx status
> processing in rt2800usb is still not completely stable in regard to
> losing tx status reports from time to time don't apply this workaround
> there.
>
> This patch fixes several problems related to incorrect tx status
> reporting. Furthermore the tx rate selection is much more stable when
> communicating with multiple STAs.
>
> Signed-off-by: Helmut Schaa <helmut.schaa at googlemail.com>
> ---
>
> Wow, this is ugly, isn't it ;)
>
> However, it really improves rate selection considerable and also fixes the
> aggregation issue I've noticed with Intel 5100 Windows STAs. Hence, I'd
> really like to submit this patch.
>
> I don't know but the "u32 status" in the queue entry is not really nice.
> Maybe I should introduce a special rt2800pci entry_priv structure that
> contains the rt2x00pci entry_priv structure + the tx status?
>
> Also, this further increases the tx status processing gap between rt2800pci
> and rt2800usb but since rt2800usb is still not as mature as rt2800pci in this
> regard I would not see this as a reason for not adding this fix for rt2800pci.
>
> Maybe it would make sense to turn TX aggregation off in rt2800usb as long as
> we know that there are still issues left?
>
> Any other comments?

Hmm, to be honest I have my doubts whether this is a step in the right direction.
Basically what this patch does is increase our dependency on all TX statuses
being reported by the hardware. With this new code we can get "holes" in the
queue with same entries still pending status and others (later in the queue) already
having their status reported.
Granted, the current code doesn't handle that fact as well, but at least it will keep
the queue organized (actually I guess we are reporting the status of the wrong frame
if it happens that we miss the TX status of a frame).

I guess your testing does show that we need to be able to handle out-of-order
status reports, but before we do so we better make very sure that a missing TX status
cannot cause issues.

With this patch I can see the reports about the following error pop up:
    Arrived at non-free entry in the non-full queue %d.

See below for some more comments on the patch itself.

>
> Thanks,
> Helmut
>  
>  drivers/net/wireless/rt2x00/rt2800pci.c   |   81 +++++++++++++++++++++++++++-
>  drivers/net/wireless/rt2x00/rt2x00queue.h |    4 ++
>  2 files changed, 82 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
> index 59316e9..0be8744 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -717,10 +717,68 @@ static void rt2800pci_wakeup(struct rt2x00_dev *rt2x00dev)
>  	rt2800_config(rt2x00dev, &libconf, IEEE80211_CONF_CHANGE_PS);
>  }
>  
> +static bool rt2800pci_txdone_entry_check(struct queue_entry *entry, u32 status)
> +{
> +	__le32 *txwi;
> +	u32 word;
> +	int wcid, tx_wcid;
> +
> +	wcid = rt2x00_get_field32(status, TX_STA_FIFO_WCID);
> +
> +	txwi = rt2800_drv_get_txwi(entry);
> +	rt2x00_desc_read(txwi, 1, &word);
> +	tx_wcid = rt2x00_get_field32(word, TXWI_W1_WIRELESS_CLI_ID);
> +
> +	return (tx_wcid == wcid);
> +}

Is there any reason why you cannot use the already existing rt2800_txdone_entry_check
function?
That one is already used by rt2800usb and checks a bit more, but these checks are
equally valid on rt2800pci.

Using that function would help keeping rt2800pci and rt2800usb closer together and
would allow for easier code sharing later on.

> +
> +static bool rt2800pci_txdone_find_entry(struct queue_entry *entry, void *data)
> +{
> +	u32 status = *(u32 *)data;
> +
> +	/*
> +	 * rt2800pci hardware might reorder frames when exchanging traffic
> +	 * with multiple BA enabled STAs.
> +	 *
> +	 * For example, a tx queue
> +	 *    [ STA1 | STA2 | STA1 | STA2 ]
> +	 * can result in tx status reports
> +	 *    [ STA1 | STA1 | STA2 | STA2 ]
> +	 * when the hw decides to aggregate the frames for STA1 into one AMPDU.
> +	 *
> +	 * To mitigate this effect, associate the tx status to the first frame
> +	 * in the tx queue with a matching wcid.
> +	 */
> +	if (rt2800pci_txdone_entry_check(entry, status) &&
> +	    !test_bit(ENTRY_DATA_STATUS_SET, &entry->flags)) {
> +		/*
> +		 * Got a matching frame, associate the tx status with
> +		 * the frame
> +		 */
> +		entry->status = status;
> +		set_bit(ENTRY_DATA_STATUS_SET, &entry->flags);
> +		return true;
> +	}
> +
> +	/* Check the next frame */
> +	return false;
> +}
> +
> +static bool rt2800pci_txdone_release_entries(struct queue_entry *entry,
> +					     void *data)
> +{
> +	if (test_bit(ENTRY_DATA_STATUS_SET, &entry->flags)) {
> +		rt2800_txdone_entry(entry, entry->status);
> +		return false;
> +	}
> +
> +	/* No more frames to release */
> +	return true;
> +}
> +

Question: Do we have report statuses in the same order as the frames
we received from mac80211?
If we can report statuses out-of-order this can be simplified by simply
reporting the status inrt2800pci_txdone_find_entry and setting a flag
there that the status is already reported. That way we don't need
the ugly status field in the queue entry and we may not even need the
new flag as I believe there is already another mechanism that indicates
whether the slot is "occupied".

>  static bool rt2800pci_txdone(struct rt2x00_dev *rt2x00dev)
>  {
>  	struct data_queue *queue;
> -	struct queue_entry *entry;
>  	u32 status;
>  	u8 qid;
>  	int max_tx_done = 16;
> @@ -758,8 +816,25 @@ static bool rt2800pci_txdone(struct rt2x00_dev *rt2x00dev)
>  			break;
>  		}
>  
> -		entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
> -		rt2800_txdone_entry(entry, status);
> +		/*
> +		 * Let's associate this tx status with the first
> +		 * matching frame.
> +		 */
> +		if (!rt2x00queue_for_each_entry(queue, Q_INDEX_DONE,
> +						Q_INDEX, &status,
> +						rt2800pci_txdone_find_entry)) {
> +			WARNING(rt2x00dev, "No matching frame found for TX "
> +					   "status on queue %u, dropping\n",
> +					   qid);
> +			break;
> +		}
> +
> +		/*
> +		 * Release all frames with a valid tx status.
> +		 */
> +		rt2x00queue_for_each_entry(queue, Q_INDEX_DONE,
> +					   Q_INDEX, NULL,
> +					   rt2800pci_txdone_release_entries);
>  
>  		if (--max_tx_done == 0)
>  			break;
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h
> index 6b66452..26aec0a 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.h
> @@ -358,6 +358,7 @@ enum queue_entry_flags {
>  	ENTRY_DATA_PENDING,
>  	ENTRY_DATA_IO_FAILED,
>  	ENTRY_DATA_STATUS_PENDING,
> +	ENTRY_DATA_STATUS_SET,
>  };
>  
>  /**
> @@ -370,6 +371,7 @@ enum queue_entry_flags {
>   * @entry_idx: The entry index number.
>   * @priv_data: Private data belonging to this queue entry. The pointer
>   *	points to data specific to a particular driver and queue type.
> + * @status: Device specific status
>   */
>  struct queue_entry {
>  	unsigned long flags;
> @@ -380,6 +382,8 @@ struct queue_entry {
>  
>  	unsigned int entry_idx;
>  
> +	u32 status;
> +
>  	void *priv_data;
>  };
>  


-- 
---
Gertjan