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

[Remi Pommarel]

On Tue, Feb 17, 2026 at 05:00:56PM +0100, Johannes Berg wrote:
> 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?

I was going to say it would not be a no-op for a buf_size update but,
if I understand correctly, even when SUPPORTS_REORDERING_BUFFER is not
set the buf_size update is ignored completely and the reorder_buf is
not resized yet a successful addba response is sent. Don't we need to
check for buf_size change as well as timeout also?

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

Just to confirm, you mean that updating the timeout is not worth it, but
fixing this issue is still needed right?

-- 
Remi