Question about power save

Commit Message

Larry Finger May 25, 2023, 4:02 p.m. UTC

Johannes,

One of the users of an rtw8821ce found an increase of power usage from 272 mW in 
kernel 5.19.13 to 579 mW in kernel 6.2.8. If he reverted commit 28977e790b5d 
("wifi: mac80211: skip powersave recalc if driver SUPPORTS_DYNAMIC_PS"), the 
original power usage is restored.

This patch says:


The driver in question has both SUPPORTS_PS and SUPPORTS_DYNAMIC_PS set, thus 
this patch enables the dependent part of this test. Is this what was intended? 
If so, then rtw88 is not supporting DYNAMIC_PS correctly.

Thanks,

Larry

Comments

Johannes Berg May 25, 2023, 6:05 p.m. UTC | #1

Hi Larry,

> One of the users of an rtw8821ce found an increase of power usage from 272 mW in 
> kernel 5.19.13 to 579 mW in kernel 6.2.8. If he reverted commit 28977e790b5d 
> ("wifi: mac80211: skip powersave recalc if driver SUPPORTS_DYNAMIC_PS"), the 
> original power usage is restored.

Yeah, I think I saw the report, but I'm travelling and didn't have that
much time to reply.


> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -1787,7 +1787,8 @@ void ieee80211_recalc_ps(struct ieee80211_local *local)
>          int count = 0;
>          int timeout;
> 
> -       if (!ieee80211_hw_check(&local->hw, SUPPORTS_PS)) {
> +       if (!ieee80211_hw_check(&local->hw, SUPPORTS_PS) ||
> +           ieee80211_hw_check(&local->hw, SUPPORTS_DYNAMIC_PS)) {
>                  local->ps_sdata = NULL;
>                  return;
>          }
> 
> The driver in question has both SUPPORTS_PS and SUPPORTS_DYNAMIC_PS set, thus 
> this patch enables the dependent part of this test. Is this what was intended? 
> If so, then rtw88 is not supporting DYNAMIC_PS correctly.

I didn't really have time to analyze this ... In mac80211.h we say:

 * Dynamic powersave is simply supported by mac80211 enabling and disabling
 * PS based on traffic. Driver needs to only set %IEEE80211_HW_SUPPORTS_PS
 * flag and mac80211 will handle everything automatically. Additionally,
 * hardware having support for the dynamic PS feature may set the
 * %IEEE80211_HW_SUPPORTS_DYNAMIC_PS flag to indicate that it can support
 * dynamic PS mode itself. The driver needs to look at the
 * @dynamic_ps_timeout hardware configuration value and use it that value
 * whenever %IEEE80211_CONF_PS is set. In this case mac80211 will disable
 * dynamic PS feature in stack and will just keep %IEEE80211_CONF_PS
 * enabled whenever user has enabled powersave.


but maybe the issue is that now CONF_PS isn't set any more?

johannes

Larry Finger May 25, 2023, 6:36 p.m. UTC | #2

On 5/25/23 13:05, Johannes Berg wrote:
> Yeah, I think I saw the report, but I'm travelling and didn't have that
> much time to reply.

Johannes,

The rtw88 drivers are definitely setting both SUPPORTS_PS and 
SUPPORTS_DYNAMIC_PS. It seems that there is a bug somewhere is those drivers.

In my repo, I will remove the SUPPORTS_DYNAMIC_PS, which will solve the problem 
raised in the GitHub issue. That will give me time to find that bug.

Thanks,

Larry

Ping-Ke Shih May 26, 2023, 11:55 a.m. UTC | #3

On Thu, 2023-05-25 at 13:36 -0500, Larry Finger wrote:
> 
> On 5/25/23 13:05, Johannes Berg wrote:
> > Yeah, I think I saw the report, but I'm travelling and didn't have that
> > much time to reply.
> 
> Johannes,
> 
> The rtw88 drivers are definitely setting both SUPPORTS_PS and
> SUPPORTS_DYNAMIC_PS. It seems that there is a bug somewhere is those drivers.
> 
> In my repo, I will remove the SUPPORTS_DYNAMIC_PS, which will solve the problem
> raised in the GitHub issue. That will give me time to find that bug.
> 

We also have been aware of this a couple days ago, so we are preparing patches
to correct this for both rtw88/89. The method is that re-calculate if we can
enter PS by changes of BSS_CHANGED_PS and vif->cfg.ps.
I will submit the patch soon. 

Ping-Ke

Ping-Ke Shih May 27, 2023, 8:41 a.m. UTC | #4

On Fri, 2023-05-26 at 19:55 +0800, Ping-Ke Shih wrote:
> On Thu, 2023-05-25 at 13:36 -0500, Larry Finger wrote:
> > On 5/25/23 13:05, Johannes Berg wrote:
> > > Yeah, I think I saw the report, but I'm travelling and didn't have that
> > > much time to reply.
> > 
> > Johannes,
> > 
> > The rtw88 drivers are definitely setting both SUPPORTS_PS and
> > SUPPORTS_DYNAMIC_PS. It seems that there is a bug somewhere is those drivers.
> > 
> > In my repo, I will remove the SUPPORTS_DYNAMIC_PS, which will solve the problem
> > raised in the GitHub issue. That will give me time to find that bug.
> > 
> 
> We also have been aware of this a couple days ago, so we are preparing patches
> to correct this for both rtw88/89. The method is that re-calculate if we can
> enter PS by changes of BSS_CHANGED_PS and vif->cfg.ps.
> I will submit the patch soon. 
> 
> 

Hi Larry,

I have sent fixes [1]. Please see the patchset about the detail.

[1] https://lore.kernel.org/linux-wireless/[e-mail]/T/#t

Ping-Ke

Larry Finger May 27, 2023, 5:30 p.m. UTC | #5

On 5/27/23 03:41, Ping-Ke Shih wrote:
> 
> I have sent fixes [1]. Please see the patchset about the detail.
> 
> [1] https://lore.kernel.org/linux-wireless/[e-mail]/T/#t
> 

Ping-Ke,

I applied those patches to the rtw88 and rtw89 repos at GitHub and asked the 
reporter of increased power usage to test and report back. I will let you know 
of the response.

Thanks,

Larry

Patch

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 52a41416b8bb..e5c058db451d 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1787,7 +1787,8 @@  void ieee80211_recalc_ps(struct ieee80211_local *local)
         int count = 0;
         int timeout;

-       if (!ieee80211_hw_check(&local->hw, SUPPORTS_PS)) {
+       if (!ieee80211_hw_check(&local->hw, SUPPORTS_PS) ||
+           ieee80211_hw_check(&local->hw, SUPPORTS_DYNAMIC_PS)) {
                 local->ps_sdata = NULL;
                 return;
         }