| Message ID | 20210227055815.14838-1-bassem@morsemicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | mac80211: Enable power save after receiving NULL packet ACK | expand |
On Sat, 2021-02-27 at 16:58 +1100, Bassem Dawood wrote: > Trigger dynamic_ps_timer to re-evaluate power saving once a null function > packet (with PM = 1) is ACKed. Can you please say what this fixes/why the change is needed? johannes
On Thu, Apr 08, 2021 at 03:26:06PM +0200, Johannes Berg wrote: > On Sat, 2021-02-27 at 16:58 +1100, Bassem Dawood wrote: > > Trigger dynamic_ps_timer to re-evaluate power saving once a null function > > packet (with PM = 1) is ACKed. > > Can you please say what this fixes/why the change is needed? > > johannes > Sure. For drivers that declare PS_NULLFUNC_STACK, when sleeping, mac80211 would send a nullfunc (with PS bit set). It won't notify the driver yet (waiting for the NULLFUNC ACK). a/net/mac80211/mlme.c: void ieee80211_dynamic_ps_enable_work(struct work_struct *work) if (ieee80211_hw_check(&local->hw, PS_NULLFUNC_STACK) && !(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) { if (drv_tx_frames_pending(local)) { mod_timer(&local->dynamic_ps_timer, jiffies + msecs_to_jiffies( local->hw.conf.dynamic_ps_timeout)); } else { ieee80211_send_nullfunc(local, sdata, true); /* Flush to get the tx status of nullfunc frame */ ieee80211_flush_queues(local, sdata, false); } } if (!(ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS) && ieee80211_hw_check(&local->hw, PS_NULLFUNC_STACK)) || (ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) { ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED; local->hw.conf.flags |= IEEE80211_CONF_PS; ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS); } On reception of the ACK, mac80211 will set the IEEE80211_STA_NULLFUNC_ACKED flag, but won't re-evaluate dynamic_ps_enable_work. So drivers won't get notified to sleep (until ieee80211_dynamic_ps_enable_work is re-evaluated through any other path). a/net/mac80211/status.c:static void __ieee80211_tx_status(struct ieee80211_hw *hw, struct ieee80211_tx_status *status, int rates_idx, int retry_count) if (ieee80211_is_any_nullfunc(fc) && ieee80211_has_pm(fc) && ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS) && !(info->flags & IEEE80211_TX_CTL_INJECTED) && local->ps_sdata && !(local->scanning)) { if (info->flags & IEEE80211_TX_STAT_ACK) { local->ps_sdata->u.mgd.flags |= IEEE80211_STA_NULLFUNC_ACKED; } else mod_timer(&local->dynamic_ps_timer, jiffies + msecs_to_jiffies(10)); } This change makes sure ieee80211_dynamic_ps_enable_work is evaluated once a NULLFUNC is ACKED for a more conssitent notification of the driver. Regards, Bassem
On Fri, Apr 09, 2021 at 11:32:19AM +1000, Bassem Fahmy wrote: > On Thu, Apr 08, 2021 at 03:26:06PM +0200, Johannes Berg wrote: > > On Sat, 2021-02-27 at 16:58 +1100, Bassem Dawood wrote: > > > Trigger dynamic_ps_timer to re-evaluate power saving once a null function > > > packet (with PM = 1) is ACKed. > > > > Can you please say what this fixes/why the change is needed? > > > > johannes > > > > Sure. > > For drivers that declare PS_NULLFUNC_STACK, when sleeping, mac80211 would send a nullfunc (with PS bit set). It won't notify the driver yet (waiting for the NULLFUNC ACK). > > > a/net/mac80211/mlme.c: void ieee80211_dynamic_ps_enable_work(struct work_struct *work) > > if (ieee80211_hw_check(&local->hw, PS_NULLFUNC_STACK) && > !(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) { > if (drv_tx_frames_pending(local)) { > mod_timer(&local->dynamic_ps_timer, jiffies + > msecs_to_jiffies( > local->hw.conf.dynamic_ps_timeout)); > } else { > ieee80211_send_nullfunc(local, sdata, true); > /* Flush to get the tx status of nullfunc frame */ > ieee80211_flush_queues(local, sdata, false); > } > } > > if (!(ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS) && > ieee80211_hw_check(&local->hw, PS_NULLFUNC_STACK)) || > (ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) { > ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED; > local->hw.conf.flags |= IEEE80211_CONF_PS; > ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS); > } > > On reception of the ACK, mac80211 will set the IEEE80211_STA_NULLFUNC_ACKED flag, but won't re-evaluate dynamic_ps_enable_work. So drivers won't get notified to sleep (until ieee80211_dynamic_ps_enable_work is re-evaluated through any other path). > > a/net/mac80211/status.c:static void __ieee80211_tx_status(struct ieee80211_hw *hw, > struct ieee80211_tx_status *status, > int rates_idx, int retry_count) > > if (ieee80211_is_any_nullfunc(fc) && > ieee80211_has_pm(fc) && > ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS) && > !(info->flags & IEEE80211_TX_CTL_INJECTED) && > local->ps_sdata && !(local->scanning)) { > if (info->flags & IEEE80211_TX_STAT_ACK) { > local->ps_sdata->u.mgd.flags |= > IEEE80211_STA_NULLFUNC_ACKED; > } else > mod_timer(&local->dynamic_ps_timer, jiffies + > msecs_to_jiffies(10)); > } > > This change makes sure ieee80211_dynamic_ps_enable_work is evaluated once a NULLFUNC is ACKED for a more conssitent notification of the driver. > > Regards, > Bassem Hi Johannes, Following up on this. Is the explaination clear or you need more description? Regareds, Bassem
diff --git a/net/mac80211/status.c b/net/mac80211/status.c index 3485610755ef..c106f81478df 100644 --- a/net/mac80211/status.c +++ b/net/mac80211/status.c @@ -1013,9 +1013,9 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw, if (info->flags & IEEE80211_TX_STAT_ACK) { local->ps_sdata->u.mgd.flags |= IEEE80211_STA_NULLFUNC_ACKED; - } else - mod_timer(&local->dynamic_ps_timer, jiffies + - msecs_to_jiffies(10)); + } + mod_timer(&local->dynamic_ps_timer, jiffies + + msecs_to_jiffies(10)); } ieee80211_report_used_skb(local, skb, false);
Trigger dynamic_ps_timer to re-evaluate power saving once a null function packet (with PM = 1) is ACKed. Signed-off-by: Bassem Dawood <bassem@morsemicro.com> --- net/mac80211/status.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)