Re: [PATCH 3/8] mac80211: A-MPDU Rx adding basic functionality

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 3987 bytes --]


> +/* 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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]