Re: [PATCH wireless] wifi: mac80211: Fix ADDBA update when HW supports reordering

[Johannes Berg]

On Tue, 2026-02-17 at 15:38 +0100, Remi Pommarel wrote:
> On Tue, Feb 17, 2026 at 02:59:34PM +0100, Johannes Berg wrote:
> > On Tue, 2026-02-17 at 14:05 +0100, Remi Pommarel wrote:
> > > On Tue, Feb 17, 2026 at 12:30:08PM +0100, Johannes Berg wrote:
> > > > On Tue, 2026-02-17 at 11:36 +0100, Remi Pommarel wrote:
> > > > > Commit f89e07d4cf26 ("mac80211: agg-rx: refuse ADDBA Request with timeout
> > > > > update") added a check to fail when ADDBA update would change the
> > > > > timeout param.
> > > > > 
> > > > > This param is kept in tid_ampdu_rx context which is only allocated on HW
> > > > > that do not set SUPPORTS_REORDERING_BUFFER. Because the timeout check
> > > > > was done regardless of this param, ADDBA update always failed on those
> > > > > HW.
> > > > 
> > > > Seems like a legit problem, but
> > > > 
> > > > > Fix this by only checking tid_ampdu_rx->timeout only when
> > > > > SUPPORTS_REORDERING_BUFFER is not set.
> > > > 
> > > > that doesn't seem right? Especially the way you implemented it, it won't
> > > > even respond at all when it's an update and SUPPORTS_REORDERING_BUFFER
> > > > is set.
> > > 
> > > I could be wrong but I think the patch format here make it difficult to
> > > read. If it's an update and SUPPORTS_REORDERING_BUFFER is set, the
> > > following "if" in the code (not fully visible in the diff here) will end
> > > calling drv_ampdu_action().
> > 
> > Yes, but it will be IEEE80211_AMPDU_RX_START which isn't really intended
> > by the state machine/API between mac80211/driver. So that doesn't seem
> > right.
> > 
> 
> That does make sense. However, if I understand correctly, it means that
> even if we end up storing the timeout for drivers that support
> reordering, a new IEEE80211_AMPDU_RX_UPDATE should still be introduced,
> right?

I guess in order to do a no-op update that doesn't change the timeout,
we could? But not sure it's all worth it?

Pablo seems to have looked up that it _is_ supported - which I didn't
expect because it's not part of the API contract, so the drivers
implemented something that can't actually ever get hit? Seems odd. And
I'm pretty sure e.g. iwlwifi wouldn't support it.

But I basically also think it's not worth it in practice; why try to
support something that's never going to happen?

johannes