Re: [PATCH wireless-next v4 3/4] wifi: mac80211: implement STA-mode peer probing

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

> +	const u8 *peer_addr;

That seems vaguely confusing when we already have "peer", maybe you
should call this dst_addr to go with src_addr.

>  	/* the lock is needed to assign the cookie later */
>  	lockdep_assert_wiphy(local->hw.wiphy);
>  
> -	rcu_read_lock();
> -	sta = sta_info_get_bss(sdata, peer);
> -	if (!sta) {
> -		ret = -ENOLINK;
> -		goto unlock;
> -	}
> -
> -	qos = sta->sta.wme;
> -
> -	if (ieee80211_vif_is_mld(&sdata->vif)) {
> -		if (sta->sta.mlo) {
> -			link_id = IEEE80211_LINK_UNSPECIFIED;
> -		} else {
> -			/*
> -			 * For non-MLO clients connected to an AP MLD, band
> -			 * information is not used; instead, sta->deflink is
> -			 * used to send packets.
> -			 */
> -			link_id = sta->deflink.link_id;
> +	switch (sdata->vif.type) {
> +	case NL80211_IFTYPE_AP:
> +	case NL80211_IFTYPE_P2P_GO:
> +		sta = sta_info_get_bss(sdata, peer);
> +		if (!sta)
> +			return -ENOLINK;

I don't really understand why you move a bunch of the per-STA handling
into the switch?

> +		qos = sta->sta.wme;

This is definitely in all the branches, and must be there. Why not pull
it out?

> +		fromds = true;
> +		break;
> +
> +	case NL80211_IFTYPE_STATION:

(nit: spurious blank line)

> +	case NL80211_IFTYPE_P2P_CLIENT:

Both of the P2P cases aren't needed here and are just confusing.

> +		link_id = IEEE80211_LINK_UNSPECIFIED;
> +		peer_addr = sdata->vif.cfg.ap_addr;
> +		src_addr = sdata->vif.addr;
> +		if (!ieee80211_vif_is_mld(&sdata->vif)) {
> +			chanctx_conf = wiphy_dereference(local->hw.wiphy,
> +							 sdata->vif.bss_conf.chanctx_conf);
> +			if (WARN_ON(!chanctx_conf))
> +				return -EINVAL;

(that WARN_ON could perhaps be triggered since you didn't check for the
STA first?)

> +			band = chanctx_conf->def.chan->band;
> +		} else {
> +			band = 0;
>  		}
> -		band = chanctx_conf->def.chan->band;
> -		link_id = 0;
> +		sta = sta_info_get(sdata, peer_addr);
> +		if (!sta)
> +			return -ENOLINK;
> +		qos = sta->sta.wme;

At the very least you could pull out 'qos = sta->sta.wme', but I wonder
if you could pull out more of the sta lookup too by just saying

	if (vif.type == NL80211_IFTYPE_STATION)
		peer = sdata->vif.cfg.ap_addr;

and then leaving more of the current behaviour intact. Even the MLO link
thing could be left since it won't actually be able to be used since the
AP will be MLO/non-MLO with the vif, unlike in AP mode.

IOW, it feels like with that you should get away with far less
difference between AP and client, perhaps no other than this and the DS
bits.

johannes