[PATCH 1/8] wifi: ath12k: Add MLO station state change handling

[Baochen Qiang]

On 11/13/2024 12:03 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang at 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. 
why would readers have to access arsta BEFORE returning to mac80211? any potential race conditions?

and even if it is necessary, how could synchronize_rcu() guarantee that? synchronize_rcu() is used to make sure all readers have exited from RCU read critical section, I don;t see how it could make sure a newly assigned entry get accessed.

> The delay shouldn't be that long anyway, I hope.
>