Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fa9429cb8d24c9bb4b810c423b150aefe116148c.camel@sipsolutions.net>
Subject: Re: Question about power save
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Larry Finger <Larry.Finger@lwfinger.net>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Date:   Thu, 25 May 2023 11:05:27 -0700
In-Reply-To: <c385be75-71db-6265-1a6c-24eca64e5d7f@lwfinger.net>
References: <c385be75-71db-6265-1a6c-24eca64e5d7f@lwfinger.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

Hi Larry,

> One of the users of an rtw8821ce found an increase of power usage from 27=
2 mW in=20
> kernel 5.19.13 to 579 mW in kernel 6.2.8. If he reverted commit 28977e790=
b5d=20
> ("wifi: mac80211: skip powersave recalc if driver SUPPORTS_DYNAMIC_PS"), =
the=20
> 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 *lo=
cal)
>          int count =3D 0;
>          int timeout;
>=20
> -       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 =3D NULL;
>                  return;
>          }
>=20
> The driver in question has both SUPPORTS_PS and SUPPORTS_DYNAMIC_PS set, =
thus=20
> this patch enables the dependent part of this test. Is this what was inte=
nded?=20
> 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