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

[Baochen Qiang <quic_bqiang@quicinc.com>]

On 11/21/2024 3:32 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
> 
>>>>> +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.
> 
> Ok, I think I now get what you mean. Does this look better:
> 
> 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;
> 
> 	ahsta->links_map &= ~BIT(link_id);
> 	rcu_assign_pointer(ahsta->link[link_id], NULL);
below synchronize_rcu() should be moved to here, such that any change to arsta can happen only AFTER all existing RCU readers finish accessing it.

> 
> 	if (arsta == &ahsta->deflink) {
> 		arsta->link_id = ATH12K_INVALID_LINK_ID;
> 		arsta->ahsta = NULL;
> 		arsta->arvif = NULL;
> 		return;
> 	}
> 
> 	synchronize_rcu();
> 	kfree(arsta);
> }
other than that this change looks good to me.

> 
> BTW your lines are really long, please check your settings:
sure, will check.

> 
> https://patchwork.kernel.org/project/linux-wireless/patch/20241106142617.660901-2-kvalo@kernel.org/
>