From: Kalle Valo <kvalo@kernel.org>
To: Baochen Qiang <quic_bqiang@quicinc.com>
Cc: <ath12k@lists.infradead.org>,  <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH 1/8] wifi: ath12k: Add MLO station state change handling
References: <20241106142617.660901-1-kvalo@kernel.org>
	<20241106142617.660901-2-kvalo@kernel.org>
	<f75d7222-514b-4c6f-985d-e6ffd67e317a@quicinc.com>
Date: Tue, 12 Nov 2024 18:03:13 +0200
In-Reply-To: <f75d7222-514b-4c6f-985d-e6ffd67e317a@quicinc.com> (Baochen
	Qiang's message of "Thu, 7 Nov 2024 10:26:56 +0800")
Message-ID: <877c984eym.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> On 11/6/2024 10:26 PM, Kalle Valo wrote:
>> +static int ath12k_mac_assign_link_sta(struct ath12k_hw *ah,
>> +				      struct ath12k_sta *ahsta,
>> +				      struct ath12k_link_sta *arsta,
>> +				      struct ath12k_vif *ahvif,
>> +				      u8 link_id)
>> +{
>> +	struct ieee80211_sta *sta = ath12k_ahsta_to_sta(ahsta);
>> +	struct ieee80211_link_sta *link_sta;
>> +	struct ath12k_link_vif *arvif;
>> +
>> +	lockdep_assert_wiphy(ah->hw->wiphy);
>> +
>> +	if (!arsta || link_id >= IEEE80211_MLD_MAX_NUM_LINKS)
>> +		return -EINVAL;
>> +
>> +	arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[link_id]);
>> +	if (!arvif)
>> +		return -EINVAL;
>> +
>> +	memset(arsta, 0, sizeof(*arsta));
>> +
>> +	link_sta = wiphy_dereference(ah->hw->wiphy, sta->link[link_id]);
>> +	if (!link_sta)
>> +		return -EINVAL;
>> +
>> +	ether_addr_copy(arsta->addr, link_sta->addr);
>> +
>> +	/* logical index of the link sta in order of creation */
>> +	arsta->link_idx = ahsta->num_peer++;
>> +
>> +	arsta->link_id = link_id;
>> +	ahsta->links_map |= BIT(arsta->link_id);
>
> would be better to put this after rcu_assign_pointer()?

My thinking is that it's racy anyway so it doesn't really matter.
links_map is not really protected properly right now but luckily there's
only one function which accesses outside of the wiphy lock. My plan is
to fix that in a later patch.

>> +	arsta->arvif = arvif;
>> +	arsta->ahsta = ahsta;
>> +	arsta->state = IEEE80211_STA_NONE;
>> +	wiphy_work_init(&arsta->update_wk, ath12k_sta_rc_update_wk);
>> +
>> +	rcu_assign_pointer(ahsta->link[link_id], arsta);
>> +
>> +	synchronize_rcu();
>
> what are we waiting for here?

That's a good question. I didn't analyse that thoroughly but I'm just
making sure here that all readers have access to the new arsta before we
return to mac80211. The delay shouldn't be that long anyway, I hope.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches