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

[Kalle Valo <kvalo@kernel.org>]

Aditya Kumar Singh <quic_adisi@quicinc.com> writes:

> On 10/29/24 21:08, Kalle Valo wrote:
>> + aditya
>> Jeff Johnson <quic_jjohnson@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?

Super clear :) Thanks!

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

Looks good to me, I'll add that if it's ok for Jeff as well.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches