diff mbox series

wifi: mac80211: Add ADDBA Extension element parsing logics

Message ID 20241024022105.6810-1-MeiChia.Chiu@mediatek.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series wifi: mac80211: Add ADDBA Extension element parsing logics | expand

Commit Message

MeiChia Chiu Oct. 24, 2024, 2:21 a.m. UTC 
Add ADDBA Extension element parsing logics in ADDBA request/response

To support BA size of 1024,
the ADDBA Extension element is needed in ADDBA request/response.
Therefore, parsing logics is added for this missing piece.

Reviewed-by: Shayne Chen <shayne.chen@mediatek.com>
Reviewed-by: Money Wang <money.wang@mediatek.com>
Co-developed-by: Peter Chiu <chui-hao.chiu@mediatek.com>
Signed-off-by: Peter Chiu <chui-hao.chiu@mediatek.com>
Signed-off-by: MeiChia Chiu <MeiChia.Chiu@mediatek.com>
---
 include/linux/ieee80211.h |  2 ++
 net/mac80211/agg-tx.c     | 41 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 3 deletions(-)

Comments

Johannes Berg Oct. 24, 2024, 7:04 a.m. UTC | #1 
On Thu, 2024-10-24 at 10:21 +0800, MeiChia Chiu wrote:
> Add ADDBA Extension element parsing logics in ADDBA request/response
> 
> To support BA size of 1024,
> the ADDBA Extension element is needed in ADDBA request/response.
> Therefore, parsing logics is added for this missing piece.

Ah yes, we had this only for the RX side, I guess.

> +++ b/net/mac80211/agg-tx.c
> @@ -66,10 +66,12 @@ static void ieee80211_send_addba_request(struct ieee80211_sub_if_data *sdata,
>  	struct ieee80211_local *local = sdata->local;
>  	struct sk_buff *skb;
>  	struct ieee80211_mgmt *mgmt;
> +	struct ieee80211_addba_ext_ie *addba_ext;
> +	u8 ext_size = agg_size >= 1024 ? 2 + sizeof(*addba_ext) : 0;
> +	u8 *pos;
>  	u16 capab;
>  
> -	skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom);
> -
> +	skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom + ext_size);

probably much simpler to just make that unconditional?
Like in ieee80211_send_addba_resp().

> +	if (agg_size >= 1024) {
> +		pos = skb_put_zero(skb, ext_size);
> +		*pos++ = WLAN_EID_ADDBA_EXT;
> +		*pos++ = sizeof(struct ieee80211_addba_ext_ie);
> +		addba_ext = (struct ieee80211_addba_ext_ie *)pos;
> +		addba_ext->data = u8_encode_bits(agg_size >> IEEE80211_ADDBA_EXT_BUF_SIZE_SHIFT,
> +						 IEEE80211_ADDBA_EXT_BUF_SIZE_MASK);
> +	}

Maybe we can reuse ieee80211_add_addbaext()?

Also you only described "parsing" in the commit message, so this isn't
really part of it ;-) Please complete the commit message.

