> +/* BACK parties */
Please write out "block-ack" or "BACK (block-ack)" :)
> +#define IEEE80211_MIN_AMPDU_BUF 0x8
> +#define IEEE80211_MAX_AMPDU_BUF 0x40
Any comments on what these represent? Should they be tunable?
> + if (tid_agg_rx->state != HT_AGG_STATE_IDLE) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> + if (net_ratelimit())
> + printk(KERN_DEBUG "unexpected Block Ack Req from "
> + "%s on tid %u\n",
> + print_mac(mac, mgmt->sa), tid);
> +#endif /* CONFIG_MAC80211_HT_DEBUG */
> + goto end;
> + }
> +
> + /* prepare reordering buffer */
> + tid_agg_rx->reorder_buf =
> + kmalloc(buf_size * sizeof(struct sk_buf *), GFP_ATOMIC);
> + if (!tid_agg_rx->reorder_buf) {
> + printk(KERN_ERR "can not allocate reordering buffer "
> + "to tid %d\n", tid);
> + tid_agg_rx->state = HT_AGG_STATE_IDLE;
That's unnecessary, you can't get here w/ state != IDLE afaict.
> + goto end;
> + }
> + memset(tid_agg_rx->reorder_buf, 0,
> + buf_size * sizeof(struct sk_buf *));
> +
> + if (local->ops->ampdu_action)
> + ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_RX_START,
> + sta->addr, tid, start_seq_num);
> + if (ret) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> + if (net_ratelimit())
> + printk(KERN_DEBUG "A-MPDU on tid %d result: %s", tid,
> + (ret == -EOPNOTSUPP)? "Not Supported": "Driver Error");
I'm not sure we need to ratelimit debug messages, and I think we should
print out the error code. People who are writing a driver will need to
decipher the number but it's an error they return so I think we should
give them a chance to see which error path was hit.
> +void ieee80211_send_delba(struct net_device *dev, const u8 *da, u16 tid,
> + u16 initiator, u16 reason_code)
> +{
> + struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> + struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> + struct ieee80211_if_sta *ifsta = &sdata->u.sta;
> + struct sk_buff *skb;
> + struct ieee80211_mgmt *mgmt;
> + u16 params;
> +
> + skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom + 1 +
> + sizeof(mgmt->u.action.u.delba));
> +
> + if (!skb) {
> + printk(KERN_ERR "%s: failed to allocate buffer "
> + "for delba frame\n", dev->name);
Do we send this as a reply to anything, ie. should it be rate-limited
too?
> +void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
> + u16 initiator, u16 reason)
> +{
> + /* check if the TID is in opertional state */
small typo
> + /* stop HW Rx aggregation */
> + if (local->ops->ampdu_action) {
Can we get into that path with ops->ampdu_action == NULL? I'd think not
because state is required to be active... I'd rather stick in a
BUG_ON(!local->ops->ampdu_action)
because it seems to me that'd be a mac80211 bug.
> + * After receiving Block Ack Request (BAR) we activated a
> + * timer after each frame arrives from the originator.
> + * if this timer expires ieee80211_sta_stop_rx_BA_session will be executed.
> + */
> +void sta_rx_agg_session_timer_expired(unsigned long data)
> +{
> + /* not an elegant detour, but there is no choice as the timer passes
> + * only one argument, and ieee80211_local is needed here */
> + int *ptid = (int *)data;
> + int *timer_to_id = ptid - *ptid;
> + struct sta_info *temp_sta = container_of(timer_to_id, struct sta_info,
> + timer_to_tid[0]);
I think this needs more comments. I can, after a while, see what it
does, but I'm not even sure it's correct. The whole timer_to_id thing is
only for this code?
> + struct ieee80211_local *local = temp_sta->local;
> + struct sta_info *sta;
> + u16 tid = (u16)*ptid;
> + sta = sta_info_get(local, temp_sta->addr);
Missing newline.
> + if (!sta)
> + return;
Why do you even do a sta_info_get() on the temp_sta's addr? Either you
already have a good pointer or the whole thing will access invalid
memory.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
