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

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

[Felix Fietkau]

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 <emmanuel.grumbach@intel.com>
Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 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

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

[Emmanuel Grumbach]

On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <nbd@nbd.name> 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 <emmanuel.grumbach@intel.com>
> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  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.

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

[Emmanuel Grumbach]

On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <nbd@nbd.name> 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 <emmanuel.grumbach@intel.com>
>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  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.

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

[Felix Fietkau]

On 2016-11-03 11:51, Emmanuel Grumbach wrote:
> On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <nbd@nbd.name> 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 <emmanuel.grumbach@intel.com>
>>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> ---
>>>  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

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

[Emmanuel Grumbach]

On Thu, Nov 3, 2016 at 1:08 PM, Felix Fietkau <nbd@nbd.name> wrote:
> On 2016-11-03 11:51, Emmanuel Grumbach wrote:
>> On Thu, Nov 3, 2016 at 12:51 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>>> On Thu, Nov 3, 2016 at 11:59 AM, Felix Fietkau <nbd@nbd.name> 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 <emmanuel.grumbach@intel.com>
>>>> Fixes: c68df2e7be0c ("mac80211: allow using AP_LINK_PS with mac80211-generated TIM IE")
>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>> ---
>>>>  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 :)