| Message ID | 20240320190943.3850106-6-quic_ramess@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: Add single wiphy support | expand |
On 3/20/2024 12:09 PM, Rameshkumar Sundaram wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > When multiple radios are advertised as a single wiphy which > supports various bands, a default scan request to mac80211 > from cfg80211 will split the driver request based on band, > so each request will have channels belonging to the same band. > With this supported by default, the ath12k driver on receiving > this request checks for one of the channels in the request and > selects the corresponding radio(ar) on which the scan is going > to be performed and creates a vdev on that radio. > > Note that on scan completion this vdev is not deleted. If a new > scan request is seen on that same vif for a different band the > vdev will be deleted and created on the new radio supporting the > request. The vdev delete logic is refactored to have this done > dynamically. > > The reason for not deleting the vdev on scan stop is to avoid > repeated delete-create sequence if the scan is on the same band. > Also, during channel assign, new vdev creation can be optimized > as well. > > Also if the scan is requested when the vdev is in started state, > no switching to new radio is allowed and scan on channels only > within same radio is allowed. > > 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: Rameshkumar Sundaram <quic_ramess@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/mac.c | 211 +++++++++++++++++++++----- > 1 file changed, 176 insertions(+), 35 deletions(-) ... > -static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, > - struct ieee80211_vif *vif) > +static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif) > { > - struct ath12k *ar; > struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); > - struct ath12k_base *ab; > + struct ath12k_base *ab = ar->ab; > unsigned long time_left; > int ret; > > - if (!arvif->is_created) > - return; > - > - ar = arvif->ar; > - ab = ar->ab; > - > - mutex_lock(&ar->conf_mutex); > - > - ath12k_dbg(ab, ATH12K_DBG_MAC, "mac remove interface (vdev %d)\n", > - arvif->vdev_id); > - > - if (arvif->vdev_type == WMI_VDEV_TYPE_AP) { > - ret = ath12k_peer_delete(ar, arvif->vdev_id, vif->addr); > - if (ret) > - ath12k_warn(ab, "failed to submit AP self-peer removal on vdev %d: %d\n", > - arvif->vdev_id, ret); > - } > - > + lockdep_assert_held(&ar->conf_mutex); > reinit_completion(&ar->vdev_delete_done); > > ret = ath12k_wmi_vdev_delete(ar, arvif->vdev_id); > if (ret) { > - ath12k_warn(ab, "failed to delete WMI vdev %d: %d\n", > + ath12k_warn(ab, "failed to delete WMI scan vdev %d: %d\n", this change seems strange. isn't ath12k_mac_vdev_delete() called from both the scan logic and from the normal ath12k_mac_op_remove_interface(), so it might not be a scan vdev that is being deleted? > arvif->vdev_id, ret); > goto err_vdev_del; > }
On 3/22/2024 1:24 AM, Jeff Johnson wrote: > On 3/20/2024 12:09 PM, Rameshkumar Sundaram wrote: >> From: Sriram R <quic_srirrama@quicinc.com> >> >> When multiple radios are advertised as a single wiphy which >> supports various bands, a default scan request to mac80211 >> from cfg80211 will split the driver request based on band, >> so each request will have channels belonging to the same band. >> With this supported by default, the ath12k driver on receiving >> this request checks for one of the channels in the request and >> selects the corresponding radio(ar) on which the scan is going >> to be performed and creates a vdev on that radio. >> >> Note that on scan completion this vdev is not deleted. If a new >> scan request is seen on that same vif for a different band the >> vdev will be deleted and created on the new radio supporting the >> request. The vdev delete logic is refactored to have this done >> dynamically. >> >> The reason for not deleting the vdev on scan stop is to avoid >> repeated delete-create sequence if the scan is on the same band. >> Also, during channel assign, new vdev creation can be optimized >> as well. >> >> Also if the scan is requested when the vdev is in started state, >> no switching to new radio is allowed and scan on channels only >> within same radio is allowed. >> >> 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: Rameshkumar Sundaram <quic_ramess@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/mac.c | 211 +++++++++++++++++++++----- >> 1 file changed, 176 insertions(+), 35 deletions(-) > ... >> -static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, >> - struct ieee80211_vif *vif) >> +static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif) >> { >> - struct ath12k *ar; >> struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); >> - struct ath12k_base *ab; >> + struct ath12k_base *ab = ar->ab; >> unsigned long time_left; >> int ret; >> >> - if (!arvif->is_created) >> - return; >> - >> - ar = arvif->ar; >> - ab = ar->ab; >> - >> - mutex_lock(&ar->conf_mutex); >> - >> - ath12k_dbg(ab, ATH12K_DBG_MAC, "mac remove interface (vdev %d)\n", >> - arvif->vdev_id); >> - >> - if (arvif->vdev_type == WMI_VDEV_TYPE_AP) { >> - ret = ath12k_peer_delete(ar, arvif->vdev_id, vif->addr); >> - if (ret) >> - ath12k_warn(ab, "failed to submit AP self-peer removal on vdev %d: %d\n", >> - arvif->vdev_id, ret); >> - } >> - >> + lockdep_assert_held(&ar->conf_mutex); >> reinit_completion(&ar->vdev_delete_done); >> >> ret = ath12k_wmi_vdev_delete(ar, arvif->vdev_id); >> if (ret) { >> - ath12k_warn(ab, "failed to delete WMI vdev %d: %d\n", >> + ath12k_warn(ab, "failed to delete WMI scan vdev %d: %d\n", > > this change seems strange. isn't ath12k_mac_vdev_delete() called from both the > scan logic and from the normal ath12k_mac_op_remove_interface(), so it might > not be a scan vdev that is being deleted? > No, in Single wiphy, the vdev logic creation for an arvif is such that at any given point of time only one vdev is created for an arvif (either by ath12k_mac_op_add_intf/assign_chanctx/hw_scan). Vdev created by mac_op_scan can either be re-used or deleted & re-created (on a different ar) by mac_op_assign_chanctx() if required. Also once mac_op_assign_chanctx has started the vdev of an arvif, mac_op_hw_scan can only use that vdev. So mac_op_remove just simply deletes the one that is currently created. >> arvif->vdev_id, ret); >> goto err_vdev_del; >> } >
On 3/25/2024 8:24 AM, Rameshkumar Sundaram wrote: > On 3/22/2024 1:24 AM, Jeff Johnson wrote: >> On 3/20/2024 12:09 PM, Rameshkumar Sundaram wrote: >>> From: Sriram R <quic_srirrama@quicinc.com> ... >>> ret = ath12k_wmi_vdev_delete(ar, arvif->vdev_id); >>> if (ret) { >>> - ath12k_warn(ab, "failed to delete WMI vdev %d: %d\n", >>> + ath12k_warn(ab, "failed to delete WMI scan vdev %d: %d\n", >> >> this change seems strange. isn't ath12k_mac_vdev_delete() called from both the >> scan logic and from the normal ath12k_mac_op_remove_interface(), so it might >> not be a scan vdev that is being deleted? >> > No, in Single wiphy, the vdev logic creation for an arvif is such that > at any given point of time only one vdev is created for an arvif (either > by ath12k_mac_op_add_intf/assign_chanctx/hw_scan). > Vdev created by mac_op_scan can either be re-used or deleted & > re-created (on a different ar) by mac_op_assign_chanctx() if required. > Also once mac_op_assign_chanctx has started the vdev of an arvif, > mac_op_hw_scan can only use that vdev. So mac_op_remove just simply > deletes the one that is currently created. then if this function is only ever used to delete a scan vdev, then shouldn't the name be changed to reflect that? the current generic name doesn't reflect that specificity. /jeff
On 3/25/2024 9:03 PM, Jeff Johnson wrote: > On 3/25/2024 8:24 AM, Rameshkumar Sundaram wrote: >> On 3/22/2024 1:24 AM, Jeff Johnson wrote: >>> On 3/20/2024 12:09 PM, Rameshkumar Sundaram wrote: >>>> From: Sriram R <quic_srirrama@quicinc.com> > ... >>>> ret = ath12k_wmi_vdev_delete(ar, arvif->vdev_id); >>>> if (ret) { >>>> - ath12k_warn(ab, "failed to delete WMI vdev %d: %d\n", >>>> + ath12k_warn(ab, "failed to delete WMI scan vdev %d: %d\n", >>> >>> this change seems strange. isn't ath12k_mac_vdev_delete() called from both the >>> scan logic and from the normal ath12k_mac_op_remove_interface(), so it might >>> not be a scan vdev that is being deleted? >>> >> No, in Single wiphy, the vdev logic creation for an arvif is such that >> at any given point of time only one vdev is created for an arvif (either >> by ath12k_mac_op_add_intf/assign_chanctx/hw_scan). >> Vdev created by mac_op_scan can either be re-used or deleted & >> re-created (on a different ar) by mac_op_assign_chanctx() if required. >> Also once mac_op_assign_chanctx has started the vdev of an arvif, >> mac_op_hw_scan can only use that vdev. So mac_op_remove just simply >> deletes the one that is currently created. > > then if this function is only ever used to delete a scan vdev, then shouldn't > the name be changed to reflect that? the current generic name doesn't reflect > that specificity. > Ah Sorry, i misunderstood your point earlier, as i mentioned vdev is created for an arvif either by ath12k_mac_op_add_intf/assign_chanctx/hw_scan). So this vdev_delete can never say if its a scan vdev. For some reason it was put as scan vdev delete in print message :( . We should remove this change. Earlier I thought you were asking why vdev delete is being called from both places and was trying explaining the logic behind. Totally missed the print message, Really sorry about that.
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index 01dff17d4a36..86bf55961905 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -244,6 +244,8 @@ static const u32 ath12k_smps_map[] = { static int ath12k_start_vdev_delay(struct ath12k *ar, struct ath12k_vif *arvif); static void ath12k_mac_stop(struct ath12k *ar); +static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif); +static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif); static const char *ath12k_mac_phymode_str(enum wmi_phy_mode mode) { @@ -3009,6 +3011,42 @@ static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw, mutex_unlock(&ar->conf_mutex); } +static struct ath12k* +ath12k_mac_select_scan_device(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct ieee80211_scan_request *req) +{ + struct ath12k_hw *ah = hw->priv; + enum nl80211_band band; + struct ath12k *ar; + int i; + + if (ah->num_radio == 1) + return ah->radio; + + /* Currently mac80211 supports splitting scan requests into + * multiple scan requests per band. + * Loop through first channel and determine the scan radio + * TODO: There could be 5 GHz low/high channels in that case + * split the hw request and perform multiple scans + */ + + if (req->req.channels[0]->center_freq < ATH12K_MIN_5G_FREQ) + band = NL80211_BAND_2GHZ; + else if (req->req.channels[0]->center_freq < ATH12K_MIN_6G_FREQ) + band = NL80211_BAND_5GHZ; + else + band = NL80211_BAND_6GHZ; + + for_each_ar(ah, ar, i) { + /* TODO 5 GHz low high split changes */ + if (ar->mac.sbands[band].channels) + return ar; + } + + return NULL; +} + void __ath12k_mac_scan_finish(struct ath12k *ar) { struct ieee80211_hw *hw = ath12k_ar_to_hw(ar); @@ -3178,15 +3216,68 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, struct ieee80211_scan_request *hw_req) { struct ath12k_hw *ah = ath12k_hw_to_ah(hw); - struct ath12k *ar; + struct ath12k *ar, *prev_ar; struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); struct cfg80211_scan_request *req = &hw_req->req; struct ath12k_wmi_scan_req_arg arg = {}; int ret; int i; + bool create = true; - ar = ath12k_ah_to_ar(ah, 0); + if (ah->num_radio == 1) { + WARN_ON(!arvif->is_created); + ar = ath12k_ah_to_ar(ah, 0); + goto scan; + } + + /* Since the targeted scan device could depend on the frequency + * requested in the hw_req, select the corresponding radio + */ + ar = ath12k_mac_select_scan_device(hw, vif, hw_req); + if (!ar) + return -EINVAL; + + /* If the vif is already assigned to a specific vdev of an ar, + * check whether its already started, vdev which is started + * are not allowed to switch to a new radio. + * If the vdev is not started, but was earlier created on a + * different ar, delete that vdev and create a new one. We don't + * delete at the scan stop as an optimization to avoid redundant + * delete-create vdev's for the same ar, in case the request is + * always on the same band for the vif + */ + if (arvif->is_created) { + if (WARN_ON(!arvif->ar)) + return -EINVAL; + + if (ar != arvif->ar && arvif->is_started) + return -EINVAL; + if (ar != arvif->ar) { + /* backup the previously used ar ptr, since the vdev delete + * would assign the arvif->ar to NULL after the call + */ + prev_ar = arvif->ar; + mutex_lock(&prev_ar->conf_mutex); + ret = ath12k_mac_vdev_delete(prev_ar, vif); + mutex_unlock(&prev_ar->conf_mutex); + if (ret) + ath12k_warn(prev_ar->ab, + "unable to delete scan vdev %d\n", ret); + } else { + create = false; + } + } + if (create) { + mutex_lock(&ar->conf_mutex); + ret = ath12k_mac_vdev_create(ar, vif); + mutex_unlock(&ar->conf_mutex); + if (ret) { + ath12k_warn(ar->ab, "unable to create scan vdev %d\n", ret); + return -EINVAL; + } + } +scan: mutex_lock(&ar->conf_mutex); spin_lock_bh(&ar->data_lock); @@ -3272,10 +3363,13 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw, static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif) { - struct ath12k_hw *ah = ath12k_hw_to_ah(hw); + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); struct ath12k *ar; - ar = ath12k_ah_to_ar(ah, 0); + if (!arvif->is_created) + return; + + ar = arvif->ar; mutex_lock(&ar->conf_mutex); ath12k_scan_abort(ar); @@ -5951,6 +6045,7 @@ static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif) if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled) ath12k_mac_monitor_vdev_create(ar); + arvif->ar = ar; return ret; err_peer_del: @@ -5977,6 +6072,7 @@ static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif) ath12k_wmi_vdev_delete(ar, arvif->vdev_id); ar->num_created_vdevs--; arvif->is_created = false; + arvif->ar = NULL; ar->allocated_vdev_map &= ~(1LL << arvif->vdev_id); ab->free_vdev_map |= 1LL << arvif->vdev_id; ab->free_vdev_stats_id_map &= ~(1LL << arvif->vdev_stats_id); @@ -5995,12 +6091,45 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, { struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); struct ath12k_hw *ah = hw->priv; + struct ath12k *ar, *prev_ar; struct ath12k_base *ab; - struct ath12k *ar; int ret; if (arvif->ar) { - WARN_ON(!arvif->is_created); + /* This is not expected really */ + if (WARN_ON(!arvif->is_created)) { + arvif->ar = NULL; + return NULL; + } + + if (ah->num_radio == 1) + goto out; + + ar = ath12k_get_ar_by_ctx(hw, ctx); + /* This can happen as scan vdev gets created during multiple scans + * across different radios before a vdev is brought up in + * a certain radio. + */ + if (ar != arvif->ar) { + if (WARN_ON(arvif->is_started)) + return NULL; + + /* backup the previously used ar ptr since arvif->ar would + * be set to NULL after vdev delete is done + */ + prev_ar = arvif->ar; + mutex_lock(&prev_ar->conf_mutex); + ret = ath12k_mac_vdev_delete(prev_ar, vif); + + if (ret) + ath12k_warn(prev_ar->ab, "unable to delete vdev %d\n", + ret); + mutex_unlock(&prev_ar->conf_mutex); + ret = ath12k_mac_vdev_create(ar, vif); + if (ret) + ath12k_warn(ar->ab, "unable to create vdev %d\n", ret); + mutex_unlock(&ar->conf_mutex); + } goto out; } @@ -6107,38 +6236,19 @@ static void ath12k_mac_vif_unref(struct ath12k_dp *dp, struct ieee80211_vif *vif } } -static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, - struct ieee80211_vif *vif) +static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif) { - struct ath12k *ar; struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); - struct ath12k_base *ab; + struct ath12k_base *ab = ar->ab; unsigned long time_left; int ret; - if (!arvif->is_created) - return; - - ar = arvif->ar; - ab = ar->ab; - - mutex_lock(&ar->conf_mutex); - - ath12k_dbg(ab, ATH12K_DBG_MAC, "mac remove interface (vdev %d)\n", - arvif->vdev_id); - - if (arvif->vdev_type == WMI_VDEV_TYPE_AP) { - ret = ath12k_peer_delete(ar, arvif->vdev_id, vif->addr); - if (ret) - ath12k_warn(ab, "failed to submit AP self-peer removal on vdev %d: %d\n", - arvif->vdev_id, ret); - } - + lockdep_assert_held(&ar->conf_mutex); reinit_completion(&ar->vdev_delete_done); ret = ath12k_wmi_vdev_delete(ar, arvif->vdev_id); if (ret) { - ath12k_warn(ab, "failed to delete WMI vdev %d: %d\n", + ath12k_warn(ab, "failed to delete WMI scan vdev %d: %d\n", arvif->vdev_id, ret); goto err_vdev_del; } @@ -6150,6 +6260,10 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, goto err_vdev_del; } + ab->free_vdev_map |= 1LL << arvif->vdev_id; + ar->allocated_vdev_map &= ~(1LL << arvif->vdev_id); + ar->num_created_vdevs--; + if (arvif->vdev_type == WMI_VDEV_TYPE_MONITOR) { ar->monitor_vdev_id = -1; ar->monitor_vdev_created = false; @@ -6157,12 +6271,6 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, ret = ath12k_mac_monitor_vdev_delete(ar); } - ab->free_vdev_map |= 1LL << (arvif->vdev_id); - ar->allocated_vdev_map &= ~(1LL << arvif->vdev_id); - ab->free_vdev_stats_id_map &= ~(1LL << arvif->vdev_stats_id); - ar->num_created_vdevs--; - arvif->is_created = false; - ath12k_dbg(ab, ATH12K_DBG_MAC, "vdev %pM deleted, vdev_id %d\n", vif->addr, arvif->vdev_id); @@ -6184,6 +6292,39 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, clear_bit(ATH12K_FLAG_MONITOR_ENABLED, &ar->monitor_flags); /* TODO: recal traffic pause state based on the available vdevs */ + arvif->is_created = false; + arvif->ar = NULL; + + return ret; +} + +static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw, + struct ieee80211_vif *vif) +{ + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); + struct ath12k_base *ab; + struct ath12k *ar; + int ret; + + if (!arvif->is_created) + return; + + ar = arvif->ar; + ab = ar->ab; + + mutex_lock(&ar->conf_mutex); + + ath12k_dbg(ab, ATH12K_DBG_MAC, "mac remove interface (vdev %d)\n", + arvif->vdev_id); + + if (arvif->vdev_type == WMI_VDEV_TYPE_AP) { + ret = ath12k_peer_delete(ar, arvif->vdev_id, vif->addr); + if (ret) + ath12k_warn(ab, "failed to submit AP self-peer removal on vdev %d: %d\n", + arvif->vdev_id, ret); + } + + ath12k_mac_vdev_delete(ar, vif); mutex_unlock(&ar->conf_mutex); }