| Message ID | 1433943751-11221-1-git-send-email-chaitanya.mgit@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, 2015-06-10 at 19:12 +0530, Chaitanya T K wrote: > From: Chaitanya T K <chaitanya.mgit@gmail.com> > > Background: When wowlan is enabled, the chipset stays in low > power state maintaining the connection as per the > mac80211 power save state. > > If suspended during TX in progress, there can be > race where the driver is put of of power-save due > to TX and during suspend dynamic_ps_time is cancelled > and TX packet is flushed, leaving the driver in ACTIVE > even after resuming until dynamic_ps_time puts > driver back in DOZE. (Which only happens if there > is another TX). > > This can lead to high power consumption of chipset > during (or) after resuming. I still don't like it. I also don't believe that what you're writing is actually true. The only problem I can see is that it leads to higher power consumption *while the system is suspended* - at resume time we send a packet and thus kick the timers... I was going to change it to this: ------- mac80211: ensure powersave is enabled when going to WoWLAN If the system suspends while the client powersave mechanism is waiting to suspend due to a previous TX, the system will go to sleep with the NIC being awake (active) rather than in doze mode. Unless the driver handles this, it leads to excessive power consumption while sleeping. ------- but I'm not convinced all of this is right. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 17, 2015 at 2:17 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2015-06-10 at 19:12 +0530, Chaitanya T K wrote: >> From: Chaitanya T K <chaitanya.mgit@gmail.com> >> >> Background: When wowlan is enabled, the chipset stays in low >> power state maintaining the connection as per the >> mac80211 power save state. >> >> If suspended during TX in progress, there can be >> race where the driver is put of of power-save due >> to TX and during suspend dynamic_ps_time is cancelled >> and TX packet is flushed, leaving the driver in ACTIVE >> even after resuming until dynamic_ps_time puts >> driver back in DOZE. (Which only happens if there >> is another TX). >> >> This can lead to high power consumption of chipset >> during (or) after resuming. > > I still don't like it. I also don't believe that what you're writing is > actually true. The only problem I can see is that it leads to higher > power consumption *while the system is suspended* - at resume time we > send a packet and thus kick the timers... "If" we send a packet, but until them the system will still be active. > > I was going to change it to this: > > ------- > mac80211: ensure powersave is enabled when going to WoWLAN > > If the system suspends while the client powersave mechanism > is waiting to suspend due to a previous TX, the system will > go to sleep with the NIC being awake (active) rather than > in doze mode. Unless the driver handles this, it leads to > excessive power consumption while sleeping. > ------- Yes, i am fine with this, but wanted to include about resume in case there is not response accepted fro the RX packet which triggered resume. > but I'm not convinced all of this is right. Normally all drivers/firmware are honoring mac80211 power state to enter Doze/not. Yes, we can ask drivers to force power save while entering suspend, but still up on resumption we must honor the mac80211 power save , so till the time we send any TX, chipset will be active drawing more power. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-06-17 at 15:52 +0530, Krishna Chaitanya wrote: > > I still don't like it. I also don't believe that what you're writing is > > actually true. The only problem I can see is that it leads to higher > > power consumption *while the system is suspended* - at resume time we > > send a packet and thus kick the timers... > "If" we send a packet, but until them the system will still be > active. But we *always* send a packet: ieee80211_reconfig: ... /* * The sta might be in psm against the ap (e.g. because * this was the state before a hw restart), so we * explicitly send a null packet in order to make sure * it'll sync against the ap (and get out of psm). */ if (!(local->hw.conf.flags & IEEE80211_CONF_PS)) { list_for_each_entry(sdata, &local->interfaces, list) { if (sdata->vif.type != NL80211_IFTYPE_STATION) continue; if (!sdata->u.mgd.associated) continue; ieee80211_send_nullfunc(local, sdata, 0); } } Then again, you're talking about WoWLAN, but then ... johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 17, 2015 at 4:31 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2015-06-17 at 15:52 +0530, Krishna Chaitanya wrote: > >> > I still don't like it. I also don't believe that what you're writing is >> > actually true. The only problem I can see is that it leads to higher >> > power consumption *while the system is suspended* - at resume time we >> > send a packet and thus kick the timers... >> "If" we send a packet, but until them the system will still be >> active. > > But we *always* send a packet: > > ieee80211_reconfig: > ... > /* > * The sta might be in psm against the ap (e.g. because > * this was the state before a hw restart), so we > * explicitly send a null packet in order to make sure > * it'll sync against the ap (and get out of psm). > */ > if (!(local->hw.conf.flags & IEEE80211_CONF_PS)) { > list_for_each_entry(sdata, &local->interfaces, list) { > if (sdata->vif.type != NL80211_IFTYPE_STATION) > continue; > if (!sdata->u.mgd.associated) > continue; > > ieee80211_send_nullfunc(local, sdata, 0); > } > } > > > > Then again, you're talking about WoWLAN, but then ... In wowlan, we simply call drv_resume and only if it fails then we will go through full resume. And without wowlan connection will not be there so this code will not be hit. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-06-17 at 17:05 +0530, Krishna Chaitanya wrote: > > Then again, you're talking about WoWLAN, but then ... > In wowlan, we simply call drv_resume and only if it fails > then we will go through full resume. And without wowlan > connection will not be there so this code will not be hit. Right, but then the important part still is that we're enabling powersave when going to sleep, no? I mean ... anyway it doesn't matter much. I'll come up with some better comment/description I guess. I really hate this whole powersave code btw - we really should refactor that into its own little library module or so... johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 17, 2015 at 5:07 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2015-06-17 at 17:05 +0530, Krishna Chaitanya wrote: > >> > Then again, you're talking about WoWLAN, but then ... >> In wowlan, we simply call drv_resume and only if it fails >> then we will go through full resume. And without wowlan >> connection will not be there so this code will not be hit. > > Right, but then the important part still is that we're enabling > powersave when going to sleep, no? I mean ... anyway it doesn't matter > much. I'll come up with some better comment/description I guess. Yes, we are forcing it to go in to power-save. > > I really hate this whole powersave code btw - we really should refactor > that into its own little library module or so... Yes, that sounds good. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I've applied this patch. > > I really hate this whole powersave code btw - we really should > > refactor > > that into its own little library module or so... > Yes, that sounds good. want to work on that? :) The whole thing is awfully broken anyway since it does hw-level configuration based on vif-level state, and ath9k does in fact support multiple vifs ... johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 17, 2015 at 2:46 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > > > I've applied this patch. Thanks. > > > > > I really hate this whole powersave code btw - we really should > > > refactor > > > that into its own little library module or so... > > Yes, that sounds good. > > want to work on that? :) Sure, but i cannot spend full time on that so the progress might be slow. > > The whole thing is awfully broken anyway since it does hw-level > configuration based on vif-level state, and ath9k does in fact support > multiple vifs ... Yes, we have faced problems with this especially handling fake ps, roc...we should move it to per vif config -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c index ac6ad62..8149a3d 100644 --- a/net/mac80211/pm.c +++ b/net/mac80211/pm.c @@ -76,6 +76,22 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan) if (sdata->vif.type != NL80211_IFTYPE_STATION) continue; ieee80211_mgd_quiesce(sdata); + /* If suspended during TX in progress, and wowlan + * is enabled (connection will be active) there + * can be a race where the driver is put out + * of power-save due to TX and during suspend + * dynamic_ps_timer is cancelled and TX packet + * is flushed, leaving the driver in ACTIVE even + * after resuming until dynamic_ps_timer puts + * driver back in DOZE. + */ + if (sdata->u.mgd.associated && + sdata->u.mgd.powersave && + !(local->hw.conf.flags & IEEE80211_CONF_PS)) { + local->hw.conf.flags |= IEEE80211_CONF_PS; + ieee80211_hw_config(local, + IEEE80211_CONF_CHANGE_PS); + } } err = drv_suspend(local, wowlan);