Re: [PATCH 1/2] wifi: mt76: mt7921: Disable powersaving by default

[Sultan Alsawaf]

On Tue, Dec 12, 2023 at 03:08:51AM -0600, Mario Limonciello wrote:
> Several users have reported awful latency when powersaving is enabled
> with certain access point combinations. It's also reported that the
> powersaving feature doesn't provide an ample enough savings to justify
> being enabled by default with these issues.
> 
> Introduce a module parameter that would control the power saving
> behavior.  Set it to default as disabled. This mirrors what some other
> WLAN drivers like iwlwifi do.
> 
> Suggested-by: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
> Link: https://codeberg.org/Hybrid-Project-Developers/linux-tkg/blame/branch/master/mt76:-mt7921:-Disable-powersave-features-by-default.mypatch
> Link: https://aur.archlinux.org/cgit/aur.git/tree/0027-mt76_-mt7921_-Disable-powersave-features-by-default.patch?h=linux-g14
> Link: https://community.frame.work/t/responded-strange-wlan-problems-with-kernel-branch-6-2/41868/4
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
>  drivers/net/wireless/mediatek/mt76/mt7921/init.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> index 7d6a9d746011..78d4197988c8 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> @@ -10,6 +10,11 @@
>  #include "../mt76_connac2_mac.h"
>  #include "mcu.h"
>  
> +static bool mt7921_powersave;
> +module_param_named(power_save, mt7921_powersave, bool, 0444);
> +MODULE_PARM_DESC(power_save,
> +		 "enable WiFi power management (default: disable)");
> +
>  static ssize_t mt7921_thermal_temp_show(struct device *dev,
>  					struct device_attribute *attr,
>  					char *buf)
> @@ -271,11 +276,13 @@ int mt7921_register_device(struct mt792x_dev *dev)
>  	dev->pm.idle_timeout = MT792x_PM_TIMEOUT;
>  	dev->pm.stats.last_wake_event = jiffies;
>  	dev->pm.stats.last_doze_event = jiffies;
> -	if (!mt76_is_usb(&dev->mt76)) {
> +	if (mt7921_powersave && !mt76_is_usb(&dev->mt76)) {
>  		dev->pm.enable_user = true;
>  		dev->pm.enable = true;
>  		dev->pm.ds_enable_user = true;
>  		dev->pm.ds_enable = true;
> +	} else {
> +		hw->wiphy->flags &= ~WIPHY_FLAG_PS_ON_BY_DEFAULT;
>  	}
>  
>  	if (!mt76_is_mmio(&dev->mt76))
> -- 
> 2.34.1
> 

A few things to note:

1. Power savings can be significant on some systems where keeping the PCIe link
   active consumes significant energy (e.g., Intel HX chipsets in laptops and
   probably desktops in general). On desktops this isn't a big deal, but on
   desktop-class laptops the battery impact will be noticeable.

2. This doesn't mirror iwlwifi, which has powersave enabled by default.

   Beacon filtering is tied to powersave in mt76, whereas it isn't in iwlwifi.
   Thus, disabling powersave on mt76 results in the loss of beacon filtering.
   This means you'll get a constant stream of interrupts from beacon frames
   transmitted by the AP, which can also have power implications.

   And iwlwifi handles powersave transitions in firmware, which allows it
   enter/exit powersave with very low latency. This isn't the case on mt76,
   which enters/exits powersave in software.
   
3. For insignificant/low-bandwidth traffic like ICMP to the AP, high latency is
   expected since the amount of traffic doesn't warrant kicking the chipset out
   of powersave. So although it's not pretty to look at, bad ping times to the
   AP aren't representative of the full user experience.

That being said, given that my patch to disable powersave from over a year ago
has apparently become a commonplace addition to mt76, it seems like users
generally aren't happy with the current powersave UX. I agree that it should be
better, though I'm not certain disabling powersave outright is the best move.
Maybe the powersave behavior can be tweaked instead?

The reason I disabled powersave on my mt76 hardware was because I wanted the
lowest latency + highest throughput possible.

I know that on smartphones, QCA chipsets exhibit the same latency issue when
pinging the AP, due to powersave. But no one seems to be upset about that on
their phone, so I think there's probably a way to make powersave work well for
all parties.

Regarding the patch itself, I think a better idea would be to tie the wiphy
powersave flag to the deep sleep flag (`dev->pm.ds_enable_user`), so that users
can really disable powersave through `iw` at runtime without needing to use
debugfs. This would eliminate the need for a module parameter too.

Also, I find it quite sad that my patch from over a year ago [1] was blatantly
reauthored in that frame.work link. The commit message is even the same, word
for word. :-(

[1] https://github.com/kerneltoast/kernel_x86_laptop/commit/ca89780690f7492c2d357e0ed2213a1d027341ae

Sultan