On 11/13/2024 12:03 AM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@xxxxxxxxxxx> writes:
>
>> On 11/6/2024 10:26 PM, Kalle Valo wrote:
>>> +static int ath12k_mac_assign_link_sta(struct ath12k_hw *ah,
>>> + struct ath12k_sta *ahsta,
>>> + struct ath12k_link_sta *arsta,
>>> + struct ath12k_vif *ahvif,
>>> + u8 link_id)
>>> +{
>>> + struct ieee80211_sta *sta = ath12k_ahsta_to_sta(ahsta);
>>> + struct ieee80211_link_sta *link_sta;
>>> + struct ath12k_link_vif *arvif;
>>> +
>>> + lockdep_assert_wiphy(ah->hw->wiphy);
>>> +
>>> + if (!arsta || link_id >= IEEE80211_MLD_MAX_NUM_LINKS)
>>> + return -EINVAL;
>>> +
>>> + arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[link_id]);
>>> + if (!arvif)
>>> + return -EINVAL;
>>> +
>>> + memset(arsta, 0, sizeof(*arsta));
>>> +
>>> + link_sta = wiphy_dereference(ah->hw->wiphy, sta->link[link_id]);
>>> + if (!link_sta)
>>> + return -EINVAL;
>>> +
>>> + ether_addr_copy(arsta->addr, link_sta->addr);
>>> +
>>> + /* logical index of the link sta in order of creation */
>>> + arsta->link_idx = ahsta->num_peer++;
>>> +
>>> + arsta->link_id = link_id;
>>> + ahsta->links_map |= BIT(arsta->link_id);
>>
>> would be better to put this after rcu_assign_pointer()?
>
> My thinking is that it's racy anyway so it doesn't really matter.
> links_map is not really protected properly right now but luckily there's
> only one function which accesses outside of the wiphy lock. My plan is
> to fix that in a later patch.
>
>>> + arsta->arvif = arvif;
>>> + arsta->ahsta = ahsta;
>>> + arsta->state = IEEE80211_STA_NONE;
>>> + wiphy_work_init(&arsta->update_wk, ath12k_sta_rc_update_wk);
>>> +
>>> + rcu_assign_pointer(ahsta->link[link_id], arsta);
>>> +
>>> + synchronize_rcu();
>>
>> what are we waiting for here?
>
> That's a good question. I didn't analyse that thoroughly but I'm just
> making sure here that all readers have access to the new arsta before we
> return to mac80211.
why would readers have to access arsta BEFORE returning to mac80211? any potential race conditions?
and even if it is necessary, how could synchronize_rcu() guarantee that? synchronize_rcu() is used to make sure all readers have exited from RCU read critical section, I don;t see how it could make sure a newly assigned entry get accessed.
> The delay shouldn't be that long anyway, I hope.
>
