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 <[email protected]>
Reported-by: Ben Greear <[email protected]>
---
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) {
--
1.7.3.2
On 2011-05-06 3:43 PM, Johannes Berg wrote:
> On Fri, 2011-05-06 at 15:35 +0200, Felix Fietkau wrote:
>
>> >> >> + info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
>
>> 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.
>
> I didn't remember/know about the case where sta is unset, but still,
> what if the driver checks for the flag and does something that's only
> valid in AP mode? I'm thinking firmware API here, not HW like ath9k.
>
>> > 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?
>
> Well, say I implement the clearing by sending something command to the
> device, like p54. If that's only valid in AP mode in the firmware, now
> this driver has to check that it's AP mode, where before it didn't have
> to. That's not counting the sta==NULL case I guess.
>
> OTOH, we only have two drivers using the flag anyway. Go figure it out
> between those two :)
mac80211 already sets the flag for all modes. If drivers were depending
on it being set only for AP mode, then those drivers would break even
without my patch.
- Felix
On Fri, 2011-05-06 at 15:35 +0200, Felix Fietkau wrote:
> >> >> + info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
> 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.
I didn't remember/know about the case where sta is unset, but still,
what if the driver checks for the flag and does something that's only
valid in AP mode? I'm thinking firmware API here, not HW like ath9k.
> > 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?
Well, say I implement the clearing by sending something command to the
device, like p54. If that's only valid in AP mode in the firmware, now
this driver has to check that it's AP mode, where before it didn't have
to. That's not counting the sta==NULL case I guess.
OTOH, we only have two drivers using the flag anyway. Go figure it out
between those two :)
johannes
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<[email protected]>
>> Reported-by: Ben Greear<[email protected]>
>> ---
>> 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.
- Felix
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 <[email protected]>
> Reported-by: Ben Greear <[email protected]>
> ---
> 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?
Regards,
Chr
On Fri, 2011-05-06 at 02:41 +0200, 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"
I guess for GO powersave this might need to change but it seems nobody
has thought about implementing that yet anyway :)
johannes
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<[email protected]>
> >> Reported-by: Ben Greear<[email protected]>
> >> ---
> >> 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.
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?
johannes
On Friday 06 May 2011 15:47:43 Felix Fietkau wrote:
> On 2011-05-06 3:43 PM, Johannes Berg wrote:
> > On Fri, 2011-05-06 at 15:35 +0200, Felix Fietkau wrote:
> >
> >> >> >> + info->flags |= IEEE80211_TX_CTL_CLEAR_PS_FILT;
> >
> >> 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.
Yes, but p54 fw can and will filter frames in STA PS mode as well [This is
great because we can sent any frames to the device without caring about
the RX/TX races in the PS design, since the FW will return all frames with
"tx rejected due to PS" until the device receives the wake-up command].
> >> 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.
> >
> > I didn't remember/know about the case where sta is unset, but still,
> > what if the driver checks for the flag and does something that's only
> > valid in AP mode? I'm thinking firmware API here, not HW like ath9k.
> >
> >> > 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?
> >
> > Well, say I implement the clearing by sending something command to the
> > device, like p54. If that's only valid in AP mode in the firmware, now
> > this driver has to check that it's AP mode, where before it didn't have
> > to. That's not counting the sta==NULL case I guess.
> >
> > OTOH, we only have two drivers using the flag anyway. Go figure it out
> > between those two :)
> mac80211 already sets the flag for all modes. If drivers were depending
> on it being set only for AP mode, then those drivers would break even
> without my patch.
Setting the flag for broadcast is not a problem and neither is it when
the frame was filtered, but now it will be always "on" [in STA mode]
and this should be reserved for PS-POLLs [which is a different flag].
Regards,
Chr
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<[email protected]>
>> >> Reported-by: Ben Greear<[email protected]>
>> >> ---
>> >> 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