On 8 July 2014 09:10, Johannes Berg [off-list ref] wrote:

On Fri, 2014-06-27 at 11:06 +0200, Michal Kazior wrote:

quoted

+/**
+ * ieee80211_start_rx_ba_session_offl - start a Rx BA session
+ *
+ * Some device drivers may offload part of the Rx aggregation flow including
+ * AddBa/DelBa negotiation but may otherwise be incapable of full Rx
+ * reordering.
+ *
+ * Create structures responsible for reordering so device drivers may call here
+ * when they complete AddBa negotiation.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback
+ * @addr: station mac address
+ * @dialog_token:
+ * @timeout: session timeout (in TU)

Why would you need the dialog token (and why no docs?) and timeout?

Good point. I'll remove both.

quoted

+ * @start_seq_num: starting frame sequence number
+ * @tid: the rx tid

quoted

+ * @buf_size: max number of frames in reorder buffer

The buf_size also isn't really needed, is it? We are allowed to use a
bigger buffer, I believe?

Good point. I'll use the max buffer size macro.

quoted

+void ieee80211_start_rx_ba_session_offl(struct ieee80211_vif *vif,
+                                     const u8 *addr, u8 dialog_token,
+                                     u16 timeout, u16 start_seq_num,
+                                     u16 tid, u16 buf_size)
+{
+     struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+     struct ieee80211_local *local = sdata->local;
+     struct ieee80211_rx_agg *rx_agg;
+     struct sk_buff *skb = dev_alloc_skb(0);
+
+     if (unlikely(!skb))
+             return;
+
+     rx_agg = (struct ieee80211_rx_agg *) &skb->cb;
+     memcpy(&rx_agg->addr, addr, ETH_ALEN);
+     rx_agg->dialog_token = dialog_token;
+     rx_agg->timeout = timeout;
+     rx_agg->start_seq_num = start_seq_num;
+     rx_agg->ba_policy = 1;
+     rx_agg->tid = tid;
+     rx_agg->buf_size = buf_size;
+
+     skb->pkt_type = IEEE80211_SDATA_QUEUE_RX_AGG_START;
+     skb_queue_tail(&sdata->skb_queue, skb);
+     ieee80211_queue_work(&local->hw, &sdata->work);

This seems problematic, since packets might be received immediately.

On the teardown path it should probably also be invalidated immediately,
no?

Then again, we have the same problem already? Hmm.

Yeah. You have to go through the iface worker so you can race with
data frames coming in in ieee80211_rx() either way.

You'd have to make ampdu_action() atomic, replace sta->ampdu_mlme.mtx
with a spinlock and move some del_timer_sync() around - at least
that's what I've found after taking a quick look.


Michał