2016-11-03 09:59:43

by Felix Fietkau

Subject: [PATCH] mac80211: fix broken AP mode handling of powersave clients

Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with
mac80211-generated TIM IE") introduced a logic error, where
__sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not
set. This prevents the beacon TIM bit from being set for all drivers
that do not implement this op (almost all of them), thus thoroughly
essential AP mode powersave functionality.

Cc: Emmanuel Grumbach <[email protected]>
Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
Signed-off-by: Felix Fietkau <[email protected]>
---
net/mac80211/sta_info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 236d47e..621734e 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
}

/* No need to do anything if the driver does all */
- if (!local->ops->set_tim)
+ if (local->ops->set_tim)
return;

if (sta->dead)
--
2.10.1

2016-11-03 11:10:13

by Emmanuel Grumbach

Subject: Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients

On Thu, Nov 3, 2016 at 1:08 PM, Felix Fietkau <[email protected]> wrote:
> On 2016-11-03 11:51, Emmanuel Grumbach wrote:
>> On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach <[email protected]> wrote:
>>> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <[email protected]> wrote:
>>>> Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with
>>>> mac80211-generated TIM IE") introduced a logic error, where
>>>> __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not
>>>> set. This prevents the beacon TIM bit from being set for all drivers
>>>> that do not implement this op (almost all of them), thus thoroughly
>>>> essential AP mode powersave functionality.
>>>>
>>>> Cc: Emmanuel Grumbach <[email protected]>
>>>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
>>>> Signed-off-by: Felix Fietkau <[email protected]>
>>>> ---
>>>> net/mac80211/sta_info.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
>>>> index 236d47e..621734e 100644
>>>> --- a/net/mac80211/sta_info.c
>>>> +++ b/net/mac80211/sta_info.c
>>>> @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
>>>> }
>>>>
>>>> /* No need to do anything if the driver does all */
>>>> - if (!local->ops->set_tim)
>>>> + if (local->ops->set_tim)
>>>> return;
>>>
>>> but ... then, you'll need call to drv_set_tim below...
>>
>> s/need/never/
>>
>>> Apparently, we can't rely on the ops pointer to enter or not enter this flow.
> Are you going to submit a fix, or should I just send a revert of your
> commit?
>

Go ahead and send a revert. I won't be fast enough :)

2016-11-03 11:08:32

by Felix Fietkau

Subject: Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients

On 2016-11-03 11:51, Emmanuel Grumbach wrote:
> On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach <[email protected]> wrote:
>> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <[email protected]> wrote:
>>> Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with
>>> mac80211-generated TIM IE") introduced a logic error, where
>>> __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not
>>> set. This prevents the beacon TIM bit from being set for all drivers
>>> that do not implement this op (almost all of them), thus thoroughly
>>> essential AP mode powersave functionality.
>>>
>>> Cc: Emmanuel Grumbach <[email protected]>
>>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
>>> Signed-off-by: Felix Fietkau <[email protected]>
>>> ---
>>> net/mac80211/sta_info.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
>>> index 236d47e..621734e 100644
>>> --- a/net/mac80211/sta_info.c
>>> +++ b/net/mac80211/sta_info.c
>>> @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
>>> }
>>>
>>> /* No need to do anything if the driver does all */
>>> - if (!local->ops->set_tim)
>>> + if (local->ops->set_tim)
>>> return;
>>
>> but ... then, you'll need call to drv_set_tim below...
>
> s/need/never/
>
>> Apparently, we can't rely on the ops pointer to enter or not enter this flow.
Are you going to submit a fix, or should I just send a revert of your
commit?

- Felix

2016-11-03 10:51:12

by Emmanuel Grumbach

Subject: Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients

On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <[email protected]> wrote:
> Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with
> mac80211-generated TIM IE") introduced a logic error, where
> __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not
> set. This prevents the beacon TIM bit from being set for all drivers
> that do not implement this op (almost all of them), thus thoroughly
> essential AP mode powersave functionality.
>
> Cc: Emmanuel Grumbach <[email protected]>
> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
> Signed-off-by: Felix Fietkau <[email protected]>
> ---
> net/mac80211/sta_info.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 236d47e..621734e 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
> }
>
> /* No need to do anything if the driver does all */
> - if (!local->ops->set_tim)
> + if (local->ops->set_tim)
> return;

but ... then, you'll need call to drv_set_tim below...
Apparently, we can't rely on the ops pointer to enter or not enter this flow.

2016-11-03 10:51:30

by Emmanuel Grumbach

Subject: Re: [PATCH] mac80211: fix broken AP mode handling of powersave clients

On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach <[email protected]> wrote:
> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <[email protected]> wrote:
>> Commit c68df2e7be0c ("mac80211: allow using AP_LINK_PS with
>> mac80211-generated TIM IE") introduced a logic error, where
>> __sta_info_recalc_tim turns into a no-op if local->ops->set_tim is not
>> set. This prevents the beacon TIM bit from being set for all drivers
>> that do not implement this op (almost all of them), thus thoroughly
>> essential AP mode powersave functionality.
>>
>> Cc: Emmanuel Grumbach <[email protected]>
>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
>> Signed-off-by: Felix Fietkau <[email protected]>
>> ---
>> net/mac80211/sta_info.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
>> index 236d47e..621734e 100644
>> --- a/net/mac80211/sta_info.c
>> +++ b/net/mac80211/sta_info.c
>> @@ -688,7 +688,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
>> }
>>
>> /* No need to do anything if the driver does all */
>> - if (!local->ops->set_tim)
>> + if (local->ops->set_tim)
>> return;
>
> but ... then, you'll need call to drv_set_tim below...

s/need/never/

> Apparently, we can't rely on the ops pointer to enter or not enter this flow.