diff mbox series

wifi: ath11k: don't force enable power save on non-running vdevs

Message ID 20240309113115.11498-1-quic_bqiang@quicinc.com (mailing list archive)
State Accepted
Commit 01296b39d3515f20a1db64d3c421c592b1e264a0
Delegated to: Kalle Valo
Headers show
Series wifi: ath11k: don't force enable power save on non-running vdevs | expand

Commit Message

Baochen Qiang March 9, 2024, 11:31 a.m. UTC 
Currently we force enable power save on non-running vdevs, this results
in unexpected ping latency in below scenarios:
	1. disable power save from userspace.
	2. trigger suspend/resume.

With step 1 power save is disabled successfully and we get a good latency:

PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=5.13 ms
64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=5.45 ms
64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=5.99 ms
64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=6.34 ms
64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=4.47 ms
64 bytes from 192.168.1.1: icmp_seq=6 ttl=64 time=6.45 ms

While after step 2, the latency becomes much larger:

PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=17.7 ms
64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=15.0 ms
64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=14.3 ms
64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=16.5 ms
64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=20.1 ms

The reason is, with step 2, power save is force enabled due to vdev not
running, although mac80211 was trying to disable it to honor userspace
configuration:

ath11k_pci 0000:03:00.0: wmi cmd sta powersave mode psmode 1 vdev id 0
Call Trace:
 ath11k_wmi_pdev_set_ps_mode
 ath11k_mac_op_bss_info_changed
 ieee80211_bss_info_change_notify
 ieee80211_reconfig
 ieee80211_resume
 wiphy_resume

This logic is taken from ath10k where it was added due to below comment:

	Firmware doesn't behave nicely and consumes more power than
	necessary if PS is disabled on a non-started vdev.

However we don't know whether such an issue also occurs to ath11k firmware
or not. But even if it does, it's not appropriate because it goes against
userspace, even cfg/mac80211 don't know we have enabled it in fact.

Remove it to fix this issue. In this way we not only get a better latency,
but also, and the most important, keeps the consistency between userspace
and kernel/driver. The biggest price for that would be the power consumption,
which is not that important, compared with the consistency.

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30

Fixes: b2beffa7d9a6 ("ath11k: enable 802.11 power save mode in station mode")
Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/mac.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)


base-commit: 7a5ed5a3801e9b6cf7bafbb0a05c70cef620b22a

Comments

Jeff Johnson March 11, 2024, 6:17 p.m. UTC | #1 
On 3/9/2024 3:31 AM, Baochen Qiang wrote:
> Currently we force enable power save on non-running vdevs, this results
> in unexpected ping latency in below scenarios:
> 	1. disable power save from userspace.
> 	2. trigger suspend/resume.
> 
> With step 1 power save is disabled successfully and we get a good latency:
> 
> PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
> 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=5.13 ms
> 64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=5.45 ms
> 64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=5.99 ms
> 64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=6.34 ms
> 64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=4.47 ms
> 64 bytes from 192.168.1.1: icmp_seq=6 ttl=64 time=6.45 ms
> 
> While after step 2, the latency becomes much larger:
> 
> PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
> 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=17.7 ms
> 64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=15.0 ms
> 64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=14.3 ms
> 64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=16.5 ms
> 64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=20.1 ms
> 
> The reason is, with step 2, power save is force enabled due to vdev not
> running, although mac80211 was trying to disable it to honor userspace
> configuration:
> 
> ath11k_pci 0000:03:00.0: wmi cmd sta powersave mode psmode 1 vdev id 0
> Call Trace:
>  ath11k_wmi_pdev_set_ps_mode
>  ath11k_mac_op_bss_info_changed
>  ieee80211_bss_info_change_notify
>  ieee80211_reconfig
>  ieee80211_resume
>  wiphy_resume
> 
> This logic is taken from ath10k where it was added due to below comment:
> 
> 	Firmware doesn't behave nicely and consumes more power than
> 	necessary if PS is disabled on a non-started vdev.
> 
> However we don't know whether such an issue also occurs to ath11k firmware
> or not. But even if it does, it's not appropriate because it goes against
> userspace, even cfg/mac80211 don't know we have enabled it in fact.
> 
> Remove it to fix this issue. In this way we not only get a better latency,
> but also, and the most important, keeps the consistency between userspace
> and kernel/driver. The biggest price for that would be the power consumption,
> which is not that important, compared with the consistency.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> 
> Fixes: b2beffa7d9a6 ("ath11k: enable 802.11 power save mode in station mode")
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Kalle Valo March 12, 2024, 3:44 p.m. UTC | #2 
Baochen Qiang <quic_bqiang@quicinc.com> wrote:

