Re: [PATCH 2.6.39] mac80211: always clear PS filtering for non-AP interfaces

[Felix Fietkau <nbd@openwrt.org>]

On 2011-05-06 3:26 PM, Johannes Berg wrote:
> On Fri, 2011-05-06 at 15:20 +0200, Felix Fietkau wrote:
>>  On 2011-05-06 3:08 PM, Christian Lamparter wrote:
>>  >  On Friday 06 May 2011 02:41:14 Felix Fietkau wrote:
>>  >>   PS filtering is only useful in AP mode and by setting the flags in mac80211,
>>  >>   vif mode checks do not have to be added to every single driver supporting
>>  >>   this.
>>  >>   Fixes part of a regression introduced in commit 93ae2dd2
>>  >>   "ath9k: assign keycache slots to unencrypted stations"
>>  >>
>>  >>   Signed-off-by: Felix Fietkau<nbd@openwrt.org>
>>  >>   Reported-by: Ben Greear<greearb@candelatech.com>
>>  >>   ---
>>  >>    net/mac80211/tx.c |    3 +++
>>  >>    1 files changed, 3 insertions(+), 0 deletions(-)
>>  >>
>>  >>   diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>>  >>   index e1a39ed..b301216 100644
>>  >>   --- a/net/mac80211/tx.c
>>  >>   +++ b/net/mac80211/tx.c
>>  >>   @@ -1267,6 +1267,9 @@ ieee80211_tx_prepare(struct ieee80211_sub_if_data *sdata,
>>  >>    		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
>>  >>    	else if (test_and_clear_sta_flags(tx->sta, WLAN_STA_CLEAR_PS_FILT))
>>  >>    		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
>>  >>   +	else if (sdata->vif.type != NL80211_IFTYPE_AP&&
>>  >>   +	         sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
>>  >>   +		info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
>>  >>
>>  >>    	hdrlen = ieee80211_hdrlen(hdr->frame_control);
>>  >>    	if (skb->len>   hdrlen + sizeof(rfc1042_header) + 2) {
>>  >>
>>  >  uh, I think this will break p54* powersave in station mode.
>>  >  But why do we set it anyway? It's not like the AP is in PS mode, right?
>>  PS filtering is not meant for station mode at all. It's meant for
>>  suppressing lots of consecutive excessive retries when in AP mode
>>  sending traffic to a STA that just entered powersave.
>
> Actually, I think we should probably leave this patch out and fix it in
> the driver instead.
>
> You're right -- this is all intended for AP mode only, so it makes no
> sense to set it in STA mode to begin with. I thought maybe it's
> acceptable since drivers wouldn't use it in non-AP mode, but I can just
> as well imagine that drivers would always check it and rely on it being
> set only in AP mode.
Rely on what being set in AP mode only? The patch ensures that the flag 
for *clearing* PS filtering gets set for all non-AP modes. This flag 
already gets set whenever tx->sta is unset, so I think it also makes 
sense to extend that for other cases where the hardware is not supposed 
to enable any PS filtering on its own.

> Your patch makes those drivers responsible for checking the mode, but
> that makes less sense than having drivers that need to do some magic in
> all modes, no?
How does my patch make drivers responsible for checking the mode?

- Felix