> Date: Mon, 24 Feb 2025 10:26:24 +0100
> From: Stefan Sperling <stsp@stsp.name>
> 
> 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,
> > 
>