[PATCH 02/10] wifi: ath12k: ath12k_mac_op_tx(): MLO support

[Baochen Qiang]

On 11/28/2024 8:32 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang at quicinc.com> writes:
> 
>> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>>> From: Sriram R <quic_srirrama at quicinc.com>
>>>
>>> @@ -6848,6 +6848,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>>>  static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>>>  {
>>>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
>>> +	struct ath12k_hw *ah = ar->ah;
>>>  	struct ath12k_skb_cb *skb_cb;
>>>  	struct ath12k_vif *ahvif;
>>>  	struct ath12k_link_vif *arvif;
>>> @@ -6865,7 +6866,15 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>>>  		}
>>>  
>>>  		ahvif = ath12k_vif_to_ahvif(skb_cb->vif);
>>> -		arvif = &ahvif->deflink;
>>> +		if (!(ahvif->links_map & BIT(skb_cb->link_id))) {
>>> +			ath12k_warn(ar->ab,
>>> +				    "invalid linkid %u in mgmt over wmi tx with linkmap 0x%X\n",
>>
>> s/0x%X/0x%x/ ?
> 
> Fixed.
> 
>>
>>> +				    skb_cb->link_id, ahvif->links_map);
>>> +			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>>> +			continue;
>>> +		}
>>> +
>>> +		arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[skb_cb->link_id]);
>>>  		if (ar->allocated_vdev_map & (1LL << arvif->vdev_id)) {
>>>  			ret = ath12k_mac_mgmt_tx_wmi(ar, arvif, skb);
>>>  			if (ret) {
>>> @@ -6875,9 +6884,10 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>>>  			}
>>>  		} else {
>>>  			ath12k_warn(ar->ab,
>>> -				    "dropping mgmt frame for vdev %d, is_started %d\n",
>>> +				    "dropping mgmt frame for vdev %d link_id %u, is_started %d\n",
>>>  				    arvif->vdev_id,
>>> -				    arvif->is_started);
>>> +				    arvif->is_started,
>>> +				    skb_cb->link_id);
>>
>> swap 'arvif->is_started' and 'skb_cb->link_id'.
> 
> Good catch! Fixed as well.
> 
>>> +/* Note: called under rcu_read_lock() */
>>> +static u8 ath12k_mac_get_tx_link(struct ieee80211_sta *sta, struct ieee80211_vif *vif,
>>> +				 u8 link, struct sk_buff *skb, u32 info_flags)
>>> +{
>>> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>> +	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>>> +	struct ieee80211_link_sta *link_sta;
>>> +	struct ieee80211_bss_conf *bss_conf;
>>> +	struct ath12k_sta *ahsta;
>>
>> better to assert RCU read lock here?
> 
> You mean something like WARN_ON(!rcu_read_lock_held())? I'm not really a
> fan of that, I think it's better that we discuss this also once we
> document locking design properly.
> 
>>> +/* Note: called under rcu_read_lock() */
>>>  static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>>  			     struct ieee80211_tx_control *control,
>>>  			     struct sk_buff *skb)
>>> @@ -6945,13 +7054,16 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>>  	struct ieee80211_vif *vif = info->control.vif;
>>>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>>>  	struct ath12k_link_vif *arvif = &ahvif->deflink;
>>> -	struct ath12k *ar = arvif->ar;
>>>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>>  	struct ieee80211_key_conf *key = info->control.hw_key;
>>> +	struct ieee80211_sta *sta = control->sta;
>>>  	u32 info_flags = info->flags;
>>> +	struct ath12k *ar;
>>>  	bool is_prb_rsp;
>>> +	u8 link_id;
>>>  	int ret;
>>>  
>> better to assert RCU read lock here?
> 
> Same comment here as above.
> 
>>
>>> +	link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
>>>  	memset(skb_cb, 0, sizeof(*skb_cb));
>>>  	skb_cb->vif = vif;
>>>  
>>> @@ -6960,6 +7072,27 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>>  		skb_cb->flags |= ATH12K_SKB_CIPHER_SET;
>>>  	}
>>>  
>>> +	/* handle only for MLO case, use deflink for non MLO case */
>>> +	if (vif->valid_links) {
>>
>> better to use ieee80211_vif_is_mld() helper?
> 
> Indeed, fixed.
> 
> I did all the changes directly in the pending branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=57ee27f3b3aa13c63978f03ce544c2f4210a9cd7

LGTM

> 
> BTW when you reply please include an empty line between the quote and
> your reply, this improves readability.

sure, will keep in mind.

>