Re: [PATCH] wifi: iwlwifi: mvm: fix power save for MLD

["Greenman, Gregory" <gregory.greenman@intel.com>]

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/