| Message ID | 20241106142617.660901-1-kvalo@kernel.org |
|---|---|
| Headers | show |
| Series | wifi: ath12k: MLO support part 3 | expand |
On 11/6/2024 6:26 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Add changes to handle multi-link station state change with proper > link handling and add code changes for ML peer creation, peer deletion. > > In ath12k_mac_assign_link_sta() initialise all arsta fields first and only then > call rcu_assign_pointer(). This is to make sure that readers don't have access > to arsta which is still modified. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-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> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 11/6/2024 6:26 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > In case of Multi-link operation, data path peer setup and tid setup should be > done only for primary link of multi-link station. Add changes to introduce > primary link is peer. Currently, association link will be considered as primary > link. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-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> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 11/6/2024 6:26 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Currently mac80211 vif's bss_conf is used to fetch any vif related > configurations in driver but with MLO multiple links are affiliated to a vif > and corresponding link configs are present in vif->link_conf[]. Fetch > link_conf for corresponding link from vif and use the same for configurations. > > Add ath12k_mac_get_link_bss_conf() helper to fetch link_conf from arvif. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-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: Rameshkumar Sundaram <quic_ramess@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 11/6/2024 10:26 PM, Kalle Valo wrote: > +static void ath12k_mac_unassign_link_sta(struct ath12k_hw *ah, > + struct ath12k_sta *ahsta, > + u8 link_id) > +{ > + lockdep_assert_wiphy(ah->hw->wiphy); > + > + ahsta->links_map &= ~BIT(link_id); > + rcu_assign_pointer(ahsta->link[link_id], NULL); > + > + synchronize_rcu(); this looks strange: generally we call synchronize_rcu() to wait for any RCU readers to finish, such that we can then safely free something. but here we do nothing ... > +} > + > +static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, > + struct ath12k_sta *ahsta, > + u8 link_id) > +{ > + struct ath12k_link_sta *arsta; > + > + lockdep_assert_wiphy(ah->hw->wiphy); > + > + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) > + return; > + > + arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); > + > + if (WARN_ON(!arsta)) > + return; > + > + ath12k_mac_unassign_link_sta(ah, ahsta, link_id); > + > + arsta->link_id = ATH12K_INVALID_LINK_ID; > + arsta->ahsta = NULL; > + arsta->arvif = NULL; if arsta is not deflink and would be freed, can we avoid these cleanup? > + > + if (arsta != &ahsta->deflink) > + kfree(arsta); I know the actual free happens here, but why split them? these two hunks give me the impression that we may (in the future?) have cases to call ath12k_mac_unassign_link_sta() alone somewhere else rather than directly calling ath12k_mac_free_unassign_link_sta(). am I feeling right? what are those cases? > +} > +
Baochen Qiang <quic_bqiang@quicinc.com> writes: > On 11/6/2024 10:26 PM, Kalle Valo wrote: >> +static void ath12k_mac_unassign_link_sta(struct ath12k_hw *ah, >> + struct ath12k_sta *ahsta, >> + u8 link_id) >> +{ >> + lockdep_assert_wiphy(ah->hw->wiphy); >> + >> + ahsta->links_map &= ~BIT(link_id); >> + rcu_assign_pointer(ahsta->link[link_id], NULL); >> + >> + synchronize_rcu(); > > this looks strange: generally we call synchronize_rcu() to wait for > any RCU readers to finish, such that we can then safely free > something. but here we do nothing ... Same comment as in the other email, this is to make sure that we don't continue the mac80211 call flow before all readers have the new value. Is that a problem? And we can always optimise later. >> +static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, >> + struct ath12k_sta *ahsta, >> + u8 link_id) >> +{ >> + struct ath12k_link_sta *arsta; >> + >> + lockdep_assert_wiphy(ah->hw->wiphy); >> + >> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) >> + return; >> + >> + arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); >> + >> + if (WARN_ON(!arsta)) >> + return; >> + >> + ath12k_mac_unassign_link_sta(ah, ahsta, link_id); >> + >> + arsta->link_id = ATH12K_INVALID_LINK_ID; >> + arsta->ahsta = NULL; >> + arsta->arvif = NULL; > > if arsta is not deflink and would be freed, can we avoid these > cleanup? I think that's something we can cleanup later if needed. Sure, it's extra assignments but it's not really doing any harm. >> + if (arsta != &ahsta->deflink) >> + kfree(arsta); > > I know the actual free happens here, but why split them? You mean why have a separate function ath12k_mac_unassign_link_sta() and instead just have all code the in ath12k_mac_free_unassign_link_sta()? > these two hunks give me the impression that we may (in the future?) > have cases to call ath12k_mac_unassign_link_sta() alone somewhere else > rather than directly calling ath12k_mac_free_unassign_link_sta(). am I > feeling right? what are those cases? At least I'm not aware of anything else calling ath12k_mac_unassign_link_sta(). So I'll just remove that function and move the code to ath12k_mac_free_unassign_link_sta().
On 11/13/2024 1:10 AM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >> On 11/6/2024 10:26 PM, Kalle Valo wrote: >>> +static void ath12k_mac_unassign_link_sta(struct ath12k_hw *ah, >>> + struct ath12k_sta *ahsta, >>> + u8 link_id) >>> +{ >>> + lockdep_assert_wiphy(ah->hw->wiphy); >>> + >>> + ahsta->links_map &= ~BIT(link_id); >>> + rcu_assign_pointer(ahsta->link[link_id], NULL); >>> + >>> + synchronize_rcu(); >> >> this looks strange: generally we call synchronize_rcu() to wait for >> any RCU readers to finish, such that we can then safely free >> something. but here we do nothing ... > > Same comment as in the other email, this is to make sure that we don't > continue the mac80211 call flow before all readers have the new value. > Is that a problem? And we can always optimise later. here this is a different situation from that in the other email you referred to: we are clearing an RCU pointer here, so a synchronize_rcu() is necessary for the purpose of safely freeing arsta. But ideally the code flow should look like: rcu_assign_pointer(ahsta->link[link_id], NULL) synchronize_rcu() if (arsta != &ahsta->deflink) kfree(arsta); However the patch is doing nothing after synchronize_rcu(). I know the actual free happens immediately after ath12k_mac_unassign_link_sta() returns, so there should be no problem. But it really looks odd to get them split. > >>> +static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, >>> + struct ath12k_sta *ahsta, >>> + u8 link_id) >>> +{ >>> + struct ath12k_link_sta *arsta; >>> + >>> + lockdep_assert_wiphy(ah->hw->wiphy); >>> + >>> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) >>> + return; >>> + >>> + arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); >>> + >>> + if (WARN_ON(!arsta)) >>> + return; >>> + >>> + ath12k_mac_unassign_link_sta(ah, ahsta, link_id); >>> + >>> + arsta->link_id = ATH12K_INVALID_LINK_ID; >>> + arsta->ahsta = NULL; >>> + arsta->arvif = NULL; >> >> if arsta is not deflink and would be freed, can we avoid these >> cleanup? > > I think that's something we can cleanup later if needed. Sure, it's > extra assignments but it's not really doing any harm. exactly, but ideally we should avoid unnecessary effort if possible. > >>> + if (arsta != &ahsta->deflink) >>> + kfree(arsta); >> >> I know the actual free happens here, but why split them? > > You mean why have a separate function ath12k_mac_unassign_link_sta() and > instead just have all code the in ath12k_mac_free_unassign_link_sta()? yes. such that we can have synchronize_rcu() and kfree() located together. > >> these two hunks give me the impression that we may (in the future?) >> have cases to call ath12k_mac_unassign_link_sta() alone somewhere else >> rather than directly calling ath12k_mac_free_unassign_link_sta(). am I >> feeling right? what are those cases? > > At least I'm not aware of anything else calling > ath12k_mac_unassign_link_sta(). So I'll just remove that function and > move the code to ath12k_mac_free_unassign_link_sta(). >
Baochen Qiang <quic_bqiang@quicinc.com> writes: >>>> +static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, >>>> + struct ath12k_sta *ahsta, >>>> + u8 link_id) >>>> +{ >>>> + struct ath12k_link_sta *arsta; >>>> + >>>> + lockdep_assert_wiphy(ah->hw->wiphy); >>>> + >>>> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) >>>> + return; >>>> + >>>> + arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); >>>> + >>>> + if (WARN_ON(!arsta)) >>>> + return; >>>> + >>>> + ath12k_mac_unassign_link_sta(ah, ahsta, link_id); >>>> + >>>> + arsta->link_id = ATH12K_INVALID_LINK_ID; >>>> + arsta->ahsta = NULL; >>>> + arsta->arvif = NULL; >>> >>> if arsta is not deflink and would be freed, can we avoid these >>> cleanup? >> >> I think that's something we can cleanup later if needed. Sure, it's >> extra assignments but it's not really doing any harm. > exactly, but ideally we should avoid unnecessary effort if possible. > >> >>>> + if (arsta != &ahsta->deflink) >>>> + kfree(arsta); >>> >>> I know the actual free happens here, but why split them? >> >> You mean why have a separate function ath12k_mac_unassign_link_sta() and >> instead just have all code the in >> ath12k_mac_free_unassign_link_sta()? > > yes. such that we can have synchronize_rcu() and kfree() located together. Ok, I think I now get what you mean. Does this look better: static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, struct ath12k_sta *ahsta, u8 link_id) { struct ath12k_link_sta *arsta; lockdep_assert_wiphy(ah->hw->wiphy); if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) return; arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); if (WARN_ON(!arsta)) return; ahsta->links_map &= ~BIT(link_id); rcu_assign_pointer(ahsta->link[link_id], NULL); if (arsta == &ahsta->deflink) { arsta->link_id = ATH12K_INVALID_LINK_ID; arsta->ahsta = NULL; arsta->arvif = NULL; return; } synchronize_rcu(); kfree(arsta); } BTW your lines are really long, please check your settings: https://patchwork.kernel.org/project/linux-wireless/patch/20241106142617.660901-2-kvalo@kernel.org/
On 11/21/2024 3:32 AM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >>>>> +static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, >>>>> + struct ath12k_sta *ahsta, >>>>> + u8 link_id) >>>>> +{ >>>>> + struct ath12k_link_sta *arsta; >>>>> + >>>>> + lockdep_assert_wiphy(ah->hw->wiphy); >>>>> + >>>>> + if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) >>>>> + return; >>>>> + >>>>> + arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); >>>>> + >>>>> + if (WARN_ON(!arsta)) >>>>> + return; >>>>> + >>>>> + ath12k_mac_unassign_link_sta(ah, ahsta, link_id); >>>>> + >>>>> + arsta->link_id = ATH12K_INVALID_LINK_ID; >>>>> + arsta->ahsta = NULL; >>>>> + arsta->arvif = NULL; >>>> >>>> if arsta is not deflink and would be freed, can we avoid these >>>> cleanup? >>> >>> I think that's something we can cleanup later if needed. Sure, it's >>> extra assignments but it's not really doing any harm. >> exactly, but ideally we should avoid unnecessary effort if possible. >> >>> >>>>> + if (arsta != &ahsta->deflink) >>>>> + kfree(arsta); >>>> >>>> I know the actual free happens here, but why split them? >>> >>> You mean why have a separate function ath12k_mac_unassign_link_sta() and >>> instead just have all code the in >>> ath12k_mac_free_unassign_link_sta()? >> >> yes. such that we can have synchronize_rcu() and kfree() located together. > > Ok, I think I now get what you mean. Does this look better: > > static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, > struct ath12k_sta *ahsta, > u8 link_id) > { > struct ath12k_link_sta *arsta; > > lockdep_assert_wiphy(ah->hw->wiphy); > > if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) > return; > > arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); > if (WARN_ON(!arsta)) > return; > > ahsta->links_map &= ~BIT(link_id); > rcu_assign_pointer(ahsta->link[link_id], NULL); below synchronize_rcu() should be moved to here, such that any change to arsta can happen only AFTER all existing RCU readers finish accessing it. > > if (arsta == &ahsta->deflink) { > arsta->link_id = ATH12K_INVALID_LINK_ID; > arsta->ahsta = NULL; > arsta->arvif = NULL; > return; > } > > synchronize_rcu(); > kfree(arsta); > } other than that this change looks good to me. > > BTW your lines are really long, please check your settings: sure, will check. > > https://patchwork.kernel.org/project/linux-wireless/patch/20241106142617.660901-2-kvalo@kernel.org/ >
Baochen Qiang <quic_bqiang@quicinc.com> writes: >> Ok, I think I now get what you mean. Does this look better: >> >> static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah, >> struct ath12k_sta *ahsta, >> u8 link_id) >> { >> struct ath12k_link_sta *arsta; >> >> lockdep_assert_wiphy(ah->hw->wiphy); >> >> if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS)) >> return; >> >> arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]); >> if (WARN_ON(!arsta)) >> return; >> >> ahsta->links_map &= ~BIT(link_id); >> rcu_assign_pointer(ahsta->link[link_id], NULL); > > below synchronize_rcu() should be moved to here, such that any change > to arsta can happen only AFTER all existing RCU readers finish > accessing it. Good point, I'll move it.
From: Kalle Valo <quic_kvalo@quicinc.com> We continue refactoring ath12k in preparation for supporting Multi-Link Operation. For example, in this patchset we modify station state handling and start to use more link level configuration. Please review. Rameshkumar Sundaram (2): wifi: ath12k: add reo queue lookup table for ML peers wifi: ath12k: modify chanctx iterators for MLO Sriram R (6): wifi: ath12k: Add MLO station state change handling wifi: ath12k: support change_sta_links() mac80211 op wifi: ath12k: add primary link for data path operations wifi: ath12k: use arsta instead of sta wifi: ath12k: Use mac80211 vif's link_conf instead of bss_conf wifi: ath12k: Use mac80211 sta's link_sta instead of deflink drivers/net/wireless/ath/ath12k/core.h | 4 + drivers/net/wireless/ath/ath12k/dp.c | 44 +- drivers/net/wireless/ath/ath12k/dp.h | 1 + drivers/net/wireless/ath/ath12k/dp_rx.c | 58 +- drivers/net/wireless/ath/ath12k/mac.c | 993 ++++++++++++++++++------ drivers/net/wireless/ath/ath12k/mac.h | 1 + drivers/net/wireless/ath/ath12k/peer.c | 117 ++- drivers/net/wireless/ath/ath12k/peer.h | 11 +- drivers/net/wireless/ath/ath12k/wmi.c | 16 +- 9 files changed, 987 insertions(+), 258 deletions(-) base-commit: d63fbff74ab1af1573c1dca20cfe1e876f8ffa62