Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: sys/ieee80211: support of uAPSD; sys/iwx: enable uAPSD when supported by AP
To:
tech@openbsd.org
Date:
Mon, 18 May 2026 10:40:08 +0200

Download raw body.

Thread
  • Kirill A. Korinsky:

    sys/ieee80211: support of uAPSD; sys/iwx: enable uAPSD when supported by AP

  • This looks great. I mostly have small cosmetic concerns which could
    be fixed after commit. And I spotted one potential problem which
    could cause regressions in older drivers. See below.
    
    On Mon, May 18, 2026 at 01:08:00AM +0200, Kirill A. Korinsky wrote:
    > +static uint8_t
    > +iwx_uapsd_ac_flags(struct ieee80211_node *ni)
    > +{
    > +	uint8_t ac_flags = 0;
    > +
    > +	if (ni->ni_uapsd_ac & IEEE80211_WMM_IE_STA_QOSINFO_AC_VO)
    > +		ac_flags |= 1 << 0;
    
    Could we please avoid magic numbers by using EDCA_AC_BE here instead of 0?
    And similarly for the following lines:
    
    > +	if (ni->ni_uapsd_ac & IEEE80211_WMM_IE_STA_QOSINFO_AC_VI)
    > +		ac_flags |= 1 << 1;
    > +	if (ni->ni_uapsd_ac & IEEE80211_WMM_IE_STA_QOSINFO_AC_BE)
    > +		ac_flags |= 1 << 2;
    > +	if (ni->ni_uapsd_ac & IEEE80211_WMM_IE_STA_QOSINFO_AC_BK)
    > +		ac_flags |= 1 << 3;
    > +
    > +	return ac_flags;
    > +}
    
    > @@ -6673,6 +6740,21 @@ iwx_set_pslevel(struct iwx_softc *sc, in
    >  		    IWX_POWER_FLAGS_POWER_MANAGEMENT_ENA_MSK);
    >  		mcmd.rx_data_timeout = htole32(pmgt->rxtimeout * 1024);
    >  		mcmd.tx_data_timeout = htole32(pmgt->txtimeout * 1024);
    > +		if (ni->ni_uapsd &&
    > +		    (sc->sc_capaflags & IWX_UCODE_TLV_FLAGS_UAPSD_SUPPORT)) {
    
    For clarity, could we rename ni->ni_uapsd to ni->ni_use_uapsd or similar?
    Or make it a flag such as ni->ni_flags & IEEE80211_NODE_UAPSD?
    
    This would provide a very obvious indication to readers of this code
    that ni_uapsd is a configuration flag, rather than some bit of data which
    was parsed from the node's beacon (or assoc request in hostap mode).
    
    > @@ -1601,6 +1611,9 @@ ieee80211_get_assoc_req(struct ieee80211
    >  		    IEEE80211_HE_MCS_NSS_SIZE(ic->ic_he_phy_cap[0]);
    >  	}
    >  
    > +	addwme = (ni->ni_flags & IEEE80211_NODE_QOS) ||
    > +	    (ic->ic_flags & IEEE80211_F_HTON);
    
    Shouldn't this be && instead of || ?
    
    So far we've only enabled WME in 11n and 11ac modes.
    I don't think enabling WME in 11a/b/g only mode (and drivers) makes sense,
    even if the peer supports QoS. Because this could break some of the older
    drivers, which are not being tested very often.
    
    
  • Kirill A. Korinsky:

    sys/ieee80211: support of uAPSD; sys/iwx: enable uAPSD when supported by AP