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

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

[Mario Limonciello]

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

[PATCH 2/2] wifi: mt76: mt7925: Disable powersaving by default

[Mario Limonciello]

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.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/net/wireless/mediatek/mt76/mt7925/init.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/init.c b/drivers/net/wireless/mediatek/mt76/mt7925/init.c
index 8f9b7a2f376c..370fe26d592b 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7925/init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7925/init.c
@@ -7,6 +7,11 @@
 #include "mac.h"
 #include "mcu.h"
 
+static bool mt7925_powersave;
+module_param_named(power_save, mt7925_powersave, bool, 0444);
+MODULE_PARM_DESC(power_save,
+		 "enable WiFi power management (default: disable)");
+
 static void
 mt7925_regd_notifier(struct wiphy *wiphy,
 		     struct regulatory_request *req)
@@ -184,11 +189,13 @@ int mt7925_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 (mt7925_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

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@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

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

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Mario Limonciello]

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

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

[James Prestwood]

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

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

[Mario Limonciello]

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,

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

[rwahler]

From: Sultan Alsawaf <sultan@kerneltoast.com>

Sultan Alsawaf <sultan@kerneltoast.com> writes:

> 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.

Without the proposed patch ping times are often > 3000ms with a packet loss of ~20%. And it's not only ICMP packets because i.e. ssh to the laptop is also not working. It is unusable slow and very often the connection breaks completely.

Kalle Valo <kvalo@kernel.org> writes:

> 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.

I'm using a FritzBox 6591 Cable Router with latest Firmware for Wlan and use a Framework13 Laptop with built in MT7921 module. I can reliably reproduce the problem with high round trip times and packet loss for inbound connections.

If i can help with some tests to find the problem i'm happy to support you.

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

[Kalle Valo]

(adding back linux-wireless, please don't drop lists and people from Cc)

rwahler@gmx.net writes:

> From: Sultan Alsawaf <sultan@kerneltoast.com>
>
> Sultan Alsawaf <sultan@kerneltoast.com> writes:
>
>> 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.
>
> Without the proposed patch ping times are often > 3000ms with a packet
> loss of ~20%. And it's not only ICMP packets because i.e. ssh to the
> laptop is also not working. It is unusable slow and very often the
> connection breaks completely.

To which direction? When reporting power save issues it's a good idea to
be specific as possible, we don't have crystal balls.

> Kalle Valo <kvalo@kernel.org> writes:
>
>> 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.
>
> I'm using a FritzBox 6591 Cable Router with latest Firmware for Wlan
> and use a Framework13 Laptop with built in MT7921 module. I can
> reliably reproduce the problem with high round trip times and packet
> loss for inbound connections.

Have you tried other clients with that AP? Especially mobile devices
like phones is good to test, they usually have pretty aggressive power
savings. Also testing with other APs is good.

> If i can help with some tests to find the problem i'm happy to support you.

I don't know about mt76 driver internals but hopefully others can help.
But what I recommend is to provide comprehensive and detailed bug
reports, even better if you can include a 802.11 frame capture from a
sniffer. Just saying "it doesn't work" doesn't get very far.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Mario Limonciello]

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.

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

[Felix Fietkau]

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

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

[Mario Limonciello]

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.