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

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

[Sultan Alsawaf]

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;
 
-- 
2.41.0

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

[Greenman, Gregory]

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?

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

[Sultan Alsawaf]

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

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

[Greenman, Gregory]

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/