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
