| Message ID | 20240312135557.1778379-1-quic_ramess@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: ath12k: Add single wiphy support | expand |
On 3/14/2024 1:26 AM, Jonas Gorski wrote: > On Wed, 13 Mar 2024 at 20:18, Jeff Johnson <quic_jjohnson@quicinc.com> wrote: >> >> On 3/13/2024 9:58 AM, Kalle Valo wrote: >>> Kalle Valo <kvalo@kernel.org> writes: >>> >>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >>>> >>>>> On 3/13/2024 5:57 AM, Rameshkumar Sundaram wrote: >>>>>> On 3/13/2024 3:23 AM, Jeff Johnson wrote: >>>>>>> and guess we have to figure out how to suppress the ath12k-check issues with >>>>>>> this macro >>>>>> ath12k-check complains about the reuse of ah and index arguments which >>>>>> may get evaluated multiple times if its an arithmetic expression, But >>>>>> areas where we use the macro in our code aren't doing so. >>>>>> Do you have any suggestions here ? or shall we go back and use this >>>>>> for-loop inline. >>>>> >>>>> The macro makes sense -- we'll need to update the overrides in ath12k-check. >>>> >>>> IIRC it is possible to avoid variable reuse in macros with typeof() >>>> operator (or something like that). I can't remember the details right >>>> now but I think there are examples in the kernel code. >>> >>> Here's the GCC documentation with an example: >>> >>> https://gcc.gnu.org/onlinedocs/gcc/Typeof.html >>> Thanks Kalle for the references, as Jeff mentioned below, we need to reuse the arguments since we write to ar and index arguments on each iteration. Defining local vars using typeof() without limiting their scope (since we are defining a for_each iterator{}) leads other issues like redefinition of variables in functions where we use this macro more than once :( Also even if we somehow manage to convince check-patch, we'll still end up evaluating index and ar arguments in every iteration of loop. This just gives an impression to check-patch that the macro is unsafe (although logically its not). Experts, what is the standard we should follow here. Please suggest. >> >> the problem here is that the macro actually writes to those arguments multiple >> times, so we actually need to reuse the arguments >> >> the macro as defined exactly matches the semantics of other for_each macros in >> the kernel, i.e. see in include/linux/list.h: >> #define list_for_each(pos, head) \ >> for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next) >> >> what I don't understand is why list_for_each() doesn't trigger the same >> checkpatch issues. even stranger is that if I copy that macro into ath12k then >> I do see the same checkpatch issues: >> CHECK: Macro argument reuse 'pos' - possible side-effects? >> #998: FILE: drivers/net/wireless/ath/ath12k/core.h:998: >> +#define list_for_each(pos, head) \ >> + for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next) >> >> CHECK: Macro argument reuse 'head' - possible side-effects? >> #998: FILE: drivers/net/wireless/ath/ath12k/core.h:998: >> +#define list_for_each(pos, head) \ >> + for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next) >> >> So I'm really confused since I don't see anything in checkpatch.pl that would >> cause the behavior to change between macros in include/linux/list.h vs macros >> in drivers/net/wireless/ath/ath12k/core.h > > The definition of the macro causes the complaint, not the usage of it. > If you run checkpatch.pl on include/linux/list.h, you'll get the same > output: > > $ ./scripts/checkpatch.pl --strict --file include/linux/list.h > ... > CHECK: Macro argument reuse 'pos' - possible side-effects? > #686: FILE: include/linux/list.h:686: > +#define list_for_each(pos, head) \ > + for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next) > > CHECK: Macro argument reuse 'head' - possible side-effects? > #686: FILE: include/linux/list.h:686: > +#define list_for_each(pos, head) \ > + for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next) > ... Thanks Jonas and Jeff for your insights!!
Rameshkumar Sundaram <quic_ramess@quicinc.com> writes: > On 3/14/2024 1:26 AM, Jonas Gorski wrote: >> On Wed, 13 Mar 2024 at 20:18, Jeff Johnson <quic_jjohnson@quicinc.com> wrote: >>> >>> On 3/13/2024 9:58 AM, Kalle Valo wrote: >>>> Kalle Valo <kvalo@kernel.org> writes: >>>> >>>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >>>>> >>>>>> On 3/13/2024 5:57 AM, Rameshkumar Sundaram wrote: >>>>>>> On 3/13/2024 3:23 AM, Jeff Johnson wrote: >>>>>>>> and guess we have to figure out how to suppress the ath12k-check issues with >>>>>>>> this macro >>>>>>> ath12k-check complains about the reuse of ah and index arguments which >>>>>>> may get evaluated multiple times if its an arithmetic expression, But >>>>>>> areas where we use the macro in our code aren't doing so. >>>>>>> Do you have any suggestions here ? or shall we go back and use this >>>>>>> for-loop inline. >>>>>> >>>>>> The macro makes sense -- we'll need to update the overrides in ath12k-check. >>>>> >>>>> IIRC it is possible to avoid variable reuse in macros with typeof() >>>>> operator (or something like that). I can't remember the details right >>>>> now but I think there are examples in the kernel code. >>>> >>>> Here's the GCC documentation with an example: >>>> >>>> https://gcc.gnu.org/onlinedocs/gcc/Typeof.html >>>> > Thanks Kalle for the references, as Jeff mentioned below, we need to > reuse the arguments since we write to ar and index arguments on each > iteration. > > Defining local vars using typeof() without limiting their scope (since > we are defining a for_each iterator{}) leads other issues like > redefinition of variables in functions where we use this macro more > than once :( > > Also even if we somehow manage to convince check-patch, we'll still > end up evaluating index and ar arguments in every iteration of loop. > This just gives an impression to check-patch that the macro is unsafe > (although logically its not). > Experts, what is the standard we should follow here. Please suggest. Yeah, typeof() won't help here as we can't create a local variable. Or at least I can't come up way to do that safely, ideas very welcome. I think it's just best to ignore the checkpatch warning for now, unless better proposals come up. ath12k-check has functionality to ignore specific warnings (see checkpatch_filter array), I can add this warning to the array.
On 3/13/2024 4:05 AM, Jeff Johnson wrote: > On 3/12/2024 6:55 AM, 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> >> --- > ... >> + /* 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 reduntant > > s/reduntant/redundant/ Sure will fix it in next version > >> + * delete-create vdev's for the same ar, in case the request is >> + * always on the same band for the vif >> + */ >
On 3/13/2024 4:31 AM, Jeff Johnson wrote: > On 3/12/2024 6:55 AM, Rameshkumar Sundaram wrote: >> From: Sriram R <quic_srirrama@quicinc.com> >> >> 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> >> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/core.h | 19 ++++ >> drivers/net/wireless/ath/ath12k/mac.c | 116 +++++++++++++++++++------ >> 2 files changed, 107 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h >> index 70daec38d486..fe151aa90687 100644 >> --- a/drivers/net/wireless/ath/ath12k/core.h >> +++ b/drivers/net/wireless/ath/ath12k/core.h >> @@ -214,6 +214,24 @@ enum ath12k_monitor_flags { >> ATH12K_FLAG_MONITOR_ENABLED, >> }; >> >> +struct ath12k_tx_conf { >> + bool changed; >> + u16 ac; >> + struct ieee80211_tx_queue_params tx_queue_params; >> +}; >> + >> +struct ath12k_key_conf { >> + bool changed; >> + enum set_key_cmd cmd; >> + struct ieee80211_key_conf *key; >> +}; >> + >> +struct ath12k_vif_cache { >> + struct ath12k_tx_conf tx_conf; >> + struct ath12k_key_conf key_conf; >> + u32 bss_conf_changed; >> +}; >> + >> struct ath12k_vif { >> u32 vdev_id; >> enum wmi_vdev_type vdev_type; >> @@ -268,6 +286,7 @@ struct ath12k_vif { >> u8 vdev_stats_id; >> u32 punct_bitmap; >> bool ps; >> + struct ath12k_vif_cache cache; > > is there a reason this isn't a *cache? > this allocation is only needed for the time between when a vdev is created and > when a channel is assigned, so why not have a dynamic allocation that is only > around for that time? otherwise for every vdev you waste this memory for the > lifetime of the vdev. > There is no specific reason behind it. Your point makes sense, we need not have this for lifetime of the vdev. Shall we have cache as such and have *tx_conf and *key_conf as dynamically allocated members ? That way we can allocate them whenever corresponding config is received and have it in cache. >> }; >> >> struct ath12k_vif_iter { >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c >> index 6d2176b0a556..fab92f08fdb7 100644 >> --- a/drivers/net/wireless/ath/ath12k/mac.c >> +++ b/drivers/net/wireless/ath/ath12k/mac.c >> @@ -3021,12 +3021,14 @@ static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw, >> >> ar = ath12k_get_ar_by_vif(hw, vif); >> >> - /* TODO if the vdev is not created on a certain radio, >> + /* if the vdev is not created on a certain radio, >> * cache the info to be updated later on vdev creation >> */ >> >> - if (!ar) >> + if (!ar) { >> + arvif->cache.bss_conf_changed |= changed; > > why don't you need to save the actual *info as well? > info(ieee80211_bss_conf) can be obtained from arvif->vif(ieee80211_vif) whenever needed. >> return; >> + } >> >> mutex_lock(&ar->conf_mutex); >> >> @@ -3517,12 +3519,11 @@ static int ath12k_clear_peer_keys(struct ath12k_vif *arvif, >> return first_errno; >> } >> >> -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) >> +static int ath12k_mac_set_key(struct ath12k *ar, enum set_key_cmd cmd, >> + struct ieee80211_vif *vif, struct ieee80211_sta *sta, >> + struct ieee80211_key_conf *key) >> { >> - struct ath12k *ar; >> - struct ath12k_base *ab; >> + struct ath12k_base *ab = ar->ab; >> struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); >> struct ath12k_peer *peer; >> struct ath12k_sta *arsta; >> @@ -3530,28 +3531,11 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, >> int ret = 0; >> u32 flags = 0; >> >> - /* 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; >> - >> - ar = ath12k_get_ar_by_vif(hw, vif); >> - if (!ar) { >> - WARN_ON_ONCE(1); >> - return -EINVAL; >> - } >> - ab = ar->ab; >> + lockdep_assert_held(&ar->conf_mutex); >> >> - if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ar->ab->dev_flags)) >> + if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ab->dev_flags)) >> return 1; >> >> - if (key->keyidx > WMI_MAX_KEY_INDEX) >> - return -ENOSPC; >> - >> - mutex_lock(&ar->conf_mutex); >> - >> if (sta) >> peer_addr = sta->addr; >> else if (arvif->vdev_type == WMI_VDEV_TYPE_STA) >> @@ -3643,6 +3627,43 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, >> spin_unlock_bh(&ab->base_lock); >> >> exit: >> + return ret; >> +} >> + >> + >> +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_vif *arvif = ath12k_vif_to_arvif(vif); >> + struct ath12k *ar; >> + 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; >> + >> + if (key->keyidx > WMI_MAX_KEY_INDEX) >> + return -ENOSPC; >> + >> + ar = ath12k_get_ar_by_vif(hw, vif); >> + if (!ar) { >> + /* ar is expected to be valid when sta ptr is available */ >> + if (sta) { >> + WARN_ON_ONCE(1); >> + return -EINVAL; >> + } >> + arvif->cache.key_conf.cmd = cmd; >> + arvif->cache.key_conf.key = key; >> + arvif->cache.key_conf.changed = true; >> + return 0; >> + } >> + >> + mutex_lock(&ar->conf_mutex); >> + ret = ath12k_mac_set_key(ar, cmd, vif, sta, key); >> mutex_unlock(&ar->conf_mutex); >> return ret; >> } >> @@ -4477,7 +4498,10 @@ static int ath12k_mac_op_conf_tx(struct ieee80211_hw *hw, >> >> ar = ath12k_get_ar_by_vif(hw, vif); >> if (!ar) { >> - /* TODO cache the info and apply after vdev is created */ >> + /* cache the info and apply after vdev is created */ >> + arvif->cache.tx_conf.changed = true; >> + arvif->cache.tx_conf.ac = ac; >> + arvif->cache.tx_conf.tx_queue_params = *params; >> return -EINVAL; >> } >> >> @@ -6121,6 +6145,41 @@ static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif) >> return ret; >> } >> >> +static void ath12k_mac_vif_cache_flush(struct ath12k *ar, struct ieee80211_vif *vif) >> +{ >> + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif); >> + struct ath12k_base *ab = ar->ab; >> + int ret; >> + >> + lockdep_assert_held(&ar->conf_mutex); >> + >> + if (arvif->cache.tx_conf.changed) { >> + ret = ath12k_mac_conf_tx(arvif, 0, arvif->cache.tx_conf.ac, >> + &arvif->cache.tx_conf.tx_queue_params); >> + if (ret) >> + ath12k_warn(ab, >> + "unable to apply tx config parameters to vdev %d\n", >> + ret); >> + } >> + >> + if (arvif->cache.bss_conf_changed) >> + ath12k_mac_bss_info_changed(ar, arvif, &vif->bss_conf, >> + arvif->cache.bss_conf_changed); >> + >> + if (arvif->cache.key_conf.changed) { >> + ret = ath12k_mac_set_key(ar, arvif->cache.key_conf.cmd, >> + vif, NULL, >> + arvif->cache.key_conf.key); >> + if (ret) { >> + WARN_ON_ONCE(1); >> + ath12k_warn(ab, "unable to apply set key param to vdev %d ret %d\n", >> + arvif->vdev_id, ret); >> + } >> + } >> + >> + memset(&arvif->cache, 0, sizeof(struct ath12k_vif_cache)); > > sizeof(arvif->cache) > >> +} >> + >> static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, >> struct ieee80211_vif *vif, >> struct ieee80211_chanctx_conf *ctx) >> @@ -6203,10 +6262,11 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw, >> goto unlock; >> } >> >> - /* TODO If the vdev is created during channel assign and not during >> + /* If the vdev is created during channel assign and not during >> * add_interface(), Apply any parameters for the vdev which were received >> * after add_interface, corresponding to this vif. >> */ >> + ath12k_mac_vif_cache_flush(ar, vif); >> unlock: >> mutex_unlock(&ar->conf_mutex); >> out: >
From: Sriram R <quic_srirrama@quicinc.com> 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 bring up and operation. Note: Monitor and hw reconfig support for Single Wiphy will be done in future patchsets. --- v4: - Updated missing Signed-off details for patches. v3: - Rebased on ToT (added additional ar check in PATCH 08/12 for p2p) - Introduced iterator to loop through ars in an ah(for_each_ar()) - Addressed comments on reverse xmas tree declaration style. v2: - Rebased on ToT and dependent patchset 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 | 38 +- drivers/net/wireless/ath/ath12k/hw.h | 1 + drivers/net/wireless/ath/ath12k/mac.c | 911 +++++++++++++++++++------ drivers/net/wireless/ath/ath12k/p2p.c | 3 +- drivers/net/wireless/ath/ath12k/p2p.h | 1 + drivers/net/wireless/ath/ath12k/reg.c | 55 +- 6 files changed, 785 insertions(+), 224 deletions(-)