| Message ID | 20161103095938.84054-1-nbd@nbd.name (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
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.
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.
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
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 :)
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)
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(-)