From: Stefan Sperling Subject: Re: iwx(4) ampdu fix To: Mark Kettenis Cc: tech@openbsd.org Date: Mon, 24 Feb 2025 10:26:24 +0100 On Sun, Feb 23, 2025 at 01:27:14PM +0100, Mark Kettenis wrote: > My rev 83 "MLD" firmware iwx(4) doesn't work as well at home as it did > in the hackroom in Valencia. There is significant packet loss. So I > started looking again at the new code to see if I missed some things. > One thing I missed is correctly setting the AMDPU density parameter. Indeed. Setting A-MPDU density to zero means "no restriction" regarding the timing of sub-frame transmissions. This could lead to packet loss in case the AP requires a gap between subframes. > I also noticed that the Linux driver uses a larger maximum aggregation > length with a not that this is the maximum size supported by the > firmware. So I bumped that too. In 11n mode, we ever won't reach A-MPDU sizes greater than 1024K. I am not sure where 2M and 4M fit in. Maybe these are already possible in 11ac, but perhaps they require 11ax with multi-TID A-MPDUs? The 11ac spec is a bit of a maze... In any case, what really matters is that we cap this size to the value reported by the peer, such that firmware won't generate aggregates which are too long for the peer to receive. Since your patch is only extending the possible maximum, I don't see a problem. We advertise 64k as our own maximum A-MPDU size. So right now the maximum values set by our driver is equal for Rx and Tx. I don't know whether firmware requires these values to match in its Tx/Rx paths. Hopefully not. In case firmware panics start popping up after this change we should try to bump our maximum Rx aggregation limit as well (in ic->ic_ampdu_params). > I also fixed a missing htole32(), although that will make not > difference on amd64. > > The diff doesn't completely fix things; there is still significant > packet loss (also in non-HT mode). But it appears to make things a > little bit better. Out of curiousity, which AP are you testing with? If it's an athn(4) hostap then I'm afraid there might be bugs on our athn driver which cause packet loss. It's better to test such behaviour against commercial 11n/11ac APs. > ok? ok stsp@ > Index: dev/pci/if_iwx.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/if_iwx.c,v > retrieving revision 1.189 > diff -u -p -r1.189 if_iwx.c > --- dev/pci/if_iwx.c 4 Feb 2025 09:15:04 -0000 1.189 > +++ dev/pci/if_iwx.c 23 Feb 2025 12:26:47 -0000 > @@ -6912,8 +6912,8 @@ iwx_mld_add_sta_cmd(struct iwx_softc *sc > struct ieee80211com *ic = &sc->sc_ic; > struct iwx_link_config_cmd link_cmd; > struct iwx_mvm_sta_cfg_cmd sta_cmd; > - uint32_t aggsize; > - const uint32_t max_aggsize = (IWX_STA_FLG_MAX_AGG_SIZE_64K >> > + uint32_t aggsize, mpdu_dens; > + const uint32_t max_aggsize = (IWX_STA_FLG_MAX_AGG_SIZE_4M >> > IWX_STA_FLG_MAX_AGG_SIZE_SHIFT); > int err, changes; > > @@ -6970,6 +6970,9 @@ iwx_mld_add_sta_cmd(struct iwx_softc *sc > if (iwx_mimo_enabled(sc)) > sta_cmd.mimo = htole32(1); > > + mpdu_dens = (in->in_ni.ni_ampdu_param & > + IEEE80211_AMPDU_PARAM_SS) >> 2; > + > if (in->in_ni.ni_flags & IEEE80211_NODE_VHT) { > aggsize = (in->in_ni.ni_vhtcaps & > IEEE80211_VHTCAP_MAX_AMPDU_LEN_MASK) >> > @@ -6981,8 +6984,8 @@ iwx_mld_add_sta_cmd(struct iwx_softc *sc > if (aggsize > max_aggsize) > aggsize = max_aggsize; > > - sta_cmd.tx_ampdu_spacing = htole32(0); > - sta_cmd.tx_ampdu_max_size = aggsize; > + sta_cmd.tx_ampdu_spacing = htole32(mpdu_dens); > + sta_cmd.tx_ampdu_max_size = htole32(aggsize); > } > > return iwx_send_cmd_pdu(sc, >