| Message ID | 20240111045045.28377-1-quic_srirrama@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: ath12k: Add single wiphy support | expand |
>-----Original Message----- >From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> >Sent: Friday, January 12, 2024 1:46 AM >To: Sriram R (QUIC) <quic_srirrama@quicinc.com>; ath12k@lists.infradead.org >Cc: linux-wireless@vger.kernel.org >Subject: Re: [PATCH 00/12] wifi: ath12k: Add single wiphy support > >On 1/10/2024 8:50 PM, Sriram R wrote: >> This patchset is dependent on "[PATCH 0/2] wifi: ath12k: Introduce hw >abstraction" > >I'm currently unable to apply the prerequisite patches to my workspace, so I won't >be able to review your patches until that is resolved. Sure. I believe its resolved with "[PATCH v2 0/2] wifi: ath12k: Introduce hw abstraction". Thanks, Sriram.R
On 1/11/2024 8:34 PM, Sriram R (QUIC) wrote: >> -----Original Message----- >> From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> >> Sent: Friday, January 12, 2024 1:46 AM >> To: Sriram R (QUIC) <quic_srirrama@quicinc.com>; ath12k@lists.infradead.org >> Cc: linux-wireless@vger.kernel.org >> Subject: Re: [PATCH 00/12] wifi: ath12k: Add single wiphy support >> >> On 1/10/2024 8:50 PM, Sriram R wrote: >>> This patchset is dependent on "[PATCH 0/2] wifi: ath12k: Introduce hw >> abstraction" >> >> I'm currently unable to apply the prerequisite patches to my workspace, so I won't >> be able to review your patches until that is resolved. > Sure. I believe its resolved with "[PATCH v2 0/2] wifi: ath12k: Introduce hw abstraction". Yes, I have now processed the prerequisite patchset, and your patchset applies cleanly on top of that. As already mentioned you need to add your S-O-B to all patches that you are posting on behalf of others. But let me review the individual patches to make sure I don't have any other comments before you re-spin. /jeff
On 1/10/2024 8:50 PM, Sriram R wrote: > Modify add and remove chanctx mac80211 ops to fetch the correct > radio(ar) based on channel context. > > This change also introduces new helper function to fetch the > radio/ar based on channel context and ieee80211_chan which internally > uses the radio's low/high freq range. > > 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> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 1/10/2024 8:50 PM, Sriram R wrote: > When mac80211 does drv start/stop, apply the state change > for all the radios within the wiphy in ath12k. > > 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> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 1/10/2024 8:50 PM, Sriram R wrote: > 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> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 1/10/2024 8:50 PM, Sriram R wrote: > Since the vdev create for a corresponding vif is deferred > until a channel is assigned, cache the information which > are received through mac80211 ops between add_interface() > and assign_vif_chanctx() and set them once the vdev is > created on one of the ath12k radios as the channel gets > assigned via assign_vif_chanctx(). > > 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> > --- ... > +static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, > + struct ieee80211_vif *vif, struct ieee80211_sta *sta, > + struct ieee80211_key_conf *key) > +{ > + struct ath12k_hw *ah = ath12k_hw_to_ah(hw); > + struct ath12k *ar; > + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); > + int ret; > + > + /* BIP needs to be done in software */ > + if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC || > + key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 || > + key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 || > + key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256) > + return 1; I know this in the existing code, but what is the significance of returning 1? Should this be returning a -errno like the error cases that follow? > + > + if (key->keyidx > WMI_MAX_KEY_INDEX) > + return -ENOSPC; > + > + mutex_lock(&ah->conf_mutex); > + > + ar = ath12k_get_ar_by_vif(hw, vif); > + if (!ar) { > + /* ar is expected to be valid when sta ptr is available */ > + if (sta) { > + mutex_unlock(&ah->conf_mutex); > + WARN_ON_ONCE(1); > + return -EINVAL; > + } > + arvif->cache.key_conf.cmd = cmd; > + arvif->cache.key_conf.key = key; > + arvif->cache.key_conf.changed = true; > + mutex_unlock(&ah->conf_mutex); > + return 0; > + } > + > + mutex_lock(&ar->conf_mutex); > + ret = ath12k_mac_set_key(ar, cmd, vif, sta, key); > mutex_unlock(&ar->conf_mutex); > + > mutex_unlock(&ah->conf_mutex); > return ret; > }
On 1/10/2024 8:50 PM, Sriram R wrote: > With all the radios being combined and registered as a single > mac80211 hw/wiphy, separate regd built from firmware rules need > not be updated to cfg80211. Rather we can pick one of the regd > built from the rules to update to cfg80211 for the whole > registered device. We prefer 6 GHz pdev based rules since it has > the rules for all bands. If the hw doesn't support 6 GHz, then update > rules from one of the pdevs. Do you need to handle the case where there are multiple instances of hardware that have divergent regds? I'm thinking of a case where there are 3 underlying hardware platforms, one that is 2 GHz, one that is 5 GHz, and one that is 6 GHz, and each of them has a regd that only has the rules for the supported band. In such a case wouldn't you need to aggregate the rules in the wiphy, not just pick one of the sets of rules? If that is not an expected case, ideally you should describe the requirements & assumptions that are in play here. > > Also, when regulatory notification is received, update to all the > underlying radios/ar so that it becomes aware of the change and as > well us it updates its local regd with the new country rules. Later > pick the appropriate pdev's regd(6 GHz if available) and apply to > cfg80211. Here again, can you elaborate on the requirements and assumptions? Do you update all channels to each hardware, even if each hardware only supports a single band? > > 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> > --- > drivers/net/wireless/ath/ath12k/core.h | 2 + > drivers/net/wireless/ath/ath12k/mac.c | 2 + > drivers/net/wireless/ath/ath12k/reg.c | 60 ++++++++++++++++++++++---- > 3 files changed, 55 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h > index 883556313fb0..ac7b48cc2e7e 100644 > --- a/drivers/net/wireless/ath/ath12k/core.h > +++ b/drivers/net/wireless/ath/ath12k/core.h > @@ -630,6 +630,8 @@ struct ath12k_hw { > struct mutex conf_mutex; > > u8 num_radio; > + bool regd_updated; > + bool use_6ghz_regd; > struct ath12k radio[] __aligned(sizeof(void *)); > }; > > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index 25d82c6fe89a..2f19d6c5b741 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -7948,6 +7948,7 @@ static int ath12k_mac_setup_channels_rates(struct ath12k *ar, > struct ath12k_wmi_hal_reg_capabilities_ext_arg *reg_cap; > void *channels; > u32 phy_id; > + struct ath12k_hw *ah = ar->ah; > > BUILD_BUG_ON((ARRAY_SIZE(ath12k_2ghz_channels) + > ARRAY_SIZE(ath12k_5ghz_channels) + > @@ -8000,6 +8001,7 @@ static int ath12k_mac_setup_channels_rates(struct ath12k *ar, > ath12k_mac_update_ch_list(ar, band, > reg_cap->low_5ghz_chan, > reg_cap->high_5ghz_chan); > + ah->use_6ghz_regd = true; > } > > if (reg_cap->low_5ghz_chan < ATH12K_MIN_6G_FREQ) { > diff --git a/drivers/net/wireless/ath/ath12k/reg.c b/drivers/net/wireless/ath/ath12k/reg.c > index da64482f1024..3a3f66d8ea11 100644 > --- a/drivers/net/wireless/ath/ath12k/reg.c > +++ b/drivers/net/wireless/ath/ath12k/reg.c > @@ -50,7 +50,7 @@ ath12k_reg_notifier(struct wiphy *wiphy, struct regulatory_request *request) > struct ath12k_wmi_init_country_arg arg; > struct ath12k_hw *ah = ath12k_hw_to_ah(hw); > struct ath12k *ar = ath12k_ah_to_ar(ah, 0); > - int ret; > + int ret, i; > > ath12k_dbg(ar->ab, ATH12K_DBG_REG, > "Regulatory Notification received for %s\n", wiphy_name(wiphy)); > @@ -85,10 +85,17 @@ ath12k_reg_notifier(struct wiphy *wiphy, struct regulatory_request *request) > memcpy(&arg.cc_info.alpha2, request->alpha2, 2); > arg.cc_info.alpha2[2] = 0; > > - ret = ath12k_wmi_send_init_country_cmd(ar, &arg); > - if (ret) > - ath12k_warn(ar->ab, > - "INIT Country code set to fw failed : %d\n", ret); > + /* Allow fresh updates to wiphy regd */ > + ah->regd_updated = false; > + > + /* Send the reg change request to all the radios */ > + for (i = 0; i < ah->num_radio; i++) { > + ret = ath12k_wmi_send_init_country_cmd(ar, &arg); > + if (ret) > + ath12k_warn(ar->ab, > + "INIT Country code set to fw failed : %d\n", ret); > + ar++; > + } > } > > int ath12k_reg_update_chan_list(struct ath12k *ar) > @@ -204,8 +211,31 @@ int ath12k_regd_update(struct ath12k *ar, bool init) > struct ieee80211_regdomain *regd, *regd_copy = NULL; > int ret, regd_len, pdev_id; > struct ath12k_base *ab; > + struct ath12k_hw *ah; > + int i; > > ab = ar->ab; > + ah = ar->ah; > + > + /* If one of the radios within ah has already updated the regd for > + * the wiphy, then avoid setting regd again > + */ > + if (ah->regd_updated) > + return 0; > + > + /* firmware provides reg rules which are similar for 2 GHz and 5 GHz > + * pdev but 6 GHz pdev has superset of all rules including rules for > + * all bands, we prefer 6 GHz pdev's rules to be used for setup of > + * the wiphy regd. > + * If 6 GHz pdev was part of the ath12k_hw, wait for the 6 GHz pdev, > + * else pick the first pdev which calls this function and use its > + * regd to update global hw regd. > + * The regd_updated flag set at the end will not allow any further > + * updates. > + */ > + if (ah->use_6ghz_regd && !ar->supports_6ghz) > + return 0; > + > pdev_id = ar->pdev_idx; > > spin_lock_bh(&ab->base_lock); > @@ -258,10 +288,22 @@ int ath12k_regd_update(struct ath12k *ar, bool init) > if (ret) > goto err; > > - if (ar->state == ATH12K_STATE_ON) { > - ret = ath12k_reg_update_chan_list(ar); > - if (ret) > - goto err; > + ah->regd_updated = true; > + > + /* Apply the new regd to all the radios, this is expected to be received only once > + * since we check for ah->regd_updated and allow here only once > + */ > + ar = ah->radio; > + ab = ar->ab; > + > + for (i = 0; i < ah->num_radio; i++) { > + if (ar->state == ATH12K_STATE_ON) { > + ret = ath12k_reg_update_chan_list(ar); > + if (ret) > + goto err; > + } > + ar++; > + ab = ar->ab; > } > > return 0;
On 1/10/2024 8:50 PM, Sriram R wrote: > Since multiple radios are abstracted under a single wiphy, > apply the rts threshold value to all the vdevs of the radios > combined under single wiphy. > > This also implies that vif specific rts threshold support > needs to be added in future from cfg80211/mac80211. > > 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> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On 1/12/2024 8:44 AM, Jeff Johnson wrote: > On 1/11/2024 8:34 PM, Sriram R (QUIC) wrote: >>> -----Original Message----- >>> From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> >>> Sent: Friday, January 12, 2024 1:46 AM >>> To: Sriram R (QUIC) <quic_srirrama@quicinc.com>; ath12k@lists.infradead.org >>> Cc: linux-wireless@vger.kernel.org >>> Subject: Re: [PATCH 00/12] wifi: ath12k: Add single wiphy support >>> >>> On 1/10/2024 8:50 PM, Sriram R wrote: >>>> This patchset is dependent on "[PATCH 0/2] wifi: ath12k: Introduce hw >>> abstraction" >>> >>> I'm currently unable to apply the prerequisite patches to my workspace, so I won't >>> be able to review your patches until that is resolved. >> Sure. I believe its resolved with "[PATCH v2 0/2] wifi: ath12k: Introduce hw abstraction". > > Yes, I have now processed the prerequisite patchset, and your patchset > applies cleanly on top of that. > > As already mentioned you need to add your S-O-B to all patches that you > are posting on behalf of others. But let me review the individual > patches to make sure I don't have any other comments before you re-spin. OK, I've reviewed the series. You are now free to incorporate the requested changes and to post a v2. Please include my Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> on all of the patches where I have already supplied it on the mailing list. /jeff
Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> +static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, >> + struct ieee80211_vif *vif, struct ieee80211_sta *sta, >> + struct ieee80211_key_conf *key) >> +{ >> + struct ath12k_hw *ah = ath12k_hw_to_ah(hw); >> + struct ath12k *ar; >> + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); >> + int ret; >> + >> + /* BIP needs to be done in software */ >> + if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC || >> + key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 || >> + key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 || >> + key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256) >> + return 1; > > I know this in the existing code, but what is the significance of > returning 1? Should this be returning a -errno like the error cases that > follow? It's to use software encryption, from mac80211.h: * Note that in the case that the @IEEE80211_HW_SW_CRYPTO_CONTROL flag is * set, mac80211 will not automatically fall back to software crypto if * enabling hardware crypto failed. The set_key() call may also return the * value 1 to permit this specific key/algorithm to be done in software. Yeah, this is confusing. IMHO there should be a define or an enum for this value.
>-----Original Message----- >From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> >Sent: Friday, January 12, 2024 11:46 PM >To: Sriram R (QUIC) <quic_srirrama@quicinc.com>; ath12k@lists.infradead.org >Cc: linux-wireless@vger.kernel.org >Subject: Re: [PATCH 09/12] wifi: ath12k: modify regulatory support for single wiphy >architecture > >On 1/10/2024 8:50 PM, Sriram R wrote: >> With all the radios being combined and registered as a single >> mac80211 hw/wiphy, separate regd built from firmware rules need not be >> updated to cfg80211. Rather we can pick one of the regd built from the >> rules to update to cfg80211 for the whole registered device. We prefer >> 6 GHz pdev based rules since it has the rules for all bands. If the hw >> doesn't support 6 GHz, then update rules from one of the pdevs. > >Do you need to handle the case where there are multiple instances of hardware >that have divergent regds? I'm thinking of a case where there are 3 underlying >hardware platforms, one that is 2 GHz, one that is 5 GHz, and one that is 6 GHz, and >each of them has a regd that only has the rules for the supported band. In such a >case wouldn't you need to aggregate the rules in the wiphy, not just pick one of the >sets of rules? > >If that is not an expected case, ideally you should describe the requirements & >assumptions that are in play here. This is not an expected case with our understanding and testing so far. The 2GHz and 5GHz radio FW would pass the regd comprising both 2GHz, 5GHz. The 6GHz FW is passing all of them - 2,5,6GHz, hence the need to club all the regd info separately is not required. I will make this assumption clear in the next revision. > >> >> Also, when regulatory notification is received, update to all the >> underlying radios/ar so that it becomes aware of the change and as >> well us it updates its local regd with the new country rules. Later >> pick the appropriate pdev's regd(6 GHz if available) and apply to >> cfg80211. > >Here again, can you elaborate on the requirements and assumptions? Do you >update all channels to each hardware, even if each hardware only supports a >single band? By update during reg_notifier here we mean the country code alone to each of the FW and the channel list update on receiving the regd would update only the corresponding supported channels to the respective FW and not the combined list. Thanks, Sriram.R
With the introduction of Multi Link Operation (MLO) support in IEEE802.11be, each EHT AP/non AP interface is capable of operating with multiple radio links. cfg80211/mac80211 expects drivers to abstract the communication between such Multi Link HW and mac80211/cfg80211 since it depends on different driver/HW implementation. Hence the single wiphy abstraction with changes in datastructures were introduced in "wifi: ath12k: Introduce hw abstraction" This patchset extends the implementation to allow combination of multiple underlying radios into a single composite hw/wiphy for registration. Since now multiple radios are represented by a single wiphy, changes are required in various mac ops that the driver supports since the driver now needs to learn on how to tunnel various mac ops properly to a specific radio. This patchset covers the basic mac80211 ops for an interface bringup and operation. Note: In addition to sanity on single radio QCN9274 and WCN7850 the single wiphy changes are tested over "wifi: ath12k: QCN9274 dualmac bring up" where multiple radios in the dualmac SoC are combined and tested. Monitor and hw reconfig support for Single Wiphy will be done in future patchsets. This patchset is dependent on "[PATCH 0/2] wifi: ath12k: Introduce hw abstraction" Karthikeyan Periyasamy (1): wifi: ath12k: add multiple radio support in a single MAC HW un/register Sriram R (11): wifi: ath12k: Modify add and remove chanctx ops for single wiphy support wifi: ath12k: modify ath12k mac start/stop ops for single wiphy wifi: ath12k: vdev statemachine changes for single wiphy wifi: ath12k: scan statemachine changes for single wiphy wifi: ath12k: fetch correct radio based on vdev status wifi: ath12k: Cache vdev configs before vdev create wifi: ath12k: Add additional checks for vif and sta iterators wifi: ath12k: modify regulatory support for single wiphy architecture wifi: ath12k: Modify set and get antenna mac ops for single wiphy wifi: ath12k: Modify rts threshold mac op for single wiphy wifi: ath12k: support get_survey mac op for single wiphy drivers/net/wireless/ath/ath12k/core.h | 40 +- drivers/net/wireless/ath/ath12k/hw.h | 3 +- drivers/net/wireless/ath/ath12k/mac.c | 1014 +++++++++++++++++++----- drivers/net/wireless/ath/ath12k/reg.c | 62 +- 4 files changed, 886 insertions(+), 233 deletions(-)