On 11/13/2024 1:10 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@xxxxxxxxxxx> 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.
here this is a different situation from that in the other email you referred to: we are clearing an RCU pointer here, so a synchronize_rcu() is necessary for the purpose of safely freeing arsta. But ideally the code flow should look like:
rcu_assign_pointer(ahsta->link[link_id], NULL)
synchronize_rcu()
if (arsta != &ahsta->deflink)
kfree(arsta);
However the patch is doing nothing after synchronize_rcu().
I know the actual free happens immediately after ath12k_mac_unassign_link_sta() returns, so there should be no problem. But it really looks odd to get them split.
>
>>> +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.
>
>> 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().
>
