Hi, On 01/18/2013 10:18 PM, Johannes Berg wrote:
I was going to apply this, but then I still had a few questions.+++ b/net/mac80211/mesh_ps.c @@ -0,0 +1,605 @@ +/* + * Copyright 2012, Marco Porsch <marco.porsch@xxxxxxxxxxxxxxxxxxxx> + * Copyright 2012, cozybit Inc.First of all, do you want 2013 now maybe? Or 2012-2013 or something?
Yeah, right. Happy new years! ;) I'll make it 2012-2013.
+static void mpsp_qos_null_append(struct sta_info *sta, + struct sk_buff_head *frames) +{ + struct ieee80211_sub_if_data *sdata = sta->sdata; + struct sk_buff *new_skb, *skb = skb_peek_tail(frames); + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; + struct ieee80211_tx_info *info; + + if (ieee80211_is_data_qos(hdr->frame_control)) + return; + + new_skb = mps_qos_null_get(sta); + if (!new_skb) + return; + + mps_dbg(sdata, "appending QoS Null in MPSP towards %pM\n", + sta->sta.addr); + + /* should be transmitted last -> lowest priority */ + new_skb->priority = 1; + skb_set_queue_mapping(new_skb, IEEE80211_AC_BK);That comment might do with exanding a bit? Without more explanation, transmission order doesn't really require BK -- except that this is done after potentially releasing multiple frames on different ACs, so it has to be BK to not pass any released BK frames. It's reasonable if you look at the (only) caller though, so I'm not worried about it.
/* * This frame has to be transmitted last. Assign lowest priority to * make sure it cannot pass other frames when releasing multiple ACs. */I am not sure if this really correct. Isn't it that all ACs are served in parallel and thus, a BK frame could be transmitted earlier than the last of multiple BE/VI/VO frames? On the other hand, this code has worked pretty reliably so far...
+ /* prepare collected frames for transmission */ + skb_queue_walk(&frames, skb) { + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); + struct ieee80211_hdr *hdr = (void *) skb->data; + + /* + * Tell TX path to send this frame even though the + * STA may still remain is PS mode after this frame + * exchange. + */ + info->flags |= IEEE80211_TX_CTL_NO_PS_BUFFER; + + if (more_data || !skb_queue_is_last(&frames, skb)) + hdr->frame_control |= + cpu_to_le16(IEEE80211_FCTL_MOREDATA); + else + hdr->frame_control &= + cpu_to_le16(~IEEE80211_FCTL_MOREDATA); + + if (skb_queue_is_last(&frames, skb) && + ieee80211_is_data_qos(hdr->frame_control)) { + u8 *qoshdr = ieee80211_get_qos_ctl(hdr); + + /* MPSP trigger frame ends service period */ + *qoshdr |= IEEE80211_QOS_CTL_EOSP; + info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; + } + }Ultimately, this is where you should also call drv_allow_buffered_frames() and/or driver_release_buffered_frames(). In fact, the *driver* might be buffering frames, so that would be necessary for proper A-MPDU operation etc. I can merge it anyway, but be aware that it is not implementing the full API correctly, so once more drivers get fixed to rely on that API (really is a fix, especially with A-MPDU) you won't have much fun. But you said it doesn't really work with 11n anyway, so ... :)
Adding driver_allow_buffered_frames and a frame release reason for MPSPs may be pretty straightforward. But I cannot foresee how it is going to work for driver_release_buffered_frames on the driver side.
I guess eventually this whole MPSP frame release should be merged into (and bloat) ieee80211_sta_ps_deliver_response. I would really like to postpone that hoping that others may pick it up once mesh PS is upstream in a proof of concept fashion.
@@ -997,6 +1006,8 @@ static void clear_sta_ps_flags(void *_sta) if (sdata->vif.type == NL80211_IFTYPE_AP || sdata->vif.type == NL80211_IFTYPE_AP_VLAN) ps = &sdata->bss->ps; + else if (ieee80211_vif_is_mesh(&sdata->vif)) + ps = &sdata->u.mesh.ps;Will this even compile with mesh turned off? It seems you should have a helper for "&sdata->u.mesh.ps" or something, so you can have just a single ifdef (you have a few places like this), maybe something like this: void *__force_mesh_link_error(void); static inline struct ... ieee80211_get_mesh_ps(sdata) { #ifdef MESH return ...; #else return __force_mesh_link_error(); #endif }@@ -329,6 +329,8 @@ static void purge_old_ps_buffers(struct ieee80211_local *local) if (sdata->vif.type == NL80211_IFTYPE_AP) ps = &sdata->u.ap.ps; + else if (ieee80211_vif_is_mesh(&sdata->vif)) + ps = &sdata->u.mesh.ps;For example here. I'd hate to see ifdefs in all those places.
Isn't that what ieee80211_vif_is_mesh is for? Everything compiled fine with CONFIG_MAC80211_MESH turned off.
static inline bool ieee80211_vif_is_mesh(struct ieee80211_vif *vif)
{
#ifdef CONFIG_MAC80211_MESH
return vif->type == NL80211_IFTYPE_MESH_POINT;
#endif
return false;
}
2 + 3 + /* DS params */ + 256 + /* TIM IE */If wonder if that should be 2 + 255? But I haven't looked up the TIM IE format again now ...
I did:IEEE802.11-2012 8.4.2.7 says the size is 5 + [1...251]. ieee80211_beacon_get_tim also uses 256 for AP mode correctly.
Regards, --Marco -- 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
