[rt2x00-users] [RFC] rt2x00: Implement tx mpdu aggregation

[Helmut Schaa]

Am Donnerstag 10 Juni 2010 schrieb Gertjan van Wingerde:
> On 06/09/10 17:17, Helmut Schaa wrote:
> > In order to implement tx mpdu aggregation we only have to implement
> > the ampdu_action callback such that mac80211 allows negotiation of
> > blockack sessions.
> > 
> > The hardware will handle everything on its own as long as the ampdu
> > flag in the TXWI struct is set up correctly and we translate the tx
> > status correctly.
> > 
> > For now, refuse requests to start rx aggregation.
> > 
> > Signed-off-by: Helmut Schaa <helmut.schaa at googlemail.com>
> > ---
> > 
> > This gets me a 10% boost in throughput when actually using tx aggregation ;)
> > but needs the RFC I sent yesterday.
> > 
> > I didn't try if failed frames get handled correctly but that should be
> > done in hw as well.
> > 
> > And I'm not sure if this works on usb devices. So maybe we should enable
> > it only on pci and soc?
> > 
> > Helmut
> > 
> >  drivers/net/wireless/rt2x00/rt2800lib.c |   34 ++++++++++++++++++++++++++++++-
> >  drivers/net/wireless/rt2x00/rt2x00dev.c |   10 +++++++++
> >  2 files changed, 43 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> > index 16c3ae8..161ff0f 100644
> > --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> > +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> > @@ -2490,7 +2490,8 @@ int rt2800_probe_hw_mode(struct rt2x00_dev *rt2x00dev)
> >  	    IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING |
> >  	    IEEE80211_HW_SIGNAL_DBM |
> >  	    IEEE80211_HW_SUPPORTS_PS |
> > -	    IEEE80211_HW_PS_NULLFUNC_STACK;
> > +	    IEEE80211_HW_PS_NULLFUNC_STACK |
> > +	    IEEE80211_HW_AMPDU_AGGREGATION;
> >  
> >  	SET_IEEE80211_DEV(rt2x00dev->hw, rt2x00dev->dev);
> >  	SET_IEEE80211_PERM_ADDR(rt2x00dev->hw,
> > @@ -2742,6 +2743,36 @@ static u64 rt2800_get_tsf(struct ieee80211_hw *hw)
> >  	return tsf;
> >  }
> >  
> > +static int rt2800_ampdu_action(struct ieee80211_hw *hw,
> > +			       struct ieee80211_vif *vif,
> > +			       enum ieee80211_ampdu_mlme_action action,
> > +			       struct ieee80211_sta *sta,
> > +			       u16 tid, u16 *ssn)
> > +{
> > +	struct rt2x00_dev *rt2x00dev = hw->priv;
> > +	int ret = 0;
> > +
> > +	/* we don't support RX aggregation yet */
> > +	switch (action) {
> > +	case IEEE80211_AMPDU_RX_START:
> > +	case IEEE80211_AMPDU_RX_STOP:
> > +		ret = -ENOTSUPP;
> > +		break;
> > +	case IEEE80211_AMPDU_TX_START:
> > +		ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
> > +		break;
> > +	case IEEE80211_AMPDU_TX_STOP:
> > +		ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
> > +		break;
> > +	case IEEE80211_AMPDU_TX_OPERATIONAL:
> > +		break;
> > +	default:
> > +		WARNING(rt2x00dev, "Unknown AMPDU action\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  const struct ieee80211_ops rt2800_mac80211_ops = {
> >  	.tx			= rt2x00mac_tx,
> >  	.start			= rt2x00mac_start,
> > @@ -2759,5 +2790,6 @@ const struct ieee80211_ops rt2800_mac80211_ops = {
> >  	.conf_tx		= rt2800_conf_tx,
> >  	.get_tsf		= rt2800_get_tsf,
> >  	.rfkill_poll		= rt2x00mac_rfkill_poll,
> > +	.ampdu_action		= rt2800_ampdu_action,
> >  };
> >  EXPORT_SYMBOL_GPL(rt2800_mac80211_ops);
> > diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> > index 826cab2..856ea1e 100644
> > --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> > +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> > @@ -286,6 +286,16 @@ void rt2x00lib_txdone(struct queue_entry *entry,
> >  			rt2x00dev->low_level_stats.dot11ACKFailureCount++;
> >  	}
> >  
> > +	/*
> > +	 * Every single frame has it's own tx status, hence report
> > +	 * every frame as ampdu of size 1
> > +	 */
> > +	if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
> > +		tx_info->flags |= IEEE80211_TX_STAT_AMPDU;
> > +		tx_info->status.ampdu_len = 1;
> > +		tx_info->status.ampdu_ack_len = success ? 1 : 0;
> > +	}
> > +
> >  	if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) {
> >  		if (success)
> >  			rt2x00dev->low_level_stats.dot11RTSSuccessCount++;
> 
> This last bit looks a bit strange. AFAICT all other drivers put in here some specifics about the AMPDU
> (including number of frames/fragments or something like that). Are we sure these are the correct values
> to report?

Ok, rechecked today. We really get one tx status (from TX_STA_FIFO) per MPDU
(not per AMPDU) and hence the tx status is on a MPDU base and not on an AMPDU
base.

Since the aggregation is completely done in hw including resending of failed
MPDUs we don't even know which frames were aggregated together. We only know
if a MPDU was aggregated or not.

So, we basically tell the device only "this frame is eligible" for an AMPDU
and the hw will decide on everything else.

So, the only possibility to report the AMPDU status is to report every single
MPDU as AMPDU if len 1.

Hope thie clarifies my intentions ;)

I'm going to update the comment in the tx status processing to be more precise.

Helmut