* [PATCH 1/8] wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
@ 2024-10-23 13:29 ` Kalle Valo
2024-10-23 15:01 ` Jeff Johnson
2024-10-23 13:29 ` [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes Kalle Valo
` (6 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:29 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Kalle Valo <quic_kvalo@quicinc.com>
In commit 477cabfdb776 ("wifi: ath12k: modify link arvif creation and removal
for MLO") I had accidentally left one personal TODO comment about using goto
instead of ret. Switch to use goto to be consistent with the error handling in
the function.
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
drivers/net/wireless/ath/ath12k/mac.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index f5f96a8b1d61..f45f32f3b5f6 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id,
arvif->bssid);
if (ret)
- /* KVALO: why not goto err? */
- return ret;
+ goto err;
ar->num_peers--;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 1/8] wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling
2024-10-23 13:29 ` [PATCH 1/8] wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling Kalle Valo
@ 2024-10-23 15:01 ` Jeff Johnson
2024-10-24 17:21 ` Kalle Valo
0 siblings, 1 reply; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 15:01 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:29 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
>
> In commit 477cabfdb776 ("wifi: ath12k: modify link arvif creation and removal
> for MLO") I had accidentally left one personal TODO comment about using goto
> instead of ret. Switch to use goto to be consistent with the error handling in
> the function.
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/mac.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index f5f96a8b1d61..f45f32f3b5f6 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
> ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id,
> arvif->bssid);
> if (ret)
> - /* KVALO: why not goto err? */
> - return ret;
> + goto err;
why does this goto err instead of err_vdev_del?
>
> ar->num_peers--;
> }
looking at the context for this patch I have a question about a different part
of this function:
param_id = WMI_VDEV_PARAM_RTS_THRESHOLD;
param_value = hw->wiphy->rts_threshold;
ret = ath12k_wmi_vdev_set_param_cmd(ar, arvif->vdev_id,
param_id, param_value);
if (ret) {
ath12k_warn(ar->ab, "failed to set rts threshold for vdev %d: %d\n",
arvif->vdev_id, ret);
NOTE: no return or goto
}
ath12k_dp_vdev_tx_attach(ar, arvif);
if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled)
ath12k_mac_monitor_vdev_create(ar);
return ret;
NOTE: this can return an error if the RTS threshold set fails, but fails
without cleaning up (dp vdev still attached and monitor vdev created)
Seems either we need error handling if the set param fails, or we should ret 0
at this point
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/8] wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling
2024-10-23 15:01 ` Jeff Johnson
@ 2024-10-24 17:21 ` Kalle Valo
0 siblings, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-24 17:21 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:29 AM, Kalle Valo wrote:
>> From: Kalle Valo <quic_kvalo@quicinc.com>
>>
>> In commit 477cabfdb776 ("wifi: ath12k: modify link arvif creation and removal
>> for MLO") I had accidentally left one personal TODO comment about using goto
>> instead of ret. Switch to use goto to be consistent with the error handling in
>> the function.
>>
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
>> drivers/net/wireless/ath/ath12k/mac.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index f5f96a8b1d61..f45f32f3b5f6 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -7047,8 +7047,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
>> ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id,
>> arvif->bssid);
>> if (ret)
>> - /* KVALO: why not goto err? */
>> - return ret;
>> + goto err;
>
> why does this goto err instead of err_vdev_del?
Good point. I did this to follow the same as the next command does:
ret = ath12k_wait_for_peer_delete_done(ar, arvif->vdev_id,
arvif->bssid);
if (ret)
goto err;
But yeah, err_vdev_del looks like more approriate.
>
>>
>> ar->num_peers--;
>> }
>
> looking at the context for this patch I have a question about a different part
> of this function:
>
> param_id = WMI_VDEV_PARAM_RTS_THRESHOLD;
> param_value = hw->wiphy->rts_threshold;
> ret = ath12k_wmi_vdev_set_param_cmd(ar, arvif->vdev_id,
> param_id, param_value);
> if (ret) {
> ath12k_warn(ar->ab, "failed to set rts threshold for vdev %d: %d\n",
> arvif->vdev_id, ret);
>
> NOTE: no return or goto
>
> }
>
> ath12k_dp_vdev_tx_attach(ar, arvif);
> if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled)
> ath12k_mac_monitor_vdev_create(ar);
>
> return ret;
>
> NOTE: this can return an error if the RTS threshold set fails, but fails
> without cleaning up (dp vdev still attached and monitor vdev created)
>
> Seems either we need error handling if the set param fails, or we should ret 0
> at this point
Yeah, I do not like this kind of vague error handling at all. I think we
should have either a proper error handling or a comment explaining why
we continue to the execution. An example about the comment:
ret = foo();
if (ret)
ath12k_warn("foo failed: %d", ret);
/* continue function because foo is optional */
I think this should all this should be cleaned up in a separate patch.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
2024-10-23 13:29 ` [PATCH 1/8] wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling Kalle Valo
@ 2024-10-23 13:29 ` Kalle Valo
2024-10-23 15:19 ` Jeff Johnson
2024-10-23 13:29 ` [PATCH 3/8] wifi: ath12k: Refactor sta state machine Kalle Valo
` (5 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:29 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Sriram R <quic_srirrama@quicinc.com>
Add changes to add the link vdevs dynamically whenever a channel is assigned
from mac80211 for a link vdev. During vdev create, update ML address of the
vdev to firmware using the new WMI parameter (WMI_TAG_MLO_VDEV_CREATE_PARAMS).
During vdev start, notify the firmware that this link vdev is newly added and
also indicate all its known partners so that the firmware can take necessary
actions to internally update the partners on the new link being added.
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>
Co-developed-by: Rameshkumar Sundaram <quic_ramess@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/mac.c | 90 ++++++++++++++++++++++++++-
drivers/net/wireless/ath/ath12k/wmi.c | 85 ++++++++++++++++++++++++-
drivers/net/wireless/ath/ath12k/wmi.h | 74 ++++++++++++++++++++++
3 files changed, 244 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index f45f32f3b5f6..d4aa4540c8e6 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -648,6 +648,18 @@ struct ath12k *ath12k_mac_get_ar_by_pdev_id(struct ath12k_base *ab, u32 pdev_id)
return NULL;
}
+static bool ath12k_mac_is_ml_arvif(struct ath12k_link_vif *arvif)
+{
+ struct ath12k_vif *ahvif = arvif->ahvif;
+
+ lockdep_assert_wiphy(ahvif->ah->hw->wiphy);
+
+ if (ahvif->vif->valid_links & BIT(arvif->link_id))
+ return true;
+
+ return false;
+}
+
static struct ath12k *ath12k_mac_get_ar_by_chan(struct ieee80211_hw *hw,
struct ieee80211_channel *channel)
{
@@ -1512,7 +1524,8 @@ static int ath12k_mac_setup_bcn_tmpl_ema(struct ath12k_link_vif *arvif)
tx_ahvif = ath12k_vif_to_ahvif(ahvif->vif->mbssid_tx_vif);
tx_arvif = &tx_ahvif->deflink;
beacons = ieee80211_beacon_get_template_ema_list(ath12k_ar_to_hw(tx_arvif->ar),
- tx_ahvif->vif, 0);
+ tx_ahvif->vif,
+ tx_arvif->link_id);
if (!beacons || !beacons->cnt) {
ath12k_warn(arvif->ar->ab,
"failed to get ema beacon templates from mac80211\n");
@@ -1577,7 +1590,7 @@ static int ath12k_mac_setup_bcn_tmpl(struct ath12k_link_vif *arvif)
}
bcn = ieee80211_beacon_get_template(ath12k_ar_to_hw(tx_arvif->ar), tx_ahvif->vif,
- &offs, 0);
+ &offs, tx_arvif->link_id);
if (!bcn) {
ath12k_warn(ab, "failed to get beacon template from mac80211\n");
return -EPERM;
@@ -1658,7 +1671,7 @@ static void ath12k_control_beaconing(struct ath12k_link_vif *arvif,
ahvif->aid = 0;
- ether_addr_copy(arvif->bssid, info->bssid);
+ ether_addr_copy(arvif->bssid, info->addr);
params.vdev_id = arvif->vdev_id;
params.aid = ahvif->aid;
@@ -6671,6 +6684,8 @@ static int ath12k_mac_setup_vdev_create_arg(struct ath12k_link_vif *arvif,
struct ath12k_vif *ahvif = arvif->ahvif;
int ret;
+ lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
arg->if_id = arvif->vdev_id;
arg->type = ahvif->vdev_type;
arg->subtype = ahvif->vdev_subtype;
@@ -6702,6 +6717,17 @@ static int ath12k_mac_setup_vdev_create_arg(struct ath12k_link_vif *arvif,
}
arg->if_stats_id = ath12k_mac_get_vdev_stats_id(arvif);
+
+ if (ath12k_mac_is_ml_arvif(arvif)) {
+ if (hweight16(ahvif->vif->valid_links) > ATH12K_WMI_MLO_MAX_LINKS) {
+ ath12k_warn(ar->ab, "too many MLO links during setting up vdev: %d",
+ ahvif->vif->valid_links);
+ return -EINVAL;
+ }
+
+ ether_addr_copy(arg->mld_addr, ahvif->vif->addr);
+ }
+
return 0;
}
@@ -7639,6 +7665,61 @@ ath12k_mac_check_down_grade_phy_mode(struct ath12k *ar,
return down_mode;
}
+static void
+ath12k_mac_mlo_get_vdev_args(struct ath12k_link_vif *arvif,
+ struct wmi_ml_arg *ml_arg)
+{
+ struct ath12k_vif *ahvif = arvif->ahvif;
+ struct wmi_ml_partner_info *partner_info;
+ struct ieee80211_bss_conf *link_conf;
+ struct ath12k_link_vif *arvif_p;
+ unsigned long links;
+ u8 link_id;
+
+ lockdep_assert_wiphy(ahvif->ah->hw->wiphy);
+
+ if (!ath12k_mac_is_ml_arvif(arvif))
+ return;
+
+ if (hweight16(ahvif->vif->valid_links) > ATH12K_WMI_MLO_MAX_LINKS)
+ return;
+
+ rcu_read_lock();
+
+ ml_arg->enabled = true;
+
+ /* Driver always add a new link via VDEV START, FW takes
+ * care of internally adding this link to existing
+ * link vdevs which are advertised as partners below
+ */
+ ml_arg->link_add = true;
+ partner_info = ml_arg->partner_info;
+
+ links = ahvif->links_map;
+ for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
+ arvif_p = rcu_dereference(ahvif->link[link_id]);
+
+ if (WARN_ON(!arvif_p))
+ continue;
+
+ if (arvif == arvif_p)
+ continue;
+
+ link_conf = rcu_dereference(ahvif->vif->link_conf[arvif_p->link_id]);
+
+ if (!link_conf)
+ continue;
+
+ partner_info->vdev_id = arvif_p->vdev_id;
+ partner_info->hw_link_id = arvif_p->ar->pdev->hw_link_id;
+ ether_addr_copy(partner_info->addr, link_conf->addr);
+ ml_arg->num_partner_links++;
+ partner_info++;
+ }
+
+ rcu_read_unlock();
+}
+
static int
ath12k_mac_vdev_start_restart(struct ath12k_link_vif *arvif,
struct ieee80211_chanctx_conf *ctx,
@@ -7717,6 +7798,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_link_vif *arvif,
arg.passive |= !!(chandef->chan->flags & IEEE80211_CHAN_NO_IR);
+ if (!restart)
+ ath12k_mac_mlo_get_vdev_args(arvif, &arg.ml);
+
ath12k_dbg(ab, ATH12K_DBG_MAC,
"mac vdev %d start center_freq %d phymode %s punct_bitmap 0x%x\n",
arg.vdev_id, arg.freq,
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index dced2aa9ba1a..e089b58bbea1 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -821,6 +821,8 @@ int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
struct wmi_vdev_create_cmd *cmd;
struct sk_buff *skb;
struct ath12k_wmi_vdev_txrx_streams_params *txrx_streams;
+ bool is_ml_vdev = is_valid_ether_addr(args->mld_addr);
+ struct wmi_vdev_create_mlo_params *ml_params;
struct wmi_tlv *tlv;
int ret, len;
void *ptr;
@@ -830,7 +832,8 @@ int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
* both the bands.
*/
len = sizeof(*cmd) + TLV_HDR_SIZE +
- (WMI_NUM_SUPPORTED_BAND_MAX * sizeof(*txrx_streams));
+ (WMI_NUM_SUPPORTED_BAND_MAX * sizeof(*txrx_streams)) +
+ (is_ml_vdev ? TLV_HDR_SIZE + sizeof(*ml_params) : 0);
skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
if (!skb)
@@ -879,6 +882,21 @@ int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
txrx_streams->supported_rx_streams =
cpu_to_le32(args->chains[NL80211_BAND_5GHZ].rx);
+ ptr += WMI_NUM_SUPPORTED_BAND_MAX * sizeof(*txrx_streams);
+
+ if (is_ml_vdev) {
+ tlv = ptr;
+ tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
+ sizeof(*ml_params));
+ ptr += TLV_HDR_SIZE;
+ ml_params = ptr;
+
+ ml_params->tlv_header =
+ ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_VDEV_CREATE_PARAMS,
+ sizeof(*ml_params));
+ ether_addr_copy(ml_params->mld_macaddr.addr, args->mld_addr);
+ }
+
ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
"WMI vdev create: id %d type %d subtype %d macaddr %pM pdevid %d\n",
args->if_id, args->type, args->subtype,
@@ -1020,19 +1038,27 @@ static void ath12k_wmi_put_wmi_channel(struct ath12k_wmi_channel_params *chan,
int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg,
bool restart)
{
+ struct wmi_vdev_start_mlo_params *ml_params;
+ struct wmi_partner_link_info *partner_info;
struct ath12k_wmi_pdev *wmi = ar->wmi;
struct wmi_vdev_start_request_cmd *cmd;
struct sk_buff *skb;
struct ath12k_wmi_channel_params *chan;
struct wmi_tlv *tlv;
void *ptr;
- int ret, len;
+ int ret, len, i, ml_arg_size = 0;
if (WARN_ON(arg->ssid_len > sizeof(cmd->ssid.ssid)))
return -EINVAL;
len = sizeof(*cmd) + sizeof(*chan) + TLV_HDR_SIZE;
+ if (!restart && arg->ml.enabled) {
+ ml_arg_size = TLV_HDR_SIZE + sizeof(*ml_params) +
+ TLV_HDR_SIZE + (arg->ml.num_partner_links *
+ sizeof(*partner_info));
+ len += ml_arg_size;
+ }
skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
if (!skb)
return -ENOMEM;
@@ -1085,6 +1111,61 @@ int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg,
ptr += sizeof(*tlv);
+ if (ml_arg_size) {
+ tlv = ptr;
+ tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
+ sizeof(*ml_params));
+ ptr += TLV_HDR_SIZE;
+
+ ml_params = ptr;
+
+ ml_params->tlv_header =
+ ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_VDEV_START_PARAMS,
+ sizeof(*ml_params));
+
+ ml_params->flags = le32_encode_bits(arg->ml.enabled,
+ ATH12K_WMI_FLAG_MLO_ENABLED) |
+ le32_encode_bits(arg->ml.assoc_link,
+ ATH12K_WMI_FLAG_MLO_ASSOC_LINK) |
+ le32_encode_bits(arg->ml.mcast_link,
+ ATH12K_WMI_FLAG_MLO_MCAST_VDEV) |
+ le32_encode_bits(arg->ml.link_add,
+ ATH12K_WMI_FLAG_MLO_LINK_ADD);
+
+ ath12k_dbg(ar->ab, ATH12K_DBG_WMI, "vdev %d start ml flags 0x%x\n",
+ arg->vdev_id, ml_params->flags);
+
+ ptr += sizeof(*ml_params);
+
+ tlv = ptr;
+ tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
+ arg->ml.num_partner_links *
+ sizeof(*partner_info));
+ ptr += TLV_HDR_SIZE;
+
+ partner_info = ptr;
+
+ for (i = 0; i < arg->ml.num_partner_links; i++) {
+ partner_info->tlv_header =
+ ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PARTNER_LINK_PARAMS,
+ sizeof(*partner_info));
+ partner_info->vdev_id =
+ cpu_to_le32(arg->ml.partner_info[i].vdev_id);
+ partner_info->hw_link_id =
+ cpu_to_le32(arg->ml.partner_info[i].hw_link_id);
+ ether_addr_copy(partner_info->vdev_addr.addr,
+ arg->ml.partner_info[i].addr);
+
+ ath12k_dbg(ar->ab, ATH12K_DBG_WMI, "partner vdev %d hw_link_id %d macaddr%pM\n",
+ partner_info->vdev_id, partner_info->hw_link_id,
+ partner_info->vdev_addr.addr);
+
+ partner_info++;
+ }
+
+ ptr = partner_info;
+ }
+
ath12k_dbg(ar->ab, ATH12K_DBG_WMI, "vdev %s id 0x%x freq 0x%x mode 0x%x\n",
restart ? "restart" : "start", arg->vdev_id,
arg->freq, arg->mode);
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index 6f55dbdf629d..33b9643644c6 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -1929,6 +1929,19 @@ enum wmi_tlv_tag {
WMI_TAG_REGULATORY_RULE_EXT_STRUCT = 0x3A9,
WMI_TAG_REG_CHAN_LIST_CC_EXT_EVENT,
WMI_TAG_EHT_RATE_SET = 0x3C4,
+ WMI_TAG_DCS_AWGN_INT_TYPE = 0x3C5,
+ WMI_TAG_MLO_TX_SEND_PARAMS,
+ WMI_TAG_MLO_PARTNER_LINK_PARAMS,
+ WMI_TAG_MLO_PARTNER_LINK_PARAMS_PEER_ASSOC,
+ WMI_TAG_MLO_SETUP_CMD = 0x3C9,
+ WMI_TAG_MLO_SETUP_COMPLETE_EVENT,
+ WMI_TAG_MLO_READY_CMD,
+ WMI_TAG_MLO_TEARDOWN_CMD,
+ WMI_TAG_MLO_TEARDOWN_COMPLETE,
+ WMI_TAG_MLO_PEER_ASSOC_PARAMS = 0x3D0,
+ WMI_TAG_MLO_PEER_CREATE_PARAMS = 0x3D5,
+ WMI_TAG_MLO_VDEV_START_PARAMS = 0x3D6,
+ WMI_TAG_MLO_VDEV_CREATE_PARAMS = 0x3D7,
WMI_TAG_PDEV_SET_BIOS_SAR_TABLE_CMD = 0x3D8,
WMI_TAG_PDEV_SET_BIOS_GEO_TABLE_CMD = 0x3D9,
WMI_TAG_PDEV_SET_BIOS_INTERFACE_CMD = 0x3FB,
@@ -2740,6 +2753,7 @@ struct ath12k_wmi_vdev_create_arg {
u8 if_stats_id;
u32 mbssid_flags;
u32 mbssid_tx_vdev_id;
+ u8 mld_addr[ETH_ALEN];
};
#define ATH12K_MAX_VDEV_STATS_ID 0x30
@@ -2766,6 +2780,44 @@ struct ath12k_wmi_vdev_txrx_streams_params {
__le32 supported_rx_streams;
} __packed;
+/* 2 word representation of MAC addr */
+struct wmi_mac_addr {
+ union {
+ u8 addr[6];
+ struct {
+ u32 word0;
+ u32 word1;
+ } __packed;
+ } __packed;
+} __packed;
+
+struct wmi_vdev_create_mlo_params {
+ __le32 tlv_header;
+ struct wmi_mac_addr mld_macaddr;
+} __packed;
+
+#define ATH12K_WMI_FLAG_MLO_ENABLED BIT(0)
+#define ATH12K_WMI_FLAG_MLO_ASSOC_LINK BIT(1)
+#define ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC BIT(2)
+#define ATH12K_WMI_FLAG_MLO_LOGICAL_LINK_IDX_VALID BIT(3)
+#define ATH12K_WMI_FLAG_MLO_PEER_ID_VALID BIT(4)
+#define ATH12K_WMI_FLAG_MLO_MCAST_VDEV BIT(5)
+#define ATH12K_WMI_FLAG_MLO_EMLSR_SUPPORT BIT(6)
+#define ATH12K_WMI_FLAG_MLO_FORCED_INACTIVE BIT(7)
+#define ATH12K_WMI_FLAG_MLO_LINK_ADD BIT(8)
+
+struct wmi_vdev_start_mlo_params {
+ __le32 tlv_header;
+ __le32 flags;
+} __packed;
+
+struct wmi_partner_link_info {
+ __le32 tlv_header;
+ __le32 vdev_id;
+ __le32 hw_link_id;
+ struct wmi_mac_addr vdev_addr;
+} __packed;
+
struct wmi_vdev_delete_cmd {
__le32 tlv_header;
__le32 vdev_id;
@@ -2909,6 +2961,27 @@ enum wmi_phy_mode {
MODE_MAX = 33,
};
+#define ATH12K_WMI_MLO_MAX_LINKS 4
+
+struct wmi_ml_partner_info {
+ u32 vdev_id;
+ u32 hw_link_id;
+ u8 addr[ETH_ALEN];
+ bool assoc_link;
+ bool primary_umac;
+ bool logical_link_idx_valid;
+ u32 logical_link_idx;
+};
+
+struct wmi_ml_arg {
+ bool enabled;
+ bool assoc_link;
+ bool mcast_link;
+ bool link_add;
+ u8 num_partner_links;
+ struct wmi_ml_partner_info partner_info[ATH12K_WMI_MLO_MAX_LINKS];
+};
+
struct wmi_vdev_start_req_arg {
u32 vdev_id;
u32 freq;
@@ -2946,6 +3019,7 @@ struct wmi_vdev_start_req_arg {
u32 mbssid_flags;
u32 mbssid_tx_vdev_id;
u32 punct_bitmap;
+ struct wmi_ml_arg ml;
};
struct ath12k_wmi_peer_create_arg {
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes
2024-10-23 13:29 ` [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes Kalle Valo
@ 2024-10-23 15:19 ` Jeff Johnson
2024-10-24 18:10 ` Kalle Valo
0 siblings, 1 reply; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 15:19 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:29 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
>
> Add changes to add the link vdevs dynamically whenever a channel is assigned
> from mac80211 for a link vdev. During vdev create, update ML address of the
> vdev to firmware using the new WMI parameter (WMI_TAG_MLO_VDEV_CREATE_PARAMS).
>
> During vdev start, notify the firmware that this link vdev is newly added and
> also indicate all its known partners so that the firmware can take necessary
> actions to internally update the partners on the new link being added.
>
> 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>
> Co-developed-by: Rameshkumar Sundaram <quic_ramess@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/mac.c | 90 ++++++++++++++++++++++++++-
> drivers/net/wireless/ath/ath12k/wmi.c | 85 ++++++++++++++++++++++++-
> drivers/net/wireless/ath/ath12k/wmi.h | 74 ++++++++++++++++++++++
> 3 files changed, 244 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index f45f32f3b5f6..d4aa4540c8e6 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -648,6 +648,18 @@ struct ath12k *ath12k_mac_get_ar_by_pdev_id(struct ath12k_base *ab, u32 pdev_id)
> return NULL;
> }
>
> +static bool ath12k_mac_is_ml_arvif(struct ath12k_link_vif *arvif)
> +{
> + struct ath12k_vif *ahvif = arvif->ahvif;
> +
> + lockdep_assert_wiphy(ahvif->ah->hw->wiphy);
should we have helper functions ath12k_<foo>_to_wiphy() to abstract out the
underlying linkages?
> +
> + if (ahvif->vif->valid_links & BIT(arvif->link_id))
> + return true;
> +
> + return false;
> +}
> +
> static struct ath12k *ath12k_mac_get_ar_by_chan(struct ieee80211_hw *hw,
> struct ieee80211_channel *channel)
> {
> @@ -1512,7 +1524,8 @@ static int ath12k_mac_setup_bcn_tmpl_ema(struct ath12k_link_vif *arvif)
> tx_ahvif = ath12k_vif_to_ahvif(ahvif->vif->mbssid_tx_vif);
> tx_arvif = &tx_ahvif->deflink;
> beacons = ieee80211_beacon_get_template_ema_list(ath12k_ar_to_hw(tx_arvif->ar),
> - tx_ahvif->vif, 0);
> + tx_ahvif->vif,
> + tx_arvif->link_id);
> if (!beacons || !beacons->cnt) {
> ath12k_warn(arvif->ar->ab,
> "failed to get ema beacon templates from mac80211\n");
> @@ -1577,7 +1590,7 @@ static int ath12k_mac_setup_bcn_tmpl(struct ath12k_link_vif *arvif)
> }
>
> bcn = ieee80211_beacon_get_template(ath12k_ar_to_hw(tx_arvif->ar), tx_ahvif->vif,
> - &offs, 0);
> + &offs, tx_arvif->link_id);
> if (!bcn) {
> ath12k_warn(ab, "failed to get beacon template from mac80211\n");
> return -EPERM;
> @@ -1658,7 +1671,7 @@ static void ath12k_control_beaconing(struct ath12k_link_vif *arvif,
>
> ahvif->aid = 0;
>
> - ether_addr_copy(arvif->bssid, info->bssid);
> + ether_addr_copy(arvif->bssid, info->addr);
>
> params.vdev_id = arvif->vdev_id;
> params.aid = ahvif->aid;
> @@ -6671,6 +6684,8 @@ static int ath12k_mac_setup_vdev_create_arg(struct ath12k_link_vif *arvif,
> struct ath12k_vif *ahvif = arvif->ahvif;
> int ret;
>
> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
> arg->if_id = arvif->vdev_id;
> arg->type = ahvif->vdev_type;
> arg->subtype = ahvif->vdev_subtype;
> @@ -6702,6 +6717,17 @@ static int ath12k_mac_setup_vdev_create_arg(struct ath12k_link_vif *arvif,
> }
>
> arg->if_stats_id = ath12k_mac_get_vdev_stats_id(arvif);
> +
> + if (ath12k_mac_is_ml_arvif(arvif)) {
> + if (hweight16(ahvif->vif->valid_links) > ATH12K_WMI_MLO_MAX_LINKS) {
> + ath12k_warn(ar->ab, "too many MLO links during setting up vdev: %d",
> + ahvif->vif->valid_links);
> + return -EINVAL;
> + }
> +
> + ether_addr_copy(arg->mld_addr, ahvif->vif->addr);
> + }
> +
> return 0;
> }
>
> @@ -7639,6 +7665,61 @@ ath12k_mac_check_down_grade_phy_mode(struct ath12k *ar,
> return down_mode;
> }
>
> +static void
> +ath12k_mac_mlo_get_vdev_args(struct ath12k_link_vif *arvif,
> + struct wmi_ml_arg *ml_arg)
> +{
> + struct ath12k_vif *ahvif = arvif->ahvif;
> + struct wmi_ml_partner_info *partner_info;
> + struct ieee80211_bss_conf *link_conf;
> + struct ath12k_link_vif *arvif_p;
> + unsigned long links;
> + u8 link_id;
> +
> + lockdep_assert_wiphy(ahvif->ah->hw->wiphy);
> +
> + if (!ath12k_mac_is_ml_arvif(arvif))
> + return;
> +
> + if (hweight16(ahvif->vif->valid_links) > ATH12K_WMI_MLO_MAX_LINKS)
> + return;
> +
> + rcu_read_lock();
what is this protecting?
do all of the statements between here and the for loop really need RCU protection?
> +
> + ml_arg->enabled = true;
> +
> + /* Driver always add a new link via VDEV START, FW takes
> + * care of internally adding this link to existing
> + * link vdevs which are advertised as partners below
> + */
> + ml_arg->link_add = true;
> + partner_info = ml_arg->partner_info;
> +
> + links = ahvif->links_map;
> + for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
> + arvif_p = rcu_dereference(ahvif->link[link_id]);
> +
> + if (WARN_ON(!arvif_p))
> + continue;
> +
> + if (arvif == arvif_p)
> + continue;
> +
> + link_conf = rcu_dereference(ahvif->vif->link_conf[arvif_p->link_id]);
> +
> + if (!link_conf)
> + continue;
> +
> + partner_info->vdev_id = arvif_p->vdev_id;
> + partner_info->hw_link_id = arvif_p->ar->pdev->hw_link_id;
> + ether_addr_copy(partner_info->addr, link_conf->addr);
> + ml_arg->num_partner_links++;
> + partner_info++;
> + }
> +
> + rcu_read_unlock();
> +}
> +
> static int
> ath12k_mac_vdev_start_restart(struct ath12k_link_vif *arvif,
> struct ieee80211_chanctx_conf *ctx,
> @@ -7717,6 +7798,9 @@ ath12k_mac_vdev_start_restart(struct ath12k_link_vif *arvif,
>
> arg.passive |= !!(chandef->chan->flags & IEEE80211_CHAN_NO_IR);
>
> + if (!restart)
> + ath12k_mac_mlo_get_vdev_args(arvif, &arg.ml);
> +
> ath12k_dbg(ab, ATH12K_DBG_MAC,
> "mac vdev %d start center_freq %d phymode %s punct_bitmap 0x%x\n",
> arg.vdev_id, arg.freq,
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index dced2aa9ba1a..e089b58bbea1 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -821,6 +821,8 @@ int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
> struct wmi_vdev_create_cmd *cmd;
> struct sk_buff *skb;
> struct ath12k_wmi_vdev_txrx_streams_params *txrx_streams;
> + bool is_ml_vdev = is_valid_ether_addr(args->mld_addr);
> + struct wmi_vdev_create_mlo_params *ml_params;
> struct wmi_tlv *tlv;
> int ret, len;
> void *ptr;
> @@ -830,7 +832,8 @@ int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
> * both the bands.
> */
> len = sizeof(*cmd) + TLV_HDR_SIZE +
> - (WMI_NUM_SUPPORTED_BAND_MAX * sizeof(*txrx_streams));
> + (WMI_NUM_SUPPORTED_BAND_MAX * sizeof(*txrx_streams)) +
> + (is_ml_vdev ? TLV_HDR_SIZE + sizeof(*ml_params) : 0);
>
> skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
> if (!skb)
> @@ -879,6 +882,21 @@ int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
> txrx_streams->supported_rx_streams =
> cpu_to_le32(args->chains[NL80211_BAND_5GHZ].rx);
>
> + ptr += WMI_NUM_SUPPORTED_BAND_MAX * sizeof(*txrx_streams);
> +
> + if (is_ml_vdev) {
> + tlv = ptr;
> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
> + sizeof(*ml_params));
> + ptr += TLV_HDR_SIZE;
> + ml_params = ptr;
> +
> + ml_params->tlv_header =
> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_VDEV_CREATE_PARAMS,
> + sizeof(*ml_params));
> + ether_addr_copy(ml_params->mld_macaddr.addr, args->mld_addr);
> + }
> +
> ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
> "WMI vdev create: id %d type %d subtype %d macaddr %pM pdevid %d\n",
> args->if_id, args->type, args->subtype,
> @@ -1020,19 +1038,27 @@ static void ath12k_wmi_put_wmi_channel(struct ath12k_wmi_channel_params *chan,
> int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg,
> bool restart)
> {
> + struct wmi_vdev_start_mlo_params *ml_params;
> + struct wmi_partner_link_info *partner_info;
> struct ath12k_wmi_pdev *wmi = ar->wmi;
> struct wmi_vdev_start_request_cmd *cmd;
> struct sk_buff *skb;
> struct ath12k_wmi_channel_params *chan;
> struct wmi_tlv *tlv;
> void *ptr;
> - int ret, len;
> + int ret, len, i, ml_arg_size = 0;
>
> if (WARN_ON(arg->ssid_len > sizeof(cmd->ssid.ssid)))
> return -EINVAL;
>
> len = sizeof(*cmd) + sizeof(*chan) + TLV_HDR_SIZE;
>
> + if (!restart && arg->ml.enabled) {
> + ml_arg_size = TLV_HDR_SIZE + sizeof(*ml_params) +
> + TLV_HDR_SIZE + (arg->ml.num_partner_links *
> + sizeof(*partner_info));
> + len += ml_arg_size;
> + }
> skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
> if (!skb)
> return -ENOMEM;
> @@ -1085,6 +1111,61 @@ int ath12k_wmi_vdev_start(struct ath12k *ar, struct wmi_vdev_start_req_arg *arg,
>
> ptr += sizeof(*tlv);
>
> + if (ml_arg_size) {
> + tlv = ptr;
> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
> + sizeof(*ml_params));
> + ptr += TLV_HDR_SIZE;
> +
> + ml_params = ptr;
> +
> + ml_params->tlv_header =
> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_VDEV_START_PARAMS,
> + sizeof(*ml_params));
> +
> + ml_params->flags = le32_encode_bits(arg->ml.enabled,
> + ATH12K_WMI_FLAG_MLO_ENABLED) |
> + le32_encode_bits(arg->ml.assoc_link,
> + ATH12K_WMI_FLAG_MLO_ASSOC_LINK) |
> + le32_encode_bits(arg->ml.mcast_link,
> + ATH12K_WMI_FLAG_MLO_MCAST_VDEV) |
> + le32_encode_bits(arg->ml.link_add,
> + ATH12K_WMI_FLAG_MLO_LINK_ADD);
> +
> + ath12k_dbg(ar->ab, ATH12K_DBG_WMI, "vdev %d start ml flags 0x%x\n",
> + arg->vdev_id, ml_params->flags);
> +
> + ptr += sizeof(*ml_params);
> +
> + tlv = ptr;
> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
> + arg->ml.num_partner_links *
> + sizeof(*partner_info));
> + ptr += TLV_HDR_SIZE;
> +
> + partner_info = ptr;
> +
> + for (i = 0; i < arg->ml.num_partner_links; i++) {
> + partner_info->tlv_header =
> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PARTNER_LINK_PARAMS,
> + sizeof(*partner_info));
> + partner_info->vdev_id =
> + cpu_to_le32(arg->ml.partner_info[i].vdev_id);
> + partner_info->hw_link_id =
> + cpu_to_le32(arg->ml.partner_info[i].hw_link_id);
> + ether_addr_copy(partner_info->vdev_addr.addr,
> + arg->ml.partner_info[i].addr);
> +
> + ath12k_dbg(ar->ab, ATH12K_DBG_WMI, "partner vdev %d hw_link_id %d macaddr%pM\n",
> + partner_info->vdev_id, partner_info->hw_link_id,
> + partner_info->vdev_addr.addr);
> +
> + partner_info++;
> + }
> +
> + ptr = partner_info;
> + }
> +
> ath12k_dbg(ar->ab, ATH12K_DBG_WMI, "vdev %s id 0x%x freq 0x%x mode 0x%x\n",
> restart ? "restart" : "start", arg->vdev_id,
> arg->freq, arg->mode);
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
> index 6f55dbdf629d..33b9643644c6 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.h
> +++ b/drivers/net/wireless/ath/ath12k/wmi.h
> @@ -1929,6 +1929,19 @@ enum wmi_tlv_tag {
> WMI_TAG_REGULATORY_RULE_EXT_STRUCT = 0x3A9,
> WMI_TAG_REG_CHAN_LIST_CC_EXT_EVENT,
> WMI_TAG_EHT_RATE_SET = 0x3C4,
> + WMI_TAG_DCS_AWGN_INT_TYPE = 0x3C5,
> + WMI_TAG_MLO_TX_SEND_PARAMS,
> + WMI_TAG_MLO_PARTNER_LINK_PARAMS,
> + WMI_TAG_MLO_PARTNER_LINK_PARAMS_PEER_ASSOC,
> + WMI_TAG_MLO_SETUP_CMD = 0x3C9,
> + WMI_TAG_MLO_SETUP_COMPLETE_EVENT,
> + WMI_TAG_MLO_READY_CMD,
> + WMI_TAG_MLO_TEARDOWN_CMD,
> + WMI_TAG_MLO_TEARDOWN_COMPLETE,
> + WMI_TAG_MLO_PEER_ASSOC_PARAMS = 0x3D0,
> + WMI_TAG_MLO_PEER_CREATE_PARAMS = 0x3D5,
> + WMI_TAG_MLO_VDEV_START_PARAMS = 0x3D6,
> + WMI_TAG_MLO_VDEV_CREATE_PARAMS = 0x3D7,
> WMI_TAG_PDEV_SET_BIOS_SAR_TABLE_CMD = 0x3D8,
> WMI_TAG_PDEV_SET_BIOS_GEO_TABLE_CMD = 0x3D9,
> WMI_TAG_PDEV_SET_BIOS_INTERFACE_CMD = 0x3FB,
> @@ -2740,6 +2753,7 @@ struct ath12k_wmi_vdev_create_arg {
> u8 if_stats_id;
> u32 mbssid_flags;
> u32 mbssid_tx_vdev_id;
> + u8 mld_addr[ETH_ALEN];
> };
>
> #define ATH12K_MAX_VDEV_STATS_ID 0x30
> @@ -2766,6 +2780,44 @@ struct ath12k_wmi_vdev_txrx_streams_params {
> __le32 supported_rx_streams;
> } __packed;
>
> +/* 2 word representation of MAC addr */
> +struct wmi_mac_addr {
> + union {
> + u8 addr[6];
> + struct {
> + u32 word0;
> + u32 word1;
> + } __packed;
> + } __packed;
> +} __packed;
we already have:
/* 2 word representation of MAC addr */
struct ath12k_wmi_mac_addr_params {
u8 addr[ETH_ALEN];
u8 padding[2];
} __packed;
why aren't we consistently using that?
> +
> +struct wmi_vdev_create_mlo_params {
> + __le32 tlv_header;
> + struct wmi_mac_addr mld_macaddr;
> +} __packed;
> +
> +#define ATH12K_WMI_FLAG_MLO_ENABLED BIT(0)
> +#define ATH12K_WMI_FLAG_MLO_ASSOC_LINK BIT(1)
> +#define ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC BIT(2)
> +#define ATH12K_WMI_FLAG_MLO_LOGICAL_LINK_IDX_VALID BIT(3)
> +#define ATH12K_WMI_FLAG_MLO_PEER_ID_VALID BIT(4)
> +#define ATH12K_WMI_FLAG_MLO_MCAST_VDEV BIT(5)
> +#define ATH12K_WMI_FLAG_MLO_EMLSR_SUPPORT BIT(6)
> +#define ATH12K_WMI_FLAG_MLO_FORCED_INACTIVE BIT(7)
> +#define ATH12K_WMI_FLAG_MLO_LINK_ADD BIT(8)
> +
> +struct wmi_vdev_start_mlo_params {
> + __le32 tlv_header;
> + __le32 flags;
> +} __packed;
> +
> +struct wmi_partner_link_info {
> + __le32 tlv_header;
> + __le32 vdev_id;
> + __le32 hw_link_id;
> + struct wmi_mac_addr vdev_addr;
> +} __packed;
> +
> struct wmi_vdev_delete_cmd {
> __le32 tlv_header;
> __le32 vdev_id;
> @@ -2909,6 +2961,27 @@ enum wmi_phy_mode {
> MODE_MAX = 33,
> };
>
> +#define ATH12K_WMI_MLO_MAX_LINKS 4
> +
> +struct wmi_ml_partner_info {
> + u32 vdev_id;
> + u32 hw_link_id;
> + u8 addr[ETH_ALEN];
> + bool assoc_link;
> + bool primary_umac;
> + bool logical_link_idx_valid;
> + u32 logical_link_idx;
> +};
> +
> +struct wmi_ml_arg {
> + bool enabled;
> + bool assoc_link;
> + bool mcast_link;
> + bool link_add;
> + u8 num_partner_links;
> + struct wmi_ml_partner_info partner_info[ATH12K_WMI_MLO_MAX_LINKS];
> +};
> +
> struct wmi_vdev_start_req_arg {
> u32 vdev_id;
> u32 freq;
> @@ -2946,6 +3019,7 @@ struct wmi_vdev_start_req_arg {
> u32 mbssid_flags;
> u32 mbssid_tx_vdev_id;
> u32 punct_bitmap;
> + struct wmi_ml_arg ml;
> };
>
> struct ath12k_wmi_peer_create_arg {
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes
2024-10-23 15:19 ` Jeff Johnson
@ 2024-10-24 18:10 ` Kalle Valo
0 siblings, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-24 18:10 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:29 AM, Kalle Valo wrote:
>
>> From: Sriram R <quic_srirrama@quicinc.com>
>>
>> Add changes to add the link vdevs dynamically whenever a channel is assigned
>> from mac80211 for a link vdev. During vdev create, update ML address of the
>> vdev to firmware using the new WMI parameter (WMI_TAG_MLO_VDEV_CREATE_PARAMS).
>>
>> During vdev start, notify the firmware that this link vdev is newly added and
>> also indicate all its known partners so that the firmware can take necessary
>> actions to internally update the partners on the new link being added.
>>
>> 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>
>> Co-developed-by: Rameshkumar Sundaram <quic_ramess@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/mac.c | 90 ++++++++++++++++++++++++++-
>> drivers/net/wireless/ath/ath12k/wmi.c | 85 ++++++++++++++++++++++++-
>> drivers/net/wireless/ath/ath12k/wmi.h | 74 ++++++++++++++++++++++
>> 3 files changed, 244 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index f45f32f3b5f6..d4aa4540c8e6 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -648,6 +648,18 @@ struct ath12k *ath12k_mac_get_ar_by_pdev_id(struct ath12k_base *ab, u32 pdev_id)
>> return NULL;
>> }
>>
>> +static bool ath12k_mac_is_ml_arvif(struct ath12k_link_vif *arvif)
>> +{
>> + struct ath12k_vif *ahvif = arvif->ahvif;
>> +
>> + lockdep_assert_wiphy(ahvif->ah->hw->wiphy);
>
> should we have helper functions ath12k_<foo>_to_wiphy() to abstract out the
> underlying linkages?
While working on these patches I started to like the current form, extra
abstractions are always an extra layer to check when working on the
code. Maybe we should revisit this after MLO works? It's easy to add the
new helper in a separate patch.
>> +static void
>> +ath12k_mac_mlo_get_vdev_args(struct ath12k_link_vif *arvif,
>> + struct wmi_ml_arg *ml_arg)
>> +{
>> + struct ath12k_vif *ahvif = arvif->ahvif;
>> + struct wmi_ml_partner_info *partner_info;
>> + struct ieee80211_bss_conf *link_conf;
>> + struct ath12k_link_vif *arvif_p;
>> + unsigned long links;
>> + u8 link_id;
>> +
>> + lockdep_assert_wiphy(ahvif->ah->hw->wiphy);
>> +
>> + if (!ath12k_mac_is_ml_arvif(arvif))
>> + return;
>> +
>> + if (hweight16(ahvif->vif->valid_links) > ATH12K_WMI_MLO_MAX_LINKS)
>> + return;
>> +
>> + rcu_read_lock();
>
> what is this protecting?
Access to ahvif->link[] and vif->link_conf[]. Protection for
ahvif->links_map is still not clear for me, I need to analyse that more.
> do all of the statements between here and the for loop really need RCU protection?
No, they do not need it. And actually we can could change it to
wiphy_dereference() so we don't need to take rcu_read_lock() at all.
I'll do that in v2.
>> +/* 2 word representation of MAC addr */
>> +struct wmi_mac_addr {
>> + union {
>> + u8 addr[6];
>> + struct {
>> + u32 word0;
>> + u32 word1;
>> + } __packed;
>> + } __packed;
>> +} __packed;
>
> we already have:
> /* 2 word representation of MAC addr */
> struct ath12k_wmi_mac_addr_params {
> u8 addr[ETH_ALEN];
> u8 padding[2];
> } __packed;
>
> why aren't we consistently using that?
Ouch, I'll switch to using that. Thanks for the review!
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
2024-10-23 13:29 ` [PATCH 1/8] wifi: ath12k: ath12k_mac_vdev_create(): use goto for error handling Kalle Valo
2024-10-23 13:29 ` [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes Kalle Valo
@ 2024-10-23 13:29 ` Kalle Valo
2024-10-23 15:38 ` Jeff Johnson
[not found] ` <e886a4c0-14f9-4299-97ef-f8cd811c94e6@quicinc.com>
2024-10-23 13:30 ` [PATCH 4/8] wifi: ath12k: introduce ath12k_hw_warn() Kalle Valo
` (4 subsequent siblings)
7 siblings, 2 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:29 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Sriram R <quic_srirrama@quicinc.com>
Refactor ath12k_mac_op_sta_state(), with generic wrappers which can be used for
both multi link stations and non-ML stations.
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: Harshitha Prem <quic_hprem@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 | 341 +++++++++++++++++--------
2 files changed, 242 insertions(+), 102 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 06b637ba8b8f..6faa46b9adc9 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -461,6 +461,9 @@ struct ath12k_link_sta {
struct ath12k_link_vif *arvif;
struct ath12k_sta *ahsta;
+ /* link address similar to ieee80211_link_sta */
+ u8 addr[ETH_ALEN];
+
/* the following are protected by ar->data_lock */
u32 changed; /* IEEE80211_RC_* */
u32 bw;
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index d4aa4540c8e6..3de6d605cd74 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -4519,10 +4519,10 @@ ath12k_mac_set_peer_vht_fixed_rate(struct ath12k_link_vif *arvif,
return ret;
}
-static int ath12k_station_assoc(struct ath12k *ar,
- struct ath12k_link_vif *arvif,
- struct ath12k_link_sta *arsta,
- bool reassoc)
+static int ath12k_mac_station_assoc(struct ath12k *ar,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta,
+ bool reassoc)
{
struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
@@ -4609,28 +4609,19 @@ static int ath12k_station_assoc(struct ath12k *ar,
return 0;
}
-static int ath12k_station_disassoc(struct ath12k *ar,
- struct ath12k_link_vif *arvif,
- struct ath12k_link_sta *arsta)
+static int ath12k_mac_station_disassoc(struct ath12k *ar,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta)
{
struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
- int ret;
lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
if (!sta->wme) {
arvif->num_legacy_stations--;
- ret = ath12k_recalc_rtscts_prot(arvif);
- if (ret)
- return ret;
+ return ath12k_recalc_rtscts_prot(arvif);
}
- ret = ath12k_clear_peer_keys(arvif, sta->addr);
- if (ret) {
- ath12k_warn(ar->ab, "failed to clear all peer keys for vdev %i: %d\n",
- arvif->vdev_id, ret);
- return ret;
- }
return 0;
}
@@ -4826,6 +4817,145 @@ static void ath12k_mac_dec_num_stations(struct ath12k_link_vif *arvif,
ar->num_stations--;
}
+static void ath12k_mac_station_post_remove(struct ath12k *ar,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta)
+{
+ struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
+ struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
+ struct ath12k_sta *ahsta = arsta->ahsta;
+ struct ath12k_peer *peer;
+
+ lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
+ ath12k_mac_dec_num_stations(arvif, arsta);
+
+ spin_lock_bh(&ar->ab->base_lock);
+
+ peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
+ if (peer && peer->sta == sta) {
+ ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
+ vif->addr, arvif->vdev_id);
+ peer->sta = NULL;
+ list_del(&peer->list);
+ kfree(peer);
+ ar->num_peers--;
+ }
+
+ spin_unlock_bh(&ar->ab->base_lock);
+
+ kfree(arsta->rx_stats);
+ arsta->rx_stats = NULL;
+
+ if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
+ rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
+ synchronize_rcu();
+ ahsta->links_map &= ~(BIT(arsta->link_id));
+ arsta->link_id = ATH12K_INVALID_LINK_ID;
+ arsta->ahsta = NULL;
+ }
+}
+
+static int ath12k_mac_station_unauthorize(struct ath12k *ar,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta)
+{
+ struct ath12k_peer *peer;
+ int ret;
+
+ lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
+ spin_lock_bh(&ar->ab->base_lock);
+
+ peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
+ if (peer)
+ peer->is_authorized = false;
+
+ spin_unlock_bh(&ar->ab->base_lock);
+
+ /* Driver should clear the peer keys during mac80211's ref ptr
+ * gets cleared in __sta_info_destroy_part2 (trans from
+ * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
+ */
+ ret = ath12k_clear_peer_keys(arvif, arsta->addr);
+ if (ret) {
+ ath12k_warn(ar->ab, "failed to clear all peer keys for vdev %i: %d\n",
+ arvif->vdev_id, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ath12k_mac_station_authorize(struct ath12k *ar,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta)
+{
+ struct ath12k_peer *peer;
+ struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
+ struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
+ int ret;
+
+ lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
+ spin_lock_bh(&ar->ab->base_lock);
+
+ peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
+ if (peer)
+ peer->is_authorized = true;
+
+ spin_unlock_bh(&ar->ab->base_lock);
+
+ if (vif->type == NL80211_IFTYPE_STATION && arvif->is_up) {
+ ret = ath12k_wmi_set_peer_param(ar, sta->addr,
+ arvif->vdev_id,
+ WMI_PEER_AUTHORIZE,
+ 1);
+ if (ret) {
+ ath12k_warn(ar->ab, "Unable to authorize peer %pM vdev %d: %d\n",
+ sta->addr, arvif->vdev_id, ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int ath12k_mac_station_remove(struct ath12k *ar,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta)
+{
+ struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
+ struct ath12k_vif *ahvif = arvif->ahvif;
+ int ret;
+
+ lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
+ wiphy_work_cancel(ar->ah->hw->wiphy, &arsta->update_wk);
+
+ if (ahvif->vdev_type == WMI_VDEV_TYPE_STA) {
+ ath12k_bss_disassoc(ar, arvif);
+ ret = ath12k_mac_vdev_stop(arvif);
+ if (ret)
+ ath12k_warn(ar->ab, "failed to stop vdev %i: %d\n",
+ arvif->vdev_id, ret);
+ }
+
+ ath12k_dp_peer_cleanup(ar, arvif->vdev_id, sta->addr);
+
+ ret = ath12k_peer_delete(ar, arvif->vdev_id, sta->addr);
+ if (ret)
+ ath12k_warn(ar->ab, "Failed to delete peer: %pM for VDEV: %d\n",
+ sta->addr, arvif->vdev_id);
+ else
+ ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "Removed peer: %pM for VDEV: %d\n",
+ sta->addr, arvif->vdev_id);
+
+ ath12k_mac_station_post_remove(ar, arvif, arsta);
+
+ return ret;
+}
+
static int ath12k_mac_station_add(struct ath12k *ar,
struct ath12k_link_vif *arvif,
struct ath12k_link_sta *arsta)
@@ -4933,31 +5063,37 @@ static u32 ath12k_mac_ieee80211_sta_bw_to_wmi(struct ath12k *ar,
return bw;
}
-static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
- struct ieee80211_vif *vif,
- struct ieee80211_sta *sta,
- enum ieee80211_sta_state old_state,
- enum ieee80211_sta_state new_state)
+static int ath12k_mac_handle_link_sta_state(struct ieee80211_hw *hw,
+ struct ath12k_link_vif *arvif,
+ struct ath12k_link_sta *arsta,
+ enum ieee80211_sta_state old_state,
+ enum ieee80211_sta_state new_state)
{
- struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
- struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
- struct ath12k *ar;
- struct ath12k_link_vif *arvif;
- struct ath12k_link_sta *arsta;
- struct ath12k_peer *peer;
+ struct ath12k *ar = arvif->ar;
+ struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
+ struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
+ struct ath12k_sta *ahsta = arsta->ahsta;
int ret = 0;
lockdep_assert_wiphy(hw->wiphy);
- arvif = &ahvif->deflink;
- arsta = &ahsta->deflink;
+ /* IEEE80211_STA_NONE -> IEEE80211_STA_NOTEXIST: Remove the station
+ * from driver
+ */
+ if ((old_state == IEEE80211_STA_NONE &&
+ new_state == IEEE80211_STA_NOTEXIST)) {
+ /* ML sta needs separate handling */
+ if (sta->mlo)
+ return 0;
- ar = ath12k_get_ar_by_vif(hw, vif);
- if (!ar) {
- WARN_ON_ONCE(1);
- return -EINVAL;
+ ret = ath12k_mac_station_remove(ar, arvif, arsta);
+ if (ret) {
+ ath12k_warn(ar->ab, "Failed to remove station: %pM for VDEV: %d\n",
+ arsta->addr, arvif->vdev_id);
+ }
}
+ /* IEEE80211_STA_NOTEXIST -> IEEE80211_STA_NONE: Add new station to driver */
if (old_state == IEEE80211_STA_NOTEXIST &&
new_state == IEEE80211_STA_NONE) {
memset(arsta, 0, sizeof(*arsta));
@@ -4975,56 +5111,16 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
if (ret)
ath12k_warn(ar->ab, "Failed to add station: %pM for VDEV: %d\n",
sta->addr, arvif->vdev_id);
- } else if ((old_state == IEEE80211_STA_NONE &&
- new_state == IEEE80211_STA_NOTEXIST)) {
- wiphy_work_cancel(hw->wiphy, &arsta->update_wk);
- if (ahvif->vdev_type == WMI_VDEV_TYPE_STA) {
- ath12k_bss_disassoc(ar, arvif);
- ret = ath12k_mac_vdev_stop(arvif);
- if (ret)
- ath12k_warn(ar->ab, "failed to stop vdev %i: %d\n",
- arvif->vdev_id, ret);
- }
- ath12k_dp_peer_cleanup(ar, arvif->vdev_id, sta->addr);
-
- ret = ath12k_peer_delete(ar, arvif->vdev_id, sta->addr);
- if (ret)
- ath12k_warn(ar->ab, "Failed to delete peer: %pM for VDEV: %d\n",
- sta->addr, arvif->vdev_id);
- else
- ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "Removed peer: %pM for VDEV: %d\n",
- sta->addr, arvif->vdev_id);
-
- ath12k_mac_dec_num_stations(arvif, arsta);
- spin_lock_bh(&ar->ab->base_lock);
- peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
- if (peer && peer->sta == sta) {
- ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
- vif->addr, arvif->vdev_id);
- peer->sta = NULL;
- list_del(&peer->list);
- kfree(peer);
- ar->num_peers--;
- }
- spin_unlock_bh(&ar->ab->base_lock);
-
- kfree(arsta->rx_stats);
- arsta->rx_stats = NULL;
-
- if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
- rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
- synchronize_rcu();
- ahsta->links_map &= ~(BIT(arsta->link_id));
- arsta->link_id = ATH12K_INVALID_LINK_ID;
- arsta->ahsta = NULL;
- }
+ /* IEEE80211_STA_AUTH -> IEEE80211_STA_ASSOC: Send station assoc command for
+ * peer associated to AP/Mesh/ADHOC vif type.
+ */
} else if (old_state == IEEE80211_STA_AUTH &&
new_state == IEEE80211_STA_ASSOC &&
(vif->type == NL80211_IFTYPE_AP ||
vif->type == NL80211_IFTYPE_MESH_POINT ||
vif->type == NL80211_IFTYPE_ADHOC)) {
- ret = ath12k_station_assoc(ar, arvif, arsta, false);
+ ret = ath12k_mac_station_assoc(ar, arvif, arsta, false);
if (ret)
ath12k_warn(ar->ab, "Failed to associate station: %pM\n",
sta->addr);
@@ -5035,40 +5131,32 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
arsta->bw_prev = sta->deflink.bandwidth;
spin_unlock_bh(&ar->data_lock);
+
+ /* IEEE80211_STA_ASSOC -> IEEE80211_STA_AUTHORIZED: set peer status as
+ * authorized
+ */
} else if (old_state == IEEE80211_STA_ASSOC &&
new_state == IEEE80211_STA_AUTHORIZED) {
- spin_lock_bh(&ar->ab->base_lock);
+ ret = ath12k_mac_station_authorize(ar, arvif, arsta);
+ if (ret)
+ ath12k_warn(ar->ab, "Failed to authorize station: %pM\n",
+ sta->addr);
- peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
- if (peer)
- peer->is_authorized = true;
-
- spin_unlock_bh(&ar->ab->base_lock);
-
- if (vif->type == NL80211_IFTYPE_STATION && arvif->is_up) {
- ret = ath12k_wmi_set_peer_param(ar, sta->addr,
- arvif->vdev_id,
- WMI_PEER_AUTHORIZE,
- 1);
- if (ret)
- ath12k_warn(ar->ab, "Unable to authorize peer %pM vdev %d: %d\n",
- sta->addr, arvif->vdev_id, ret);
- }
+ /* IEEE80211_STA_AUTHORIZED -> IEEE80211_STA_ASSOC: station may be in removal,
+ * deauthorize it.
+ */
} else if (old_state == IEEE80211_STA_AUTHORIZED &&
new_state == IEEE80211_STA_ASSOC) {
- spin_lock_bh(&ar->ab->base_lock);
-
- peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
- if (peer)
- peer->is_authorized = false;
-
- spin_unlock_bh(&ar->ab->base_lock);
+ ath12k_mac_station_unauthorize(ar, arvif, arsta);
+ /* IEEE80211_STA_ASSOC -> IEEE80211_STA_AUTH: disassoc peer connected to
+ * AP/mesh/ADHOC vif type.
+ */
} else if (old_state == IEEE80211_STA_ASSOC &&
new_state == IEEE80211_STA_AUTH &&
(vif->type == NL80211_IFTYPE_AP ||
vif->type == NL80211_IFTYPE_MESH_POINT ||
vif->type == NL80211_IFTYPE_ADHOC)) {
- ret = ath12k_station_disassoc(ar, arvif, arsta);
+ ret = ath12k_mac_station_disassoc(ar, arvif, arsta);
if (ret)
ath12k_warn(ar->ab, "Failed to disassociate station: %pM\n",
sta->addr);
@@ -5077,6 +5165,55 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
return ret;
}
+static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ enum ieee80211_sta_state old_state,
+ enum ieee80211_sta_state new_state)
+{
+ struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
+ struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
+ struct ath12k_link_vif *arvif;
+ struct ath12k_link_sta *arsta;
+ int ret;
+ u8 link_id = 0;
+
+ lockdep_assert_wiphy(hw->wiphy);
+
+ if (ieee80211_vif_is_mld(vif) && sta->valid_links) {
+ WARN_ON(!sta->mlo && hweight16(sta->valid_links) != 1);
+ link_id = ffs(sta->valid_links) - 1;
+ }
+
+ /* Handle for non-ML station */
+ if (!sta->mlo) {
+ arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
+ arsta = &ahsta->deflink;
+ arsta->ahsta = ahsta;
+
+ if (WARN_ON(!arvif || !arsta)) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ /* vdev might be in deleted */
+ if (WARN_ON(!arvif->ar)) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ ret = ath12k_mac_handle_link_sta_state(hw, arvif, arsta,
+ old_state, new_state);
+ if (ret)
+ goto exit;
+ }
+
+ ret = 0;
+
+exit:
+ return ret;
+}
+
static int ath12k_mac_op_sta_set_txpwr(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta)
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-23 13:29 ` [PATCH 3/8] wifi: ath12k: Refactor sta state machine Kalle Valo
@ 2024-10-23 15:38 ` Jeff Johnson
2024-10-29 15:29 ` Kalle Valo
2024-10-29 15:38 ` Kalle Valo
[not found] ` <e886a4c0-14f9-4299-97ef-f8cd811c94e6@quicinc.com>
1 sibling, 2 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 15:38 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:29 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
>
> Refactor ath12k_mac_op_sta_state(), with generic wrappers which can be used for
> both multi link stations and non-ML stations.
>
> 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: Harshitha Prem <quic_hprem@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 | 341 +++++++++++++++++--------
> 2 files changed, 242 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 06b637ba8b8f..6faa46b9adc9 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -461,6 +461,9 @@ struct ath12k_link_sta {
> struct ath12k_link_vif *arvif;
> struct ath12k_sta *ahsta;
>
> + /* link address similar to ieee80211_link_sta */
> + u8 addr[ETH_ALEN];
> +
> /* the following are protected by ar->data_lock */
> u32 changed; /* IEEE80211_RC_* */
> u32 bw;
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index d4aa4540c8e6..3de6d605cd74 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -4519,10 +4519,10 @@ ath12k_mac_set_peer_vht_fixed_rate(struct ath12k_link_vif *arvif,
> return ret;
> }
>
> -static int ath12k_station_assoc(struct ath12k *ar,
> - struct ath12k_link_vif *arvif,
> - struct ath12k_link_sta *arsta,
> - bool reassoc)
> +static int ath12k_mac_station_assoc(struct ath12k *ar,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta,
> + bool reassoc)
> {
> struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
> struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> @@ -4609,28 +4609,19 @@ static int ath12k_station_assoc(struct ath12k *ar,
> return 0;
> }
>
> -static int ath12k_station_disassoc(struct ath12k *ar,
> - struct ath12k_link_vif *arvif,
> - struct ath12k_link_sta *arsta)
> +static int ath12k_mac_station_disassoc(struct ath12k *ar,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta)
> {
> struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> - int ret;
>
> lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>
> if (!sta->wme) {
> arvif->num_legacy_stations--;
> - ret = ath12k_recalc_rtscts_prot(arvif);
> - if (ret)
> - return ret;
> + return ath12k_recalc_rtscts_prot(arvif);
> }
>
> - ret = ath12k_clear_peer_keys(arvif, sta->addr);
> - if (ret) {
> - ath12k_warn(ar->ab, "failed to clear all peer keys for vdev %i: %d\n",
> - arvif->vdev_id, ret);
> - return ret;
> - }
> return 0;
> }
>
> @@ -4826,6 +4817,145 @@ static void ath12k_mac_dec_num_stations(struct ath12k_link_vif *arvif,
> ar->num_stations--;
> }
>
> +static void ath12k_mac_station_post_remove(struct ath12k *ar,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta)
> +{
> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> + struct ath12k_sta *ahsta = arsta->ahsta;
> + struct ath12k_peer *peer;
> +
> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
> + ath12k_mac_dec_num_stations(arvif, arsta);
> +
> + spin_lock_bh(&ar->ab->base_lock);
> +
> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
> + if (peer && peer->sta == sta) {
> + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
> + vif->addr, arvif->vdev_id);
> + peer->sta = NULL;
> + list_del(&peer->list);
> + kfree(peer);
> + ar->num_peers--;
> + }
> +
> + spin_unlock_bh(&ar->ab->base_lock);
> +
> + kfree(arsta->rx_stats);
> + arsta->rx_stats = NULL;
> +
> + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
> + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
> + synchronize_rcu();
I've mentioned this in the past in some internal discussion and seems now is a
good time to bring this to light.
It concerns me that this happens so late in the process. In theory another
thread could already have a valid arsta pointer and could be trying to
dereference that pointer while the code above is destroying underlying data
(i.e. arsta->rx_stats).
Should we set this to NULL and synchronize RCU at the beginning of the process
so that we know all access to the struct has finished before we start
destroying the data?
Or can this not actually happen in practice due to other synchronization
mechansims? And if so, should we document that somewhere?
> + ahsta->links_map &= ~(BIT(arsta->link_id));
> + arsta->link_id = ATH12K_INVALID_LINK_ID;
> + arsta->ahsta = NULL;
> + }
> +}
> +
> +static int ath12k_mac_station_unauthorize(struct ath12k *ar,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta)
> +{
> + struct ath12k_peer *peer;
> + int ret;
> +
> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
> + spin_lock_bh(&ar->ab->base_lock);
> +
> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
> + if (peer)
> + peer->is_authorized = false;
> +
> + spin_unlock_bh(&ar->ab->base_lock);
> +
> + /* Driver should clear the peer keys during mac80211's ref ptr
> + * gets cleared in __sta_info_destroy_part2 (trans from
> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
I'm unable to understand this comment
> + */
> + ret = ath12k_clear_peer_keys(arvif, arsta->addr);
> + if (ret) {
> + ath12k_warn(ar->ab, "failed to clear all peer keys for vdev %i: %d\n",
> + arvif->vdev_id, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ath12k_mac_station_authorize(struct ath12k *ar,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta)
> +{
> + struct ath12k_peer *peer;
> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> + int ret;
> +
> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
> + spin_lock_bh(&ar->ab->base_lock);
> +
> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
> + if (peer)
> + peer->is_authorized = true;
> +
> + spin_unlock_bh(&ar->ab->base_lock);
> +
> + if (vif->type == NL80211_IFTYPE_STATION && arvif->is_up) {
> + ret = ath12k_wmi_set_peer_param(ar, sta->addr,
> + arvif->vdev_id,
> + WMI_PEER_AUTHORIZE,
> + 1);
> + if (ret) {
> + ath12k_warn(ar->ab, "Unable to authorize peer %pM vdev %d: %d\n",
> + sta->addr, arvif->vdev_id, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int ath12k_mac_station_remove(struct ath12k *ar,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta)
> +{
> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> + struct ath12k_vif *ahvif = arvif->ahvif;
> + int ret;
> +
> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
> + wiphy_work_cancel(ar->ah->hw->wiphy, &arsta->update_wk);
> +
> + if (ahvif->vdev_type == WMI_VDEV_TYPE_STA) {
> + ath12k_bss_disassoc(ar, arvif);
> + ret = ath12k_mac_vdev_stop(arvif);
> + if (ret)
> + ath12k_warn(ar->ab, "failed to stop vdev %i: %d\n",
> + arvif->vdev_id, ret);
> + }
> +
> + ath12k_dp_peer_cleanup(ar, arvif->vdev_id, sta->addr);
> +
> + ret = ath12k_peer_delete(ar, arvif->vdev_id, sta->addr);
> + if (ret)
> + ath12k_warn(ar->ab, "Failed to delete peer: %pM for VDEV: %d\n",
> + sta->addr, arvif->vdev_id);
> + else
> + ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "Removed peer: %pM for VDEV: %d\n",
> + sta->addr, arvif->vdev_id);
> +
> + ath12k_mac_station_post_remove(ar, arvif, arsta);
> +
> + return ret;
> +}
> +
> static int ath12k_mac_station_add(struct ath12k *ar,
> struct ath12k_link_vif *arvif,
> struct ath12k_link_sta *arsta)
> @@ -4933,31 +5063,37 @@ static u32 ath12k_mac_ieee80211_sta_bw_to_wmi(struct ath12k *ar,
> return bw;
> }
>
> -static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
> - struct ieee80211_vif *vif,
> - struct ieee80211_sta *sta,
> - enum ieee80211_sta_state old_state,
> - enum ieee80211_sta_state new_state)
> +static int ath12k_mac_handle_link_sta_state(struct ieee80211_hw *hw,
> + struct ath12k_link_vif *arvif,
> + struct ath12k_link_sta *arsta,
> + enum ieee80211_sta_state old_state,
> + enum ieee80211_sta_state new_state)
> {
> - struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
> - struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
> - struct ath12k *ar;
> - struct ath12k_link_vif *arvif;
> - struct ath12k_link_sta *arsta;
> - struct ath12k_peer *peer;
> + struct ath12k *ar = arvif->ar;
> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> + struct ath12k_sta *ahsta = arsta->ahsta;
> int ret = 0;
>
> lockdep_assert_wiphy(hw->wiphy);
>
> - arvif = &ahvif->deflink;
> - arsta = &ahsta->deflink;
> + /* IEEE80211_STA_NONE -> IEEE80211_STA_NOTEXIST: Remove the station
> + * from driver
> + */
> + if ((old_state == IEEE80211_STA_NONE &&
> + new_state == IEEE80211_STA_NOTEXIST)) {
> + /* ML sta needs separate handling */
> + if (sta->mlo)
> + return 0;
>
> - ar = ath12k_get_ar_by_vif(hw, vif);
> - if (!ar) {
> - WARN_ON_ONCE(1);
> - return -EINVAL;
> + ret = ath12k_mac_station_remove(ar, arvif, arsta);
> + if (ret) {
> + ath12k_warn(ar->ab, "Failed to remove station: %pM for VDEV: %d\n",
> + arsta->addr, arvif->vdev_id);
> + }
> }
>
> + /* IEEE80211_STA_NOTEXIST -> IEEE80211_STA_NONE: Add new station to driver */
> if (old_state == IEEE80211_STA_NOTEXIST &&
> new_state == IEEE80211_STA_NONE) {
> memset(arsta, 0, sizeof(*arsta));
> @@ -4975,56 +5111,16 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
> if (ret)
> ath12k_warn(ar->ab, "Failed to add station: %pM for VDEV: %d\n",
> sta->addr, arvif->vdev_id);
> - } else if ((old_state == IEEE80211_STA_NONE &&
> - new_state == IEEE80211_STA_NOTEXIST)) {
> - wiphy_work_cancel(hw->wiphy, &arsta->update_wk);
>
> - if (ahvif->vdev_type == WMI_VDEV_TYPE_STA) {
> - ath12k_bss_disassoc(ar, arvif);
> - ret = ath12k_mac_vdev_stop(arvif);
> - if (ret)
> - ath12k_warn(ar->ab, "failed to stop vdev %i: %d\n",
> - arvif->vdev_id, ret);
> - }
> - ath12k_dp_peer_cleanup(ar, arvif->vdev_id, sta->addr);
> -
> - ret = ath12k_peer_delete(ar, arvif->vdev_id, sta->addr);
> - if (ret)
> - ath12k_warn(ar->ab, "Failed to delete peer: %pM for VDEV: %d\n",
> - sta->addr, arvif->vdev_id);
> - else
> - ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "Removed peer: %pM for VDEV: %d\n",
> - sta->addr, arvif->vdev_id);
> -
> - ath12k_mac_dec_num_stations(arvif, arsta);
> - spin_lock_bh(&ar->ab->base_lock);
> - peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
> - if (peer && peer->sta == sta) {
> - ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
> - vif->addr, arvif->vdev_id);
> - peer->sta = NULL;
> - list_del(&peer->list);
> - kfree(peer);
> - ar->num_peers--;
> - }
> - spin_unlock_bh(&ar->ab->base_lock);
> -
> - kfree(arsta->rx_stats);
> - arsta->rx_stats = NULL;
> -
> - if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
> - rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
> - synchronize_rcu();
> - ahsta->links_map &= ~(BIT(arsta->link_id));
> - arsta->link_id = ATH12K_INVALID_LINK_ID;
> - arsta->ahsta = NULL;
> - }
> + /* IEEE80211_STA_AUTH -> IEEE80211_STA_ASSOC: Send station assoc command for
> + * peer associated to AP/Mesh/ADHOC vif type.
> + */
> } else if (old_state == IEEE80211_STA_AUTH &&
> new_state == IEEE80211_STA_ASSOC &&
> (vif->type == NL80211_IFTYPE_AP ||
> vif->type == NL80211_IFTYPE_MESH_POINT ||
> vif->type == NL80211_IFTYPE_ADHOC)) {
> - ret = ath12k_station_assoc(ar, arvif, arsta, false);
> + ret = ath12k_mac_station_assoc(ar, arvif, arsta, false);
> if (ret)
> ath12k_warn(ar->ab, "Failed to associate station: %pM\n",
> sta->addr);
> @@ -5035,40 +5131,32 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
> arsta->bw_prev = sta->deflink.bandwidth;
>
> spin_unlock_bh(&ar->data_lock);
> +
> + /* IEEE80211_STA_ASSOC -> IEEE80211_STA_AUTHORIZED: set peer status as
> + * authorized
> + */
> } else if (old_state == IEEE80211_STA_ASSOC &&
> new_state == IEEE80211_STA_AUTHORIZED) {
> - spin_lock_bh(&ar->ab->base_lock);
> + ret = ath12k_mac_station_authorize(ar, arvif, arsta);
> + if (ret)
> + ath12k_warn(ar->ab, "Failed to authorize station: %pM\n",
> + sta->addr);
>
> - peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
> - if (peer)
> - peer->is_authorized = true;
> -
> - spin_unlock_bh(&ar->ab->base_lock);
> -
> - if (vif->type == NL80211_IFTYPE_STATION && arvif->is_up) {
> - ret = ath12k_wmi_set_peer_param(ar, sta->addr,
> - arvif->vdev_id,
> - WMI_PEER_AUTHORIZE,
> - 1);
> - if (ret)
> - ath12k_warn(ar->ab, "Unable to authorize peer %pM vdev %d: %d\n",
> - sta->addr, arvif->vdev_id, ret);
> - }
> + /* IEEE80211_STA_AUTHORIZED -> IEEE80211_STA_ASSOC: station may be in removal,
> + * deauthorize it.
> + */
> } else if (old_state == IEEE80211_STA_AUTHORIZED &&
> new_state == IEEE80211_STA_ASSOC) {
> - spin_lock_bh(&ar->ab->base_lock);
> -
> - peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
> - if (peer)
> - peer->is_authorized = false;
> -
> - spin_unlock_bh(&ar->ab->base_lock);
> + ath12k_mac_station_unauthorize(ar, arvif, arsta);
> + /* IEEE80211_STA_ASSOC -> IEEE80211_STA_AUTH: disassoc peer connected to
> + * AP/mesh/ADHOC vif type.
> + */
> } else if (old_state == IEEE80211_STA_ASSOC &&
> new_state == IEEE80211_STA_AUTH &&
> (vif->type == NL80211_IFTYPE_AP ||
> vif->type == NL80211_IFTYPE_MESH_POINT ||
> vif->type == NL80211_IFTYPE_ADHOC)) {
> - ret = ath12k_station_disassoc(ar, arvif, arsta);
> + ret = ath12k_mac_station_disassoc(ar, arvif, arsta);
> if (ret)
> ath12k_warn(ar->ab, "Failed to disassociate station: %pM\n",
> sta->addr);
> @@ -5077,6 +5165,55 @@ static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
> return ret;
> }
>
> +static int ath12k_mac_op_sta_state(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> + enum ieee80211_sta_state old_state,
> + enum ieee80211_sta_state new_state)
> +{
> + struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
> + struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
> + struct ath12k_link_vif *arvif;
> + struct ath12k_link_sta *arsta;
> + int ret;
> + u8 link_id = 0;
> +
> + lockdep_assert_wiphy(hw->wiphy);
> +
> + if (ieee80211_vif_is_mld(vif) && sta->valid_links) {
> + WARN_ON(!sta->mlo && hweight16(sta->valid_links) != 1);
> + link_id = ffs(sta->valid_links) - 1;
> + }
> +
> + /* Handle for non-ML station */
> + if (!sta->mlo) {
> + arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
> + arsta = &ahsta->deflink;
> + arsta->ahsta = ahsta;
> +
> + if (WARN_ON(!arvif || !arsta)) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + /* vdev might be in deleted */
> + if (WARN_ON(!arvif->ar)) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + ret = ath12k_mac_handle_link_sta_state(hw, arvif, arsta,
> + old_state, new_state);
> + if (ret)
> + goto exit;
> + }
> +
> + ret = 0;
> +
> +exit:
> + return ret;
> +}
> +
> static int ath12k_mac_op_sta_set_txpwr(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif,
> struct ieee80211_sta *sta)
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-23 15:38 ` Jeff Johnson
@ 2024-10-29 15:29 ` Kalle Valo
2024-10-29 15:35 ` Jeff Johnson
2024-10-29 15:38 ` Kalle Valo
1 sibling, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-29 15:29 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:29 AM, Kalle Valo wrote:
>
>> +static void ath12k_mac_station_post_remove(struct ath12k *ar,
>> + struct ath12k_link_vif *arvif,
>> + struct ath12k_link_sta *arsta)
>> +{
>> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
>> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
>> + struct ath12k_sta *ahsta = arsta->ahsta;
>> + struct ath12k_peer *peer;
>> +
>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>> +
>> + ath12k_mac_dec_num_stations(arvif, arsta);
>> +
>> + spin_lock_bh(&ar->ab->base_lock);
>> +
>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
>> + if (peer && peer->sta == sta) {
>> + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
>> + vif->addr, arvif->vdev_id);
>> + peer->sta = NULL;
>> + list_del(&peer->list);
>> + kfree(peer);
>> + ar->num_peers--;
>> + }
>> +
>> + spin_unlock_bh(&ar->ab->base_lock);
>> +
>> + kfree(arsta->rx_stats);
>> + arsta->rx_stats = NULL;
>> +
>> + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
>> + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
>> + synchronize_rcu();
>
> I've mentioned this in the past in some internal discussion and seems now is a
> good time to bring this to light.
>
> It concerns me that this happens so late in the process. In theory another
> thread could already have a valid arsta pointer and could be trying to
> dereference that pointer while the code above is destroying underlying data
> (i.e. arsta->rx_stats).
>
> Should we set this to NULL and synchronize RCU at the beginning of the process
> so that we know all access to the struct has finished before we start
> destroying the data?
>
> Or can this not actually happen in practice due to other synchronization
> mechansims? And if so, should we document that somewhere?
I think you are correct, AFAICS the kfree(arsta->rx_stats) should be
after synchronize_rcu(). But this race was already in the code before
this patch so we need to fix in a separate patch. I have added this to
my todo list.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-29 15:29 ` Kalle Valo
@ 2024-10-29 15:35 ` Jeff Johnson
0 siblings, 0 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-29 15:35 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath12k, linux-wireless
On 10/29/2024 8:29 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
>> On 10/23/2024 6:29 AM, Kalle Valo wrote:
>>
>>> +static void ath12k_mac_station_post_remove(struct ath12k *ar,
>>> + struct ath12k_link_vif *arvif,
>>> + struct ath12k_link_sta *arsta)
>>> +{
>>> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
>>> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
>>> + struct ath12k_sta *ahsta = arsta->ahsta;
>>> + struct ath12k_peer *peer;
>>> +
>>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>> +
>>> + ath12k_mac_dec_num_stations(arvif, arsta);
>>> +
>>> + spin_lock_bh(&ar->ab->base_lock);
>>> +
>>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
>>> + if (peer && peer->sta == sta) {
>>> + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
>>> + vif->addr, arvif->vdev_id);
>>> + peer->sta = NULL;
>>> + list_del(&peer->list);
>>> + kfree(peer);
>>> + ar->num_peers--;
>>> + }
>>> +
>>> + spin_unlock_bh(&ar->ab->base_lock);
>>> +
>>> + kfree(arsta->rx_stats);
>>> + arsta->rx_stats = NULL;
>>> +
>>> + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
>>> + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
>>> + synchronize_rcu();
>>
>> I've mentioned this in the past in some internal discussion and seems now is a
>> good time to bring this to light.
>>
>> It concerns me that this happens so late in the process. In theory another
>> thread could already have a valid arsta pointer and could be trying to
>> dereference that pointer while the code above is destroying underlying data
>> (i.e. arsta->rx_stats).
>>
>> Should we set this to NULL and synchronize RCU at the beginning of the process
>> so that we know all access to the struct has finished before we start
>> destroying the data?
>>
>> Or can this not actually happen in practice due to other synchronization
>> mechansims? And if so, should we document that somewhere?
>
> I think you are correct, AFAICS the kfree(arsta->rx_stats) should be
> after synchronize_rcu(). But this race was already in the code before
> this patch so we need to fix in a separate patch. I have added this to
> my todo list.
>
Sounds reasonable to me
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-23 15:38 ` Jeff Johnson
2024-10-29 15:29 ` Kalle Valo
@ 2024-10-29 15:38 ` Kalle Valo
2024-10-30 4:05 ` Aditya Kumar Singh
1 sibling, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-29 15:38 UTC (permalink / raw)
To: Jeff Johnson, Aditya Kumar Singh; +Cc: ath12k, linux-wireless
+ aditya
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar,
>> + struct ath12k_link_vif *arvif,
>> + struct ath12k_link_sta *arsta)
>> +{
>> + struct ath12k_peer *peer;
>> + int ret;
>> +
>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>> +
>> + spin_lock_bh(&ar->ab->base_lock);
>> +
>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
>> + if (peer)
>> + peer->is_authorized = false;
>> +
>> + spin_unlock_bh(&ar->ab->base_lock);
>> +
>> + /* Driver should clear the peer keys during mac80211's ref ptr
>> + * gets cleared in __sta_info_destroy_part2 (trans from
>> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
>
> I'm unable to understand this comment
Indeed, that's weird. Aditya, do you have any idea what the comment is
trying to say?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-29 15:38 ` Kalle Valo
@ 2024-10-30 4:05 ` Aditya Kumar Singh
2024-10-30 18:28 ` Kalle Valo
0 siblings, 1 reply; 38+ messages in thread
From: Aditya Kumar Singh @ 2024-10-30 4:05 UTC (permalink / raw)
To: Kalle Valo, Jeff Johnson; +Cc: ath12k, linux-wireless
On 10/29/24 21:08, Kalle Valo wrote:
> + aditya
>
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
>>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar,
>>> + struct ath12k_link_vif *arvif,
>>> + struct ath12k_link_sta *arsta)
>>> +{
>>> + struct ath12k_peer *peer;
>>> + int ret;
>>> +
>>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>> +
>>> + spin_lock_bh(&ar->ab->base_lock);
>>> +
>>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
>>> + if (peer)
>>> + peer->is_authorized = false;
>>> +
>>> + spin_unlock_bh(&ar->ab->base_lock);
>>> +
>>> + /* Driver should clear the peer keys during mac80211's ref ptr
>>> + * gets cleared in __sta_info_destroy_part2 (trans from
>>> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
>>
>> I'm unable to understand this comment
>
> Indeed, that's weird. Aditya, do you have any idea what the comment is
> trying to say?
>
At present, ath12k clear the keys in ath12k_station_disassoc() which
gets executed in state change from IEEE80211_STA_ASSOC to
IEEE80211_STA_AUTH.
However, in mac80211, once the station moves from
IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC itself, the keys are
deleted. Please see - __sta_info_destroy_part2() ->
ieee80211_free_sta_keys().
Now, ath12k peer object (struct ath12k_peer) holds the key reference
from mac80211 (see ath12k_peer::keys[]). Hence, once mac80211 deletes
the key, driver should not keep a reference to it or else it could lead
to issues.
Therefore, it is important that the driver should clear the peer keys
during transition from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC
it self since we know that once we return from here, mac80211 is going
to remove the keys.
ath12k_mac_station_unauthorize() gets called when station moves from
state IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC hence call to
ath12k_clear_peer_keys() is moved from ath12k_station_disassoc() to
ath12k_mac_station_unauthorize().
Is this clear now?
May be the comment in the code could be re-written as below?
/* Driver must clear the keys during the state change from
* IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after
* returning from here, mac80211 is going to delete the keys
* in __sta_info_destroy_part2(). This will ensure that the driver does
* not retain stale key references after mac80211 deletes the keys.
*/
--
Aditya
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-30 4:05 ` Aditya Kumar Singh
@ 2024-10-30 18:28 ` Kalle Valo
2024-10-30 18:39 ` Jeff Johnson
0 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-30 18:28 UTC (permalink / raw)
To: Aditya Kumar Singh; +Cc: Jeff Johnson, ath12k, linux-wireless
Aditya Kumar Singh <quic_adisi@quicinc.com> writes:
> On 10/29/24 21:08, Kalle Valo wrote:
>> + aditya
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>
>>>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar,
>>>> + struct ath12k_link_vif *arvif,
>>>> + struct ath12k_link_sta *arsta)
>>>> +{
>>>> + struct ath12k_peer *peer;
>>>> + int ret;
>>>> +
>>>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>>> +
>>>> + spin_lock_bh(&ar->ab->base_lock);
>>>> +
>>>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
>>>> + if (peer)
>>>> + peer->is_authorized = false;
>>>> +
>>>> + spin_unlock_bh(&ar->ab->base_lock);
>>>> +
>>>> + /* Driver should clear the peer keys during mac80211's ref ptr
>>>> + * gets cleared in __sta_info_destroy_part2 (trans from
>>>> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
>>>
>>> I'm unable to understand this comment
>> Indeed, that's weird. Aditya, do you have any idea what the comment
>> is
>> trying to say?
>>
>
> At present, ath12k clear the keys in ath12k_station_disassoc() which
> gets executed in state change from IEEE80211_STA_ASSOC to
> IEEE80211_STA_AUTH.
>
> However, in mac80211, once the station moves from
> IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC itself, the keys are
> deleted. Please see - __sta_info_destroy_part2() ->
> ieee80211_free_sta_keys().
>
> Now, ath12k peer object (struct ath12k_peer) holds the key reference
> from mac80211 (see ath12k_peer::keys[]). Hence, once mac80211 deletes
> the key, driver should not keep a reference to it or else it could
> lead to issues.
>
> Therefore, it is important that the driver should clear the peer keys
> during transition from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC
> it self since we know that once we return from here, mac80211 is going
> to remove the keys.
>
> ath12k_mac_station_unauthorize() gets called when station moves from
> state IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC hence call to
> ath12k_clear_peer_keys() is moved from ath12k_station_disassoc() to
> ath12k_mac_station_unauthorize().
>
> Is this clear now?
Super clear :) Thanks!
> May be the comment in the code could be re-written as below?
>
> /* Driver must clear the keys during the state change from
> * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after
> * returning from here, mac80211 is going to delete the keys
> * in __sta_info_destroy_part2(). This will ensure that the driver does
> * not retain stale key references after mac80211 deletes the keys.
> */
Looks good to me, I'll add that if it's ok for Jeff as well.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
2024-10-30 18:28 ` Kalle Valo
@ 2024-10-30 18:39 ` Jeff Johnson
0 siblings, 0 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-30 18:39 UTC (permalink / raw)
To: Kalle Valo, Aditya Kumar Singh; +Cc: ath12k, linux-wireless
On 10/30/2024 11:28 AM, Kalle Valo wrote:
> Aditya Kumar Singh <quic_adisi@quicinc.com> writes:
>> May be the comment in the code could be re-written as below?
>>
>> /* Driver must clear the keys during the state change from
>> * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after
>> * returning from here, mac80211 is going to delete the keys
>> * in __sta_info_destroy_part2(). This will ensure that the driver does
>> * not retain stale key references after mac80211 deletes the keys.
>> */
>
> Looks good to me, I'll add that if it's ok for Jeff as well.
>
Definitely ok, thanks for the clarification
^ permalink raw reply [flat|nested] 38+ messages in thread
[parent not found: <e886a4c0-14f9-4299-97ef-f8cd811c94e6@quicinc.com>]
* Re: [PATCH 3/8] wifi: ath12k: Refactor sta state machine
[not found] ` <e886a4c0-14f9-4299-97ef-f8cd811c94e6@quicinc.com>
@ 2024-10-26 9:08 ` Kalle Valo
0 siblings, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-26 9:08 UTC (permalink / raw)
To: Baochen Qiang; +Cc: ath12k, linux-wireless
Baochen Qiang <quic_bqiang@quicinc.com> writes:
> On 10/23/2024 9:29 PM, Kalle Valo wrote:
>> +static void ath12k_mac_station_post_remove(struct ath12k *ar,
>> + struct ath12k_link_vif *arvif,
>> + struct ath12k_link_sta *arsta)
>> +{
>> + struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
>> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
>> + struct ath12k_sta *ahsta = arsta->ahsta;
>> + struct ath12k_peer *peer;
>> +
>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>> +
>> + ath12k_mac_dec_num_stations(arvif, arsta);
>> +
>> + spin_lock_bh(&ar->ab->base_lock);
>> +
>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, sta->addr);
>> + if (peer && peer->sta == sta) {
>> + ath12k_warn(ar->ab, "Found peer entry %pM n vdev %i after it was supposedly removed\n",
>> + vif->addr, arvif->vdev_id);
>> + peer->sta = NULL;
>> + list_del(&peer->list);
>> + kfree(peer);
>> + ar->num_peers--;
>> + }
>> +
>> + spin_unlock_bh(&ar->ab->base_lock);
>> +
>> + kfree(arsta->rx_stats);
>> + arsta->rx_stats = NULL;
>> +
>> + if (arsta->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
>> + rcu_assign_pointer(ahsta->link[arsta->link_id], NULL);
>> + synchronize_rcu();
>> + ahsta->links_map &= ~(BIT(arsta->link_id));
>
> should we put this ahead of rcu_assign_pointer()?
I agree, I'll do that in v2.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 4/8] wifi: ath12k: introduce ath12k_hw_warn()
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
` (2 preceding siblings ...)
2024-10-23 13:29 ` [PATCH 3/8] wifi: ath12k: Refactor sta state machine Kalle Valo
@ 2024-10-23 13:30 ` Kalle Valo
2024-10-23 15:38 ` Jeff Johnson
2024-10-23 13:30 ` [PATCH 5/8] wifi: ath12k: Add helpers for multi link peer creation and deletion Kalle Valo
` (3 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:30 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Kalle Valo <quic_kvalo@quicinc.com>
In the following patch we need to use ath12k_warn() but don't easily have
access to struct ath12k_base (ab) but do have access to struct ath12k_hw (ah).
So add a new warning helper ath12_hw_warn() which takes the latter but the log
output is still identical but uses the struct device pointer stored to struct
ath12k_hw.
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
drivers/net/wireless/ath/ath12k/core.h | 2 ++
drivers/net/wireless/ath/ath12k/debug.c | 4 ++--
drivers/net/wireless/ath/ath12k/debug.h | 5 ++++-
drivers/net/wireless/ath/ath12k/mac.c | 2 ++
4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 6faa46b9adc9..9c4e5fae8930 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -684,6 +684,8 @@ struct ath12k {
struct ath12k_hw {
struct ieee80211_hw *hw;
+ struct device *dev;
+
/* Protect the write operation of the hardware state ath12k_hw::state
* between hardware start<=>reconfigure<=>stop transitions.
*/
diff --git a/drivers/net/wireless/ath/ath12k/debug.c b/drivers/net/wireless/ath/ath12k/debug.c
index fe5a732ba9ec..c5c8c7624cdb 100644
--- a/drivers/net/wireless/ath/ath12k/debug.c
+++ b/drivers/net/wireless/ath/ath12k/debug.c
@@ -36,7 +36,7 @@ void ath12k_err(struct ath12k_base *ab, const char *fmt, ...)
va_end(args);
}
-void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...)
+void __ath12k_warn(struct device *dev, const char *fmt, ...)
{
struct va_format vaf = {
.fmt = fmt,
@@ -45,7 +45,7 @@ void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...)
va_start(args, fmt);
vaf.va = &args;
- dev_warn_ratelimited(ab->dev, "%pV", &vaf);
+ dev_warn_ratelimited(dev, "%pV", &vaf);
/* TODO: Trace the log */
va_end(args);
}
diff --git a/drivers/net/wireless/ath/ath12k/debug.h b/drivers/net/wireless/ath/ath12k/debug.h
index f7005917362c..90e801136bc6 100644
--- a/drivers/net/wireless/ath/ath12k/debug.h
+++ b/drivers/net/wireless/ath/ath12k/debug.h
@@ -31,7 +31,10 @@ enum ath12k_debug_mask {
__printf(2, 3) void ath12k_info(struct ath12k_base *ab, const char *fmt, ...);
__printf(2, 3) void ath12k_err(struct ath12k_base *ab, const char *fmt, ...);
-__printf(2, 3) void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...);
+__printf(2, 3) void __ath12k_warn(struct device *dev, const char *fmt, ...);
+
+#define ath12k_warn(ab, fmt, ...) __ath12k_warn((ab)->dev, fmt, ##__VA_ARGS__)
+#define ath12k_hw_warn(ah, fmt, ...) __ath12k_warn((ah)->dev, fmt, ##__VA_ARGS__)
extern unsigned int ath12k_debug_mask;
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 3de6d605cd74..19c445cf52f1 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -10193,6 +10193,8 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
goto err;
}
+ ah->dev = ab->dev;
+
ab->ah[i] = ah;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 4/8] wifi: ath12k: introduce ath12k_hw_warn()
2024-10-23 13:30 ` [PATCH 4/8] wifi: ath12k: introduce ath12k_hw_warn() Kalle Valo
@ 2024-10-23 15:38 ` Jeff Johnson
2024-10-29 15:41 ` Kalle Valo
0 siblings, 1 reply; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 15:38 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:30 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
>
> In the following patch we need to use ath12k_warn() but don't easily have
> access to struct ath12k_base (ab) but do have access to struct ath12k_hw (ah).
> So add a new warning helper ath12_hw_warn() which takes the latter but the log
> output is still identical but uses the struct device pointer stored to struct
> ath12k_hw.
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/core.h | 2 ++
> drivers/net/wireless/ath/ath12k/debug.c | 4 ++--
Qualcomm Innovation Center copyright missing 2024
> drivers/net/wireless/ath/ath12k/debug.h | 5 ++++-
> drivers/net/wireless/ath/ath12k/mac.c | 2 ++
> 4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 6faa46b9adc9..9c4e5fae8930 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -684,6 +684,8 @@ struct ath12k {
>
> struct ath12k_hw {
> struct ieee80211_hw *hw;
> + struct device *dev;
> +
> /* Protect the write operation of the hardware state ath12k_hw::state
> * between hardware start<=>reconfigure<=>stop transitions.
> */
> diff --git a/drivers/net/wireless/ath/ath12k/debug.c b/drivers/net/wireless/ath/ath12k/debug.c
> index fe5a732ba9ec..c5c8c7624cdb 100644
> --- a/drivers/net/wireless/ath/ath12k/debug.c
> +++ b/drivers/net/wireless/ath/ath12k/debug.c
> @@ -36,7 +36,7 @@ void ath12k_err(struct ath12k_base *ab, const char *fmt, ...)
> va_end(args);
> }
>
> -void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...)
> +void __ath12k_warn(struct device *dev, const char *fmt, ...)
> {
> struct va_format vaf = {
> .fmt = fmt,
> @@ -45,7 +45,7 @@ void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...)
>
> va_start(args, fmt);
> vaf.va = &args;
> - dev_warn_ratelimited(ab->dev, "%pV", &vaf);
> + dev_warn_ratelimited(dev, "%pV", &vaf);
> /* TODO: Trace the log */
> va_end(args);
> }
> diff --git a/drivers/net/wireless/ath/ath12k/debug.h b/drivers/net/wireless/ath/ath12k/debug.h
> index f7005917362c..90e801136bc6 100644
> --- a/drivers/net/wireless/ath/ath12k/debug.h
> +++ b/drivers/net/wireless/ath/ath12k/debug.h
> @@ -31,7 +31,10 @@ enum ath12k_debug_mask {
>
> __printf(2, 3) void ath12k_info(struct ath12k_base *ab, const char *fmt, ...);
> __printf(2, 3) void ath12k_err(struct ath12k_base *ab, const char *fmt, ...);
> -__printf(2, 3) void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...);
> +__printf(2, 3) void __ath12k_warn(struct device *dev, const char *fmt, ...);
> +
> +#define ath12k_warn(ab, fmt, ...) __ath12k_warn((ab)->dev, fmt, ##__VA_ARGS__)
> +#define ath12k_hw_warn(ah, fmt, ...) __ath12k_warn((ah)->dev, fmt, ##__VA_ARGS__)
for consistency should we do this for the other log levels as well?
are there places where we currently retrieve ab just for logging where we
already have ah, and hence could avoid the extra dereference?
>
> extern unsigned int ath12k_debug_mask;
>
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 3de6d605cd74..19c445cf52f1 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -10193,6 +10193,8 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
> goto err;
> }
>
> + ah->dev = ab->dev;
> +
> ab->ah[i] = ah;
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/8] wifi: ath12k: introduce ath12k_hw_warn()
2024-10-23 15:38 ` Jeff Johnson
@ 2024-10-29 15:41 ` Kalle Valo
0 siblings, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-29 15:41 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:30 AM, Kalle Valo wrote:
>> From: Kalle Valo <quic_kvalo@quicinc.com>
>>
>> In the following patch we need to use ath12k_warn() but don't easily have
>> access to struct ath12k_base (ab) but do have access to struct ath12k_hw (ah).
>> So add a new warning helper ath12_hw_warn() which takes the latter but the log
>> output is still identical but uses the struct device pointer stored to struct
>> ath12k_hw.
>>
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
>> drivers/net/wireless/ath/ath12k/core.h | 2 ++
>> drivers/net/wireless/ath/ath12k/debug.c | 4 ++--
>
> Qualcomm Innovation Center copyright missing 2024
Will fix.
>> --- a/drivers/net/wireless/ath/ath12k/debug.h
>> +++ b/drivers/net/wireless/ath/ath12k/debug.h
>> @@ -31,7 +31,10 @@ enum ath12k_debug_mask {
>>
>> __printf(2, 3) void ath12k_info(struct ath12k_base *ab, const char *fmt, ...);
>> __printf(2, 3) void ath12k_err(struct ath12k_base *ab, const char *fmt, ...);
>> -__printf(2, 3) void ath12k_warn(struct ath12k_base *ab, const char *fmt, ...);
>> +__printf(2, 3) void __ath12k_warn(struct device *dev, const char *fmt, ...);
>> +
>> +#define ath12k_warn(ab, fmt, ...) __ath12k_warn((ab)->dev, fmt, ##__VA_ARGS__)
>> +#define ath12k_hw_warn(ah, fmt, ...) __ath12k_warn((ah)->dev, fmt, ##__VA_ARGS__)
>
> for consistency should we do this for the other log levels as well?
>
> are there places where we currently retrieve ab just for logging where we
> already have ah, and hence could avoid the extra dereference?
That's a good idea but IMHO the cleanup can wait after MLO has landed.
Right now there should not be many places where we have to use the
ath12k_hw_warn() variant.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 5/8] wifi: ath12k: Add helpers for multi link peer creation and deletion
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
` (3 preceding siblings ...)
2024-10-23 13:30 ` [PATCH 4/8] wifi: ath12k: introduce ath12k_hw_warn() Kalle Valo
@ 2024-10-23 13:30 ` Kalle Valo
2024-10-23 15:43 ` Jeff Johnson
2024-10-23 13:30 ` [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command Kalle Valo
` (2 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:30 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Sriram R <quic_srirrama@quicinc.com>
Add helper functions for multi link peer addition and deletion. And add address
validation to ensure we are not creating link peers (belonging to different
clients) with same MLD address. To aid in this validation for faster lookup,
add a new list of ML peers to struct ath12k_hw::ml_peers and use the same for
parsing for the above address validation use cases.
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: Harshitha Prem <quic_hprem@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
drivers/net/wireless/ath/ath12k/core.h | 13 ++++
drivers/net/wireless/ath/ath12k/mac.c | 2 +
drivers/net/wireless/ath/ath12k/peer.c | 99 ++++++++++++++++++++++++++
drivers/net/wireless/ath/ath12k/peer.h | 8 +++
4 files changed, 122 insertions(+)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 9c4e5fae8930..0a0c1a1594f2 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -63,6 +63,13 @@
#define ATH12K_RECONFIGURE_TIMEOUT_HZ (10 * HZ)
#define ATH12K_RECOVER_START_TIMEOUT_HZ (20 * HZ)
+#define ATH12K_MAX_SOCS 3
+#define ATH12K_INVALID_GROUP_ID 0xFF
+#define ATH12K_INVALID_DEVICE_ID 0xFF
+
+#define ATH12K_MAX_MLO_PEERS 256
+#define ATH12K_MLO_PEER_ID_INVALID 0xFFFF
+
enum ath12k_bdf_search {
ATH12K_BDF_SEARCH_DEFAULT,
ATH12K_BDF_SEARCH_BUS_AND_BOARD,
@@ -488,6 +495,7 @@ struct ath12k_sta {
struct ath12k_link_sta __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS];
/* indicates bitmap of link sta created in FW */
u16 links_map;
+ u16 ml_peer_id;
};
#define ATH12K_MIN_5G_FREQ 4150
@@ -696,6 +704,11 @@ struct ath12k_hw {
u8 num_radio;
+ DECLARE_BITMAP(free_ml_peer_id_map, ATH12K_MAX_MLO_PEERS);
+
+ /* protected by wiphy_lock() */
+ struct list_head ml_peers;
+
/* Keep last */
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 19c445cf52f1..019a1a6c6777 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -5103,6 +5103,7 @@ static int ath12k_mac_handle_link_sta_state(struct ieee80211_hw *hw,
ahsta->links_map = BIT(arsta->link_id);
arsta->ahsta = ahsta;
arsta->arvif = arvif;
+ ether_addr_copy(arsta->addr, sta->addr);
wiphy_work_init(&arsta->update_wk, ath12k_sta_rc_update_wk);
synchronize_rcu();
@@ -10124,6 +10125,7 @@ static struct ath12k_hw *ath12k_mac_hw_allocate(struct ath12k_base *ab,
ah->num_radio = num_pdev_map;
mutex_init(&ah->hw_mutex);
+ INIT_LIST_HEAD(&ah->ml_peers);
for (i = 0; i < num_pdev_map; i++) {
ab = pdev_map[i].ab;
diff --git a/drivers/net/wireless/ath/ath12k/peer.c b/drivers/net/wireless/ath/ath12k/peer.c
index 7a62665b8af9..39b371c7433c 100644
--- a/drivers/net/wireless/ath/ath12k/peer.c
+++ b/drivers/net/wireless/ath/ath12k/peer.c
@@ -8,6 +8,23 @@
#include "peer.h"
#include "debug.h"
+static struct ath12k_ml_peer *ath12k_ml_peer_find(struct ath12k_hw *ah,
+ const u8 *addr)
+{
+ struct ath12k_ml_peer *ml_peer;
+
+ lockdep_assert_wiphy(ah->hw->wiphy);
+
+ list_for_each_entry(ml_peer, &ah->ml_peers, list) {
+ if (!ether_addr_equal(ml_peer->addr, addr))
+ continue;
+
+ return ml_peer;
+ }
+
+ return NULL;
+}
+
struct ath12k_peer *ath12k_peer_find(struct ath12k_base *ab, int vdev_id,
const u8 *addr)
{
@@ -341,3 +358,85 @@ int ath12k_peer_create(struct ath12k *ar, struct ath12k_link_vif *arvif,
return 0;
}
+
+static u16 ath12k_mac_alloc_ml_peer_id(struct ath12k_hw *ah)
+{
+ u16 ml_peer_id;
+
+ lockdep_assert_wiphy(ah->hw->wiphy);
+
+ for (ml_peer_id = 0; ml_peer_id < ATH12K_MAX_MLO_PEERS; ml_peer_id++) {
+ if (test_bit(ml_peer_id, ah->free_ml_peer_id_map))
+ continue;
+
+ set_bit(ml_peer_id, ah->free_ml_peer_id_map);
+ break;
+ }
+
+ if (ml_peer_id == ATH12K_MAX_MLO_PEERS)
+ ml_peer_id = ATH12K_MLO_PEER_ID_INVALID;
+
+ return ml_peer_id;
+}
+
+int ath12k_peer_mlo_create(struct ath12k_hw *ah, struct ieee80211_sta *sta)
+{
+ struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
+ struct ath12k_ml_peer *ml_peer;
+
+ lockdep_assert_wiphy(ah->hw->wiphy);
+
+ if (!sta->mlo)
+ return -EINVAL;
+
+ ml_peer = ath12k_ml_peer_find(ah, sta->addr);
+ if (ml_peer) {
+ ath12k_hw_warn(ah, "ML peer (%d) exists already, unable to add new entry for %pM",
+ ml_peer->id, sta->addr);
+ return -EEXIST;
+ }
+
+ ml_peer = kzalloc(sizeof(*ml_peer), GFP_ATOMIC);
+ if (!ml_peer)
+ return -ENOMEM;
+
+ ahsta->ml_peer_id = ath12k_mac_alloc_ml_peer_id(ah);
+
+ if (ahsta->ml_peer_id == ATH12K_MLO_PEER_ID_INVALID) {
+ ath12k_hw_warn(ah, "unable to allocate ML peer id for sta %pM",
+ sta->addr);
+ kfree(ml_peer);
+ return -ENOMEM;
+ }
+
+ ether_addr_copy(ml_peer->addr, sta->addr);
+ ml_peer->id = ahsta->ml_peer_id;
+ list_add(&ml_peer->list, &ah->ml_peers);
+
+ return 0;
+}
+
+int ath12k_peer_mlo_delete(struct ath12k_hw *ah, struct ieee80211_sta *sta)
+{
+ struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
+ struct ath12k_ml_peer *ml_peer;
+
+ lockdep_assert_wiphy(ah->hw->wiphy);
+
+ if (!sta->mlo)
+ return -EINVAL;
+
+ clear_bit(ahsta->ml_peer_id, ah->free_ml_peer_id_map);
+ ahsta->ml_peer_id = ATH12K_MLO_PEER_ID_INVALID;
+
+ ml_peer = ath12k_ml_peer_find(ah, sta->addr);
+ if (!ml_peer) {
+ ath12k_hw_warn(ah, "ML peer for %pM not found", sta->addr);
+ return -EINVAL;
+ }
+
+ list_del(&ml_peer->list);
+ kfree(ml_peer);
+
+ return 0;
+}
diff --git a/drivers/net/wireless/ath/ath12k/peer.h b/drivers/net/wireless/ath/ath12k/peer.h
index b955f0cdf598..b91bb2106b76 100644
--- a/drivers/net/wireless/ath/ath12k/peer.h
+++ b/drivers/net/wireless/ath/ath12k/peer.h
@@ -49,6 +49,12 @@ struct ath12k_peer {
bool dp_setup_done;
};
+struct ath12k_ml_peer {
+ struct list_head list;
+ u8 addr[ETH_ALEN];
+ u16 id;
+};
+
void ath12k_peer_unmap_event(struct ath12k_base *ab, u16 peer_id);
void ath12k_peer_map_event(struct ath12k_base *ab, u8 vdev_id, u16 peer_id,
u8 *mac_addr, u16 ast_hash, u16 hw_peer_id);
@@ -66,5 +72,7 @@ int ath12k_wait_for_peer_delete_done(struct ath12k *ar, u32 vdev_id,
const u8 *addr);
bool ath12k_peer_exist_by_vdev_id(struct ath12k_base *ab, int vdev_id);
struct ath12k_peer *ath12k_peer_find_by_ast(struct ath12k_base *ab, int ast_hash);
+int ath12k_peer_mlo_create(struct ath12k_hw *ah, struct ieee80211_sta *sta);
+int ath12k_peer_mlo_delete(struct ath12k_hw *ah, struct ieee80211_sta *sta);
#endif /* _PEER_H_ */
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 5/8] wifi: ath12k: Add helpers for multi link peer creation and deletion
2024-10-23 13:30 ` [PATCH 5/8] wifi: ath12k: Add helpers for multi link peer creation and deletion Kalle Valo
@ 2024-10-23 15:43 ` Jeff Johnson
2024-10-26 9:09 ` Kalle Valo
0 siblings, 1 reply; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 15:43 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:30 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
>
> Add helper functions for multi link peer addition and deletion. And add address
> validation to ensure we are not creating link peers (belonging to different
> clients) with same MLD address. To aid in this validation for faster lookup,
> add a new list of ML peers to struct ath12k_hw::ml_peers and use the same for
> parsing for the above address validation use cases.
>
> 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: Harshitha Prem <quic_hprem@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
...
> +int ath12k_peer_mlo_create(struct ath12k_hw *ah, struct ieee80211_sta *sta)
> +{
> + struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
> + struct ath12k_ml_peer *ml_peer;
> +
> + lockdep_assert_wiphy(ah->hw->wiphy);
> +
> + if (!sta->mlo)
> + return -EINVAL;
> +
> + ml_peer = ath12k_ml_peer_find(ah, sta->addr);
> + if (ml_peer) {
> + ath12k_hw_warn(ah, "ML peer (%d) exists already, unable to add new entry for %pM",
The Linux coding style says:
Printing numbers in parentheses (%d) adds no value and should be avoided.
> + ml_peer->id, sta->addr);
> + return -EEXIST;
> + }
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/8] wifi: ath12k: Add helpers for multi link peer creation and deletion
2024-10-23 15:43 ` Jeff Johnson
@ 2024-10-26 9:09 ` Kalle Valo
0 siblings, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-26 9:09 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:30 AM, Kalle Valo wrote:
>> From: Sriram R <quic_srirrama@quicinc.com>
>>
>> Add helper functions for multi link peer addition and deletion. And add address
>> validation to ensure we are not creating link peers (belonging to different
>> clients) with same MLD address. To aid in this validation for faster lookup,
>> add a new list of ML peers to struct ath12k_hw::ml_peers and use the same for
>> parsing for the above address validation use cases.
>>
>> 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: Harshitha Prem <quic_hprem@quicinc.com>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
> ...
>> +int ath12k_peer_mlo_create(struct ath12k_hw *ah, struct ieee80211_sta *sta)
>> +{
>> + struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
>> + struct ath12k_ml_peer *ml_peer;
>> +
>> + lockdep_assert_wiphy(ah->hw->wiphy);
>> +
>> + if (!sta->mlo)
>> + return -EINVAL;
>> +
>> + ml_peer = ath12k_ml_peer_find(ah, sta->addr);
>> + if (ml_peer) {
>> + ath12k_hw_warn(ah, "ML peer (%d) exists already, unable to add new entry for %pM",
>
> The Linux coding style says:
> Printing numbers in parentheses (%d) adds no value and should be avoided.
Good point, I'll remove them.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
` (4 preceding siblings ...)
2024-10-23 13:30 ` [PATCH 5/8] wifi: ath12k: Add helpers for multi link peer creation and deletion Kalle Valo
@ 2024-10-23 13:30 ` Kalle Valo
2024-10-23 15:54 ` Jeff Johnson
2024-10-24 1:22 ` Ping-Ke Shih
2024-10-23 13:30 ` [PATCH 7/8] wifi: ath12k: add helper to find multi-link station Kalle Valo
2024-10-23 13:30 ` [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support Kalle Valo
7 siblings, 2 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:30 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Sriram R <quic_srirrama@quicinc.com>
Driver should indicate to firmware whether a peer is multi-link or not in peer
create command using multi-link flag. Add changes to support
WMI_TAG_MLO_PEER_CREATE_PARAMS in WMI_PEER_CREATE_CMDID.
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: Harshitha Prem <quic_hprem@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
drivers/net/wireless/ath/ath12k/mac.c | 5 +++--
drivers/net/wireless/ath/ath12k/wmi.c | 27 +++++++++++++++++++++++----
drivers/net/wireless/ath/ath12k/wmi.h | 6 ++++++
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 019a1a6c6777..b628bc2fd0f5 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -4981,8 +4981,9 @@ static int ath12k_mac_station_add(struct ath12k *ar,
}
peer_param.vdev_id = arvif->vdev_id;
- peer_param.peer_addr = sta->addr;
+ peer_param.peer_addr = arsta->addr;
peer_param.peer_type = WMI_PEER_TYPE_DEFAULT;
+ peer_param.ml_enabled = sta->mlo;
ret = ath12k_peer_create(ar, arvif, sta, &peer_param);
if (ret) {
@@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
struct ath12k_vif *ahvif = arvif->ahvif;
struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
struct ath12k_wmi_vdev_create_arg vdev_arg = {0};
- struct ath12k_wmi_peer_create_arg peer_param;
+ struct ath12k_wmi_peer_create_arg peer_param = {0};
struct ieee80211_bss_conf *link_conf;
u32 param_id, param_value;
u16 nss;
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index e089b58bbea1..0583d832fac7 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -1230,9 +1230,14 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
struct ath12k_wmi_pdev *wmi = ar->wmi;
struct wmi_peer_create_cmd *cmd;
struct sk_buff *skb;
- int ret;
+ int ret, len;
+ struct wmi_peer_create_mlo_params *ml_param;
+ void *ptr;
+ struct wmi_tlv *tlv;
- skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
+ len = sizeof(*cmd) + TLV_HDR_SIZE + sizeof(*ml_param);
+
+ skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
if (!skb)
return -ENOMEM;
@@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
cmd->peer_type = cpu_to_le32(arg->peer_type);
cmd->vdev_id = cpu_to_le32(arg->vdev_id);
+ ptr = skb->data + sizeof(*cmd);
+ tlv = ptr;
+ tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
+ sizeof(*ml_param));
+ ptr += TLV_HDR_SIZE;
+ ml_param = ptr;
+ ml_param->tlv_header =
+ ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
+ sizeof(*ml_param));
+ if (arg->ml_enabled)
+ ml_param->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
+
+ ptr += sizeof(*ml_param);
+
ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
- "WMI peer create vdev_id %d peer_addr %pM\n",
- arg->vdev_id, arg->peer_addr);
+ "WMI peer create vdev_id %d peer_addr %pM ml_flags 0x%x\n",
+ arg->vdev_id, arg->peer_addr, ml_param->flags);
ret = ath12k_wmi_cmd_send(wmi, skb, WMI_PEER_CREATE_CMDID);
if (ret) {
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index 33b9643644c6..07bd275608bf 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -3026,6 +3026,12 @@ struct ath12k_wmi_peer_create_arg {
const u8 *peer_addr;
u32 peer_type;
u32 vdev_id;
+ bool ml_enabled;
+};
+
+struct wmi_peer_create_mlo_params {
+ __le32 tlv_header;
+ __le32 flags;
};
struct ath12k_wmi_pdev_set_regdomain_arg {
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-23 13:30 ` [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command Kalle Valo
@ 2024-10-23 15:54 ` Jeff Johnson
2024-10-29 15:54 ` Kalle Valo
2024-10-24 1:22 ` Ping-Ke Shih
1 sibling, 1 reply; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 15:54 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:30 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
>
> Driver should indicate to firmware whether a peer is multi-link or not in peer
> create command using multi-link flag. Add changes to support
> WMI_TAG_MLO_PEER_CREATE_PARAMS in WMI_PEER_CREATE_CMDID.
>
> 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: Harshitha Prem <quic_hprem@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/mac.c | 5 +++--
> drivers/net/wireless/ath/ath12k/wmi.c | 27 +++++++++++++++++++++++----
> drivers/net/wireless/ath/ath12k/wmi.h | 6 ++++++
> 3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 019a1a6c6777..b628bc2fd0f5 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -4981,8 +4981,9 @@ static int ath12k_mac_station_add(struct ath12k *ar,
> }
>
> peer_param.vdev_id = arvif->vdev_id;
> - peer_param.peer_addr = sta->addr;
> + peer_param.peer_addr = arsta->addr;
> peer_param.peer_type = WMI_PEER_TYPE_DEFAULT;
> + peer_param.ml_enabled = sta->mlo;
>
> ret = ath12k_peer_create(ar, arvif, sta, &peer_param);
> if (ret) {
> @@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
> struct ath12k_vif *ahvif = arvif->ahvif;
> struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
> struct ath12k_wmi_vdev_create_arg vdev_arg = {0};
> - struct ath12k_wmi_peer_create_arg peer_param;
> + struct ath12k_wmi_peer_create_arg peer_param = {0};
> struct ieee80211_bss_conf *link_conf;
> u32 param_id, param_value;
> u16 nss;
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index e089b58bbea1..0583d832fac7 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -1230,9 +1230,14 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
> struct ath12k_wmi_pdev *wmi = ar->wmi;
> struct wmi_peer_create_cmd *cmd;
> struct sk_buff *skb;
> - int ret;
> + int ret, len;
> + struct wmi_peer_create_mlo_params *ml_param;
> + void *ptr;
> + struct wmi_tlv *tlv;
>
> - skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
> + len = sizeof(*cmd) + TLV_HDR_SIZE + sizeof(*ml_param);
> +
> + skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
> if (!skb)
> return -ENOMEM;
>
> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
> cmd->peer_type = cpu_to_le32(arg->peer_type);
> cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>
> + ptr = skb->data + sizeof(*cmd);
> + tlv = ptr;
> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
> + sizeof(*ml_param));
using the same TLV size both here and for the TLV that follows doesn't seem
logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
> + ptr += TLV_HDR_SIZE;
> + ml_param = ptr;
> + ml_param->tlv_header =
> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
> + sizeof(*ml_param));
> + if (arg->ml_enabled)
> + ml_param->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
> +
> + ptr += sizeof(*ml_param);
> +
> ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
> - "WMI peer create vdev_id %d peer_addr %pM\n",
> - arg->vdev_id, arg->peer_addr);
> + "WMI peer create vdev_id %d peer_addr %pM ml_flags 0x%x\n",
> + arg->vdev_id, arg->peer_addr, ml_param->flags);
>
> ret = ath12k_wmi_cmd_send(wmi, skb, WMI_PEER_CREATE_CMDID);
> if (ret) {
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
> index 33b9643644c6..07bd275608bf 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.h
> +++ b/drivers/net/wireless/ath/ath12k/wmi.h
> @@ -3026,6 +3026,12 @@ struct ath12k_wmi_peer_create_arg {
> const u8 *peer_addr;
> u32 peer_type;
> u32 vdev_id;
> + bool ml_enabled;
> +};
> +
> +struct wmi_peer_create_mlo_params {
> + __le32 tlv_header;
> + __le32 flags;
> };
>
> struct ath12k_wmi_pdev_set_regdomain_arg {
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-23 15:54 ` Jeff Johnson
@ 2024-10-29 15:54 ` Kalle Valo
2024-10-29 16:01 ` Jeff Johnson
0 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-29 15:54 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>> cmd->peer_type = cpu_to_le32(arg->peer_type);
>> cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>
>> + ptr = skb->data + sizeof(*cmd);
>> + tlv = ptr;
>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>> + sizeof(*ml_param));
>
> using the same TLV size both here and for the TLV that follows doesn't seem
> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
I have forgotten the details of WMI voodoo so I can't really comment
right now :)
>> + ptr += TLV_HDR_SIZE;
>> + ml_param = ptr;
>> + ml_param->tlv_header =
>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>> + sizeof(*ml_param));
But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
reduces the header size:
static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
{
return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
}
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-29 15:54 ` Kalle Valo
@ 2024-10-29 16:01 ` Jeff Johnson
2024-10-29 16:04 ` Jeff Johnson
2024-11-01 14:06 ` Kalle Valo
0 siblings, 2 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-29 16:01 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath12k, linux-wireless
On 10/29/2024 8:54 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>> cmd->peer_type = cpu_to_le32(arg->peer_type);
>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>
>>> + ptr = skb->data + sizeof(*cmd);
>>> + tlv = ptr;
>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>> + sizeof(*ml_param));
>>
>> using the same TLV size both here and for the TLV that follows doesn't seem
>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
>
> I have forgotten the details of WMI voodoo so I can't really comment
> right now :)
>
>>> + ptr += TLV_HDR_SIZE;
>>> + ml_param = ptr;
>>> + ml_param->tlv_header =
>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>> + sizeof(*ml_param));
>
> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
> reduces the header size:
>
> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
> {
> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
> }
>
Yes, I missed that since that is evil to use the _cmd_ TLV function on
something that isn't the command TLV.
Please fix to use the standard function and subtract the thv header size from
the length param
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-29 16:01 ` Jeff Johnson
@ 2024-10-29 16:04 ` Jeff Johnson
2024-11-01 14:06 ` Kalle Valo
1 sibling, 0 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-29 16:04 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath12k, linux-wireless
On 10/29/2024 9:01 AM, Jeff Johnson wrote:
> On 10/29/2024 8:54 AM, Kalle Valo wrote:
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>
>>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>>> cmd->peer_type = cpu_to_le32(arg->peer_type);
>>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>>
>>>> + ptr = skb->data + sizeof(*cmd);
>>>> + tlv = ptr;
>>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>>> + sizeof(*ml_param));
>>>
>>> using the same TLV size both here and for the TLV that follows doesn't seem
>>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
>>
>> I have forgotten the details of WMI voodoo so I can't really comment
>> right now :)
>>
>>>> + ptr += TLV_HDR_SIZE;
>>>> + ml_param = ptr;
>>>> + ml_param->tlv_header =
>>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>>> + sizeof(*ml_param));
>>
>> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
>> reduces the header size:
>>
>> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
>> {
>> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
>> }
>>
>
> Yes, I missed that since that is evil to use the _cmd_ TLV function on
> something that isn't the command TLV.
>
> Please fix to use the standard function and subtract the thv header size from
the *tlv* header
> the length param
also note there is the existing inconsistency that some WMI params structs
include the tlv header and some do not. IMO that lack of consistency will also
impact maintainability.
again something for the TODO list.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-29 16:01 ` Jeff Johnson
2024-10-29 16:04 ` Jeff Johnson
@ 2024-11-01 14:06 ` Kalle Valo
2024-11-01 15:37 ` Jeff Johnson
1 sibling, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-11-01 14:06 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/29/2024 8:54 AM, Kalle Valo wrote:
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>
>>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>>> cmd->peer_type = cpu_to_le32(arg->peer_type);
>>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>>
>>>> + ptr = skb->data + sizeof(*cmd);
>>>> + tlv = ptr;
>>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>>> + sizeof(*ml_param));
>>>
>>> using the same TLV size both here and for the TLV that follows doesn't seem
>>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
So I assume you are referring to this:
tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
sizeof(*ml_param));
ptr += TLV_HDR_SIZE;
ml_param = ptr;
ml_param->tlv_header =
ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
sizeof(*ml_param));
I have never figured out how WMI_TAG_ARRAY_STRUCT is supposed to work
but I see a similar pattern also in ath12k_wmi_wow_add_pattern(). Any
ideas?
>>>> + ptr += TLV_HDR_SIZE;
>>>> + ml_param = ptr;
>>>> + ml_param->tlv_header =
>>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>>> + sizeof(*ml_param));
>>
>> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
>> reduces the header size:
>>
>> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
>> {
>> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
>> }
>>
>
> Yes, I missed that since that is evil to use the _cmd_ TLV function on
> something that isn't the command TLV.
Ok, so you are saying that we should have a identical function but with
name ath12k_wmi_tlv_param_hdr() or similar? That makes sense but I think
that's separate cleanup as ath12k_wmi_tlv_cmd_hdr() is already used with
several WMI params, like in ath12k_wmi_wow_add_pattern().
> Please fix to use the standard function and subtract the thv header size from
> the length param
I'm not a fan of manually subtracting lengths, as then it's easy to miss
something. I would prefer to have functions for handling the length
calculation, like ath12k_wmi_tlv_cmd_hdr() ath12k_wmi_tlv_param_hdr().
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-11-01 14:06 ` Kalle Valo
@ 2024-11-01 15:37 ` Jeff Johnson
0 siblings, 0 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-11-01 15:37 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath12k, linux-wireless
On 11/1/2024 7:06 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
>> On 10/29/2024 8:54 AM, Kalle Valo wrote:
>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>>
>>>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>>>> cmd->peer_type = cpu_to_le32(arg->peer_type);
>>>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>>>
>>>>> + ptr = skb->data + sizeof(*cmd);
>>>>> + tlv = ptr;
>>>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>>>> + sizeof(*ml_param));
>>>>
>>>> using the same TLV size both here and for the TLV that follows doesn't seem
>>>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
>
> So I assume you are referring to this:
>
> tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
> sizeof(*ml_param));
> ptr += TLV_HDR_SIZE;
> ml_param = ptr;
> ml_param->tlv_header =
> ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
> sizeof(*ml_param));
>
> I have never figured out how WMI_TAG_ARRAY_STRUCT is supposed to work
> but I see a similar pattern also in ath12k_wmi_wow_add_pattern(). Any
> ideas?
>
>>>>> + ptr += TLV_HDR_SIZE;
>>>>> + ml_param = ptr;
>>>>> + ml_param->tlv_header =
>>>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>>>> + sizeof(*ml_param));
>>>
>>> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
>>> reduces the header size:
>>>
>>> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
>>> {
>>> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
>>> }
>>>
>>
>> Yes, I missed that since that is evil to use the _cmd_ TLV function on
>> something that isn't the command TLV.
>
> Ok, so you are saying that we should have a identical function but with
> name ath12k_wmi_tlv_param_hdr() or similar? That makes sense but I think
> that's separate cleanup as ath12k_wmi_tlv_cmd_hdr() is already used with
> several WMI params, like in ath12k_wmi_wow_add_pattern().
>
>> Please fix to use the standard function and subtract the thv header size from
>> the length param
>
> I'm not a fan of manually subtracting lengths, as then it's easy to miss
> something. I would prefer to have functions for handling the length
> calculation, like ath12k_wmi_tlv_cmd_hdr() ath12k_wmi_tlv_param_hdr().
>
Whether it is manually subtracting lengths or using tlv_hdr vs tlv_param_hdr,
both are easy to miss due to the lack of consistency in the params struct
definitions. IMO the only way to avoid this is to consistently either always
have the tlv header or to never have the tlv header in the params structs.
This lack of consistency is the real underlying issue since whether or not you
have the tlv header in the params struct is what dictates whether or not you
need to account for the header when filling the TLV length, as well as if you
have to separately account for it when you are determining the buffer
allocation size and when you are populating the TLV header in the buffer.
But the current code actually allocates and fills the data as firmware expects
it for this command, so let's keep the current code for now, and we can
discuss if it is appropriate to later update to achieve the consistency mentioned.
/jeff
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command
2024-10-23 13:30 ` [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command Kalle Valo
2024-10-23 15:54 ` Jeff Johnson
@ 2024-10-24 1:22 ` Ping-Ke Shih
1 sibling, 0 replies; 38+ messages in thread
From: Ping-Ke Shih @ 2024-10-24 1:22 UTC (permalink / raw)
To: Kalle Valo, ath12k@lists.infradead.org; +Cc: linux-wireless@vger.kernel.org
Kalle Valo <kvalo@kernel.org> wrote:
> @@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
> struct ath12k_vif *ahvif = arvif->ahvif;
> struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
> struct ath12k_wmi_vdev_create_arg vdev_arg = {0};
> - struct ath12k_wmi_peer_create_arg peer_param;
> + struct ath12k_wmi_peer_create_arg peer_param = {0};
Using '= {}' as initializer would be better, no matter first field of struct
will be changed in the future.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 7/8] wifi: ath12k: add helper to find multi-link station
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
` (5 preceding siblings ...)
2024-10-23 13:30 ` [PATCH 6/8] wifi: ath12k: add multi-link flag in peer create command Kalle Valo
@ 2024-10-23 13:30 ` Kalle Valo
2024-10-23 16:01 ` Jeff Johnson
2024-10-23 13:30 ` [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support Kalle Valo
7 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:30 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Sriram R <quic_srirrama@quicinc.com>
Multi-link stations are identified in driver using the multi-link
peer id. Add a helper to find multi-link station using the ML
peer id.
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: Harshitha Prem <quic_hprem@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
drivers/net/wireless/ath/ath12k/dp.h | 2 ++
drivers/net/wireless/ath/ath12k/peer.c | 17 +++++++++++++++++
drivers/net/wireless/ath/ath12k/peer.h | 2 ++
3 files changed, 21 insertions(+)
diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
index 2e05fc19410e..66b60f772efb 100644
--- a/drivers/net/wireless/ath/ath12k/dp.h
+++ b/drivers/net/wireless/ath/ath12k/dp.h
@@ -1796,6 +1796,8 @@ static inline void ath12k_dp_get_mac_addr(u32 addr_l32, u16 addr_h16, u8 *addr)
memcpy(addr + 4, &addr_h16, ETH_ALEN - 4);
}
+#define ATH12K_ML_PEER_ID_VALID BIT(13)
+
int ath12k_dp_service_srng(struct ath12k_base *ab,
struct ath12k_ext_irq_grp *irq_grp,
int budget);
diff --git a/drivers/net/wireless/ath/ath12k/peer.c b/drivers/net/wireless/ath/ath12k/peer.c
index 39b371c7433c..c7eb60723d83 100644
--- a/drivers/net/wireless/ath/ath12k/peer.c
+++ b/drivers/net/wireless/ath/ath12k/peer.c
@@ -80,6 +80,20 @@ struct ath12k_peer *ath12k_peer_find_by_addr(struct ath12k_base *ab,
return NULL;
}
+static struct ath12k_peer *ath12k_peer_find_by_ml_id(struct ath12k_base *ab,
+ int ml_peer_id)
+{
+ struct ath12k_peer *peer;
+
+ lockdep_assert_held(&ab->base_lock);
+
+ list_for_each_entry(peer, &ab->peers, list)
+ if (ml_peer_id == peer->ml_peer_id)
+ return peer;
+
+ return NULL;
+}
+
struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
int peer_id)
{
@@ -87,6 +101,9 @@ struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
lockdep_assert_held(&ab->base_lock);
+ if (peer_id & ATH12K_ML_PEER_ID_VALID)
+ return ath12k_peer_find_by_ml_id(ab, peer_id);
+
list_for_each_entry(peer, &ab->peers, list)
if (peer_id == peer->peer_id)
return peer;
diff --git a/drivers/net/wireless/ath/ath12k/peer.h b/drivers/net/wireless/ath/ath12k/peer.h
index b91bb2106b76..5b718fc5c795 100644
--- a/drivers/net/wireless/ath/ath12k/peer.h
+++ b/drivers/net/wireless/ath/ath12k/peer.h
@@ -47,6 +47,8 @@ struct ath12k_peer {
/* protected by ab->data_lock */
bool dp_setup_done;
+
+ u16 ml_peer_id;
};
struct ath12k_ml_peer {
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 7/8] wifi: ath12k: add helper to find multi-link station
2024-10-23 13:30 ` [PATCH 7/8] wifi: ath12k: add helper to find multi-link station Kalle Valo
@ 2024-10-23 16:01 ` Jeff Johnson
2024-10-29 16:02 ` Kalle Valo
2024-11-01 14:33 ` Kalle Valo
0 siblings, 2 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 16:01 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:30 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
>
> Multi-link stations are identified in driver using the multi-link
> peer id. Add a helper to find multi-link station using the ML
> peer id.
>
> 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: Harshitha Prem <quic_hprem@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/dp.h | 2 ++
> drivers/net/wireless/ath/ath12k/peer.c | 17 +++++++++++++++++
> drivers/net/wireless/ath/ath12k/peer.h | 2 ++
> 3 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
> index 2e05fc19410e..66b60f772efb 100644
> --- a/drivers/net/wireless/ath/ath12k/dp.h
> +++ b/drivers/net/wireless/ath/ath12k/dp.h
> @@ -1796,6 +1796,8 @@ static inline void ath12k_dp_get_mac_addr(u32 addr_l32, u16 addr_h16, u8 *addr)
> memcpy(addr + 4, &addr_h16, ETH_ALEN - 4);
> }
>
> +#define ATH12K_ML_PEER_ID_VALID BIT(13)
> +
this seems to be randomly placed without any context
> int ath12k_dp_service_srng(struct ath12k_base *ab,
> struct ath12k_ext_irq_grp *irq_grp,
> int budget);
> diff --git a/drivers/net/wireless/ath/ath12k/peer.c b/drivers/net/wireless/ath/ath12k/peer.c
> index 39b371c7433c..c7eb60723d83 100644
> --- a/drivers/net/wireless/ath/ath12k/peer.c
> +++ b/drivers/net/wireless/ath/ath12k/peer.c
> @@ -80,6 +80,20 @@ struct ath12k_peer *ath12k_peer_find_by_addr(struct ath12k_base *ab,
> return NULL;
> }
>
> +static struct ath12k_peer *ath12k_peer_find_by_ml_id(struct ath12k_base *ab,
> + int ml_peer_id)
> +{
> + struct ath12k_peer *peer;
> +
> + lockdep_assert_held(&ab->base_lock);
> +
> + list_for_each_entry(peer, &ab->peers, list)
> + if (ml_peer_id == peer->ml_peer_id)
> + return peer;
> +
> + return NULL;
> +}
> +
> struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
> int peer_id)
> {
> @@ -87,6 +101,9 @@ struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
>
> lockdep_assert_held(&ab->base_lock);
>
> + if (peer_id & ATH12K_ML_PEER_ID_VALID)
where is code that sets the bit?
does other code elsewhere need to mask this bit off to have the "true" peer_id?
the commit text for this patch seems to need a lot more description
> + return ath12k_peer_find_by_ml_id(ab, peer_id);
> +
> list_for_each_entry(peer, &ab->peers, list)
> if (peer_id == peer->peer_id)
> return peer;
> diff --git a/drivers/net/wireless/ath/ath12k/peer.h b/drivers/net/wireless/ath/ath12k/peer.h
> index b91bb2106b76..5b718fc5c795 100644
> --- a/drivers/net/wireless/ath/ath12k/peer.h
> +++ b/drivers/net/wireless/ath/ath12k/peer.h
> @@ -47,6 +47,8 @@ struct ath12k_peer {
>
> /* protected by ab->data_lock */
> bool dp_setup_done;
> +
> + u16 ml_peer_id;
> };
>
> struct ath12k_ml_peer {
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 7/8] wifi: ath12k: add helper to find multi-link station
2024-10-23 16:01 ` Jeff Johnson
@ 2024-10-29 16:02 ` Kalle Valo
2024-11-01 14:33 ` Kalle Valo
1 sibling, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-10-29 16:02 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:30 AM, Kalle Valo wrote:
>
>> --- a/drivers/net/wireless/ath/ath12k/dp.h
>> +++ b/drivers/net/wireless/ath/ath12k/dp.h
>> @@ -1796,6 +1796,8 @@ static inline void ath12k_dp_get_mac_addr(u32 addr_l32, u16 addr_h16, u8 *addr)
>> memcpy(addr + 4, &addr_h16, ETH_ALEN - 4);
>> }
>>
>> +#define ATH12K_ML_PEER_ID_VALID BIT(13)
>> +
>
> this seems to be randomly placed without any context
Yeah, it is. I'll try to find a better place for it.
>> @@ -87,6 +101,9 @@ struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
>>
>> lockdep_assert_held(&ab->base_lock);
>>
>> + if (peer_id & ATH12K_ML_PEER_ID_VALID)
>
> where is code that sets the bit?
That will come later in patch 'wifi: ath12k: Add support for HTT MLO peer map and unmap event'.
> does other code elsewhere need to mask this bit off to have the "true" peer_id?
I'll investigate this.
> the commit text for this patch seems to need a lot more description
Will fix.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/8] wifi: ath12k: add helper to find multi-link station
2024-10-23 16:01 ` Jeff Johnson
2024-10-29 16:02 ` Kalle Valo
@ 2024-11-01 14:33 ` Kalle Valo
1 sibling, 0 replies; 38+ messages in thread
From: Kalle Valo @ 2024-11-01 14:33 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>> --- a/drivers/net/wireless/ath/ath12k/peer.c
>> +++ b/drivers/net/wireless/ath/ath12k/peer.c
>> @@ -80,6 +80,20 @@ struct ath12k_peer *ath12k_peer_find_by_addr(struct ath12k_base *ab,
>> return NULL;
>> }
>>
>> +static struct ath12k_peer *ath12k_peer_find_by_ml_id(struct ath12k_base *ab,
>> + int ml_peer_id)
>> +{
>> + struct ath12k_peer *peer;
>> +
>> + lockdep_assert_held(&ab->base_lock);
>> +
>> + list_for_each_entry(peer, &ab->peers, list)
>> + if (ml_peer_id == peer->ml_peer_id)
>> + return peer;
>> +
>> + return NULL;
>> +}
>> +
>> struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
>> int peer_id)
>> {
>> @@ -87,6 +101,9 @@ struct ath12k_peer *ath12k_peer_find_by_id(struct ath12k_base *ab,
>>
>> lockdep_assert_held(&ab->base_lock);
>>
>> + if (peer_id & ATH12K_ML_PEER_ID_VALID)
[...]
> does other code elsewhere need to mask this bit off to have the "true" peer_id?
Based on my investigation the peer id is stored with
ATH12K_ML_PEER_ID_VALID so it should not be masked (unless I'm missing
something). This is not pretty but I guess keeps the code simpler.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support
2024-10-23 13:29 [PATCH 0/8] wifi: ath12k: MLO support part 2 Kalle Valo
` (6 preceding siblings ...)
2024-10-23 13:30 ` [PATCH 7/8] wifi: ath12k: add helper to find multi-link station Kalle Valo
@ 2024-10-23 13:30 ` Kalle Valo
2024-10-23 16:10 ` Jeff Johnson
7 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-23 13:30 UTC (permalink / raw)
To: ath12k; +Cc: linux-wireless
From: Sriram R <quic_srirrama@quicinc.com>
Add changes to send MLO peer assoc command with partner link details and
primary umac details
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: Harshitha Prem <quic_hprem@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
drivers/net/wireless/ath/ath12k/core.h | 7 +++
drivers/net/wireless/ath/ath12k/mac.c | 62 ++++++++++++++++++++
drivers/net/wireless/ath/ath12k/wmi.c | 79 ++++++++++++++++++++++++--
drivers/net/wireless/ath/ath12k/wmi.h | 46 +++++++++++++++
4 files changed, 188 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 0a0c1a1594f2..e7a2d43e7b8a 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -487,9 +487,16 @@ struct ath12k_link_sta {
struct ath12k_rx_peer_stats *rx_stats;
struct ath12k_wbm_tx_stats *wbm_tx_stats;
u32 bw_prev;
+
+ /* For now the assoc link will be considered primary */
+ bool is_assoc_link;
+
+ /* for firmware use only */
+ u8 link_idx;
};
struct ath12k_sta {
+ struct ath12k_vif *ahvif;
enum hal_pn_type pn_type;
struct ath12k_link_sta deflink;
struct ath12k_link_sta __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS];
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index b628bc2fd0f5..2e79849974f0 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -2873,6 +2873,67 @@ static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
arg->punct_bitmap = ~arvif->punct_bitmap;
}
+static void ath12k_peer_assoc_h_mlo(struct ath12k_link_sta *arsta,
+ struct ath12k_wmi_peer_assoc_arg *arg)
+{
+ struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
+ struct peer_assoc_mlo_params *ml = &arg->ml;
+ struct ath12k_sta *ahsta = arsta->ahsta;
+ struct ath12k_link_sta *arsta_p;
+ struct ath12k_link_vif *arvif;
+ unsigned long links;
+ u8 link_id;
+ int i;
+
+ if (!sta->mlo || ahsta->ml_peer_id == ATH12K_MLO_PEER_ID_INVALID)
+ return;
+
+ ml->enabled = true;
+ ml->assoc_link = arsta->is_assoc_link;
+
+ /* For now considering the primary umac based on assoc link */
+ ml->primary_umac = arsta->is_assoc_link;
+ ml->peer_id_valid = true;
+ ml->logical_link_idx_valid = true;
+
+ ether_addr_copy(ml->mld_addr, sta->addr);
+ ml->logical_link_idx = arsta->link_idx;
+ ml->ml_peer_id = ahsta->ml_peer_id;
+ ml->ieee_link_id = arsta->link_id;
+ ml->num_partner_links = 0;
+ links = ahsta->links_map;
+
+ rcu_read_lock();
+
+ i = 0;
+
+ for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
+ if (i >= ATH12K_WMI_MLO_MAX_LINKS)
+ break;
+
+ arsta_p = rcu_dereference(ahsta->link[link_id]);
+ arvif = rcu_dereference(ahsta->ahvif->link[link_id]);
+
+ if (arsta_p == arsta)
+ continue;
+
+ if (!arvif->is_started)
+ continue;
+
+ ml->partner_info[i].vdev_id = arvif->vdev_id;
+ ml->partner_info[i].hw_link_id = arvif->ar->pdev->hw_link_id;
+ ml->partner_info[i].assoc_link = arsta_p->is_assoc_link;
+ ml->partner_info[i].primary_umac = arsta_p->is_assoc_link;
+ ml->partner_info[i].logical_link_idx_valid = true;
+ ml->partner_info[i].logical_link_idx = arsta_p->link_idx;
+ ml->num_partner_links++;
+
+ i++;
+ }
+
+ rcu_read_unlock();
+}
+
static void ath12k_peer_assoc_prepare(struct ath12k *ar,
struct ath12k_link_vif *arvif,
struct ath12k_link_sta *arsta,
@@ -2897,6 +2958,7 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
ath12k_peer_assoc_h_qos(ar, arvif, arsta, arg);
ath12k_peer_assoc_h_phymode(ar, arvif, arsta, arg);
ath12k_peer_assoc_h_smps(arsta, arg);
+ ath12k_peer_assoc_h_mlo(arsta, arg);
/* TODO: amsdu_disable req? */
}
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index 0583d832fac7..73c1c6bcf48b 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -2101,12 +2101,15 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
struct ath12k_wmi_vht_rate_set_params *mcs;
struct ath12k_wmi_he_rate_set_params *he_mcs;
struct ath12k_wmi_eht_rate_set_params *eht_mcs;
+ struct wmi_peer_assoc_mlo_params *ml_params;
+ struct wmi_peer_assoc_mlo_partner_info *partner_info;
struct sk_buff *skb;
struct wmi_tlv *tlv;
void *ptr;
u32 peer_legacy_rates_align;
u32 peer_ht_rates_align;
int i, ret, len;
+ __le32 v;
peer_legacy_rates_align = roundup(arg->peer_legacy_rates.num_rates,
sizeof(u32));
@@ -2118,8 +2121,13 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
TLV_HDR_SIZE + (peer_ht_rates_align * sizeof(u8)) +
sizeof(*mcs) + TLV_HDR_SIZE +
(sizeof(*he_mcs) * arg->peer_he_mcs_count) +
- TLV_HDR_SIZE + (sizeof(*eht_mcs) * arg->peer_eht_mcs_count) +
- TLV_HDR_SIZE + TLV_HDR_SIZE;
+ TLV_HDR_SIZE + (sizeof(*eht_mcs) * arg->peer_eht_mcs_count);
+
+ if (arg->ml.enabled)
+ len += TLV_HDR_SIZE + sizeof(*ml_params) +
+ TLV_HDR_SIZE + (arg->ml.num_partner_links * sizeof(*partner_info));
+ else
+ len += (2 * TLV_HDR_SIZE);
skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
if (!skb)
@@ -2243,12 +2251,38 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
ptr += sizeof(*he_mcs);
}
- /* MLO header tag with 0 length */
- len = 0;
tlv = ptr;
+ len = arg->ml.enabled ? sizeof(*ml_params) : 0;
tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
ptr += TLV_HDR_SIZE;
+ if (!len)
+ goto skip_ml_params;
+ ml_params = ptr;
+ ml_params->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_ASSOC_PARAMS,
+ len);
+ ml_params->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
+
+ if (arg->ml.assoc_link)
+ ml_params->flags |= cpu_to_le32(ATH12K_WMI_FLAG_MLO_ASSOC_LINK);
+
+ if (arg->ml.primary_umac)
+ ml_params->flags |= cpu_to_le32(ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC);
+
+ if (arg->ml.logical_link_idx_valid)
+ ml_params->flags |=
+ cpu_to_le32(ATH12K_WMI_FLAG_MLO_LOGICAL_LINK_IDX_VALID);
+
+ if (arg->ml.peer_id_valid)
+ ml_params->flags |= cpu_to_le32(ATH12K_WMI_FLAG_MLO_PEER_ID_VALID);
+
+ ether_addr_copy(ml_params->mld_addr.addr, arg->ml.mld_addr);
+ ml_params->logical_link_idx = cpu_to_le32(arg->ml.logical_link_idx);
+ ml_params->ml_peer_id = cpu_to_le32(arg->ml.ml_peer_id);
+ ml_params->ieee_link_id = cpu_to_le32(arg->ml.ieee_link_id);
+ ptr += sizeof(*ml_params);
+
+skip_ml_params:
/* Loop through the EHT rate set */
len = arg->peer_eht_mcs_count * sizeof(*eht_mcs);
tlv = ptr;
@@ -2265,12 +2299,45 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
ptr += sizeof(*eht_mcs);
}
- /* ML partner links tag with 0 length */
- len = 0;
tlv = ptr;
+ len = arg->ml.enabled ? arg->ml.num_partner_links * sizeof(*partner_info) : 0;
+ /* fill ML Partner links */
tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
ptr += TLV_HDR_SIZE;
+ if (len == 0)
+ goto send;
+
+ for (i = 0; i < arg->ml.num_partner_links; i++) {
+ u32 cmd = WMI_TAG_MLO_PARTNER_LINK_PARAMS_PEER_ASSOC;
+
+ partner_info = ptr;
+ partner_info->tlv_header = ath12k_wmi_tlv_cmd_hdr(cmd,
+ sizeof(*partner_info));
+ partner_info->vdev_id = cpu_to_le32(arg->ml.partner_info[i].vdev_id);
+ partner_info->hw_link_id =
+ cpu_to_le32(arg->ml.partner_info[i].hw_link_id);
+ partner_info->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
+
+ if (arg->ml.partner_info[i].assoc_link)
+ partner_info->flags |=
+ cpu_to_le32(ATH12K_WMI_FLAG_MLO_ASSOC_LINK);
+
+ if (arg->ml.partner_info[i].primary_umac)
+ partner_info->flags |=
+ cpu_to_le32(ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC);
+
+ if (arg->ml.partner_info[i].logical_link_idx_valid) {
+ v = cpu_to_le32(ATH12K_WMI_FLAG_MLO_LINK_ID_VALID);
+ partner_info->flags |= v;
+ }
+
+ partner_info->logical_link_idx =
+ cpu_to_le32(arg->ml.partner_info[i].logical_link_idx);
+ ptr += sizeof(*partner_info);
+ }
+
+send:
ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
"wmi peer assoc vdev id %d assoc id %d peer mac %pM peer_flags %x rate_caps %x peer_caps %x listen_intval %d ht_caps %x max_mpdu %d nss %d phymode %d peer_mpdu_density %d vht_caps %x he cap_info %x he ops %x he cap_info_ext %x he phy %x %x %x peer_bw_rxnss_override %x peer_flags_ext %x eht mac_cap %x %x eht phy_cap %x %x %x\n",
cmd->vdev_id, cmd->peer_associd, arg->peer_mac,
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index 07bd275608bf..e93f74e97771 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -3698,6 +3698,24 @@ struct wmi_vdev_install_key_arg {
#define WMI_HECAP_TXRX_MCS_NSS_IDX_160 1
#define WMI_HECAP_TXRX_MCS_NSS_IDX_80_80 2
+#define ATH12K_WMI_MLO_MAX_PARTNER_LINKS \
+ (ATH12K_WMI_MLO_MAX_LINKS + ATH12K_MAX_NUM_BRIDGE_LINKS - 1)
+
+struct peer_assoc_mlo_params {
+ bool enabled;
+ bool assoc_link;
+ bool primary_umac;
+ bool peer_id_valid;
+ bool logical_link_idx_valid;
+ bool bridge_peer;
+ u8 mld_addr[ETH_ALEN];
+ u32 logical_link_idx;
+ u32 ml_peer_id;
+ u32 ieee_link_id;
+ u8 num_partner_links;
+ struct wmi_ml_partner_info partner_info[ATH12K_WMI_MLO_MAX_LINKS];
+};
+
struct wmi_rate_set_arg {
u32 num_rates;
u8 rates[WMI_MAX_SUPPORTED_RATES];
@@ -3772,8 +3790,36 @@ struct ath12k_wmi_peer_assoc_arg {
u32 peer_eht_tx_mcs_set[WMI_MAX_EHTCAP_RATE_SET];
struct ath12k_wmi_ppe_threshold_arg peer_eht_ppet;
u32 punct_bitmap;
+ bool is_assoc;
+ struct peer_assoc_mlo_params ml;
};
+#define ATH12K_WMI_FLAG_MLO_ENABLED BIT(0)
+#define ATH12K_WMI_FLAG_MLO_ASSOC_LINK BIT(1)
+#define ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC BIT(2)
+#define ATH12K_WMI_FLAG_MLO_LINK_ID_VALID BIT(3)
+#define ATH12K_WMI_FLAG_MLO_PEER_ID_VALID BIT(4)
+
+struct wmi_peer_assoc_mlo_partner_info {
+ __le32 tlv_header;
+ __le32 vdev_id;
+ __le32 hw_link_id;
+ __le32 flags;
+ __le32 logical_link_idx;
+} __packed;
+
+struct wmi_peer_assoc_mlo_params {
+ __le32 tlv_header;
+ __le32 flags;
+ struct wmi_mac_addr mld_addr;
+ __le32 logical_link_idx;
+ __le32 ml_peer_id;
+ __le32 ieee_link_id;
+ __le32 emlsr_trans_timeout_us;
+ __le32 emlsr_trans_delay_us;
+ __le32 emlsr_padding_delay_us;
+} __packed;
+
struct wmi_peer_assoc_complete_cmd {
__le32 tlv_header;
struct ath12k_wmi_mac_addr_params peer_macaddr;
--
2.39.5
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support
2024-10-23 13:30 ` [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support Kalle Valo
@ 2024-10-23 16:10 ` Jeff Johnson
2024-10-29 16:05 ` Kalle Valo
0 siblings, 1 reply; 38+ messages in thread
From: Jeff Johnson @ 2024-10-23 16:10 UTC (permalink / raw)
To: Kalle Valo, ath12k; +Cc: linux-wireless
On 10/23/2024 6:30 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
>
> Add changes to send MLO peer assoc command with partner link details and
> primary umac details
>
> 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: Harshitha Prem <quic_hprem@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/core.h | 7 +++
> drivers/net/wireless/ath/ath12k/mac.c | 62 ++++++++++++++++++++
> drivers/net/wireless/ath/ath12k/wmi.c | 79 ++++++++++++++++++++++++--
> drivers/net/wireless/ath/ath12k/wmi.h | 46 +++++++++++++++
> 4 files changed, 188 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 0a0c1a1594f2..e7a2d43e7b8a 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -487,9 +487,16 @@ struct ath12k_link_sta {
> struct ath12k_rx_peer_stats *rx_stats;
> struct ath12k_wbm_tx_stats *wbm_tx_stats;
> u32 bw_prev;
> +
> + /* For now the assoc link will be considered primary */
> + bool is_assoc_link;
> +
> + /* for firmware use only */
> + u8 link_idx;
> };
>
> struct ath12k_sta {
> + struct ath12k_vif *ahvif;
> enum hal_pn_type pn_type;
> struct ath12k_link_sta deflink;
> struct ath12k_link_sta __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS];
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index b628bc2fd0f5..2e79849974f0 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -2873,6 +2873,67 @@ static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
> arg->punct_bitmap = ~arvif->punct_bitmap;
> }
>
> +static void ath12k_peer_assoc_h_mlo(struct ath12k_link_sta *arsta,
> + struct ath12k_wmi_peer_assoc_arg *arg)
> +{
> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(arsta->ahsta);
> + struct peer_assoc_mlo_params *ml = &arg->ml;
> + struct ath12k_sta *ahsta = arsta->ahsta;
> + struct ath12k_link_sta *arsta_p;
> + struct ath12k_link_vif *arvif;
> + unsigned long links;
> + u8 link_id;
> + int i;
> +
> + if (!sta->mlo || ahsta->ml_peer_id == ATH12K_MLO_PEER_ID_INVALID)
> + return;
> +
> + ml->enabled = true;
> + ml->assoc_link = arsta->is_assoc_link;
> +
> + /* For now considering the primary umac based on assoc link */
> + ml->primary_umac = arsta->is_assoc_link;
> + ml->peer_id_valid = true;
> + ml->logical_link_idx_valid = true;
> +
> + ether_addr_copy(ml->mld_addr, sta->addr);
> + ml->logical_link_idx = arsta->link_idx;
> + ml->ml_peer_id = ahsta->ml_peer_id;
> + ml->ieee_link_id = arsta->link_id;
> + ml->num_partner_links = 0;
> + links = ahsta->links_map;
> +
> + rcu_read_lock();
> +
> + i = 0;
nit: setting i=0 doesn't need to be RCU protected
> +
> + for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
> + if (i >= ATH12K_WMI_MLO_MAX_LINKS)
> + break;
> +
> + arsta_p = rcu_dereference(ahsta->link[link_id]);
> + arvif = rcu_dereference(ahsta->ahvif->link[link_id]);
> +
> + if (arsta_p == arsta)
> + continue;
> +
> + if (!arvif->is_started)
> + continue;
> +
> + ml->partner_info[i].vdev_id = arvif->vdev_id;
> + ml->partner_info[i].hw_link_id = arvif->ar->pdev->hw_link_id;
> + ml->partner_info[i].assoc_link = arsta_p->is_assoc_link;
> + ml->partner_info[i].primary_umac = arsta_p->is_assoc_link;
> + ml->partner_info[i].logical_link_idx_valid = true;
> + ml->partner_info[i].logical_link_idx = arsta_p->link_idx;
> + ml->num_partner_links++;
> +
> + i++;
> + }
> +
> + rcu_read_unlock();
> +}
> +
> static void ath12k_peer_assoc_prepare(struct ath12k *ar,
> struct ath12k_link_vif *arvif,
> struct ath12k_link_sta *arsta,
> @@ -2897,6 +2958,7 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
> ath12k_peer_assoc_h_qos(ar, arvif, arsta, arg);
> ath12k_peer_assoc_h_phymode(ar, arvif, arsta, arg);
> ath12k_peer_assoc_h_smps(arsta, arg);
> + ath12k_peer_assoc_h_mlo(arsta, arg);
>
> /* TODO: amsdu_disable req? */
> }
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index 0583d832fac7..73c1c6bcf48b 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -2101,12 +2101,15 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
> struct ath12k_wmi_vht_rate_set_params *mcs;
> struct ath12k_wmi_he_rate_set_params *he_mcs;
> struct ath12k_wmi_eht_rate_set_params *eht_mcs;
> + struct wmi_peer_assoc_mlo_params *ml_params;
> + struct wmi_peer_assoc_mlo_partner_info *partner_info;
> struct sk_buff *skb;
> struct wmi_tlv *tlv;
> void *ptr;
> u32 peer_legacy_rates_align;
> u32 peer_ht_rates_align;
> int i, ret, len;
> + __le32 v;
>
> peer_legacy_rates_align = roundup(arg->peer_legacy_rates.num_rates,
> sizeof(u32));
> @@ -2118,8 +2121,13 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
> TLV_HDR_SIZE + (peer_ht_rates_align * sizeof(u8)) +
> sizeof(*mcs) + TLV_HDR_SIZE +
> (sizeof(*he_mcs) * arg->peer_he_mcs_count) +
> - TLV_HDR_SIZE + (sizeof(*eht_mcs) * arg->peer_eht_mcs_count) +
> - TLV_HDR_SIZE + TLV_HDR_SIZE;
> + TLV_HDR_SIZE + (sizeof(*eht_mcs) * arg->peer_eht_mcs_count);
> +
> + if (arg->ml.enabled)
> + len += TLV_HDR_SIZE + sizeof(*ml_params) +
> + TLV_HDR_SIZE + (arg->ml.num_partner_links * sizeof(*partner_info));
> + else
> + len += (2 * TLV_HDR_SIZE);
>
> skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
> if (!skb)
> @@ -2243,12 +2251,38 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
> ptr += sizeof(*he_mcs);
> }
>
> - /* MLO header tag with 0 length */
> - len = 0;
> tlv = ptr;
> + len = arg->ml.enabled ? sizeof(*ml_params) : 0;
> tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
> ptr += TLV_HDR_SIZE;
> + if (!len)
> + goto skip_ml_params;
>
> + ml_params = ptr;
> + ml_params->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_ASSOC_PARAMS,
> + len);
this is another instance where we are using the same length for two
consecutive TLVs -- that doesn't seem right
> + ml_params->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
> +
> + if (arg->ml.assoc_link)
> + ml_params->flags |= cpu_to_le32(ATH12K_WMI_FLAG_MLO_ASSOC_LINK);
> +
> + if (arg->ml.primary_umac)
> + ml_params->flags |= cpu_to_le32(ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC);
> +
> + if (arg->ml.logical_link_idx_valid)
> + ml_params->flags |=
> + cpu_to_le32(ATH12K_WMI_FLAG_MLO_LOGICAL_LINK_IDX_VALID);
> +
> + if (arg->ml.peer_id_valid)
> + ml_params->flags |= cpu_to_le32(ATH12K_WMI_FLAG_MLO_PEER_ID_VALID);
> +
> + ether_addr_copy(ml_params->mld_addr.addr, arg->ml.mld_addr);
> + ml_params->logical_link_idx = cpu_to_le32(arg->ml.logical_link_idx);
> + ml_params->ml_peer_id = cpu_to_le32(arg->ml.ml_peer_id);
> + ml_params->ieee_link_id = cpu_to_le32(arg->ml.ieee_link_id);
> + ptr += sizeof(*ml_params);
> +
> +skip_ml_params:
> /* Loop through the EHT rate set */
> len = arg->peer_eht_mcs_count * sizeof(*eht_mcs);
> tlv = ptr;
> @@ -2265,12 +2299,45 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
> ptr += sizeof(*eht_mcs);
> }
>
> - /* ML partner links tag with 0 length */
> - len = 0;
> tlv = ptr;
> + len = arg->ml.enabled ? arg->ml.num_partner_links * sizeof(*partner_info) : 0;
> + /* fill ML Partner links */
> tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
> ptr += TLV_HDR_SIZE;
>
> + if (len == 0)
> + goto send;
> +
> + for (i = 0; i < arg->ml.num_partner_links; i++) {
> + u32 cmd = WMI_TAG_MLO_PARTNER_LINK_PARAMS_PEER_ASSOC;
> +
> + partner_info = ptr;
> + partner_info->tlv_header = ath12k_wmi_tlv_cmd_hdr(cmd,
> + sizeof(*partner_info));
> + partner_info->vdev_id = cpu_to_le32(arg->ml.partner_info[i].vdev_id);
> + partner_info->hw_link_id =
> + cpu_to_le32(arg->ml.partner_info[i].hw_link_id);
> + partner_info->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
> +
> + if (arg->ml.partner_info[i].assoc_link)
> + partner_info->flags |=
> + cpu_to_le32(ATH12K_WMI_FLAG_MLO_ASSOC_LINK);
> +
> + if (arg->ml.partner_info[i].primary_umac)
> + partner_info->flags |=
> + cpu_to_le32(ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC);
> +
> + if (arg->ml.partner_info[i].logical_link_idx_valid) {
> + v = cpu_to_le32(ATH12K_WMI_FLAG_MLO_LINK_ID_VALID);
> + partner_info->flags |= v;
> + }
> +
> + partner_info->logical_link_idx =
> + cpu_to_le32(arg->ml.partner_info[i].logical_link_idx);
> + ptr += sizeof(*partner_info);
> + }
> +
> +send:
> ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
> "wmi peer assoc vdev id %d assoc id %d peer mac %pM peer_flags %x rate_caps %x peer_caps %x listen_intval %d ht_caps %x max_mpdu %d nss %d phymode %d peer_mpdu_density %d vht_caps %x he cap_info %x he ops %x he cap_info_ext %x he phy %x %x %x peer_bw_rxnss_override %x peer_flags_ext %x eht mac_cap %x %x eht phy_cap %x %x %x\n",
> cmd->vdev_id, cmd->peer_associd, arg->peer_mac,
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
> index 07bd275608bf..e93f74e97771 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.h
> +++ b/drivers/net/wireless/ath/ath12k/wmi.h
> @@ -3698,6 +3698,24 @@ struct wmi_vdev_install_key_arg {
> #define WMI_HECAP_TXRX_MCS_NSS_IDX_160 1
> #define WMI_HECAP_TXRX_MCS_NSS_IDX_80_80 2
>
> +#define ATH12K_WMI_MLO_MAX_PARTNER_LINKS \
> + (ATH12K_WMI_MLO_MAX_LINKS + ATH12K_MAX_NUM_BRIDGE_LINKS - 1)
> +
> +struct peer_assoc_mlo_params {
> + bool enabled;
> + bool assoc_link;
> + bool primary_umac;
> + bool peer_id_valid;
> + bool logical_link_idx_valid;
> + bool bridge_peer;
> + u8 mld_addr[ETH_ALEN];
> + u32 logical_link_idx;
> + u32 ml_peer_id;
> + u32 ieee_link_id;
> + u8 num_partner_links;
> + struct wmi_ml_partner_info partner_info[ATH12K_WMI_MLO_MAX_LINKS];
> +};
> +
> struct wmi_rate_set_arg {
> u32 num_rates;
> u8 rates[WMI_MAX_SUPPORTED_RATES];
> @@ -3772,8 +3790,36 @@ struct ath12k_wmi_peer_assoc_arg {
> u32 peer_eht_tx_mcs_set[WMI_MAX_EHTCAP_RATE_SET];
> struct ath12k_wmi_ppe_threshold_arg peer_eht_ppet;
> u32 punct_bitmap;
> + bool is_assoc;
> + struct peer_assoc_mlo_params ml;
> };
>
> +#define ATH12K_WMI_FLAG_MLO_ENABLED BIT(0)
> +#define ATH12K_WMI_FLAG_MLO_ASSOC_LINK BIT(1)
> +#define ATH12K_WMI_FLAG_MLO_PRIMARY_UMAC BIT(2)
> +#define ATH12K_WMI_FLAG_MLO_LINK_ID_VALID BIT(3)
> +#define ATH12K_WMI_FLAG_MLO_PEER_ID_VALID BIT(4)
> +
> +struct wmi_peer_assoc_mlo_partner_info {
this doesn't follow the WMI naming convention, should be _params
> + __le32 tlv_header;
> + __le32 vdev_id;
> + __le32 hw_link_id;
> + __le32 flags;
> + __le32 logical_link_idx;
> +} __packed;
> +
> +struct wmi_peer_assoc_mlo_params {
> + __le32 tlv_header;
> + __le32 flags;
> + struct wmi_mac_addr mld_addr;
> + __le32 logical_link_idx;
> + __le32 ml_peer_id;
> + __le32 ieee_link_id;
> + __le32 emlsr_trans_timeout_us;
> + __le32 emlsr_trans_delay_us;
> + __le32 emlsr_padding_delay_us;
> +} __packed;
> +
> struct wmi_peer_assoc_complete_cmd {
> __le32 tlv_header;
> struct ath12k_wmi_mac_addr_params peer_macaddr;
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support
2024-10-23 16:10 ` Jeff Johnson
@ 2024-10-29 16:05 ` Kalle Valo
2024-10-29 16:10 ` Jeff Johnson
0 siblings, 1 reply; 38+ messages in thread
From: Kalle Valo @ 2024-10-29 16:05 UTC (permalink / raw)
To: Jeff Johnson; +Cc: ath12k, linux-wireless
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 10/23/2024 6:30 AM, Kalle Valo wrote:
>
>> + rcu_read_lock();
>> +
>> + i = 0;
>
> nit: setting i=0 doesn't need to be RCU protected
Yeah, but that doesn't cause any issues and this way it's closer to the
for loop where it's used:
>> + for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
>> + if (i >= ATH12K_WMI_MLO_MAX_LINKS)
>> + break;
[...]
>> @@ -2243,12 +2251,38 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
>> ptr += sizeof(*he_mcs);
>> }
>>
>> - /* MLO header tag with 0 length */
>> - len = 0;
>> tlv = ptr;
>> + len = arg->ml.enabled ? sizeof(*ml_params) : 0;
>> tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
>> ptr += TLV_HDR_SIZE;
>> + if (!len)
>> + goto skip_ml_params;
>>
>> + ml_params = ptr;
>> + ml_params->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_ASSOC_PARAMS,
>> + len);
>
> this is another instance where we are using the same length for two
> consecutive TLVs -- that doesn't seem right
This is also a similar case of _hdr() vs _cmd_hdr(), does that look ok?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 8/8] wifi: ath12k: Add MLO peer assoc command support
2024-10-29 16:05 ` Kalle Valo
@ 2024-10-29 16:10 ` Jeff Johnson
0 siblings, 0 replies; 38+ messages in thread
From: Jeff Johnson @ 2024-10-29 16:10 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath12k, linux-wireless
On 10/29/2024 9:05 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
>> On 10/23/2024 6:30 AM, Kalle Valo wrote:
>>
>>> + rcu_read_lock();
>>> +
>>> + i = 0;
>>
>> nit: setting i=0 doesn't need to be RCU protected
>
> Yeah, but that doesn't cause any issues and this way it's closer to the
> for loop where it's used:
>
>>> + for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
>>> + if (i >= ATH12K_WMI_MLO_MAX_LINKS)
>>> + break;
>
> [...]
>
>>> @@ -2243,12 +2251,38 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
>>> ptr += sizeof(*he_mcs);
>>> }
>>>
>>> - /* MLO header tag with 0 length */
>>> - len = 0;
>>> tlv = ptr;
>>> + len = arg->ml.enabled ? sizeof(*ml_params) : 0;
>>> tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
>>> ptr += TLV_HDR_SIZE;
>>> + if (!len)
>>> + goto skip_ml_params;
>>>
>>> + ml_params = ptr;
>>> + ml_params->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_ASSOC_PARAMS,
>>> + len);
>>
>> this is another instance where we are using the same length for two
>> consecutive TLVs -- that doesn't seem right
>
> This is also a similar case of _hdr() vs _cmd_hdr(), does that look ok?
>
here again this is evil. please change to _hdr() and explicitly subtract out
the tlv header size
^ permalink raw reply [flat|nested] 38+ messages in thread