Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 20 Apr 2021 14:58:39 +1000
From:   Bassem Dawood <bassem@morsemicro.com>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [PATCH] mac80211: Enable power save after receiving NULL packet
 ACK
Message-ID: <20210420045838.GA16360@bdawood>
References: <20210227055815.14838-1-bassem@morsemicro.com>
 <9282f2d2c6c8302de0c53dfb90743bd97e5a5a32.camel@sipsolutions.net>
 <20210409013218.GE9075@bdawood>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210409013218.GE9075@bdawood>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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