| Message ID | 20231212090852.162787-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] wifi: mt76: mt7921: Disable powersaving by default | expand |
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@kerneltoast.com> > 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@amd.com> > --- > 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
Mario Limonciello <mario.limonciello@amd.com> writes: > Several users have reported awful latency when powersaving is enabled > with certain access point combinations. What APs are these exactly? In the past 802.11 Power Save Mode was challenging due to badly behaving APs. But nowadays with so many mobile devices in the market I would assume that APs work a lot better. It would be best to investigate the issues in detail and try to fix them in mt76, assuming the bugs are in mt76 driver or firmware. > It's also reported that the powersaving feature doesn't provide an > ample enough savings to justify being enabled by default with these > issues. Any numbers or how was this concluded? > 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. We have already several ways to control 802.11 power save mode: * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the default setting) Adding module parameters as a fourth method sounds confusing so not really a fan of this. And the bar is quite high for adding new module parameters anyway.
> Mario Limonciello <mario.limonciello@amd.com> writes: > > > Several users have reported awful latency when powersaving is enabled > > with certain access point combinations. > > What APs are these exactly? In the past 802.11 Power Save Mode was > challenging due to badly behaving APs. But nowadays with so many mobile > devices in the market I would assume that APs work a lot better. It > would be best to investigate the issues in detail and try to fix them in > mt76, assuming the bugs are in mt76 driver or firmware. > > > It's also reported that the powersaving feature doesn't provide an > > ample enough savings to justify being enabled by default with these > > issues. > > Any numbers or how was this concluded? > > > 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. > > We have already several ways to control 802.11 power save mode: > > * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') > > * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) > > * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the default setting) > > Adding module parameters as a fourth method sounds confusing so not > really a fan of this. And the bar is quite high for adding new module > parameters anyway. agree, I think we do not need a new parameter for this, just use the current APIs. Regards, Lorenzo > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 12.12.23 10:08, 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@kerneltoast.com> > 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@amd.com> This patch is disabling two different things at the same time: - Wifi powersaving - Device hardware powersaving Have you tried simply disabling 802.11 powersave via nl80211 to mitigate the issues with affected APs? - Felix
On 12/13/23 5:26 AM, Lorenzo Bianconi wrote: >> Mario Limonciello <mario.limonciello@amd.com> writes: >> >>> Several users have reported awful latency when powersaving is enabled >>> with certain access point combinations. >> >> What APs are these exactly? In the past 802.11 Power Save Mode was >> challenging due to badly behaving APs. But nowadays with so many mobile >> devices in the market I would assume that APs work a lot better. It >> would be best to investigate the issues in detail and try to fix them in >> mt76, assuming the bugs are in mt76 driver or firmware. >> >>> It's also reported that the powersaving feature doesn't provide an >>> ample enough savings to justify being enabled by default with these >>> issues. >> >> Any numbers or how was this concluded? >> >>> 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. >> >> We have already several ways to control 802.11 power save mode: >> >> * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') >> >> * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) >> >> * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the default setting) >> >> Adding module parameters as a fourth method sounds confusing so not >> really a fan of this. And the bar is quite high for adding new module >> parameters anyway. > > agree, I think we do not need a new parameter for this, just use the current > APIs. Is there a convenient way for a user to make any of those options above stick through reboots? To me, the ability to set system defaults through reboots is a nice feature of module options. Thanks, Ben
On 12/13/2023 06:45, Kalle Valo wrote: > Mario Limonciello <mario.limonciello@amd.com> writes: > >> Several users have reported awful latency when powersaving is enabled >> with certain access point combinations. > > What APs are these exactly? In the past 802.11 Power Save Mode was > challenging due to badly behaving APs. But nowadays with so many mobile > devices in the market I would assume that APs work a lot better. It > would be best to investigate the issues in detail and try to fix them in > mt76, assuming the bugs are in mt76 driver or firmware. In my case I could reproduce the behavior on my Unifi access points. I've got a few that devices could roam between. I also happen to have a laptop on my desk with a WCN6855 that behaves just fine with those same APs. The other people with problems I've asked to come to this thread to share more about their devices. > >> It's also reported that the powersaving feature doesn't provide an >> ample enough savings to justify being enabled by default with these >> issues. > > Any numbers or how was this concluded? It was just a data point in the original patch from Sultan (who is on CC). I haven't corroborated it myself. Once I saw that other kernel drivers like iwlwifi are also "defaulting" to off with a module parameter I figured it made sense to bring that for discussion. > >> 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. > > We have already several ways to control 802.11 power save mode: > > * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') I'll experiment with an unpatched kernel just undoing this from here to see if it alone improves things. > > * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) > > * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the default setting) > > Adding module parameters as a fourth method sounds confusing so not > really a fan of this. And the bar is quite high for adding new module > parameters anyway. > Should we also discuss removing the iwlwifi one then too perhaps? Seems incongruent to offer it for some drivers but not others.
On 12/13/2023 08:45, Ben Greear wrote: > On 12/13/23 5:26 AM, Lorenzo Bianconi wrote: >>> Mario Limonciello <mario.limonciello@amd.com> writes: >>> >>>> Several users have reported awful latency when powersaving is enabled >>>> with certain access point combinations. >>> >>> What APs are these exactly? In the past 802.11 Power Save Mode was >>> challenging due to badly behaving APs. But nowadays with so many mobile >>> devices in the market I would assume that APs work a lot better. It >>> would be best to investigate the issues in detail and try to fix them in >>> mt76, assuming the bugs are in mt76 driver or firmware. >>> >>>> It's also reported that the powersaving feature doesn't provide an >>>> ample enough savings to justify being enabled by default with these >>>> issues. >>> >>> Any numbers or how was this concluded? >>> >>>> 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. >>> >>> We have already several ways to control 802.11 power save mode: >>> >>> * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') >>> >>> * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) >>> >>> * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the default >>> setting) >>> >>> Adding module parameters as a fourth method sounds confusing so not >>> really a fan of this. And the bar is quite high for adding new module >>> parameters anyway. >> >> agree, I think we do not need a new parameter for this, just use the >> current >> APIs. > > Is there a convenient way for a user to make any of those options above > stick through > reboots? > > To me, the ability to set system defaults through reboots is a nice > feature of > module options. > > Thanks, > Ben > Some userspace has the ability to do this. For example in Network Manager: https://unix.stackexchange.com/questions/595116/wi-fi-powersaving-in-networkmanager
On 12/13/2023 07:35, Felix Fietkau wrote: > On 12.12.23 10:08, 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@kerneltoast.com> >> 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@amd.com> > > This patch is disabling two different things at the same time: > - Wifi powersaving > - Device hardware powersaving > > Have you tried simply disabling 802.11 powersave via nl80211 to mitigate > the issues with affected APs? > > - Felix Kalle suggested this as well, I will experiment, thanks.
On 12/13/23 11:27, Mario Limonciello wrote: > On 12/13/2023 08:45, Ben Greear wrote: >> On 12/13/23 5:26 AM, Lorenzo Bianconi wrote: >>>> Mario Limonciello <mario.limonciello@amd.com> writes: >>>> >>>>> Several users have reported awful latency when powersaving is enabled >>>>> with certain access point combinations. >>>> >>>> What APs are these exactly? In the past 802.11 Power Save Mode was >>>> challenging due to badly behaving APs. But nowadays with so many >>>> mobile >>>> devices in the market I would assume that APs work a lot better. It >>>> would be best to investigate the issues in detail and try to fix >>>> them in >>>> mt76, assuming the bugs are in mt76 driver or firmware. >>>> >>>>> It's also reported that the powersaving feature doesn't provide an >>>>> ample enough savings to justify being enabled by default with these >>>>> issues. >>>> >>>> Any numbers or how was this concluded? >>>> >>>>> 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. >>>> >>>> We have already several ways to control 802.11 power save mode: >>>> >>>> * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') >>>> >>>> * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) >>>> >>>> * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the >>>> default setting) >>>> >>>> Adding module parameters as a fourth method sounds confusing so not >>>> really a fan of this. And the bar is quite high for adding new module >>>> parameters anyway. >>> >>> agree, I think we do not need a new parameter for this, just use the >>> current >>> APIs. >> >> Is there a convenient way for a user to make any of those options >> above stick through >> reboots? >> >> To me, the ability to set system defaults through reboots is a nice >> feature of >> module options. >> >> Thanks, >> Ben >> > > Some userspace has the ability to do this. For example in Network > Manager: > > https://unix.stackexchange.com/questions/595116/wi-fi-powersaving-in-networkmanager > And recently added to IWD for this very reason, there are no decent ways to persist between reboots (except when using NM). https://git.kernel.org/pub/scm/network/wireless/iwd.git/commit/?id=29edb1626d88bb713db71f7b374d8f24832fd94f Thanks, James
On 12/14/2023 06:39, James Prestwood wrote: > On 12/13/23 11:27, Mario Limonciello wrote: >> On 12/13/2023 08:45, Ben Greear wrote: >>> On 12/13/23 5:26 AM, Lorenzo Bianconi wrote: >>>>> Mario Limonciello <mario.limonciello@amd.com> writes: >>>>> >>>>>> Several users have reported awful latency when powersaving is enabled >>>>>> with certain access point combinations. >>>>> >>>>> What APs are these exactly? In the past 802.11 Power Save Mode was >>>>> challenging due to badly behaving APs. But nowadays with so many >>>>> mobile >>>>> devices in the market I would assume that APs work a lot better. It >>>>> would be best to investigate the issues in detail and try to fix >>>>> them in >>>>> mt76, assuming the bugs are in mt76 driver or firmware. >>>>> >>>>>> It's also reported that the powersaving feature doesn't provide an >>>>>> ample enough savings to justify being enabled by default with these >>>>>> issues. >>>>> >>>>> Any numbers or how was this concluded? >>>>> >>>>>> 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. >>>>> >>>>> We have already several ways to control 802.11 power save mode: >>>>> >>>>> * NL80211_CMD_SET_POWER_SAVE (for example used by 'iw set power_save') >>>>> >>>>> * CONFIG_CFG80211_DEFAULT_PS (for kernel level default) >>>>> >>>>> * WIPHY_FLAG_PS_ON_BY_DEFAULT (for the driver to control the >>>>> default setting) >>>>> >>>>> Adding module parameters as a fourth method sounds confusing so not >>>>> really a fan of this. And the bar is quite high for adding new module >>>>> parameters anyway. >>>> >>>> agree, I think we do not need a new parameter for this, just use the >>>> current >>>> APIs. >>> >>> Is there a convenient way for a user to make any of those options >>> above stick through >>> reboots? >>> >>> To me, the ability to set system defaults through reboots is a nice >>> feature of >>> module options. >>> >>> Thanks, >>> Ben >>> >> >> Some userspace has the ability to do this. For example in Network >> Manager: >> >> https://unix.stackexchange.com/questions/595116/wi-fi-powersaving-in-networkmanager > > And recently added to IWD for this very reason, there are no decent ways > to persist between reboots (except when using NM). > > https://git.kernel.org/pub/scm/network/wireless/iwd.git/commit/?id=29edb1626d88bb713db71f7b374d8f24832fd94f > > Thanks, > > James > All, Just wanted to update you that I looked at this issue again over the holidays and it's fixed by upgrading the linux-firmware for the mt7921 that was submitted in late November. I get the correct performance and latency without modifying power saving now on my Unifi access points. Thanks,
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))
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@kerneltoast.com> 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@amd.com> --- drivers/net/wireless/mediatek/mt76/mt7921/init.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)