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

[Kalle Valo <kvalo@kernel.org>]

Baochen Qiang <quic_bqiang@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.

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

>> +	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()?

> 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().

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

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