[rt2x00-users] [RFC v2] Add autowake support for USB hardware

[Johannes Stezenbach]

Hi Ivo,

On Mon, Apr 11, 2011 at 09:18:44PM +0200, Ivo van Doorn wrote:
> Please review carefully, during my ping test I got 3% packet loss which is quite reasonable
> in my testing environment. Compared to v1 I got a big improvement in the ping times,
> basically it was mostly 4ms (which matches my atheros card with PS enabled, or my rt2800usb
> stick with PS disabled). However at some times I got ping times of 64ms for a few seconds, before
> it dropped back to 4ms again.

Since the traffic is buffered during power save I think it is normal
to get ping times up to the DTIM period?  I'm not sure how the
Atheros card manages to get low ping times during PS.  Maybe the
Atheros cards stays awake a while after TX before going to sleep,
in case there are replies. You should be able to see in traffic dump
when the STA sets the PS bit in the frame control field.


> @@ -204,6 +211,18 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
>  	if (ieee80211_flags & IEEE80211_CONF_CHANGE_CHANNEL)
>  		rt2x00link_reset_tuner(rt2x00dev, false);
>  
> +	if (test_bit(REQUIRE_PS_AUTOWAKE, &rt2x00dev->flags) &&
> +	    (ieee80211_flags & IEEE80211_CONF_CHANGE_PS) &&
> +	    (conf->flags & IEEE80211_CONF_PS))
> +		queue_delayed_work(rt2x00dev->workqueue,
> +				   &rt2x00dev->autowakeup_work,
> +				   msecs_to_jiffies(rt2x00dev->beacon_int) - 1);

This fixed delay assumes rt2x00lib_config() is always called right
after the beacon and not e.g. in the middle of the beacon interval.
But is this the case if there is a sufficient amount of traffic?

Also what happens when a TX packet is queued in the middle of
the beacon interval?  Does mac80211 change PS mode immediately?

If I understand the logic of your code correctly,
then the radio is turned off if there is no RX traffic within
one beacon interval, and it stays on for the whole beacon interval
if there is at least one RX frame buffered at the AP.
But what I expected is:  Sleep for DTIM period, then after
wakeup process pending RX frames and go back to sleep immediately.

Maybe that's good enough, but it should be mentioned in the
patch description.

> +
> +	if (conf->flags & IEEE80211_CONF_PS)
> +		test_bit(CONFIG_POWERSAVING, &rt2x00dev->flags);

typo?  should probably be set_bit()

> +	/* 2. Maybe the AP wants to send multicast/broadcast data? */
> +	cam |= !!(tim_ie->bitmap_ctrl & 0x01);

the double !! seems to be unneccessary

> +	if (!cam) {
> +		rt2x00lib_config(rt2x00dev, &rt2x00dev->hw->conf,
> +				 IEEE80211_CONF_CHANGE_PS);
> +	}

and the braces, too


Otherwise it looks good.


Thanks,
Johannes