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

[Kalle Valo <kvalo@kernel.org>]

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);

	if (arsta == &ahsta->deflink) {
		arsta->link_id = ATH12K_INVALID_LINK_ID;
		arsta->ahsta = NULL;
		arsta->arvif = NULL;
		return;
	}

	synchronize_rcu();
	kfree(arsta);
}

BTW your lines are really long, please check your settings:

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

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

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