| Message ID | 20230822040745.260086-1-sultan@kerneltoast.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: iwlwifi: mvm: fix power save for MLD | expand |
Hi, On Mon, 2023-08-21 at 21:07 -0700, Sultan Alsawaf wrote: > From: Sultan Alsawaf <sultan@kerneltoast.com> > > The conversion from bss_info_changed() to vif_cfg/link_info_changed() > removed a call to iwl_mvm_bss_info_changed_station_common() from what > became the vif_cfg_changed() path. As a result, BSS_CHANGED_PS and other > power save changes sent via ieee80211_vif_cfg_change_notify() are ignored > for MLD, which breaks power save entirely. This has a noticeable impact on > power consumption, causing idle package power to hover at 10 W on my > i9-13980HX laptop with an AX210 while connected to WiFi. > > Add the missing iwl_mvm_bss_info_changed_station_common() call to the > vif_cfg_changed() path to fix power save for MLD. This brings idle package > power down to 1 W on my laptop, a 9 W improvement. > > Fixes: 22c588343529 ("wifi: iwlwifi: mvm: replace bss_info_changed() with vif_cfg/link_info_changed()") > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com> > --- > drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c > index 8b6c641772ee..6abc05976870 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c > @@ -731,6 +731,8 @@ static void iwl_mvm_mld_vif_cfg_changed_station(struct iwl_mvm *mvm, > > mvmvif->associated = vif->cfg.assoc; > > + iwl_mvm_bss_info_changed_station_common(mvm, vif, &vif->bss_conf, > + changes); > if (!(changes & BSS_CHANGED_ASSOC)) > return; > I agree that it looks like the call to iwl_mvm_bss_info_changed_station_common() is missing here, but I think if is is added here then the call to iwl_mvm_power_update_mac() at the end of this function is not needed anymore?
On Tue, 2023-08-29 at 13:21 -0700, Sultan Alsawaf wrote: > On Tue, Aug 29, 2023 at 04:25:45PM +0000, Greenman, Gregory wrote: > > Hi, > > > > On Mon, 2023-08-21 at 21:07 -0700, Sultan Alsawaf wrote: > > > From: Sultan Alsawaf <sultan@kerneltoast.com> > > > > > > The conversion from bss_info_changed() to vif_cfg/link_info_changed() > > > removed a call to iwl_mvm_bss_info_changed_station_common() from what > > > became the vif_cfg_changed() path. As a result, BSS_CHANGED_PS and other > > > power save changes sent via ieee80211_vif_cfg_change_notify() are ignored > > > for MLD, which breaks power save entirely. This has a noticeable impact on > > > power consumption, causing idle package power to hover at 10 W on my > > > i9-13980HX laptop with an AX210 while connected to WiFi. > > > > > > Add the missing iwl_mvm_bss_info_changed_station_common() call to the > > > vif_cfg_changed() path to fix power save for MLD. This brings idle package > > > power down to 1 W on my laptop, a 9 W improvement. > > > > > > Fixes: 22c588343529 ("wifi: iwlwifi: mvm: replace bss_info_changed() with vif_cfg/link_info_changed()") > > > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com> > > > --- > > > drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c > > > index 8b6c641772ee..6abc05976870 100644 > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c > > > @@ -731,6 +731,8 @@ static void iwl_mvm_mld_vif_cfg_changed_station(struct iwl_mvm *mvm, > > > > > > mvmvif->associated = vif->cfg.assoc; > > > > > > + iwl_mvm_bss_info_changed_station_common(mvm, vif, &vif->bss_conf, > > > + changes); > > > if (!(changes & BSS_CHANGED_ASSOC)) > > > return; > > > > > > > I agree that it looks like the call to iwl_mvm_bss_info_changed_station_common() is missing here, but > > I think if is is added here then the call to iwl_mvm_power_update_mac() at the end of this function > > is not needed anymore? > > That call to iwl_mvm_power_update_mac() only exists in the backport driver [1] > at the moment, hence why power save is currently broken in the upstream driver. > > I wasn't aware that the backport driver had that change until after I sent this, > FWIW. :) > > I agree that the iwl_mvm_power_update_mac() call in the backport driver would be > redundant with this patch. > > How would you like to go about this? Shall I send a v2 reformatted for the > backport driver, with an updated commit message since power save isn't broken in > the backport driver? > > Or could this be staged upstream with [1] subsequently getting dropped from the > backport driver? > > Thanks, > Sultan > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/backport-iwlwifi.git/commit/?id=c3201e3d7d88bdfa0f0a94e4141c626c35724655 Sorry for the delay. I've sent the patch in [1] from our backport tree. Can I ask you to try it? Thanks, Gregory [1] https://lore.kernel.org/linux-wireless/20230905162939.5ef0c8230de6.Ieed265014988c50ec68fbff6d33821e4215f987f@changeid/
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c index 8b6c641772ee..6abc05976870 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mld-mac80211.c @@ -731,6 +731,8 @@ static void iwl_mvm_mld_vif_cfg_changed_station(struct iwl_mvm *mvm, mvmvif->associated = vif->cfg.assoc; + iwl_mvm_bss_info_changed_station_common(mvm, vif, &vif->bss_conf, + changes); if (!(changes & BSS_CHANGED_ASSOC)) return;