| Message ID | 1637571856-1191-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: Configure keep-alive packet on suspend | expand |
Loic Poulain <loic.poulain@linaro.org> writes: > When system enter suspend, there is no more wireless traffic, and > if there is no incoming data, most of the AP kick-out the client > station after few minutes because of inactivity. > > The usual way to prevent this is to submit a Null function frame > periodically as a keep-alive. This is supported by brcm controllers > and can be configured via the mkeep_alive IOVAR. This is with brcmfmac in client mode, right? Wouldn't it make more sense to disconnect entirely during suspend? Nobody is processing the data packets anyway during suspend.
Hi Kalle, On Mon, 22 Nov 2021 at 12:01, Kalle Valo <kvalo@codeaurora.org> wrote: > > Loic Poulain <loic.poulain@linaro.org> writes: > > > When system enter suspend, there is no more wireless traffic, and > > if there is no incoming data, most of the AP kick-out the client > > station after few minutes because of inactivity. > > > > The usual way to prevent this is to submit a Null function frame > > periodically as a keep-alive. This is supported by brcm controllers > > and can be configured via the mkeep_alive IOVAR. > > This is with brcmfmac in client mode, right? Right, it's in client mode. > Wouldn't it make more sense to disconnect entirely during suspend? > Nobody is processing the data packets anyway during suspend. Disconnect is performed automatically when wowlan is not enabled, otherwise we may want to wake-up on events (disconnect, 4-way-handshake) or data packets (magic, unicast, etc...). Some devices use suspend aggressively such as Android in which the network link is expected to be maintained. Regards, Loic
Loic Poulain <loic.poulain@linaro.org> writes: > Hi Kalle, > > On Mon, 22 Nov 2021 at 12:01, Kalle Valo <kvalo@codeaurora.org> wrote: >> >> Loic Poulain <loic.poulain@linaro.org> writes: >> >> > When system enter suspend, there is no more wireless traffic, and >> > if there is no incoming data, most of the AP kick-out the client >> > station after few minutes because of inactivity. >> > >> > The usual way to prevent this is to submit a Null function frame >> > periodically as a keep-alive. This is supported by brcm controllers >> > and can be configured via the mkeep_alive IOVAR. >> >> This is with brcmfmac in client mode, right? > > Right, it's in client mode. > >> Wouldn't it make more sense to disconnect entirely during suspend? >> Nobody is processing the data packets anyway during suspend. > > Disconnect is performed automatically when wowlan is not enabled, > otherwise we may want to wake-up on events (disconnect, > 4-way-handshake) or data packets (magic, unicast, etc...). Some > devices use suspend aggressively such as Android in which the network > link is expected to be maintained. Sure, for wowlan it makes sense but you didn't mention that in the commit log so I assumed that was disabled.
Hi Loic,
I love your patch! Perhaps something to improve:
[auto build test WARNING on kvalo-wireless-drivers-next/master]
[also build test WARNING on kvalo-wireless-drivers/master v5.16-rc2 next-20211118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Loic-Poulain/brcmfmac-Configure-keep-alive-packet-on-suspend/20211122-165520
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: riscv-randconfig-r002-20211122 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c133fb321f7ca6083ce15b6aa5bf89de6600e649)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/13a67e895024e7cdbf0faa409fe3349cb0d741fc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Loic-Poulain/brcmfmac-Configure-keep-alive-packet-on-suspend/20211122-165520
git checkout 13a67e895024e7cdbf0faa409fe3349cb0d741fc
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3904:5: warning: no previous prototype for function 'brcmf_keepalive_start' [-Wmissing-prototypes]
int brcmf_keepalive_start(struct brcmf_if *ifp, unsigned int interval)
^
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3904:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int brcmf_keepalive_start(struct brcmf_if *ifp, unsigned int interval)
^
static
1 warning generated.
vim +/brcmf_keepalive_start +3904 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
3903
> 3904 int brcmf_keepalive_start(struct brcmf_if *ifp, unsigned int interval)
3905 {
3906 struct brcmf_mkeep_alive_pkt_le kalive = {0};
3907 int ret = 0;
3908
3909 /* Configure Null function/data keepalive */
3910 kalive.version = cpu_to_le16(1);
3911 kalive.period_msec = cpu_to_le16(interval * MSEC_PER_SEC);
3912 kalive.len_bytes = cpu_to_le16(0);
3913 kalive.keep_alive_id = cpu_to_le16(0);
3914
3915 ret = brcmf_fil_iovar_data_set(ifp, "mkeep_alive", &kalive, sizeof(kalive));
3916 if (ret)
3917 brcmf_err("keep-alive packet config failed, ret=%d\n", ret);
3918
3919 return ret;
3920 }
3921
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, 22 Nov 2021 at 15:23, Kalle Valo <kvalo@codeaurora.org> wrote: > > Loic Poulain <loic.poulain@linaro.org> writes: > > > Hi Kalle, > > > > On Mon, 22 Nov 2021 at 12:01, Kalle Valo <kvalo@codeaurora.org> wrote: > >> > >> Loic Poulain <loic.poulain@linaro.org> writes: > >> > >> > When system enter suspend, there is no more wireless traffic, and > >> > if there is no incoming data, most of the AP kick-out the client > >> > station after few minutes because of inactivity. > >> > > >> > The usual way to prevent this is to submit a Null function frame > >> > periodically as a keep-alive. This is supported by brcm controllers > >> > and can be configured via the mkeep_alive IOVAR. > >> > >> This is with brcmfmac in client mode, right? > > > > Right, it's in client mode. > > > >> Wouldn't it make more sense to disconnect entirely during suspend? > >> Nobody is processing the data packets anyway during suspend. > > > > Disconnect is performed automatically when wowlan is not enabled, > > otherwise we may want to wake-up on events (disconnect, > > 4-way-handshake) or data packets (magic, unicast, etc...). Some > > devices use suspend aggressively such as Android in which the network > > link is expected to be maintained. > > Sure, for wowlan it makes sense but you didn't mention that in the > commit log so I assumed that was disabled. Ah right, I'll do that in V2. Thanks, Loic
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index fb72777..afa8ceda 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -3901,6 +3901,24 @@ static void brcmf_configure_wowl(struct brcmf_cfg80211_info *cfg, cfg->wowl.active = true; } +int brcmf_keepalive_start(struct brcmf_if *ifp, unsigned int interval) +{ + struct brcmf_mkeep_alive_pkt_le kalive = {0}; + int ret = 0; + + /* Configure Null function/data keepalive */ + kalive.version = cpu_to_le16(1); + kalive.period_msec = cpu_to_le16(interval * MSEC_PER_SEC); + kalive.len_bytes = cpu_to_le16(0); + kalive.keep_alive_id = cpu_to_le16(0); + + ret = brcmf_fil_iovar_data_set(ifp, "mkeep_alive", &kalive, sizeof(kalive)); + if (ret) + brcmf_err("keep-alive packet config failed, ret=%d\n", ret); + + return ret; +} + static s32 brcmf_cfg80211_suspend(struct wiphy *wiphy, struct cfg80211_wowlan *wowl) { @@ -3947,6 +3965,9 @@ static s32 brcmf_cfg80211_suspend(struct wiphy *wiphy, } else { /* Configure WOWL paramaters */ brcmf_configure_wowl(cfg, ifp, wowl); + + /* Prevent disassociation due to inactivity with keep-alive */ + brcmf_keepalive_start(ifp, 30); } exit: diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h index ff2ef55..e69d1e5 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h @@ -1052,4 +1052,23 @@ struct brcmf_gscan_config { struct brcmf_gscan_bucket_config bucket[1]; }; +/** + * struct brcmf_mkeep_alive_pkt_le - configuration data for keep-alive frame. + * + * @version: version for mkeep_alive + * @length: length of fixed parameters in the structure. + * @period_msec: keep-alive period in milliseconds. + * @len_bytes: size of the data. + * @keep_alive_id: ID (0 - 3). + * @data: keep-alive frame data. + */ +struct brcmf_mkeep_alive_pkt_le { + __le16 version; + __le16 length; + __le32 period_msec; + __le16 len_bytes; + u8 keep_alive_id; + u8 data[0]; +} __packed; + #endif /* FWIL_TYPES_H_ */
When system enter suspend, there is no more wireless traffic, and if there is no incoming data, most of the AP kick-out the client station after few minutes because of inactivity. The usual way to prevent this is to submit a Null function frame periodically as a keep-alive. This is supported by brcm controllers and can be configured via the mkeep_alive IOVAR. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 21 +++++++++++++++++++++ .../broadcom/brcm80211/brcmfmac/fwil_types.h | 19 +++++++++++++++++++ 2 files changed, 40 insertions(+)