Re: [PATCH wireless-next v2 3/4] wifi: mac80211: add per-link PROBE_PEER support

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2026-04-17 at 19:01 +0530, Priyansha Tiwari wrote:
> 
> -			u8 pad2;
> +			u8 link_valid:1, link_id:4, pad2:3;

You don't need to pad bitfields.

> +	if (sdata->vif.type == NL80211_IFTYPE_STATION ||
> +	    sdata->vif.type == NL80211_IFTYPE_P2P_CLIENT) {

Err ...

> +	/* STA/P2P: userspace must NOT provide peer MAC, AP is implied.	 */

> +		if (peer)
> +			return -EINVAL;

That check belongs into cfg80211, not here?

> +		if (!sdata->u.mgd.associated)
> +			return -ENOTCONN;

maybe be consistent, cfg80211 returns -ENOLINK?

> +	} else if (!peer) {
> +		/* AP/GO: must have a peer MAC. */
> +		return -EINVAL;
>  	}
>  
> -	qos = sta->sta.wme;
> +	guard(rcu)();
>  
> -	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;
> +	if (sdata->vif.type == NL80211_IFTYPE_AP ||
> +	    sdata->vif.type == NL80211_IFTYPE_P2P_GO) {

Again, what?

And unrelated, maybe this would all be better as a switch statement?

> +		sta = sta_info_get_bss(sdata, peer);
> +		if (!sta)
> +			return -ENOLINK;
>  
> -			conf = rcu_dereference(sdata->vif.link_conf[link_id]);
> +		qos = sta->sta.wme;
>  
> -			if (unlikely(!conf)) {
> -				ret = -ENOLINK;
> -				goto unlock;
> +		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,
> +				 * use the link address for the client's link.
> +				 */
> +				link_id = sta->deflink.link_id;
> +				conf = rcu_dereference(sdata->vif.link_conf[link_id]);
> +				if (unlikely(!conf))
> +					return -ENOLINK;
>  			}
> +			/* MLD transmissions must not rely on the band */
> +			band = 0;
> +		} else {
> +			chanctx_conf = rcu_dereference(sdata->vif.bss_conf.chanctx_conf);
> +			if (WARN_ON(!chanctx_conf))
> +				return -EINVAL;
> +			band = chanctx_conf->def.chan->band;
> +			link_id = 0;
> +		}
> +
> +		size = sizeof(*nullfunc);
> +		fc = cpu_to_le16(IEEE80211_FTYPE_DATA |
> +				 (qos ? IEEE80211_STYPE_QOS_NULLFUNC
> +				      : IEEE80211_STYPE_NULLFUNC) |
> +				 IEEE80211_FCTL_FROMDS);
> +		if (!qos)
> +			size -= 2;
> +
> +		skb = dev_alloc_skb(local->hw.extra_tx_headroom + size);
> +		if (!skb)
> +			return -ENOMEM;
> +
> +		skb->dev = dev;
> +		skb_reserve(skb, local->hw.extra_tx_headroom);
> +
> +		nullfunc = skb_put(skb, size);
> +		memset(nullfunc, 0, size);
> +		nullfunc->frame_control = fc;
> +
> +		memcpy(nullfunc->addr1, sta->sta.addr, ETH_ALEN);
> +		if (ieee80211_vif_is_mld(&sdata->vif) && !sta->sta.mlo) {
> +			memcpy(nullfunc->addr2, conf->addr, ETH_ALEN);
> +			memcpy(nullfunc->addr3, conf->addr, ETH_ALEN);
> +		} else {
> +			memcpy(nullfunc->addr2, sdata->vif.addr, ETH_ALEN);
> +			memcpy(nullfunc->addr3, sdata->vif.addr, ETH_ALEN);
> +		}
> +
> +		info = IEEE80211_SKB_CB(skb);
> +		info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS |
> +			       IEEE80211_TX_INTFL_NL80211_FRAME_TX;
> +		info->band = band;
> +		info->control.flags |= u32_encode_bits(link_id,
> +						       IEEE80211_TX_CTRL_MLO_LINK);
> +
> +		skb_set_queue_mapping(skb, IEEE80211_AC_VO);
> +		skb->priority = 7;
> +		if (qos)
> +			nullfunc->qos_ctrl = cpu_to_le16(7);
> +
> +		ret = ieee80211_attach_ack_skb(local, skb, cookie, GFP_ATOMIC);
> +		if (ret) {
> +			kfree_skb(skb);
> +			return ret;
>  		}
> -		/* MLD transmissions must not rely on the band */
> +
> +		local_bh_disable();
> +		ieee80211_xmit(sdata, sta, skb);
> +		local_bh_enable();
> +
> +		return 0;
> +	}
> +
> +	/*
> +	 * STA/P2P: send a nullfunc to probe the AP/peer.
> +	 * For MLO, let the driver/firmware decide which link to use.
> +	 */
> +	if (ieee80211_vif_is_mld(&sdata->vif)) {
> +		link_id = IEEE80211_LINK_UNSPECIFIED;
> +		peer_addr = sdata->vif.cfg.ap_addr;
> +		src_addr = sdata->vif.addr;
>  		band = 0;
> +		sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
>  	} else {
> -		chanctx_conf = rcu_dereference(sdata->vif.bss_conf.chanctx_conf);
> -		if (WARN_ON(!chanctx_conf)) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -		band = chanctx_conf->def.chan->band;
>  		link_id = 0;
> +		conf = rcu_dereference(sdata->vif.link_conf[0]);
> +		if (!conf)
> +			return -ENOLINK;
> +		band = conf->chanreq.oper.chan->band;
> +		peer_addr = conf->bssid;
> +		src_addr = conf->addr;
> +		sta = sta_info_get_bss(sdata, peer_addr);
>  	}

I really don't like the layout and code duplication here. Off-hand I'd
think the duplication isn't needed, but otherwise perhaps it should be
in separate functions or so.

johannes