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

[Baochen Qiang]

On 11/13/2024 1:10 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang at quicinc.com> writes:
> 
>> On 11/6/2024 10:26 PM, Kalle Valo wrote:
>>> +static void ath12k_mac_unassign_link_sta(struct ath12k_hw *ah,
>>> +					 struct ath12k_sta *ahsta,
>>> +					 u8 link_id)
>>> +{
>>> +	lockdep_assert_wiphy(ah->hw->wiphy);
>>> +
>>> +	ahsta->links_map &= ~BIT(link_id);
>>> +	rcu_assign_pointer(ahsta->link[link_id], NULL);
>>> +
>>> +	synchronize_rcu();
>>
>> this looks strange: generally we call synchronize_rcu() to wait for
>> any RCU readers to finish, such that we can then safely free
>> something. but here we do nothing ...
> 
> Same comment as in the other email, this is to make sure that we don't
> continue the mac80211 call flow before all readers have the new value.
> Is that a problem? And we can always optimise later.
here this is a different situation from that in the other email you referred to: we are clearing an RCU pointer here, so a synchronize_rcu() is necessary for the purpose of safely freeing arsta. But ideally the code flow should look like:

	rcu_assign_pointer(ahsta->link[link_id], NULL)	
	synchronize_rcu()
	if (arsta != &ahsta->deflink)
		kfree(arsta);

However the patch is doing nothing after synchronize_rcu().

I know the actual free happens immediately after ath12k_mac_unassign_link_sta() returns, so there should be no problem. But it really looks odd to get them split.

> 
>>> +static void ath12k_mac_free_unassign_link_sta(struct ath12k_hw *ah,
>>> +					      struct ath12k_sta *ahsta,
>>> +					      u8 link_id)
>>> +{
>>> +	struct ath12k_link_sta *arsta;
>>> +
>>> +	lockdep_assert_wiphy(ah->hw->wiphy);
>>> +
>>> +	if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
>>> +		return;
>>> +
>>> +	arsta = wiphy_dereference(ah->hw->wiphy, ahsta->link[link_id]);
>>> +
>>> +	if (WARN_ON(!arsta))
>>> +		return;
>>> +
>>> +	ath12k_mac_unassign_link_sta(ah, ahsta, link_id);
>>> +
>>> +	arsta->link_id = ATH12K_INVALID_LINK_ID;
>>> +	arsta->ahsta = NULL;
>>> +	arsta->arvif = NULL;
>>
>> if arsta is not deflink and would be freed, can we avoid these
>> cleanup?
> 
> I think that's something we can cleanup later if needed. Sure, it's
> extra assignments but it's not really doing any harm.
exactly, but ideally we should avoid unnecessary effort if possible.

> 
>>> +	if (arsta != &ahsta->deflink)
>>> +		kfree(arsta);
>>
>> I know the actual free happens here, but why split them?
> 
> You mean why have a separate function ath12k_mac_unassign_link_sta() and
> instead just have all code the in ath12k_mac_free_unassign_link_sta()?
yes. such that we can have synchronize_rcu() and kfree() located together.

> 
>> these two hunks give me the impression that we may (in the future?)
>> have cases to call ath12k_mac_unassign_link_sta() alone somewhere else
>> rather than directly calling ath12k_mac_free_unassign_link_sta(). am I
>> feeling right? what are those cases?
> 
> At least I'm not aware of anything else calling
> ath12k_mac_unassign_link_sta(). So I'll just remove that function and
> move the code to ath12k_mac_free_unassign_link_sta().
>