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>
	<2e706d58-5d83-4867-9963-c62441cdd4da@quicinc.com>
	<87y11o2x9h.fsf@kernel.org>
	<97f3f465-6ebd-4ca1-b672-4c8c7f42220d@quicinc.com>
Date: Wed, 20 Nov 2024 21:32:50 +0200
In-Reply-To: <97f3f465-6ebd-4ca1-b672-4c8c7f42220d@quicinc.com> (Baochen
	Qiang's message of "Wed, 13 Nov 2024 10:55:22 +0800")
Message-ID: <87bjy9zonx.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:

>>>> +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