[PATCH 3/8] wifi: ath12k: Refactor sta state machine

[Aditya Kumar Singh]

On 10/29/24 21:08, Kalle Valo wrote:
> + aditya
> 
> Jeff Johnson <quic_jjohnson at quicinc.com> writes:
> 
>>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar,
>>> +					  struct ath12k_link_vif *arvif,
>>> +					  struct ath12k_link_sta *arsta)
>>> +{
>>> +	struct ath12k_peer *peer;
>>> +	int ret;
>>> +
>>> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>> +
>>> +	spin_lock_bh(&ar->ab->base_lock);
>>> +
>>> +	peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
>>> +	if (peer)
>>> +		peer->is_authorized = false;
>>> +
>>> +	spin_unlock_bh(&ar->ab->base_lock);
>>> +
>>> +	/* Driver should clear the peer keys during mac80211's ref ptr
>>> +	 * gets cleared in __sta_info_destroy_part2 (trans from
>>> +	 * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
>>
>> I'm unable to understand this comment
> 
> Indeed, that's weird. Aditya, do you have any idea what the comment is
> trying to say?
> 

At present, ath12k clear the keys in ath12k_station_disassoc() which 
gets executed in state change from IEEE80211_STA_ASSOC to 
IEEE80211_STA_AUTH.

However, in mac80211, once the station moves from 
IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC itself, the keys are 
deleted. Please see - __sta_info_destroy_part2() -> 
ieee80211_free_sta_keys().

Now, ath12k peer object (struct ath12k_peer) holds the key reference 
from mac80211 (see ath12k_peer::keys[]). Hence, once mac80211 deletes 
the key, driver should not keep a reference to it or else it could lead 
to issues.

Therefore, it is important that the driver should clear the peer keys 
during transition from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC 
it self since we know that once we return from here, mac80211 is going 
to remove the keys.

ath12k_mac_station_unauthorize() gets called when station moves from 
state IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC hence call to 
ath12k_clear_peer_keys() is moved from ath12k_station_disassoc() to 
ath12k_mac_station_unauthorize().

Is this clear now?

May be the comment in the code could be re-written as below?

/* Driver must clear the keys during the state change from
  * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after
  * returning from here, mac80211 is going to delete the keys
  * in __sta_info_destroy_part2(). This will ensure that the driver does
  * not retain stale key references after mac80211 deletes the keys.
  */

-- 
Aditya