| Message ID | E1sClp4-00Evu7-8v@rmk-PC.armlinux.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [wireless,v2] wifi: wlcore: fix wlcore AP mode | expand |
Hi Kale, I see all my TI Wilink patches have been marked as "deferred" in the wireless patchwork. Please could you explain what the plan is with these patches, especially this one which fixes a serious frustrating failing that makes AP mode on this hardware very unreliable and thus useless. Thanks. On Thu, May 30, 2024 at 08:52:26PM +0100, Russell King (Oracle) wrote: > Using wl183x devices in AP mode with various firmwares is not stable. > > The driver currently adds a station to firmware with basic rates when it > is first known to the stack using the CMD_ADD_PEER command. Once the > station has finished authorising, another CMD_ADD_PEER command is issued > to update the firmware with the rates the station can use. > > However, after a random amount of time, the firmware ignores the power > management nullfunc frames from the station, and tries to send packets > while the station is asleep, resulting in lots of retries dropping down > in rate due to no response. This restricts the available bandwidth. > > With this happening with several stations, the user visible effect is > the latency of interactive connections increases significantly, packets > get dropped, and in general the WiFi connections become unreliable and > unstable. > > Eventually, the firmware transmit queue appears to get stuck - with > packets and blocks allocated that never clear. > > TI have a couple of patches that address this, but they touch the > mac80211 core to disable NL80211_FEATURE_FULL_AP_CLIENT_STATE for *all* > wireless drivers, which has the effect of not adding the station to the > stack until later when the rates are known. This is a sledge hammer > approach to solving the problem. > > The solution implemented here has the same effect, but without > impacting all drivers. > > We delay adding the station to firmware until it has been authorised > in the driver, and correspondingly remove the station when unwinding > from authorised state. Adding the station to firmware allocates a hlid, > which will now happen later than the driver expects. Therefore, we need > to track when this happens so that we transmit using the correct hlid. > > This patch is an equivalent fix to these two patches in TI's > wilink8-wlan repository: > > https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0004-mac80211-patch.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 > https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0005-wlcore-patch.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 > > Reported-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Co-developed-by: Johannes Berg <johannes.berg@intel.com> > Tested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > v2: switch authorship and credits as requested by Johannes. > --- > drivers/net/wireless/ti/wlcore/cmd.c | 7 ------- > drivers/net/wireless/ti/wlcore/main.c | 17 ++++++++--------- > drivers/net/wireless/ti/wlcore/tx.c | 7 ++----- > drivers/net/wireless/ti/wlcore/wlcore_i.h | 6 ++++++ > 4 files changed, 16 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c > index a939fd89a7f5..92fc2d456c2c 100644 > --- a/drivers/net/wireless/ti/wlcore/cmd.c > +++ b/drivers/net/wireless/ti/wlcore/cmd.c > @@ -1566,13 +1566,6 @@ int wl12xx_cmd_add_peer(struct wl1271 *wl, struct wl12xx_vif *wlvif, > cpu_to_le32(wl1271_tx_enabled_rates_get(wl, sta_rates, > wlvif->band)); > > - if (!cmd->supported_rates) { > - wl1271_debug(DEBUG_CMD, > - "peer has no supported rates yet, configuring basic rates: 0x%x", > - wlvif->basic_rate_set); > - cmd->supported_rates = cpu_to_le32(wlvif->basic_rate_set); > - } > - > wl1271_debug(DEBUG_CMD, "new peer rates=0x%x queues=0x%x", > cmd->supported_rates, sta->uapsd_queues); > > diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c > index ef12169f8044..492cd7aef44f 100644 > --- a/drivers/net/wireless/ti/wlcore/main.c > +++ b/drivers/net/wireless/ti/wlcore/main.c > @@ -5139,19 +5139,23 @@ static int wl12xx_update_sta_state(struct wl1271 *wl, > > /* Add station (AP mode) */ > if (is_ap && > - old_state == IEEE80211_STA_NOTEXIST && > - new_state == IEEE80211_STA_NONE) { > + old_state == IEEE80211_STA_AUTH && > + new_state == IEEE80211_STA_ASSOC) { > ret = wl12xx_sta_add(wl, wlvif, sta); > if (ret) > return ret; > > + wl_sta->fw_added = true; > + > wlcore_update_inconn_sta(wl, wlvif, wl_sta, true); > } > > /* Remove station (AP mode) */ > if (is_ap && > - old_state == IEEE80211_STA_NONE && > - new_state == IEEE80211_STA_NOTEXIST) { > + old_state == IEEE80211_STA_ASSOC && > + new_state == IEEE80211_STA_AUTH) { > + wl_sta->fw_added = false; > + > /* must not fail */ > wl12xx_sta_remove(wl, wlvif, sta); > > @@ -5165,11 +5169,6 @@ static int wl12xx_update_sta_state(struct wl1271 *wl, > if (ret < 0) > return ret; > > - /* reconfigure rates */ > - ret = wl12xx_cmd_add_peer(wl, wlvif, sta, wl_sta->hlid); > - if (ret < 0) > - return ret; > - > ret = wl1271_acx_set_ht_capabilities(wl, &sta->deflink.ht_cap, > true, > wl_sta->hlid); > diff --git a/drivers/net/wireless/ti/wlcore/tx.c b/drivers/net/wireless/ti/wlcore/tx.c > index 7bd3ce2f0804..464587d16ab2 100644 > --- a/drivers/net/wireless/ti/wlcore/tx.c > +++ b/drivers/net/wireless/ti/wlcore/tx.c > @@ -140,11 +140,8 @@ EXPORT_SYMBOL(wl12xx_is_dummy_packet); > static u8 wl12xx_tx_get_hlid_ap(struct wl1271 *wl, struct wl12xx_vif *wlvif, > struct sk_buff *skb, struct ieee80211_sta *sta) > { > - if (sta) { > - struct wl1271_station *wl_sta; > - > - wl_sta = (struct wl1271_station *)sta->drv_priv; > - return wl_sta->hlid; > + if (sta && wl1271_station(sta)->fw_added) { > + return wl1271_station(sta)->hlid; > } else { > struct ieee80211_hdr *hdr; > > diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h > index eefae3f867b9..817a8a61cac6 100644 > --- a/drivers/net/wireless/ti/wlcore/wlcore_i.h > +++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h > @@ -324,6 +324,7 @@ struct wl12xx_rx_filter { > > struct wl1271_station { > u8 hlid; > + bool fw_added; > bool in_connection; > > /* > @@ -335,6 +336,11 @@ struct wl1271_station { > u64 total_freed_pkts; > }; > > +static inline struct wl1271_station *wl1271_station(struct ieee80211_sta *sta) > +{ > + return (struct wl1271_station *)sta->drv_priv; > +} > + > struct wl12xx_vif { > struct wl1271 *wl; > struct list_head list; > -- > 2.30.2 > >
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > I see all my TI Wilink patches have been marked as "deferred" in the > wireless patchwork. Please could you explain what the plan is with > these patches, especially this one which fixes a serious frustrating > failing that makes AP mode on this hardware very unreliable and thus > useless. I'm just swamped with patches, I'll try to look at these soon. I wish that TI would take a more active role in upstream, for example reviewing and testing patches would help a lot.
On Mon, Jun 17, 2024 at 01:56:48PM +0300, Kalle Valo wrote: > "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > > > I see all my TI Wilink patches have been marked as "deferred" in the > > wireless patchwork. Please could you explain what the plan is with > > these patches, especially this one which fixes a serious frustrating > > failing that makes AP mode on this hardware very unreliable and thus > > useless. > > I'm just swamped with patches, I'll try to look at these soon. > > I wish that TI would take a more active role in upstream, for example > reviewing and testing patches would help a lot. I believe the problem has been that TI have had an attitude of "we only support people using 4.19.38, if you can't reproduce the problem there we aren't interested". To see the versions they support: https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 basically, all are ancient. They also appear take the attitude that all the kernel code is ripe for them to hack about with - whcih is why this fix has had to be reworked so it isn't removing NL80211_FEATURE_FULL_AP_CLIENT_STATE for _all_ kernel wireless drivers! Also, I think they also require one to use their hostapd and wpa_supplicant, probably for a similar reason. I know that in some of the patches they've hacked in API changes... Then one can see the attitude of lock-step firmware and driver upgrade - you can't use 8.9.1.x.x firmware with their older driver, and you can't use 8.9.0.x.x with their newer driver. That, of course, is not acceptable to mainline. So, given all this, IMHO it's probably a good thing TI aren't trying to submit their stuff upstream... that is, unless they are willing to learn how to "do things correctly". Maybe I'm being too hard on TI's wireless division, but that seems to be what has been going on.
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > On Mon, Jun 17, 2024 at 01:56:48PM +0300, Kalle Valo wrote: >> "Russell King (Oracle)" <linux@armlinux.org.uk> writes: >> >> > I see all my TI Wilink patches have been marked as "deferred" in the >> > wireless patchwork. Please could you explain what the plan is with >> > these patches, especially this one which fixes a serious frustrating >> > failing that makes AP mode on this hardware very unreliable and thus >> > useless. >> >> I'm just swamped with patches, I'll try to look at these soon. >> >> I wish that TI would take a more active role in upstream, for example >> reviewing and testing patches would help a lot. > > I believe the problem has been that TI have had an attitude of "we > only support people using 4.19.38, if you can't reproduce the problem > there we aren't interested". To see the versions they support: > > https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 > > basically, all are ancient. > > They also appear take the attitude that all the kernel code is ripe > for them to hack about with - whcih is why this fix has had to be > reworked so it isn't removing NL80211_FEATURE_FULL_AP_CLIENT_STATE > for _all_ kernel wireless drivers! > > Also, I think they also require one to use their hostapd and > wpa_supplicant, probably for a similar reason. I know that in some > of the patches they've hacked in API changes... > > Then one can see the attitude of lock-step firmware and driver > upgrade - you can't use 8.9.1.x.x firmware with their older driver, > and you can't use 8.9.0.x.x with their newer driver. That, of course, > is not acceptable to mainline. > > So, given all this, IMHO it's probably a good thing TI aren't trying > to submit their stuff upstream... that is, unless they are willing > to learn how to "do things correctly". > > Maybe I'm being too hard on TI's wireless division, but that seems to > be what has been going on. Yeah, the all you describe above is very common in wireless vendors :/ But vendors do learn, Realtek is a great example of that. Let's hope that TI does too.
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote: > Using wl183x devices in AP mode with various firmwares is not stable. > > The driver currently adds a station to firmware with basic rates when it > is first known to the stack using the CMD_ADD_PEER command. Once the > station has finished authorising, another CMD_ADD_PEER command is issued > to update the firmware with the rates the station can use. > > However, after a random amount of time, the firmware ignores the power > management nullfunc frames from the station, and tries to send packets > while the station is asleep, resulting in lots of retries dropping down > in rate due to no response. This restricts the available bandwidth. > > With this happening with several stations, the user visible effect is > the latency of interactive connections increases significantly, packets > get dropped, and in general the WiFi connections become unreliable and > unstable. > > Eventually, the firmware transmit queue appears to get stuck - with > packets and blocks allocated that never clear. > > TI have a couple of patches that address this, but they touch the > mac80211 core to disable NL80211_FEATURE_FULL_AP_CLIENT_STATE for *all* > wireless drivers, which has the effect of not adding the station to the > stack until later when the rates are known. This is a sledge hammer > approach to solving the problem. > > The solution implemented here has the same effect, but without > impacting all drivers. > > We delay adding the station to firmware until it has been authorised > in the driver, and correspondingly remove the station when unwinding > from authorised state. Adding the station to firmware allocates a hlid, > which will now happen later than the driver expects. Therefore, we need > to track when this happens so that we transmit using the correct hlid. > > This patch is an equivalent fix to these two patches in TI's > wilink8-wlan repository: > > https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0004-mac80211-patch.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 > https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0005-wlcore-patch.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 > > Reported-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Co-developed-by: Johannes Berg <johannes.berg@intel.com> > Tested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Patch applied to wireless.git, thanks. 14f89946b6b3 wifi: wlcore: fix wlcore AP mode
On 6/18/2024 12:16 PM, Kalle Valo wrote: > > "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > >> On Mon, Jun 17, 2024 at 01:56:48PM +0300, Kalle Valo wrote: >>> "Russell King (Oracle)" <linux@armlinux.org.uk> writes: >>> >>> > I see all my TI Wilink patches have been marked as "deferred" in the >>> > wireless patchwork. Please could you explain what the plan is with >>> > these patches, especially this one which fixes a serious frustrating >>> > failing that makes AP mode on this hardware very unreliable and thus >>> > useless. >>> >>> I'm just swamped with patches, I'll try to look at these soon. >>> >>> I wish that TI would take a more active role in upstream, for example >>> reviewing and testing patches would help a lot. >> >> I believe the problem has been that TI have had an attitude of "we >> only support people using 4.19.38, if you can't reproduce the problem >> there we aren't interested". To see the versions they support: >> >> https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7 >> >> basically, all are ancient. >> >> They also appear take the attitude that all the kernel code is ripe >> for them to hack about with - whcih is why this fix has had to be >> reworked so it isn't removing NL80211_FEATURE_FULL_AP_CLIENT_STATE >> for _all_ kernel wireless drivers! >> >> Also, I think they also require one to use their hostapd and >> wpa_supplicant, probably for a similar reason. I know that in some >> of the patches they've hacked in API changes... >> >> Then one can see the attitude of lock-step firmware and driver >> upgrade - you can't use 8.9.1.x.x firmware with their older driver, >> and you can't use 8.9.0.x.x with their newer driver. That, of course, >> is not acceptable to mainline. >> >> So, given all this, IMHO it's probably a good thing TI aren't trying >> to submit their stuff upstream... that is, unless they are willing >> to learn how to "do things correctly". >> >> Maybe I'm being too hard on TI's wireless division, but that seems to >> be what has been going on. > > Yeah, the all you describe above is very common in wireless vendors :/ > But vendors do learn, Realtek is a great example of that. Let's hope > that TI does too. I can say that the driver for the next generation of chips (CC33xx) does not and will not require any modifications to mac80211, cfg80211 and supplicant. I will also try to aid and review patches for wl18xx. Michael.
diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c index a939fd89a7f5..92fc2d456c2c 100644 --- a/drivers/net/wireless/ti/wlcore/cmd.c +++ b/drivers/net/wireless/ti/wlcore/cmd.c @@ -1566,13 +1566,6 @@ int wl12xx_cmd_add_peer(struct wl1271 *wl, struct wl12xx_vif *wlvif, cpu_to_le32(wl1271_tx_enabled_rates_get(wl, sta_rates, wlvif->band)); - if (!cmd->supported_rates) { - wl1271_debug(DEBUG_CMD, - "peer has no supported rates yet, configuring basic rates: 0x%x", - wlvif->basic_rate_set); - cmd->supported_rates = cpu_to_le32(wlvif->basic_rate_set); - } - wl1271_debug(DEBUG_CMD, "new peer rates=0x%x queues=0x%x", cmd->supported_rates, sta->uapsd_queues); diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c index ef12169f8044..492cd7aef44f 100644 --- a/drivers/net/wireless/ti/wlcore/main.c +++ b/drivers/net/wireless/ti/wlcore/main.c @@ -5139,19 +5139,23 @@ static int wl12xx_update_sta_state(struct wl1271 *wl, /* Add station (AP mode) */ if (is_ap && - old_state == IEEE80211_STA_NOTEXIST && - new_state == IEEE80211_STA_NONE) { + old_state == IEEE80211_STA_AUTH && + new_state == IEEE80211_STA_ASSOC) { ret = wl12xx_sta_add(wl, wlvif, sta); if (ret) return ret; + wl_sta->fw_added = true; + wlcore_update_inconn_sta(wl, wlvif, wl_sta, true); } /* Remove station (AP mode) */ if (is_ap && - old_state == IEEE80211_STA_NONE && - new_state == IEEE80211_STA_NOTEXIST) { + old_state == IEEE80211_STA_ASSOC && + new_state == IEEE80211_STA_AUTH) { + wl_sta->fw_added = false; + /* must not fail */ wl12xx_sta_remove(wl, wlvif, sta); @@ -5165,11 +5169,6 @@ static int wl12xx_update_sta_state(struct wl1271 *wl, if (ret < 0) return ret; - /* reconfigure rates */ - ret = wl12xx_cmd_add_peer(wl, wlvif, sta, wl_sta->hlid); - if (ret < 0) - return ret; - ret = wl1271_acx_set_ht_capabilities(wl, &sta->deflink.ht_cap, true, wl_sta->hlid); diff --git a/drivers/net/wireless/ti/wlcore/tx.c b/drivers/net/wireless/ti/wlcore/tx.c index 7bd3ce2f0804..464587d16ab2 100644 --- a/drivers/net/wireless/ti/wlcore/tx.c +++ b/drivers/net/wireless/ti/wlcore/tx.c @@ -140,11 +140,8 @@ EXPORT_SYMBOL(wl12xx_is_dummy_packet); static u8 wl12xx_tx_get_hlid_ap(struct wl1271 *wl, struct wl12xx_vif *wlvif, struct sk_buff *skb, struct ieee80211_sta *sta) { - if (sta) { - struct wl1271_station *wl_sta; - - wl_sta = (struct wl1271_station *)sta->drv_priv; - return wl_sta->hlid; + if (sta && wl1271_station(sta)->fw_added) { + return wl1271_station(sta)->hlid; } else { struct ieee80211_hdr *hdr; diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h index eefae3f867b9..817a8a61cac6 100644 --- a/drivers/net/wireless/ti/wlcore/wlcore_i.h +++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h @@ -324,6 +324,7 @@ struct wl12xx_rx_filter { struct wl1271_station { u8 hlid; + bool fw_added; bool in_connection; /* @@ -335,6 +336,11 @@ struct wl1271_station { u64 total_freed_pkts; }; +static inline struct wl1271_station *wl1271_station(struct ieee80211_sta *sta) +{ + return (struct wl1271_station *)sta->drv_priv; +} + struct wl12xx_vif { struct wl1271 *wl; struct list_head list;