| Message ID | 20231212090852.162787-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] wifi: mt76: mt7921: Disable powersaving by default | expand |
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.
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(-)