> Currently we force enable power save on non-running vdevs, this results
> in unexpected ping latency in below scenarios:
>         1. disable power save from userspace.
>         2. trigger suspend/resume.
> 
> With step 1 power save is disabled successfully and we get a good latency:
> 
> PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
> 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=5.13 ms
> 64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=5.45 ms
> 64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=5.99 ms
> 64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=6.34 ms
> 64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=4.47 ms
> 64 bytes from 192.168.1.1: icmp_seq=6 ttl=64 time=6.45 ms
> 
> While after step 2, the latency becomes much larger:
> 
> PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
> 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=17.7 ms
> 64 bytes from 192.168.1.1: icmp_seq=2 ttl=64 time=15.0 ms
> 64 bytes from 192.168.1.1: icmp_seq=3 ttl=64 time=14.3 ms
> 64 bytes from 192.168.1.1: icmp_seq=4 ttl=64 time=16.5 ms
> 64 bytes from 192.168.1.1: icmp_seq=5 ttl=64 time=20.1 ms
> 
> The reason is, with step 2, power save is force enabled due to vdev not
> running, although mac80211 was trying to disable it to honor userspace
> configuration:
> 
> ath11k_pci 0000:03:00.0: wmi cmd sta powersave mode psmode 1 vdev id 0
> Call Trace:
>  ath11k_wmi_pdev_set_ps_mode
>  ath11k_mac_op_bss_info_changed
>  ieee80211_bss_info_change_notify
>  ieee80211_reconfig
>  ieee80211_resume
>  wiphy_resume
> 
> This logic is taken from ath10k where it was added due to below comment:
> 
>         Firmware doesn't behave nicely and consumes more power than
>         necessary if PS is disabled on a non-started vdev.
> 
> However we don't know whether such an issue also occurs to ath11k firmware
> or not. But even if it does, it's not appropriate because it goes against
> userspace, even cfg/mac80211 don't know we have enabled it in fact.
> 
> Remove it to fix this issue. In this way we not only get a better latency,
> but also, and the most important, keeps the consistency between userspace
> and kernel/driver. The biggest price for that would be the power consumption,
> which is not that important, compared with the consistency.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> 
> Fixes: b2beffa7d9a6 ("ath11k: enable 802.11 power save mode in station mode")
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

01296b39d351 wifi: ath11k: don't force enable power save on non-running vdevs
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index a6a37d67a50a..42a6a51a6a9d 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -1231,14 +1231,7 @@  static int ath11k_mac_vif_setup_ps(struct ath11k_vif *arvif)
 
 	enable_ps = arvif->ps;
 
-	if (!arvif->is_started) {
-		/* mac80211 can update vif powersave state while disconnected.
-		 * Firmware doesn't behave nicely and consumes more power than
-		 * necessary if PS is disabled on a non-started vdev. Hence
-		 * force-enable PS for non-running vdevs.
-		 */
-		psmode = WMI_STA_PS_MODE_ENABLED;
-	} else if (enable_ps) {
+	if (enable_ps) {
 		psmode = WMI_STA_PS_MODE_ENABLED;
 		param = WMI_STA_PS_PARAM_INACTIVITY_TIME;