On 8 July 2014 09:10, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> On Fri, 2014-06-27 at 11:06 +0200, Michal Kazior wrote:
>
>> +/**
>> + * 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.
>> + * @start_seq_num: starting frame sequence number
>> + * @tid: the rx tid
>
>> + * @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.
>> +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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
