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,
>