On 11/21/2024 3:32 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@xxxxxxxxxxx> 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);
below synchronize_rcu() should be moved to here, such that any change to arsta can happen only AFTER all existing RCU readers finish accessing it.
>
> if (arsta == &ahsta->deflink) {
> arsta->link_id = ATH12K_INVALID_LINK_ID;
> arsta->ahsta = NULL;
> arsta->arvif = NULL;
> return;
> }
>
> synchronize_rcu();
> kfree(arsta);
> }
other than that this change looks good to me.
>
> BTW your lines are really long, please check your settings:
sure, will check.
>
> https://patchwork.kernel.org/project/linux-wireless/patch/20241106142617.660901-2-kvalo@xxxxxxxxxx/
>
