Re: brcfmac: add possibility to turn off powersave on wiphy
On Tue, 25 Jul 2017, Arend van Spriel wrote: On 7/21/2017 11:19 PM, Rafal wrote: On Fri, 21 Jul 2017, Dan Williams wrote: On Fri, 2017-07-21 at 16:56 +0200, Rafal wrote: I have a board with ap6212 chip and I have encountered two problems with the brcmfmac driver operating with this device. First problem I describe below, second one in separate mail. Namely, I have noticed quite poor connection with the device despite good signal strength. Ping to the device was very uneven, about 50 - 100 ms in average, 10 - 20% of packets loss. After some investigations I have found that problem is caused by powersave feature on wiphy (BRCMF_C_SET_PM command). When the value is set to PM_OFF, the problem does not appear - ping is quite stable, ~2ms. When set to PM_FAST, the ping is bad. The brcmfmac driver currently does not have possibility to turn off the powersave feature. I have added the possibility on 4.11.6 kernel I don't think that's true? The driver implements the nl80211 set_power_mgmt hook, which lets you turn off powersave with 'iw'. That seems like a much better option than a module parameter. I know other drivers have the module parameter, but I personally consider that legacy/holdover, given that we have runtime toggles for this kind of thing. brcmf_cfg80211_set_power_mgmt() should do what you need, right? Yes, it does. In fact, I needed only a parameter in OF database. This is a bug in the device or firmware and I needed to disable the feature for this chip. Many device drivers allow to specify in OF database that driver should use a quirk. This is similar situation. I have added the kernel parameter as an additional feature. I thought that not all platforms are using devicetree database. But maybe it is a bad idea. I was going to make the same remark as Dan. Anyway, the ping times are not really unexpected. It depends on your beacon period and dtim setting. However, the packet loss is not expected and will require analyzing a sniff capture. Regards, Arend Ping times are very uneven, but it depends. Let's I give some output. In ouptuts below, the "wilk" machine is my PC, the "nano" machine is the device with ap6212 chip. First, the power save is OFF: rafal@wilk:~$ ping -c 10 nano PING nano (192.168.1.22) 56(84) bytes of data. 64 bytes from nano (192.168.1.22): icmp_seq=1 ttl=64 time=1.90 ms 64 bytes from nano (192.168.1.22): icmp_seq=2 ttl=64 time=1.82 ms 64 bytes from nano (192.168.1.22): icmp_seq=3 ttl=64 time=1.98 ms 64 bytes from nano (192.168.1.22): icmp_seq=4 ttl=64 time=1.77 ms 64 bytes from nano (192.168.1.22): icmp_seq=5 ttl=64 time=1.73 ms 64 bytes from nano (192.168.1.22): icmp_seq=6 ttl=64 time=1.79 ms 64 bytes from nano (192.168.1.22): icmp_seq=7 ttl=64 time=1.86 ms 64 bytes from nano (192.168.1.22): icmp_seq=8 ttl=64 time=1.92 ms 64 bytes from nano (192.168.1.22): icmp_seq=9 ttl=64 time=1.93 ms 64 bytes from nano (192.168.1.22): icmp_seq=10 ttl=64 time=1.80 ms --- nano ping statistics --- 10 packets transmitted, 10 received, 0% packet loss, time 9015ms rtt min/avg/max/mdev = 1.734/1.854/1.980/0.085 ms Now, the ping with power save ON, two tests run one by one: rafal@wilk:~$ ping -c 10 nano PING nano (192.168.1.22) 56(84) bytes of data. 64 bytes from nano (192.168.1.22): icmp_seq=1 ttl=64 time=69.4 ms 64 bytes from nano (192.168.1.22): icmp_seq=2 ttl=64 time=91.7 ms 64 bytes from nano (192.168.1.22): icmp_seq=3 ttl=64 time=113 ms 64 bytes from nano (192.168.1.22): icmp_seq=4 ttl=64 time=31.4 ms 64 bytes from nano (192.168.1.22): icmp_seq=5 ttl=64 time=53.8 ms 64 bytes from nano (192.168.1.22): icmp_seq=6 ttl=64 time=141 ms 64 bytes from nano (192.168.1.22): icmp_seq=7 ttl=64 time=98.0 ms 64 bytes from nano (192.168.1.22): icmp_seq=8 ttl=64 time=1249 ms 64 bytes from nano (192.168.1.22): icmp_seq=9 ttl=64 time=241 ms 64 bytes from nano (192.168.1.22): icmp_seq=10 ttl=64 time=58.6 ms --- nano ping statistics --- 10 packets transmitted, 10 received, 0% packet loss, time 9021ms rtt min/avg/max/mdev = 31.443/214.922/1249.321/349.321 ms, pipe 2 rafal@wilk:~$ ping -c 20 nano PING nano (192.168.1.22) 56(84) bytes of data. 64 bytes from nano (192.168.1.22): icmp_seq=3 ttl=64 time=56.7 ms 64 bytes from nano (192.168.1.22): icmp_seq=4 ttl=64 time=179 ms 64 bytes from nano (192.168.1.22): icmp_seq=7 ttl=64 time=19.0 ms 64 bytes from nano (192.168.1.22): icmp_seq=8 ttl=64 time=141 ms 64 bytes from nano (192.168.1.22): icmp_seq=10 ttl=64 time=593 ms 64 bytes from nano (192.168.1.22): icmp_seq=9 ttl=64 time=1598 ms 64 bytes from nano (192.168.1.22): icmp_seq=11 ttl=64 time=104 ms 64 bytes from nano (192.168.1.22): icmp_seq=12 ttl=64 time=127 ms 64 bytes from nano (192.168.1.22): icmp_seq=15 ttl=64 time=34.6 ms 64 bytes from nano (192.168.1.22): icmp_seq=16 ttl=64 time=167 ms 64 bytes from nano (192.168.1.22): icmp_seq=19 ttl=64 time=107 ms 64 bytes from nano (192.168.1.22): icmp_seq=20 ttl=64 time=230 ms --- nano ping statistics --- 20 packets transmitted, 12 received,
Re: brcfmac: add possibility to turn off powersave on wiphy
On 7/21/2017 11:19 PM, Rafal wrote: On Fri, 21 Jul 2017, Dan Williams wrote: On Fri, 2017-07-21 at 16:56 +0200, Rafal wrote: I have a board with ap6212 chip and I have encountered two problems with the brcmfmac driver operating with this device. First problem I describe below, second one in separate mail. Namely, I have noticed quite poor connection with the device despite good signal strength. Ping to the device was very uneven, about 50 - 100 ms in average, 10 - 20% of packets loss. After some investigations I have found that problem is caused by powersave feature on wiphy (BRCMF_C_SET_PM command). When the value is set to PM_OFF, the problem does not appear - ping is quite stable, ~2ms. When set to PM_FAST, the ping is bad. The brcmfmac driver currently does not have possibility to turn off the powersave feature. I have added the possibility on 4.11.6 kernel I don't think that's true? The driver implements the nl80211 set_power_mgmt hook, which lets you turn off powersave with 'iw'. That seems like a much better option than a module parameter. I know other drivers have the module parameter, but I personally consider that legacy/holdover, given that we have runtime toggles for this kind of thing. brcmf_cfg80211_set_power_mgmt() should do what you need, right? Yes, it does. In fact, I needed only a parameter in OF database. This is a bug in the device or firmware and I needed to disable the feature for this chip. Many device drivers allow to specify in OF database that driver should use a quirk. This is similar situation. I have added the kernel parameter as an additional feature. I thought that not all platforms are using devicetree database. But maybe it is a bad idea. I was going to make the same remark as Dan. Anyway, the ping times are not really unexpected. It depends on your beacon period and dtim setting. However, the packet loss is not expected and will require analyzing a sniff capture. Regards, Arend
Re: brcfmac: add possibility to turn off powersave on wiphy
On Fri, 2017-07-21 at 23:19 +0200, Rafal wrote: > On Fri, 21 Jul 2017, Dan Williams wrote: > > > On Fri, 2017-07-21 at 16:56 +0200, Rafal wrote: > > > I have a board with ap6212 chip and I have encountered two > > > problems > > > with the brcmfmac driver operating with this device. First > > > problem I > > > describe below, second one in separate mail. > > > > > > > > > Namely, I have noticed quite poor connection with the device > > > despite > > > good signal strength. Ping to the device was very uneven, about > > > 50 - > > > 100 ms in average, 10 - 20% of packets loss. After some > > > investigations I have found that problem is caused by powersave > > > feature on wiphy (BRCMF_C_SET_PM command). When the value is set > > > to > > > PM_OFF, the problem does not appear - ping is quite stable, ~2ms. > > > When set to PM_FAST, the ping is bad. > > > > > > The brcmfmac driver currently does not have possibility to turn > > > off > > > the powersave feature. I have added the possibility on 4.11.6 > > > kernel > > > > I don't think that's true? The driver implements the nl80211 > > set_power_mgmt hook, which lets you turn off powersave with > > 'iw'. That > > seems like a much better option than a module parameter. I know > > other > > drivers have the module parameter, but I personally consider that > > legacy/holdover, given that we have runtime toggles for this kind > > of > > thing. > > > > brcmf_cfg80211_set_power_mgmt() should do what you need, right? > > Yes, it does. > > In fact, I needed only a parameter in OF database. This is a bug in > the > device or firmware and I needed to disable the feature for this chip. > Many device drivers allow to specify in OF database that driver > should > use a quirk. This is similar situation. Does the power management issue cause problems before any association happens? If not, then I'd suggest 'iw' in startup scripts somewhere. Or better yet, use the normal quirk method of detecting the hardware version via IDs or detecting the firmware via version or feature strings and quirking on that. Dan > I have added the kernel parameter as an additional feature. I thought > that not all platforms are using devicetree database. But maybe it is > a > bad idea. > > Rafal > > > > > Dan > >
Re: brcfmac: add possibility to turn off powersave on wiphy
On Fri, 21 Jul 2017, Dan Williams wrote: On Fri, 2017-07-21 at 16:56 +0200, Rafal wrote: I have a board with ap6212 chip and I have encountered two problems with the brcmfmac driver operating with this device. First problem I describe below, second one in separate mail. Namely, I have noticed quite poor connection with the device despite good signal strength. Ping to the device was very uneven, about 50 - 100 ms in average, 10 - 20% of packets loss. After some investigations I have found that problem is caused by powersave feature on wiphy (BRCMF_C_SET_PM command). When the value is set to PM_OFF, the problem does not appear - ping is quite stable, ~2ms. When set to PM_FAST, the ping is bad. The brcmfmac driver currently does not have possibility to turn off the powersave feature. I have added the possibility on 4.11.6 kernel I don't think that's true? The driver implements the nl80211 set_power_mgmt hook, which lets you turn off powersave with 'iw'. That seems like a much better option than a module parameter. I know other drivers have the module parameter, but I personally consider that legacy/holdover, given that we have runtime toggles for this kind of thing. brcmf_cfg80211_set_power_mgmt() should do what you need, right? Yes, it does. In fact, I needed only a parameter in OF database. This is a bug in the device or firmware and I needed to disable the feature for this chip. Many device drivers allow to specify in OF database that driver should use a quirk. This is similar situation. I have added the kernel parameter as an additional feature. I thought that not all platforms are using devicetree database. But maybe it is a bad idea. Rafal Dan
Re: brcfmac: add possibility to turn off powersave on wiphy
On Fri, 2017-07-21 at 16:56 +0200, Rafal wrote: > I have a board with ap6212 chip and I have encountered two problems > with the brcmfmac driver operating with this device. First problem I > describe below, second one in separate mail. > > > Namely, I have noticed quite poor connection with the device despite > good signal strength. Ping to the device was very uneven, about 50 - > 100 ms in average, 10 - 20% of packets loss. After some > investigations I have found that problem is caused by powersave > feature on wiphy (BRCMF_C_SET_PM command). When the value is set to > PM_OFF, the problem does not appear - ping is quite stable, ~2ms. > When set to PM_FAST, the ping is bad. > > The brcmfmac driver currently does not have possibility to turn off > the powersave feature. I have added the possibility on 4.11.6 kernel I don't think that's true? The driver implements the nl80211 set_power_mgmt hook, which lets you turn off powersave with 'iw'. That seems like a much better option than a module parameter. I know other drivers have the module parameter, but I personally consider that legacy/holdover, given that we have runtime toggles for this kind of thing. brcmf_cfg80211_set_power_mgmt() should do what you need, right? Dan > in my sandbox, but maybe the change is worth to add in general. I'm > providing my patch for reference. This change allows to turn off > powersave in two ways. First one, by specify "brcm,powersave-default- > off" option in OF devicetree. Second one - as a module parameter. The > parameter is named powersave_default and is tri-state: > > value >0 enables powersave > value =0 disables powersave > value <0 (the default) does not override powersave value, i.e. value > from devicetree or driver default is in effect. > > Rafal > > > diff --git > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index 944b83c..86cd1f7 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -5702,7 +5702,7 @@ static s32 wl_init_priv(struct > brcmf_cfg80211_info *cfg) > s32 err = 0; > > cfg->scan_request = NULL; > - cfg->pwr_save = true; > + cfg->pwr_save = !cfg->pub->settings->powersave_default_off; > cfg->active_scan = true;/* we do active scan per > default */ > cfg->dongle_up = false; /* dongle is not up > yet */ > err = brcmf_init_priv_mem(cfg); > @@ -6450,8 +6450,9 @@ static int brcmf_setup_wiphy(struct wiphy > *wiphy, struct brcmf_if *ifp) > BIT(NL80211_BSS_SELECT_ATTR_BAN > D_PREF) | > BIT(NL80211_BSS_SELECT_ATTR_RSS > I_ADJUST); > > - wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT | > - WIPHY_FLAG_OFFCHAN_TX | > + if( ! ifp->drvr->settings->powersave_default_off ) > + wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT; > + wiphy->flags |= WIPHY_FLAG_OFFCHAN_TX | > WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; > if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_TDLS)) > wiphy->flags |= WIPHY_FLAG_SUPPORTS_TDLS; > diff --git > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > index 33b133f..191424e 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > @@ -80,6 +80,10 @@ module_param_named(ignore_probe_fail, > brcmf_ignore_probe_fail, int, 0); > MODULE_PARM_DESC(ignore_probe_fail, "always succeed probe for > debugging"); > #endif > > +static int brcmf_powersave_default = -1; > +module_param_named(powersave_default, brcmf_powersave_default, int, > 0); > +MODULE_PARM_DESC(powersave_default, "Set powersave default on/off on > wiphy"); > + > static struct brcmfmac_platform_data *brcmfmac_pdata; > struct brcmf_mp_global_t brcmf_mp_global; > > @@ -319,6 +323,8 @@ struct brcmf_mp_device > *brcmf_get_module_param(struct device *dev, > /* No platform data for this device, try OF (Open > Firwmare) */ > brcmf_of_probe(dev, bus_type, settings); > } > + if( brcmf_powersave_default >= 0 ) > + settings->powersave_default_off = > !brcmf_powersave_default; > return settings; > } > > diff --git > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > index a62f8e7..ab67fcf 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > @@ -59,6 +59,7 @@ struct brcmf_mp_device { > int fcmode; > boolroamoff; > boolignore_probe_fail; > + boolpowersave_default_off; > struct brcmfmac_pd_cc *country_codes; > union { >
brcfmac: add possibility to turn off powersave on wiphy
I have a board with ap6212 chip and I have encountered two problems with the brcmfmac driver operating with this device. First problem I describe below, second one in separate mail. Namely, I have noticed quite poor connection with the device despite good signal strength. Ping to the device was very uneven, about 50 - 100 ms in average, 10 - 20% of packets loss. After some investigations I have found that problem is caused by powersave feature on wiphy (BRCMF_C_SET_PM command). When the value is set to PM_OFF, the problem does not appear - ping is quite stable, ~2ms. When set to PM_FAST, the ping is bad. The brcmfmac driver currently does not have possibility to turn off the powersave feature. I have added the possibility on 4.11.6 kernel in my sandbox, but maybe the change is worth to add in general. I'm providing my patch for reference. This change allows to turn off powersave in two ways. First one, by specify "brcm,powersave-default-off" option in OF devicetree. Second one - as a module parameter. The parameter is named powersave_default and is tri-state: value >0 enables powersave value =0 disables powersave value <0 (the default) does not override powersave value, i.e. value from devicetree or driver default is in effect. Rafal diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 944b83c..86cd1f7 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -5702,7 +5702,7 @@ static s32 wl_init_priv(struct brcmf_cfg80211_info *cfg) s32 err = 0; cfg->scan_request = NULL; - cfg->pwr_save = true; + cfg->pwr_save = !cfg->pub->settings->powersave_default_off; cfg->active_scan = true; /* we do active scan per default */ cfg->dongle_up = false; /* dongle is not up yet */ err = brcmf_init_priv_mem(cfg); @@ -6450,8 +6450,9 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp) BIT(NL80211_BSS_SELECT_ATTR_BAND_PREF) | BIT(NL80211_BSS_SELECT_ATTR_RSSI_ADJUST); - wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT | - WIPHY_FLAG_OFFCHAN_TX | + if( ! ifp->drvr->settings->powersave_default_off ) + wiphy->flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT; + wiphy->flags |= WIPHY_FLAG_OFFCHAN_TX | WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_TDLS)) wiphy->flags |= WIPHY_FLAG_SUPPORTS_TDLS; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index 33b133f..191424e 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -80,6 +80,10 @@ module_param_named(ignore_probe_fail, brcmf_ignore_probe_fail, int, 0); MODULE_PARM_DESC(ignore_probe_fail, "always succeed probe for debugging"); #endif +static int brcmf_powersave_default = -1; +module_param_named(powersave_default, brcmf_powersave_default, int, 0); +MODULE_PARM_DESC(powersave_default, "Set powersave default on/off on wiphy"); + static struct brcmfmac_platform_data *brcmfmac_pdata; struct brcmf_mp_global_t brcmf_mp_global; @@ -319,6 +323,8 @@ struct brcmf_mp_device *brcmf_get_module_param(struct device *dev, /* No platform data for this device, try OF (Open Firwmare) */ brcmf_of_probe(dev, bus_type, settings); } + if( brcmf_powersave_default >= 0 ) + settings->powersave_default_off = !brcmf_powersave_default; return settings; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h index a62f8e7..ab67fcf 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h @@ -59,6 +59,7 @@ struct brcmf_mp_device { int fcmode; boolroamoff; boolignore_probe_fail; + boolpowersave_default_off; struct brcmfmac_pd_cc *country_codes; union { struct brcmfmac_sdio_pd sdio; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index aee6e59..904ba11 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c @@ -39,6 +39,9 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, if (of_property_read_u32(np, "brcm,drive-strength", ) == 0) sdio->drive_strength = val; + settings->powersave_default_off = of_property_read_bool(np, +