Index | Thread | Search

From:
Marcus Glocker <marcus@nazgul.ch>
Subject:
Re: sys/qwz: handle QoS in native WiFi frames
To:
"Kirill A. Korinsky" <kirill@korins.ky>
Cc:
tech@openbsd.org
Date:
Fri, 22 May 2026 21:13:04 +0200

Download raw body.

Thread
On Mon, May 18, 2026 at 07:02:14PM +0200, Kirill A. Korinsky wrote:

> tech@,
> 
> I'd like to qebuild the QoS control field for native WiFi RX frames from
> descriptor metadata before passing them to net80211.
> 
> On TX, strip the hostside QoS control field when using native WiFi
> encapsulation, after deriving the TID for the firmware descriptor.
> 
> Unifi at least stop complaining that AP/Client signal balance is poor.
> 
> Ok?

In general the approach looks fine, and no regression found with my qwz.

A few comments inline.
 
> Index: sys/dev/ic/qwz.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/qwz.c,v
> diff -u -p -r1.29 qwz.c
> --- sys/dev/ic/qwz.c	18 May 2026 13:47:32 -0000	1.29
> +++ sys/dev/ic/qwz.c	18 May 2026 17:01:31 -0000
> @@ -14926,13 +14926,67 @@ void
>  qwz_dp_rx_h_undecap_nwifi(struct qwz_softc *sc, struct qwz_rx_msdu *msdu,
>      uint8_t *first_hdr, enum hal_encrypt_type enctype)
>  {
> -	/*
> -	* This function will need to do some work once we are receiving
> -	* aggregated frames. For now, it needs to do nothing.
> -	*/
> +	struct rx_mpdu_start_qcn9274 *mpdu;
> +	struct ieee80211_frame *wh;
> +	struct mbuf *m = msdu->m;
> +	uint8_t decap_hdr[IEEE80211_MAX_FRAME_HDR_LEN];
> +	size_t hdrlen;
> +	uint16_t qos_ctl;
>  
> -	if (!msdu->is_first_msdu)
> -		printf("%s: not implemented\n", __func__);
> +	if (m == NULL)
> +		return;
> +
> +	if (m->m_len < sizeof(*wh) &&
> +	    (m = m_pullup(m, sizeof(*wh))) == NULL) {
> +		msdu->m = NULL;
> +		return;
> +	}
> +	msdu->m = m;
> +
> +	mpdu = &msdu->rx_desc->u.wcn7850.mpdu_start;
> +	wh = mtod(m, struct ieee80211_frame *);
> +	if ((mpdu->info6 & RX_MPDU_START_INFO6_NON_QOS) ||
> +	    ieee80211_has_qos(wh))
> +		return;
> +
> +	hdrlen = ieee80211_get_hdrlen(wh);
> +	if (hdrlen > sizeof(decap_hdr))
> +		return;
> +
> +	if (m->m_len < hdrlen &&
> +	    (m = m_pullup(m, hdrlen)) == NULL) {
> +		msdu->m = NULL;
> +		return;
> +	}
> +	msdu->m = m;
> +
> +	wh = mtod(m, struct ieee80211_frame *);
> +	hdrlen = ieee80211_get_hdrlen(wh);
> +	if (hdrlen > sizeof(decap_hdr))
> +		return;
> +
> +	memcpy(decap_hdr, wh, hdrlen);
> +	wh = (struct ieee80211_frame *)decap_hdr;
> +	wh->i_fc[0] |= IEEE80211_FC0_SUBTYPE_QOS;
> +	wh->i_fc[1] &= ~IEEE80211_FC1_ORDER;
> +	qos_ctl = mpdu->qos_ctrl;

The Linux ath12k driver is only rebuilding the qos_ctl field from the
TID, plus applying the optional mesh bit:

	qos_ctl = rxcb->tid;
	if (rx_info->mesh_ctrl_present)
		qos_ctl |= IEEE80211_QOS_CTL_MESH_CONTROL_PRESENT;

While you are copying the entire hardware captured QoS field.
Is there a specific reason for that?

For example, the hardware should already de-aggregate A-MSDU in to
separate MSDUs, and a copied over A-MSDU bit could make net80211
mis-parse a plain MSDU as aggregated.

> +
> +	m_adj(m, hdrlen);
> +
> +	M_PREPEND(m, sizeof(qos_ctl), M_DONTWAIT);
> +	if (m == NULL) {
> +		msdu->m = NULL;
> +		return;
> +	}
> +	memcpy(mtod(m, void *), &qos_ctl, sizeof(qos_ctl));
> +
> +	M_PREPEND(m, hdrlen, M_DONTWAIT);
> +	if (m == NULL) {
> +		msdu->m = NULL;
> +		return;
> +	}
> +	msdu->m = m;
> +	memcpy(mtod(m, void *), decap_hdr, hdrlen);
>  }
>  
>  void
> @@ -22907,6 +22961,46 @@ qwz_dp_tx_get_tid(struct mbuf *m)
>  	return ieee80211_get_qos(wh) & IEEE80211_QOS_TID;
>  }
>  
> +int
> +qwz_dp_tx_encap_nwifi(struct mbuf **mp)
> +{
> +	struct mbuf *m = *mp;
> +	struct ieee80211_frame *wh;
> +	uint8_t *qos_ctl;
> +	int hdrlen;
> +
> +	if (m->m_len < sizeof(*wh) &&
> +	    (m = m_pullup(m, sizeof(*wh))) == NULL)
> +		return ENOBUFS;

You miss the mp = NULL, otherwise we have a double free on return.

> +
> +	wh = mtod(m, struct ieee80211_frame *);
> +	if (!ieee80211_has_qos(wh)) {
> +		*mp = m;
> +		return 0;
> +	}
> +
> +	hdrlen = ieee80211_get_hdrlen(wh);
> +	if (m->m_len < hdrlen &&
> +	    (m = m_pullup(m, hdrlen)) == NULL)
> +		return ENOBUFS;

Same.

> +
> +	wh = mtod(m, struct ieee80211_frame *);
> +	if (ieee80211_has_addr4(wh))
> +		qos_ctl = ((struct ieee80211_qosframe_addr4 *)wh)->i_qos;
> +	else
> +		qos_ctl = ((struct ieee80211_qosframe *)wh)->i_qos;
> +
> +	memmove(mtod(m, uint8_t *) + sizeof(uint16_t), mtod(m, uint8_t *),
> +	    qos_ctl - mtod(m, uint8_t *));
> +	m_adj(m, sizeof(uint16_t));
> +
> +	wh = mtod(m, struct ieee80211_frame *);
> +	wh->i_fc[0] &= ~IEEE80211_FC0_SUBTYPE_QOS;
> +
> +	*mp = m;
> +	return 0;
> +}
> +
>  /*
>   * Build a WCN7850 / ath12k WiFi7 TCL data descriptor. Encap/encrypt/
>   * search settings live in the bank addressed by ti->bank_id; the
> @@ -23103,28 +23197,14 @@ qwz_dp_tx(struct qwz_softc *sc, struct q
>  	ti.flags1 |= FIELD_PREP(HAL_TCL_DATA_CMD_INFO3_TID_OVERWRITE, 1);
>  
>  	ti.tid = qwz_dp_tx_get_tid(m);
> -#if 0
> -	switch (ti.encap_type) {
> -	case HAL_TCL_ENCAP_TYPE_NATIVE_WIFI:
> -		ath12k_dp_tx_encap_nwifi(skb);
> -		break;
> -	case HAL_TCL_ENCAP_TYPE_RAW:
> -		if (!test_bit(ATH12K_FLAG_RAW_MODE, &ab->dev_flags)) {
> -			ret = -EINVAL;
> -			goto fail_remove_idr;
> +	if (ti.encap_type == HAL_TCL_ENCAP_TYPE_NATIVE_WIFI) {
> +		ret = qwz_dp_tx_encap_nwifi(&m);
> +		if (ret) {
> +			m_freem(m);
> +			return ret;
>  		}
> -		break;
> -	case HAL_TCL_ENCAP_TYPE_ETHERNET:
> -		/* no need to encap */
> -		break;
> -	case HAL_TCL_ENCAP_TYPE_802_3:
> -	default:
> -		/* TODO: Take care of other encap modes as well */
> -		ret = -EINVAL;
> -		atomic_inc(&ab->soc_stats.tx_err.misc_fail);
> -		goto fail_remove_idr;
>  	}
> -#endif
> +
>  	ret = bus_dmamap_load_mbuf(sc->sc_dmat, tx_data->map,
>  	    m, BUS_DMA_WRITE | BUS_DMA_NOWAIT);
>  	if (ret && ret != EFBIG) {
> 
> -- 
> wbr, Kirill
>