On Mon, 2012-10-08 at 11:07 -0700, Marco Porsch wrote:
> This patch prepares mac80211 for a later implementation of mesh or ad-hoc
> powersave.
I think you should mention "powersave clients" in the subject, rather
than just "powersave", since that could also mean "as a client"
> The structures related to powersave (buffer, TIM map, counters) are moved
> from the AP-specific interface structure to a generic structure that can
> be embedded into any interface type.
> The functions related to powersave are prepared to allow easy extension
> with different interface types. For example with:
>
> + } else if (sta->sdata->vif.type == NL80211_IFTYPE_MESH_POINT) {
> + ps = &sdata->u.mesh.ps;
>
>
> Some references to the AP's beacon structure are removed where they were
> obviously not used. Other occurrences are in ieee80211_get_buffered_bc
> where I am not sure if they make any sense or not. Should they be removed
> too?
That might be stale since the dtim counter moved out of the beacon
structure again? Somebody pointed this out to me before, maybe even you?
> Does it make any sense to test for NL80211_IFTYPE_AP and
> NL80211_IFTYPE_AP_VLAN everytime?
Not sure how you mean that?
> if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
> - BUG_ON(!sdata->bss);
> + if (sta->sdata->vif.type == NL80211_IFTYPE_AP ||
> + sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
> + BUG_ON(!sdata->bss);
might be a good time to remove the BUG_ON usage now, or as a follow-up
patch
> /*
> * broadcast/multicast frame
> *
> - * If any of the associated stations is in power save mode,
> + * If any of the neighbor stations is in power save mode,
I'd prefer to be more generic here or list the options, "neighbor" is a
mesh-only term.
> @@ -2651,29 +2659,37 @@ ieee80211_get_buffered_bc(struct ieee80211_hw *hw,
> struct sk_buff *skb = NULL;
> struct ieee80211_tx_data tx;
> struct ieee80211_sub_if_data *sdata;
> - struct ieee80211_if_ap *bss = NULL;
> - struct beacon_data *beacon;
> + struct ps_data *ps;
> struct ieee80211_tx_info *info;
>
> sdata = vif_to_sdata(vif);
> - bss = &sdata->u.ap;
>
> rcu_read_lock();
> - beacon = rcu_dereference(bss->beacon);
>
> - if (sdata->vif.type != NL80211_IFTYPE_AP || !beacon || !beacon->head)
> + if (sdata->vif.type == NL80211_IFTYPE_AP) {
> + struct beacon_data *beacon =
> + rcu_dereference(sdata->u.ap.beacon);
> +
> + /* XXX is there any use for beacon here? */
> +
> + if (!beacon || !beacon->head)
> + goto out;
Checking to return NULL, apparently, if no beacons are active? If you
can say that's always true (list empty) then it could be removed, I
suspect it is going to be removed properly so this might not be relevant
(any more).
Looks good.
johannes
--
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
