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

[Baochen Qiang]

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

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

> +
> +	if (arsta != &ahsta->deflink)
> +		kfree(arsta);
I know the actual free happens here, but why split them?

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?

> +}
> +