> @@ -460,8 +471,11 @@ static void ieee80211_send_addba_with_timeout(struct sta_info *sta,
>  	sta->ampdu_mlme.addba_req_num[tid]++;
>  	spin_unlock_bh(&sta->lock);
>  
> -	if (sta->sta.deflink.he_cap.has_he) {
> +	if (sta->sta.deflink.eht_cap.has_eht) {
>  		buf_size = local->hw.max_tx_aggregation_subframes;
> +	} else if (sta->sta.deflink.he_cap.has_he) {
> +		buf_size = min_t(u16, local->hw.max_tx_aggregation_subframes,
> +				 IEEE80211_MAX_AMPDU_BUF_HE);
>  	} else {

That's related, but not precisely part of what you describe in the
commit message. Just make the commit message more general, I guess, such
as "support EHT 1024 aggregation size in TX" or so?

> @@ -970,6 +986,25 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
>  	amsdu = capab & IEEE80211_ADDBA_PARAM_AMSDU_MASK;
>  	tid = u16_get_bits(capab, IEEE80211_ADDBA_PARAM_TID_MASK);
>  	buf_size = u16_get_bits(capab, IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK);
> +	ext_elem_len = len - offsetof(struct ieee80211_mgmt,
> +				      u.action.u.addba_resp.variable);
> +
> +	if (ext_elem_len) {
> +		elems = ieee802_11_parse_elems(mgmt->u.action.u.addba_resp.variable,
> +					       ext_elem_len, true, NULL);
> +
> +		if (elems && !elems->parse_error) {
> +			if (sta->sta.deflink.eht_cap.has_eht && elems->addba_ext_ie) {
> +				u8 buf_size_1k = u8_get_bits(elems->addba_ext_ie->data,
> +							     IEEE80211_ADDBA_EXT_BUF_SIZE_MASK);
> +				buf_size |=
> +					((u16)buf_size_1k) << IEEE80211_ADDBA_EXT_BUF_SIZE_SHIFT;
> +			}
> +		}
> +
> +		kfree(elems);
> +	}

This is all also very similar to ieee80211_process_addba_request(), so
again perhaps it could be reused - would have to pass 'ext_elem_len' and
the buffer to a function.

johannes
diff mbox series

Patch

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 456bca45ff05..05dedc45505c 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -1445,6 +1445,8 @@  struct ieee80211_mgmt {
 					__le16 status;
 					__le16 capab;
 					__le16 timeout;
+					/* followed by BA Extension */
+					u8 variable[];
 				} __packed addba_resp;
 				struct{
 					u8 action_code;
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 04cb45cfb310..2070ca86be0c 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -66,10 +66,12 @@  static void ieee80211_send_addba_request(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	struct sk_buff *skb;
 	struct ieee80211_mgmt *mgmt;
+	struct ieee80211_addba_ext_ie *addba_ext;
+	u8 ext_size = agg_size >= 1024 ? 2 + sizeof(*addba_ext) : 0;
+	u8 *pos;
 	u16 capab;
 
-	skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom);
-
+	skb = dev_alloc_skb(sizeof(*mgmt) + local->hw.extra_tx_headroom + ext_size);
 	if (!skb)
 		return;
 
@@ -93,6 +95,15 @@  static void ieee80211_send_addba_request(struct ieee80211_sub_if_data *sdata,
 	mgmt->u.action.u.addba_req.start_seq_num =
 					cpu_to_le16(start_seq_num << 4);
 
+	if (agg_size >= 1024) {
+		pos = skb_put_zero(skb, ext_size);
+		*pos++ = WLAN_EID_ADDBA_EXT;
+		*pos++ = sizeof(struct ieee80211_addba_ext_ie);
+		addba_ext = (struct ieee80211_addba_ext_ie *)pos;
+		addba_ext->data = u8_encode_bits(agg_size >> IEEE80211_ADDBA_EXT_BUF_SIZE_SHIFT,
+						 IEEE80211_ADDBA_EXT_BUF_SIZE_MASK);
+	}
+
 	ieee80211_tx_skb_tid(sdata, skb, tid, -1);
 }
 
@@ -460,8 +471,11 @@  static void ieee80211_send_addba_with_timeout(struct sta_info *sta,
 	sta->ampdu_mlme.addba_req_num[tid]++;
 	spin_unlock_bh(&sta->lock);
 
-	if (sta->sta.deflink.he_cap.has_he) {
+	if (sta->sta.deflink.eht_cap.has_eht) {
 		buf_size = local->hw.max_tx_aggregation_subframes;
+	} else if (sta->sta.deflink.he_cap.has_he) {
+		buf_size = min_t(u16, local->hw.max_tx_aggregation_subframes,
+				 IEEE80211_MAX_AMPDU_BUF_HE);
 	} else {
 		/*
 		 * We really should use what the driver told us it will
@@ -961,8 +975,10 @@  void ieee80211_process_addba_resp(struct ieee80211_local *local,
 {
 	struct tid_ampdu_tx *tid_tx;
 	struct ieee80211_txq *txq;
+	struct ieee802_11_elems *elems;
 	u16 capab, tid, buf_size;
 	bool amsdu;
+	int ext_elem_len;
 
 	lockdep_assert_wiphy(sta->local->hw.wiphy);
 
@@ -970,6 +986,25 @@  void ieee80211_process_addba_resp(struct ieee80211_local *local,
 	amsdu = capab & IEEE80211_ADDBA_PARAM_AMSDU_MASK;
 	tid = u16_get_bits(capab, IEEE80211_ADDBA_PARAM_TID_MASK);
 	buf_size = u16_get_bits(capab, IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK);
+	ext_elem_len = len - offsetof(struct ieee80211_mgmt,
+				      u.action.u.addba_resp.variable);
+
+	if (ext_elem_len) {
+		elems = ieee802_11_parse_elems(mgmt->u.action.u.addba_resp.variable,
+					       ext_elem_len, true, NULL);
+
+		if (elems && !elems->parse_error) {
+			if (sta->sta.deflink.eht_cap.has_eht && elems->addba_ext_ie) {
+				u8 buf_size_1k = u8_get_bits(elems->addba_ext_ie->data,
+							     IEEE80211_ADDBA_EXT_BUF_SIZE_MASK);
+				buf_size |=
+					((u16)buf_size_1k) << IEEE80211_ADDBA_EXT_BUF_SIZE_SHIFT;
+			}
+		}
+
+		kfree(elems);
+	}
+
 	buf_size = min(buf_size, local->hw.max_tx_aggregation_subframes);
 
 	txq = sta->sta.txq[tid];