Message ID | 20241126171139.2350704-7-kvalo@kernel.org (mailing list archive) |
---|---|
State | Accepted |
Commit | 90570ba4610bdb1db39ef45f2b271a9f89680a9d |
Delegated to: | Kalle Valo |
Headers | show |
Series | wifi: ath12k: MLO support part 4 | expand |
On 11/27/2024 1:11 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > When a scan request is received, driver selects a link id for which the arvif > can be mapped. Same link is also used for getting the link conf address. > Currently, we return 0 as link id for a non ML vif, which is correct since that > is the default link id. Also when any of the link vif is active and the scan > request is for a channel in the active link we return its link id. But, when we > don't hit both of the above cases (i.e not a ML vif or no active link vif for > the channel is present) we currently return 0 as the link id. > > Bu the problemis that this might not work out always, eg., when only one link > (eg. linkid = 1) is added to vif, then we won't find any link conf for link id > 0 in the vif resulting in scan failure. During AP bringup, such scan failure > causes bringup issues. Hence avoid sending link id 0 as default. Rather use a > default link for scan and default link address for the same. This scan vdev > will either be deleted if another scan is requested on same vif or when AP is > broughtup on same link or during interface cleanup. > > 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> > --- > drivers/net/wireless/ath/ath12k/core.h | 3 +- > drivers/net/wireless/ath/ath12k/mac.c | 65 +++++++++++++++++++------- > drivers/net/wireless/ath/ath12k/mac.h | 6 +++ > 3 files changed, 56 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h > index e246e3d3c162..f4a710d49584 100644 > --- a/drivers/net/wireless/ath/ath12k/core.h > +++ b/drivers/net/wireless/ath/ath12k/core.h > @@ -322,10 +322,11 @@ struct ath12k_vif { > bool ps; > > struct ath12k_link_vif deflink; > - struct ath12k_link_vif __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS]; > + struct ath12k_link_vif __rcu *link[ATH12K_NUM_MAX_LINKS]; > struct ath12k_vif_cache *cache[IEEE80211_MLD_MAX_NUM_LINKS]; > /* indicates bitmap of link vif created in FW */ > u16 links_map; > + u8 last_scan_link; > > /* Must be last - ends in a flexible-array member. > * > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index 8287c2e6b765..85cfbe1e4b9f 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -3792,6 +3792,9 @@ static void ath12k_ahvif_put_link_key_cache(struct ath12k_vif_cache *cache) > > static void ath12k_ahvif_put_link_cache(struct ath12k_vif *ahvif, u8 link_id) > { > + if (link_id >= IEEE80211_MLD_MAX_NUM_LINKS) > + return; > + > ath12k_ahvif_put_link_key_cache(ahvif->cache[link_id]); > kfree(ahvif->cache[link_id]); > ahvif->cache[link_id] = NULL; > @@ -3852,9 +3855,9 @@ static struct ath12k_link_vif *ath12k_mac_assign_link_vif(struct ath12k_hw *ah, > arvif = &ahvif->deflink; > } else { > /* If this is the first link arvif being created for an ML VIF > - * use the preallocated deflink memory > + * use the preallocated deflink memory except for scan arvifs > */ > - if (!ahvif->links_map) { > + if (!ahvif->links_map && link_id != ATH12K_DEFAULT_SCAN_LINK) { > arvif = &ahvif->deflink; > } else { > arvif = (struct ath12k_link_vif *) > @@ -4154,10 +4157,10 @@ ath12k_mac_find_link_id_by_ar(struct ath12k_vif *ahvif, struct ath12k *ar) > return link_id; > } > > - /* input ar is not assigned to any of the links, use link id > - * 0 for scan vdev creation. > + /* input ar is not assigned to any of the links of ML VIF, use scan > + * link (15) for scan vdev creation. > */ > - return 0; > + return ATH12K_DEFAULT_SCAN_LINK; > } > > static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, > @@ -4188,7 +4191,7 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, > > /* check if any of the links of ML VIF is already started on > * radio(ar) correpsondig to given scan frequency and use it, > - * if not use deflink(link 0) for scan purpose. > + * if not use scan link (link 15) for scan purpose. > */ > link_id = ath12k_mac_find_link_id_by_ar(ahvif, ar); > arvif = ath12k_mac_assign_link_vif(ah, vif, link_id); > @@ -4298,6 +4301,13 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, > spin_unlock_bh(&ar->data_lock); > } > > + /* As per cfg80211/mac80211 scan design, it allows only one > + * scan at a time. Hence last_scan link id is used for > + * tracking the link id on which the scan is been done on > + * this vif. > + */ > + ahvif->last_scan_link = arvif->link_id; > + > /* Add a margin to account for event/command processing */ > ieee80211_queue_delayed_work(ath12k_ar_to_hw(ar), &ar->scan.timeout, > msecs_to_jiffies(arg->max_scan_time + > @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw, > struct ieee80211_vif *vif) > { > struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif); > + u16 link_id = ahvif->last_scan_link; > struct ath12k_link_vif *arvif; > struct ath12k *ar; > > lockdep_assert_wiphy(hw->wiphy); > > - arvif = &ahvif->deflink; > - > - if (!arvif->is_created) > + arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]); > + if (!arvif || arvif->is_created) s/arvif->is_created/!arvif->is_created/ ? > return; > > ar = arvif->ar; > @@ -7688,10 +7698,19 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) > u16 nss; > int i; > int ret, vdev_id; > + u8 link_id; > > lockdep_assert_wiphy(hw->wiphy); > > - link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[arvif->link_id]); > + /* If no link is active and scan vdev is requested > + * use a default link conf for scan address purpose. > + */ > + if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links) > + link_id = ffs(vif->valid_links) - 1; > + else > + link_id = arvif->link_id; > + > + link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]); > if (!link_conf) { > ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n", > vif->addr, arvif->link_id); > @@ -7971,7 +7990,9 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, > struct ath12k_link_vif *arvif, > struct ieee80211_chanctx_conf *ctx) > { > - struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); > + struct ath12k_vif *ahvif = arvif->ahvif; > + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif); > + struct ath12k_link_vif *scan_arvif; > struct ath12k_hw *ah = hw->priv; > struct ath12k *ar; > struct ath12k_base *ab; > @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, > if (!ar) > return NULL; > > + /* cleanup the scan vdev if we are done scan on that ar > + * and now we want to create for actual usage. > + */ > + if (vif->valid_links) { better to use ieee80211_vif_is_mld()? > + scan_arvif = wiphy_dereference(hw->wiphy, > + ahvif->link[ATH12K_DEFAULT_SCAN_LINK]); > + if (scan_arvif && scan_arvif->ar == ar) { > + ar->scan.vdev_id = -1; > + ath12k_mac_remove_link_interface(hw, scan_arvif); > + ath12k_mac_unassign_link_vif(scan_arvif); > + } > + } > + > if (arvif->ar) { > /* This is not expected really */ > if (WARN_ON(!arvif->is_created)) { > @@ -8194,7 +8228,7 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, > > lockdep_assert_wiphy(hw->wiphy); > > - for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) { > + for (link_id = 0; link_id < ATH12K_NUM_MAX_LINKS; link_id++) { > /* if we cached some config but never received assign chanctx, > * free the allocated cache. > */ > @@ -9042,11 +9076,8 @@ ath12k_mac_op_assign_vif_chanctx(struct ieee80211_hw *hw, > return -ENOMEM; > } > > - if (!arvif->is_started) { > - ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx); > - if (!ar) > - return -EINVAL; > - } else { > + ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx); > + if (!ar) { > ath12k_warn(arvif->ar->ab, "failed to assign chanctx for vif %pM link id %u link vif is already started", > vif->addr, link_id); > return -EINVAL; > diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h > index c13630ee479a..abdc9a6c0740 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.h > +++ b/drivers/net/wireless/ath/ath12k/mac.h > @@ -44,6 +44,12 @@ struct ath12k_generic_iter { > #define ATH12K_DEFAULT_LINK_ID 0 > #define ATH12K_INVALID_LINK_ID 255 > > +/* Default link after the IEEE802.11 defined Max link id limit > + * for driver usage purpose. > + */ > +#define ATH12K_DEFAULT_SCAN_LINK IEEE80211_MLD_MAX_NUM_LINKS > +#define ATH12K_NUM_MAX_LINKS (IEEE80211_MLD_MAX_NUM_LINKS + 1) > + > enum ath12k_supported_bw { > ATH12K_BW_20 = 0, > ATH12K_BW_40 = 1,
Kalle Valo <kvalo@kernel.org> wrote: > @@ -7688,10 +7698,19 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) > u16 nss; > int i; > int ret, vdev_id; > + u8 link_id; > > lockdep_assert_wiphy(hw->wiphy); > > - link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[arvif->link_id]); > + /* If no link is active and scan vdev is requested > + * use a default link conf for scan address purpose. > + */ > + if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links) > + link_id = ffs(vif->valid_links) - 1; link_id = __ffs(vif->valid_links); since it checked vif->valid_links against 0 first. > + else > + link_id = arvif->link_id; > + > + link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]); > if (!link_conf) { > ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n", > vif->addr, arvif->link_id);
Baochen Qiang <quic_bqiang@quicinc.com> writes: > On 11/27/2024 1:11 AM, Kalle Valo wrote: >> From: Sriram R <quic_srirrama@quicinc.com> >> >> @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw, >> struct ieee80211_vif *vif) >> { >> struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif); >> + u16 link_id = ahvif->last_scan_link; >> struct ath12k_link_vif *arvif; >> struct ath12k *ar; >> >> lockdep_assert_wiphy(hw->wiphy); >> >> - arvif = &ahvif->deflink; >> - >> - if (!arvif->is_created) >> + arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]); >> + if (!arvif || arvif->is_created) > > s/arvif->is_created/!arvif->is_created/ ? Another good catch! Fixed now. >> @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, >> if (!ar) >> return NULL; >> >> + /* cleanup the scan vdev if we are done scan on that ar >> + * and now we want to create for actual usage. >> + */ >> + if (vif->valid_links) { > > better to use ieee80211_vif_is_mld()? Yup, fixed in the pending branch: https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=54504518cb26fef3dbaf16457cde91a9fd7e9c3d Thanks for the detailed review, very much appreciated.
On 11/28/2024 8:34 PM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >> On 11/27/2024 1:11 AM, Kalle Valo wrote: >>> From: Sriram R <quic_srirrama@quicinc.com> >>> >>> @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw, >>> struct ieee80211_vif *vif) >>> { >>> struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif); >>> + u16 link_id = ahvif->last_scan_link; >>> struct ath12k_link_vif *arvif; >>> struct ath12k *ar; >>> >>> lockdep_assert_wiphy(hw->wiphy); >>> >>> - arvif = &ahvif->deflink; >>> - >>> - if (!arvif->is_created) >>> + arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]); >>> + if (!arvif || arvif->is_created) >> >> s/arvif->is_created/!arvif->is_created/ ? > > Another good catch! Fixed now. > >>> @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, >>> if (!ar) >>> return NULL; >>> >>> + /* cleanup the scan vdev if we are done scan on that ar >>> + * and now we want to create for actual usage. >>> + */ >>> + if (vif->valid_links) { >> >> better to use ieee80211_vif_is_mld()? > > Yup, fixed in the pending branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=54504518cb26fef3dbaf16457cde91a9fd7e9c3d LGTM > > Thanks for the detailed review, very much appreciated. >
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h index e246e3d3c162..f4a710d49584 100644 --- a/drivers/net/wireless/ath/ath12k/core.h +++ b/drivers/net/wireless/ath/ath12k/core.h @@ -322,10 +322,11 @@ struct ath12k_vif { bool ps; struct ath12k_link_vif deflink; - struct ath12k_link_vif __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS]; + struct ath12k_link_vif __rcu *link[ATH12K_NUM_MAX_LINKS]; struct ath12k_vif_cache *cache[IEEE80211_MLD_MAX_NUM_LINKS]; /* indicates bitmap of link vif created in FW */ u16 links_map; + u8 last_scan_link; /* Must be last - ends in a flexible-array member. * diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index 8287c2e6b765..85cfbe1e4b9f 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -3792,6 +3792,9 @@ static void ath12k_ahvif_put_link_key_cache(struct ath12k_vif_cache *cache) static void ath12k_ahvif_put_link_cache(struct ath12k_vif *ahvif, u8 link_id) { + if (link_id >= IEEE80211_MLD_MAX_NUM_LINKS) + return; + ath12k_ahvif_put_link_key_cache(ahvif->cache[link_id]); kfree(ahvif->cache[link_id]); ahvif->cache[link_id] = NULL; @@ -3852,9 +3855,9 @@ static struct ath12k_link_vif *ath12k_mac_assign_link_vif(struct ath12k_hw *ah, arvif = &ahvif->deflink; } else { /* If this is the first link arvif being created for an ML VIF - * use the preallocated deflink memory + * use the preallocated deflink memory except for scan arvifs */ - if (!ahvif->links_map) { + if (!ahvif->links_map && link_id != ATH12K_DEFAULT_SCAN_LINK) { arvif = &ahvif->deflink; } else { arvif = (struct ath12k_link_vif *) @@ -4154,10 +4157,10 @@ ath12k_mac_find_link_id_by_ar(struct ath12k_vif *ahvif, struct ath12k *ar) return link_id; } - /* input ar is not assigned to any of the links, use link id - * 0 for scan vdev creation. + /* input ar is not assigned to any of the links of ML VIF, use scan + * link (15) for scan vdev creation. */ - return 0; + return ATH12K_DEFAULT_SCAN_LINK; } static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, @@ -4188,7 +4191,7 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, /* check if any of the links of ML VIF is already started on * radio(ar) correpsondig to given scan frequency and use it, - * if not use deflink(link 0) for scan purpose. + * if not use scan link (link 15) for scan purpose. */ link_id = ath12k_mac_find_link_id_by_ar(ahvif, ar); arvif = ath12k_mac_assign_link_vif(ah, vif, link_id); @@ -4298,6 +4301,13 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, spin_unlock_bh(&ar->data_lock); } + /* As per cfg80211/mac80211 scan design, it allows only one + * scan at a time. Hence last_scan link id is used for + * tracking the link id on which the scan is been done on + * this vif. + */ + ahvif->last_scan_link = arvif->link_id; + /* Add a margin to account for event/command processing */ ieee80211_queue_delayed_work(ath12k_ar_to_hw(ar), &ar->scan.timeout, msecs_to_jiffies(arg->max_scan_time + @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif) { struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif); + u16 link_id = ahvif->last_scan_link; struct ath12k_link_vif *arvif; struct ath12k *ar; lockdep_assert_wiphy(hw->wiphy); - arvif = &ahvif->deflink; - - if (!arvif->is_created) + arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]); + if (!arvif || arvif->is_created) return; ar = arvif->ar; @@ -7688,10 +7698,19 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) u16 nss; int i; int ret, vdev_id; + u8 link_id; lockdep_assert_wiphy(hw->wiphy); - link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[arvif->link_id]); + /* If no link is active and scan vdev is requested + * use a default link conf for scan address purpose. + */ + if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links) + link_id = ffs(vif->valid_links) - 1; + else + link_id = arvif->link_id; + + link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]); if (!link_conf) { ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n", vif->addr, arvif->link_id); @@ -7971,7 +7990,9 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, struct ath12k_link_vif *arvif, struct ieee80211_chanctx_conf *ctx) { - struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif); + struct ath12k_vif *ahvif = arvif->ahvif; + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif); + struct ath12k_link_vif *scan_arvif; struct ath12k_hw *ah = hw->priv; struct ath12k *ar; struct ath12k_base *ab; @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, if (!ar) return NULL; + /* cleanup the scan vdev if we are done scan on that ar + * and now we want to create for actual usage. + */ + if (vif->valid_links) { + scan_arvif = wiphy_dereference(hw->wiphy, + ahvif->link[ATH12K_DEFAULT_SCAN_LINK]); + if (scan_arvif && scan_arvif->ar == ar) { + ar->scan.vdev_id = -1; + ath12k_mac_remove_link_interface(hw, scan_arvif); + ath12k_mac_unassign_link_vif(scan_arvif); + } + } + if (arvif->ar) { /* This is not expected really */ if (WARN_ON(!arvif->is_created)) { @@ -8194,7 +8228,7 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, lockdep_assert_wiphy(hw->wiphy); - for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) { + for (link_id = 0; link_id < ATH12K_NUM_MAX_LINKS; link_id++) { /* if we cached some config but never received assign chanctx, * free the allocated cache. */ @@ -9042,11 +9076,8 @@ ath12k_mac_op_assign_vif_chanctx(struct ieee80211_hw *hw, return -ENOMEM; } - if (!arvif->is_started) { - ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx); - if (!ar) - return -EINVAL; - } else { + ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx); + if (!ar) { ath12k_warn(arvif->ar->ab, "failed to assign chanctx for vif %pM link id %u link vif is already started", vif->addr, link_id); return -EINVAL; diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h index c13630ee479a..abdc9a6c0740 100644 --- a/drivers/net/wireless/ath/ath12k/mac.h +++ b/drivers/net/wireless/ath/ath12k/mac.h @@ -44,6 +44,12 @@ struct ath12k_generic_iter { #define ATH12K_DEFAULT_LINK_ID 0 #define ATH12K_INVALID_LINK_ID 255 +/* Default link after the IEEE802.11 defined Max link id limit + * for driver usage purpose. + */ +#define ATH12K_DEFAULT_SCAN_LINK IEEE80211_MLD_MAX_NUM_LINKS +#define ATH12K_NUM_MAX_LINKS (IEEE80211_MLD_MAX_NUM_LINKS + 1) + enum ath12k_supported_bw { ATH12K_BW_20 = 0, ATH12K_BW_40 = 1,