| Message ID | 20241023133004.2253830-1-kvalo@kernel.org |
|---|---|
| Headers | show |
| Series | wifi: ath12k: MLO support part 2 | expand |
On 10/23/2024 6:29 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Refactor ath12k_mac_op_sta_state(), with generic wrappers which can be used for > both multi link stations and non-ML stations. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 > > Signed-off-by: Sriram R <quic_srirrama@quicinc.com> > Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/core.h | 3 + > drivers/net/wireless/ath/ath12k/mac.c | 341 +++++++++++++++++-------- > 2 files changed, 242 insertions(+), 102 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h > index 06b637ba8b8f..6faa46b9adc9 100644 > --- a/drivers/net/wireless/ath/ath12k/core.h > +++ b/drivers/net/wireless/ath/ath12k/core.h > @@ -461,6 +461,9 @@ struct ath12k_link_sta { > struct ath12k_link_vif *arvif; > struct ath12k_sta *ahsta; > > + /* link address similar to ieee80211_link_sta */ > + u8 addr[ETH_ALEN]; > + > /* the following are protected by ar->data_lock */ > u32 changed; /* IEEE80211_RC_* */ > u32 bw; > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index d4aa4540c8e6..3de6d605cd74 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -4519,10 +4519,10 @@ ath12k_mac_set_peer_vht_fixed_rate(struct ath12k_link_vif *arvif, > return ret; > } > > -static int ath12k_station_assoc(struct ath12k *ar, > - struct ath12k_link_vif *arvif, > - struct ath12k_link_sta *arsta, > - bool reassoc) > +static int ath12k_mac_station_assoc(struct ath12k *ar, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta, > + bool reassoc) > { > struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); > struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); > @@ -4609,28 +4609,19 @@ static int ath12k_station_assoc(struct ath12k *ar, > return 0; > } > > -static int ath12k_station_disassoc(struct ath12k *ar, > - struct ath12k_link_vif *arvif, > - struct ath12k_link_sta *arsta) > +static int ath12k_mac_station_disassoc(struct ath12k *ar, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta) > { > struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); > - int ret; > > lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); > > if (!sta->wme) { > arvif->num_legacy_stations--; > - ret = ath12k_recalc_rtscts_prot(arvif); > - if (ret) > - return ret; > + return ath12k_recalc_rtscts_prot(arvif); > } > > - ret = ath12k_clear_peer_keys(arvif, sta->addr); > - if (ret) { > - ath12k_warn(ar->ab, "failed to clear all peer keys for vdev %i: %d\n", > - arvif->vdev_id, ret); > - return ret; > - } > return 0; > } > > @@ -4826,6 +4817,145 @@ static void ath12k_mac_dec_num_stations(struct ath12k_link_vif *arvif, > ar->num_stations--; > } > > +static void ath12k_mac_station_post_remove(struct ath12k *ar, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta) > +{ > + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); > + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); > + struct ath12k_sta *ahsta = arsta->ahsta; > + struct ath12k_peer *peer; > + > + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); > + > + ath12k_mac_dec_num_stations(arvif, arsta); > + > + spin_lock_bh(&ar->ab->base_lock); > + > + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); > + if (peer && peer->sta == sta) { > + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n", > + vif->addr, arvif->vdev_id); > + peer->sta = NULL; > + list_del(&peer->list); > + kfree(peer); > + ar->num_peers--; > + } > + > + spin_unlock_bh(&ar->ab->base_lock); > + > + kfree(arsta->rx_stats); > + arsta->rx_stats = NULL; > + > + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) { > + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL); > + synchronize_rcu(); I've mentioned this in the past in some internal discussion and seems now is a good time to bring this to light. It concerns me that this happens so late in the process. In theory another thread could already have a valid arsta pointer and could be trying to dereference that pointer while the code above is destroying underlying data (i.e. arsta->rx_stats). Should we set this to NULL and synchronize RCU at the beginning of the process so that we know all access to the struct has finished before we start destroying the data? Or can this not actually happen in practice due to other synchronization mechansims? And if so, should we document that somewhere? > + ahsta->links_map &= ~(BIT(arsta->link_id)); > + arsta->link_id = ATH12K_INVALID_LINK_ID; > + arsta->ahsta = NULL; > + } > +} > + > +static int ath12k_mac_station_unauthorize(struct ath12k *ar, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta) > +{ > + struct ath12k_peer *peer; > + int ret; > + > + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); > + > + spin_lock_bh(&ar->ab->base_lock); > + > + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr); > + if (peer) > + peer->is_authorized = false; > + > + spin_unlock_bh(&ar->ab->base_lock); > + > + /* Driver should clear the peer keys during mac80211's ref ptr > + * gets cleared in __sta_info_destroy_part2 (trans from > + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC) I'm unable to understand this comment > + */ > + ret = ath12k_clear_peer_keys(arvif, arsta->addr); > + if (ret) { > + ath12k_warn(ar->ab, "failed to clear all peer keys for vdev %i: %d\n", > + arvif->vdev_id, ret); > + return ret; > + } > + > + return 0; > +} > + > +static int ath12k_mac_station_authorize(struct ath12k *ar, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta) > +{ > + struct ath12k_peer *peer; > + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); > + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); > + int ret; > + > + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); > + > + spin_lock_bh(&ar->ab->base_lock); > + > + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); > + if (peer) > + peer->is_authorized = true; > + > + spin_unlock_bh(&ar->ab->base_lock); > + > + if (vif->type == NL80211_IFTYPE_STATION && arvif->is_up) { > + ret = ath12k_wmi_set_peer_param(ar, sta->addr, > + arvif->vdev_id, > + WMI_PEER_AUTHORIZE, > + 1); > + if (ret) { > + ath12k_warn(ar->ab, "Unable to authorize peer %pM vdev %d: %d\n", > + sta->addr, arvif->vdev_id, ret); > + return ret; > + } > + } > + > + return 0; > +} > + > +static int ath12k_mac_station_remove(struct ath12k *ar, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta) > +{ > + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); > + struct ath12k_vif *ahvif = arvif->ahvif; > + int ret; > + > + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); > + > + wiphy_work_cancel(ar->ah->hw->wiphy, &arsta->update_wk); > + > + if (ahvif->vdev_type == WMI_VDEV_TYPE_STA) { > + ath12k_bss_disassoc(ar, arvif); > + ret = ath12k_mac_vdev_stop(arvif); > + if (ret) > + ath12k_warn(ar->ab, "failed to stop vdev %i: %d\n", > + arvif->vdev_id, ret); > + } > + > + ath12k_dp_peer_cleanup(ar, arvif->vdev_id, sta->addr); > + > + ret = ath12k_peer_delete(ar, arvif->vdev_id, sta->addr); > + if (ret) > + ath12k_warn(ar->ab, "Failed to delete peer: %pM for VDEV: %d\n", > + sta->addr, arvif->vdev_id); > + else > + ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "Removed peer: %pM for VDEV: %d\n", > + sta->addr, arvif->vdev_id); > + > + ath12k_mac_station_post_remove(ar, arvif, arsta); > + > + return ret; > +} > + > static int ath12k_mac_station_add(struct ath12k *ar, > struct ath12k_link_vif *arvif, > struct ath12k_link_sta *arsta) > @@ -4933,31 +5063,37 @@ static u32 ath12k_mac_ieee80211_sta_bw_to_wmi(struct ath12k *ar, > return bw; > } > > -static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw, > - struct ieee80211_vif *vif, > - struct ieee80211_sta *sta, > - enum ieee80211_sta_state old_state, > - enum ieee80211_sta_state new_state) > +static int ath12k_mac_handle_link_sta_state(struct ieee80211_hw *hw, > + struct ath12k_link_vif *arvif, > + struct ath12k_link_sta *arsta, > + enum ieee80211_sta_state old_state, > + enum ieee80211_sta_state new_state) > { > - struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif); > - struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta); > - struct ath12k *ar; > - struct ath12k_link_vif *arvif; > - struct ath12k_link_sta *arsta; > - struct ath12k_peer *peer; > + struct ath12k *ar = arvif->ar; > + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); > + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); > + struct ath12k_sta *ahsta = arsta->ahsta; > int ret = 0; > > lockdep_assert_wiphy(hw->wiphy); > > - arvif = &ahvif->deflink; > - arsta = &ahsta->deflink; > + /* IEEE80211_STA_NONE -> IEEE80211_STA_NOTEXIST: Remove the station > + * from driver > + */ > + if ((old_state == IEEE80211_STA_NONE && > + new_state == IEEE80211_STA_NOTEXIST)) { > + /* ML sta needs separate handling */ > + if (sta->mlo) > + return 0; > > - ar = ath12k_get_ar_by_vif(hw, vif); > - if (!ar) { > - WARN_ON_ONCE(1); > - return -EINVAL; > + ret = ath12k_mac_station_remove(ar, arvif, arsta); > + if (ret) { > + ath12k_warn(ar->ab, "Failed to remove station: %pM for VDEV: %d\n", > + arsta->addr, arvif->vdev_id); > + } > } > > + /* IEEE80211_STA_NOTEXIST -> IEEE80211_STA_NONE: Add new station to driver */ > if (old_state == IEEE80211_STA_NOTEXIST && > new_state == IEEE80211_STA_NONE) { > memset(arsta, 0, sizeof(*arsta)); > @@ -4975,56 +5111,16 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw, > if (ret) > ath12k_warn(ar->ab, "Failed to add station: %pM for VDEV: %d\n", > sta->addr, arvif->vdev_id); > - } else if ((old_state == IEEE80211_STA_NONE && > - new_state == IEEE80211_STA_NOTEXIST)) { > - wiphy_work_cancel(hw->wiphy, &arsta->update_wk); > > - if (ahvif->vdev_type == WMI_VDEV_TYPE_STA) { > - ath12k_bss_disassoc(ar, arvif); > - ret = ath12k_mac_vdev_stop(arvif); > - if (ret) > - ath12k_warn(ar->ab, "failed to stop vdev %i: %d\n", > - arvif->vdev_id, ret); > - } > - ath12k_dp_peer_cleanup(ar, arvif->vdev_id, sta->addr); > - > - ret = ath12k_peer_delete(ar, arvif->vdev_id, sta->addr); > - if (ret) > - ath12k_warn(ar->ab, "Failed to delete peer: %pM for VDEV: %d\n", > - sta->addr, arvif->vdev_id); > - else > - ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "Removed peer: %pM for VDEV: %d\n", > - sta->addr, arvif->vdev_id); > - > - ath12k_mac_dec_num_stations(arvif, arsta); > - spin_lock_bh(&ar->ab->base_lock); > - peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); > - if (peer && peer->sta == sta) { > - ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n", > - vif->addr, arvif->vdev_id); > - peer->sta = NULL; > - list_del(&peer->list); > - kfree(peer); > - ar->num_peers--; > - } > - spin_unlock_bh(&ar->ab->base_lock); > - > - kfree(arsta->rx_stats); > - arsta->rx_stats = NULL; > - > - if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) { > - rcu_assign_pointer(ahsta->link[arsta->link_id], NULL); > - synchronize_rcu(); > - ahsta->links_map &= ~(BIT(arsta->link_id)); > - arsta->link_id = ATH12K_INVALID_LINK_ID; > - arsta->ahsta = NULL; > - } > + /* IEEE80211_STA_AUTH -> IEEE80211_STA_ASSOC: Send station assoc command for > + * peer associated to AP/Mesh/ADHOC vif type. > + */ > } else if (old_state == IEEE80211_STA_AUTH && > new_state == IEEE80211_STA_ASSOC && > (vif->type == NL80211_IFTYPE_AP || > vif->type == NL80211_IFTYPE_MESH_POINT || > vif->type == NL80211_IFTYPE_ADHOC)) { > - ret = ath12k_station_assoc(ar, arvif, arsta, false); > + ret = ath12k_mac_station_assoc(ar, arvif, arsta, false); > if (ret) > ath12k_warn(ar->ab, "Failed to associate station: %pM\n", > sta->addr); > @@ -5035,40 +5131,32 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw, > arsta->bw_prev = sta->deflink.bandwidth; > > spin_unlock_bh(&ar->data_lock); > + > + /* IEEE80211_STA_ASSOC -> IEEE80211_STA_AUTHORIZED: set peer status as > + * authorized > + */ > } else if (old_state == IEEE80211_STA_ASSOC && > new_state == IEEE80211_STA_AUTHORIZED) { > - spin_lock_bh(&ar->ab->base_lock); > + ret = ath12k_mac_station_authorize(ar, arvif, arsta); > + if (ret) > + ath12k_warn(ar->ab, "Failed to authorize station: %pM\n", > + sta->addr); > > - peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); > - if (peer) > - peer->is_authorized = true; > - > - spin_unlock_bh(&ar->ab->base_lock); > - > - if (vif->type == NL80211_IFTYPE_STATION && arvif->is_up) { > - ret = ath12k_wmi_set_peer_param(ar, sta->addr, > - arvif->vdev_id, > - WMI_PEER_AUTHORIZE, > - 1); > - if (ret) > - ath12k_warn(ar->ab, "Unable to authorize peer %pM vdev %d: %d\n", > - sta->addr, arvif->vdev_id, ret); > - } > + /* IEEE80211_STA_AUTHORIZED -> IEEE80211_STA_ASSOC: station may be in removal, > + * deauthorize it. > + */ > } else if (old_state == IEEE80211_STA_AUTHORIZED && > new_state == IEEE80211_STA_ASSOC) { > - spin_lock_bh(&ar->ab->base_lock); > - > - peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); > - if (peer) > - peer->is_authorized = false; > - > - spin_unlock_bh(&ar->ab->base_lock); > + ath12k_mac_station_unauthorize(ar, arvif, arsta); > + /* IEEE80211_STA_ASSOC -> IEEE80211_STA_AUTH: disassoc peer connected to > + * AP/mesh/ADHOC vif type. > + */ > } else if (old_state == IEEE80211_STA_ASSOC && > new_state == IEEE80211_STA_AUTH && > (vif->type == NL80211_IFTYPE_AP || > vif->type == NL80211_IFTYPE_MESH_POINT || > vif->type == NL80211_IFTYPE_ADHOC)) { > - ret = ath12k_station_disassoc(ar, arvif, arsta); > + ret = ath12k_mac_station_disassoc(ar, arvif, arsta); > if (ret) > ath12k_warn(ar->ab, "Failed to disassociate station: %pM\n", > sta->addr); > @@ -5077,6 +5165,55 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw, > return ret; > } > > +static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif, > + struct ieee80211_sta *sta, > + enum ieee80211_sta_state old_state, > + enum ieee80211_sta_state new_state) > +{ > + struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif); > + struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta); > + struct ath12k_link_vif *arvif; > + struct ath12k_link_sta *arsta; > + int ret; > + u8 link_id = 0; > + > + lockdep_assert_wiphy(hw->wiphy); > + > + if (ieee80211_vif_is_mld(vif) && sta->valid_links) { > + WARN_ON(!sta->mlo && hweight16(sta->valid_links) != 1); > + link_id = ffs(sta->valid_links) - 1; > + } > + > + /* Handle for non-ML station */ > + if (!sta->mlo) { > + arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]); > + arsta = &ahsta->deflink; > + arsta->ahsta = ahsta; > + > + if (WARN_ON(!arvif || !arsta)) { > + ret = -EINVAL; > + goto exit; > + } > + > + /* vdev might be in deleted */ > + if (WARN_ON(!arvif->ar)) { > + ret = -EINVAL; > + goto exit; > + } > + > + ret = ath12k_mac_handle_link_sta_state(hw, arvif, arsta, > + old_state, new_state); > + if (ret) > + goto exit; > + } > + > + ret = 0; > + > +exit: > + return ret; > +} > + > static int ath12k_mac_op_sta_set_txpwr(struct ieee80211_hw *hw, > struct ieee80211_vif *vif, > struct ieee80211_sta *sta)
On 10/23/2024 6:30 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Add helper functions for multi link peer addition and deletion. And add address > validation to ensure we are not creating link peers (belonging to different > clients) with same MLD address. To aid in this validation for faster lookup, > add a new list of ML peers to struct ath12k_hw::ml_peers and use the same for > parsing for the above address validation use cases. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 > > Signed-off-by: Sriram R <quic_srirrama@quicinc.com> > Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- ... > +int ath12k_peer_mlo_create(struct ath12k_hw *ah, struct ieee80211_sta *sta) > +{ > + struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta); > + struct ath12k_ml_peer *ml_peer; > + > + lockdep_assert_wiphy(ah->hw->wiphy); > + > + if (!sta->mlo) > + return -EINVAL; > + > + ml_peer = ath12k_ml_peer_find(ah, sta->addr); > + if (ml_peer) { > + ath12k_hw_warn(ah, "ML peer (%d) exists already, unable to add new entry for %pM", The Linux coding style says: Printing numbers in parentheses (%d) adds no value and should be avoided. > + ml_peer->id, sta->addr); > + return -EEXIST; > + }
On 10/23/2024 6:30 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Multi-link stations are identified in driver using the multi-link > peer id. Add a helper to find multi-link station using the ML > peer id. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 > > Signed-off-by: Sriram R <quic_srirrama@quicinc.com> > Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/dp.h | 2 ++ > drivers/net/wireless/ath/ath12k/peer.c | 17 +++++++++++++++++ > drivers/net/wireless/ath/ath12k/peer.h | 2 ++ > 3 files changed, 21 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h > index 2e05fc19410e..66b60f772efb 100644 > --- a/drivers/net/wireless/ath/ath12k/dp.h > +++ b/drivers/net/wireless/ath/ath12k/dp.h > @@ -1796,6 +1796,8 @@ static inline void ath12k_dp_get_mac_addr(u32 addr_l32, u16 addr_h16, u8 *addr) > memcpy(addr + 4, &addr_h16, ETH_ALEN - 4); > } > > +#define ATH12K_ML_PEER_ID_VALID BIT(13) > + this seems to be randomly placed without any context > int ath12k_dp_service_srng(struct ath12k_base *ab, > struct ath12k_ext_irq_grp *irq_grp, > int budget); > diff --git a/drivers/net/wireless/ath/ath12k/peer.c b/drivers/net/wireless/ath/ath12k/peer.c > index 39b371c7433c..c7eb60723d83 100644 > --- a/drivers/net/wireless/ath/ath12k/peer.c > +++ b/drivers/net/wireless/ath/ath12k/peer.c > @@ -80,6 +80,20 @@ struct ath12k_peer *ath12k_peer_find_by_addr(struct ath12k_base *ab, > return NULL; > } > > +static struct ath12k_peer *ath12k_peer_find_by_ml_id(struct ath12k_base *ab, > + int ml_peer_id) > +{ > + struct ath12k_peer *peer; > + > + lockdep_assert_held(&ab->base_lock); > + > + list_for_each_entry(peer, &ab->peers, list) > + if (ml_peer_id == peer->ml_peer_id) > + return peer; > + > + return NULL; > +} > + > struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab, > int peer_id) > { > @@ -87,6 +101,9 @@ struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab, > > lockdep_assert_held(&ab->base_lock); > > + if (peer_id & ATH12K_ML_PEER_ID_VALID) where is code that sets the bit? does other code elsewhere need to mask this bit off to have the "true" peer_id? the commit text for this patch seems to need a lot more description > + return ath12k_peer_find_by_ml_id(ab, peer_id); > + > list_for_each_entry(peer, &ab->peers, list) > if (peer_id == peer->peer_id) > return peer; > diff --git a/drivers/net/wireless/ath/ath12k/peer.h b/drivers/net/wireless/ath/ath12k/peer.h > index b91bb2106b76..5b718fc5c795 100644 > --- a/drivers/net/wireless/ath/ath12k/peer.h > +++ b/drivers/net/wireless/ath/ath12k/peer.h > @@ -47,6 +47,8 @@ struct ath12k_peer { > > /* protected by ab->data_lock */ > bool dp_setup_done; > + > + u16 ml_peer_id; > }; > > struct ath12k_ml_peer {
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 10/23/2024 6:29 AM, Kalle Valo wrote: >> From: Kalle Valo <quic_kvalo@quicinc.com> >> >> In commit 477cabfdb776 ("wifi: ath12k: modify link arvif creation and removal >> for MLO") I had accidentally left one personal TODO comment about using goto >> instead of ret. Switch to use goto to be consistent with the error handling in >> the function. >> >> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 >> >> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/mac.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c >> index f5f96a8b1d61..f45f32f3b5f6 100644 >> --- a/drivers/net/wireless/ath/ath12k/mac.c >> +++ b/drivers/net/wireless/ath/ath12k/mac.c >> @@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) >> ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id, >> arvif->bssid); >> if (ret) >> - /* KVALO: why not goto err? */ >> - return ret; >> + goto err; > > why does this goto err instead of err_vdev_del? Good point. I did this to follow the same as the next command does: ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id, arvif->bssid); if (ret) goto err; But yeah, err_vdev_del looks like more approriate. > >> >> ar->num_peers--; >> } > > looking at the context for this patch I have a question about a different part > of this function: > > param_id = WMI_VDEV_PARAM_RTS_THRESHOLD; > param_value = hw->wiphy->rts_threshold; > ret = ath12k_wmi_vdev_set_param_cmd(ar, arvif->vdev_id, > param_id, param_value); > if (ret) { > ath12k_warn(ar->ab, "failed to set rts threshold for vdev %d: %d\n", > arvif->vdev_id, ret); > > NOTE: no return or goto > > } > > ath12k_dp_vdev_tx_attach(ar, arvif); > if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled) > ath12k_mac_monitor_vdev_create(ar); > > return ret; > > NOTE: this can return an error if the RTS threshold set fails, but fails > without cleaning up (dp vdev still attached and monitor vdev created) > > Seems either we need error handling if the set param fails, or we should ret 0 > at this point Yeah, I do not like this kind of vague error handling at all. I think we should have either a proper error handling or a comment explaining why we continue to the execution. An example about the comment: ret = foo(); if (ret) ath12k_warn("foo failed: %d", ret); /* continue function because foo is optional */ I think this should all this should be cleaned up in a separate patch.
Baochen Qiang <quic_bqiang@quicinc.com> writes: > On 10/23/2024 9:29 PM, Kalle Valo wrote: >> +static void ath12k_mac_station_post_remove(struct ath12k *ar, >> + struct ath12k_link_vif *arvif, >> + struct ath12k_link_sta *arsta) >> +{ >> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); >> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); >> + struct ath12k_sta *ahsta = arsta->ahsta; >> + struct ath12k_peer *peer; >> + >> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); >> + >> + ath12k_mac_dec_num_stations(arvif, arsta); >> + >> + spin_lock_bh(&ar->ab->base_lock); >> + >> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); >> + if (peer && peer->sta == sta) { >> + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n", >> + vif->addr, arvif->vdev_id); >> + peer->sta = NULL; >> + list_del(&peer->list); >> + kfree(peer); >> + ar->num_peers--; >> + } >> + >> + spin_unlock_bh(&ar->ab->base_lock); >> + >> + kfree(arsta->rx_stats); >> + arsta->rx_stats = NULL; >> + >> + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) { >> + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL); >> + synchronize_rcu(); >> + ahsta->links_map &= ~(BIT(arsta->link_id)); > > should we put this ahead of rcu_assign_pointer()? I agree, I'll do that in v2.
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 10/23/2024 6:29 AM, Kalle Valo wrote: > >> +static void ath12k_mac_station_post_remove(struct ath12k *ar, >> + struct ath12k_link_vif *arvif, >> + struct ath12k_link_sta *arsta) >> +{ >> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); >> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta); >> + struct ath12k_sta *ahsta = arsta->ahsta; >> + struct ath12k_peer *peer; >> + >> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); >> + >> + ath12k_mac_dec_num_stations(arvif, arsta); >> + >> + spin_lock_bh(&ar->ab->base_lock); >> + >> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr); >> + if (peer && peer->sta == sta) { >> + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n", >> + vif->addr, arvif->vdev_id); >> + peer->sta = NULL; >> + list_del(&peer->list); >> + kfree(peer); >> + ar->num_peers--; >> + } >> + >> + spin_unlock_bh(&ar->ab->base_lock); >> + >> + kfree(arsta->rx_stats); >> + arsta->rx_stats = NULL; >> + >> + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) { >> + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL); >> + synchronize_rcu(); > > I've mentioned this in the past in some internal discussion and seems now is a > good time to bring this to light. > > It concerns me that this happens so late in the process. In theory another > thread could already have a valid arsta pointer and could be trying to > dereference that pointer while the code above is destroying underlying data > (i.e. arsta->rx_stats). > > Should we set this to NULL and synchronize RCU at the beginning of the process > so that we know all access to the struct has finished before we start > destroying the data? > > Or can this not actually happen in practice due to other synchronization > mechansims? And if so, should we document that somewhere? I think you are correct, AFAICS the kfree(arsta->rx_stats) should be after synchronize_rcu(). But this race was already in the code before this patch so we need to fix in a separate patch. I have added this to my todo list.
+ aditya Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> +static int ath12k_mac_station_unauthorize(struct ath12k *ar, >> + struct ath12k_link_vif *arvif, >> + struct ath12k_link_sta *arsta) >> +{ >> + struct ath12k_peer *peer; >> + int ret; >> + >> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); >> + >> + spin_lock_bh(&ar->ab->base_lock); >> + >> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr); >> + if (peer) >> + peer->is_authorized = false; >> + >> + spin_unlock_bh(&ar->ab->base_lock); >> + >> + /* Driver should clear the peer keys during mac80211's ref ptr >> + * gets cleared in __sta_info_destroy_part2 (trans from >> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC) > > I'm unable to understand this comment Indeed, that's weird. Aditya, do you have any idea what the comment is trying to say?
On 10/29/24 21:08, Kalle Valo wrote: > + aditya > > Jeff Johnson <quic_jjohnson@quicinc.com> writes: > >>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar, >>> + struct ath12k_link_vif *arvif, >>> + struct ath12k_link_sta *arsta) >>> +{ >>> + struct ath12k_peer *peer; >>> + int ret; >>> + >>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); >>> + >>> + spin_lock_bh(&ar->ab->base_lock); >>> + >>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr); >>> + if (peer) >>> + peer->is_authorized = false; >>> + >>> + spin_unlock_bh(&ar->ab->base_lock); >>> + >>> + /* Driver should clear the peer keys during mac80211's ref ptr >>> + * gets cleared in __sta_info_destroy_part2 (trans from >>> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC) >> >> I'm unable to understand this comment > > Indeed, that's weird. Aditya, do you have any idea what the comment is > trying to say? > At present, ath12k clear the keys in ath12k_station_disassoc() which gets executed in state change from IEEE80211_STA_ASSOC to IEEE80211_STA_AUTH. However, in mac80211, once the station moves from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC itself, the keys are deleted. Please see - __sta_info_destroy_part2() -> ieee80211_free_sta_keys(). Now, ath12k peer object (struct ath12k_peer) holds the key reference from mac80211 (see ath12k_peer::keys[]). Hence, once mac80211 deletes the key, driver should not keep a reference to it or else it could lead to issues. Therefore, it is important that the driver should clear the peer keys during transition from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC it self since we know that once we return from here, mac80211 is going to remove the keys. ath12k_mac_station_unauthorize() gets called when station moves from state IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC hence call to ath12k_clear_peer_keys() is moved from ath12k_station_disassoc() to ath12k_mac_station_unauthorize(). Is this clear now? May be the comment in the code could be re-written as below? /* Driver must clear the keys during the state change from * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after * returning from here, mac80211 is going to delete the keys * in __sta_info_destroy_part2(). This will ensure that the driver does * not retain stale key references after mac80211 deletes the keys. */
Aditya Kumar Singh <quic_adisi@quicinc.com> writes: > On 10/29/24 21:08, Kalle Valo wrote: >> + aditya >> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> >>>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar, >>>> + struct ath12k_link_vif *arvif, >>>> + struct ath12k_link_sta *arsta) >>>> +{ >>>> + struct ath12k_peer *peer; >>>> + int ret; >>>> + >>>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy); >>>> + >>>> + spin_lock_bh(&ar->ab->base_lock); >>>> + >>>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr); >>>> + if (peer) >>>> + peer->is_authorized = false; >>>> + >>>> + spin_unlock_bh(&ar->ab->base_lock); >>>> + >>>> + /* Driver should clear the peer keys during mac80211's ref ptr >>>> + * gets cleared in __sta_info_destroy_part2 (trans from >>>> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC) >>> >>> I'm unable to understand this comment >> Indeed, that's weird. Aditya, do you have any idea what the comment >> is >> trying to say? >> > > At present, ath12k clear the keys in ath12k_station_disassoc() which > gets executed in state change from IEEE80211_STA_ASSOC to > IEEE80211_STA_AUTH. > > However, in mac80211, once the station moves from > IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC itself, the keys are > deleted. Please see - __sta_info_destroy_part2() -> > ieee80211_free_sta_keys(). > > Now, ath12k peer object (struct ath12k_peer) holds the key reference > from mac80211 (see ath12k_peer::keys[]). Hence, once mac80211 deletes > the key, driver should not keep a reference to it or else it could > lead to issues. > > Therefore, it is important that the driver should clear the peer keys > during transition from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC > it self since we know that once we return from here, mac80211 is going > to remove the keys. > > ath12k_mac_station_unauthorize() gets called when station moves from > state IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC hence call to > ath12k_clear_peer_keys() is moved from ath12k_station_disassoc() to > ath12k_mac_station_unauthorize(). > > Is this clear now? Super clear :) Thanks! > May be the comment in the code could be re-written as below? > > /* Driver must clear the keys during the state change from > * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after > * returning from here, mac80211 is going to delete the keys > * in __sta_info_destroy_part2(). This will ensure that the driver does > * not retain stale key references after mac80211 deletes the keys. > */ Looks good to me, I'll add that if it's ok for Jeff as well.
On 10/30/2024 11:28 AM, Kalle Valo wrote: > Aditya Kumar Singh <quic_adisi@quicinc.com> writes: >> May be the comment in the code could be re-written as below? >> >> /* Driver must clear the keys during the state change from >> * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after >> * returning from here, mac80211 is going to delete the keys >> * in __sta_info_destroy_part2(). This will ensure that the driver does >> * not retain stale key references after mac80211 deletes the keys. >> */ > > Looks good to me, I'll add that if it's ok for Jeff as well. > Definitely ok, thanks for the clarification
Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> --- a/drivers/net/wireless/ath/ath12k/peer.c >> +++ b/drivers/net/wireless/ath/ath12k/peer.c >> @@ -80,6 +80,20 @@ struct ath12k_peer *ath12k_peer_find_by_addr(struct ath12k_base *ab, >> return NULL; >> } >> >> +static struct ath12k_peer *ath12k_peer_find_by_ml_id(struct ath12k_base *ab, >> + int ml_peer_id) >> +{ >> + struct ath12k_peer *peer; >> + >> + lockdep_assert_held(&ab->base_lock); >> + >> + list_for_each_entry(peer, &ab->peers, list) >> + if (ml_peer_id == peer->ml_peer_id) >> + return peer; >> + >> + return NULL; >> +} >> + >> struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab, >> int peer_id) >> { >> @@ -87,6 +101,9 @@ struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab, >> >> lockdep_assert_held(&ab->base_lock); >> >> + if (peer_id & ATH12K_ML_PEER_ID_VALID) [...] > does other code elsewhere need to mask this bit off to have the "true" peer_id? Based on my investigation the peer id is stored with ATH12K_ML_PEER_ID_VALID so it should not be masked (unless I'm missing something). This is not pretty but I guess keeps the code simpler.
From: Kalle Valo <quic_kvalo@quicinc.com> Here we continue to refactor mac.c to support multiple links and extend peer assoc WMI command to support MLO. Kalle Valo (2): wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling wifi: ath12k: introduce ath12k_hw_warn() Sriram R (6): wifi: ath12k: MLO vdev bringup changes wifi: ath12k: Refactor sta state machine wifi: ath12k: Add helpers for multi link peer creation and deletion wifi: ath12k: add multi-link flag in peer create command wifi: ath12k: add helper to find multi-link station wifi: ath12k: Add MLO peer assoc command support drivers/net/wireless/ath/ath12k/core.h | 25 ++ drivers/net/wireless/ath/ath12k/debug.c | 4 +- drivers/net/wireless/ath/ath12k/debug.h | 5 +- drivers/net/wireless/ath/ath12k/dp.h | 2 + drivers/net/wireless/ath/ath12k/mac.c | 505 +++++++++++++++++++----- drivers/net/wireless/ath/ath12k/peer.c | 116 ++++++ drivers/net/wireless/ath/ath12k/peer.h | 10 + drivers/net/wireless/ath/ath12k/wmi.c | 191 ++++++++- drivers/net/wireless/ath/ath12k/wmi.h | 126 ++++++ 9 files changed, 860 insertions(+), 124 deletions(-) base-commit: fa934bf3e0a825ee09f035c6580af513187d59a2