Re: [PATCH 2/8] wifi: ath12k: MLO vdev bringup changes

[Kalle Valo <kvalo@kernel.org>]

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