From: Mark Kettenis Subject: Re: iwx(4) ampdu fix To: Stefan Sperling Cc: tech@openbsd.org Date: Mon, 24 Feb 2025 16:09:23 +0100 > Date: Mon, 24 Feb 2025 10:26:24 +0100 > From: Stefan Sperling > > 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... Entirely possible. I'm just copying what the Linux driver does. I'm happy to cap it at 64K instead as this is what my AP advertises. > 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. I'm testing with my fritzbox AP. It is an older model so probably only 11n. > > > 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, > > >