On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
> This patch is based on Kalle's initial RFC patches on dynamic power save.
> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
> frame, it is appropriate to do it from mac80211.
> This patch enables mac80211 to send a null frame and also to
> check for tim in the beacon if power save is enabled.
Nice! Looks pretty much good to me, but could use some documentation
(Kalle will hopefully add some too). For instance, a casual observer
might wonder how the hardware can be saving power when the host software
has to parse beacons -- obviously things will only fall into place once
we support beacon miss offload.
> +static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
> +{
> + u8 mask;
> + u8 index, indexn1, indexn2;
> + struct ieee80211_tim_ie *tim = (struct ieee80211_tim_ie *) elems->tim;
> +
> + aid &= 0x3fff;
> + index = aid / 8;
> + mask = 1 << (aid & 7);
> +
> + indexn1 = tim->bitmap_ctrl & 0xfe;
> + indexn2 = elems->tim_len + indexn1 - 4;
> +
> + if (index < indexn1 || index > indexn2)
> + return false;
> +
> + index -= indexn1;
> +
> + if (tim->bitmap_ctrl & 0x01)
> + *is_mc = true;
Shouldn't you update *is_mc before the above return false? And also set
it to false in the other case, I think.
> + return (bool)(tim->virtual_map[index] & mask);
I think
return !!(... & mask);
would be more appropriate.
> @@ -1734,6 +1759,15 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
> ieee80211_sta_wmm_params(local, ifsta, elems.wmm_param,
> elems.wmm_param_len);
>
> + directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
> +
> + if (directed_tim || is_mc) {
> + if (local->hw.conf.flags && IEEE80211_CONF_PS) {
> + local->hw.conf.flags &= ~IEEE80211_CONF_PS;
> + ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> + ieee80211_send_nullfunc(local, sdata, 0);
> + }
> + }
How will we get back into PS mode after
> @@ -2616,13 +2650,15 @@ void ieee80211_mlme_notify_scan_completed(struct ieee80211_local *local)
>
> void ieee80211_ps_enable_work(struct work_struct *work)
> {
> - struct ieee80211_local *local = container_of(work,
> + struct ieee80211_local *local = container_of(work,
> struct ieee80211_local,
> ps_enable_work);
Huh, what's the change here?
> diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
> index 9e74e9f..254fb4e 100644
> --- a/net/mac80211/wext.c
> +++ b/net/mac80211/wext.c
> @@ -985,12 +985,18 @@ set:
> local->powersave = ps;
>
> if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
> - if (local->powersave)
> + if (local->powersave) {
> + ieee80211_send_nullfunc(local, sdata, 1);
> conf->flags |= IEEE80211_CONF_PS;
> - else
> + ret = ieee80211_hw_config(local,
> + IEEE80211_CONF_CHANGE_PS);
> + }
> + else {
The indentation here looks a bit odd, please put the closing brace
before the else
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
