Re: [PATCH wireless-next] wifi: mac80211: Fix ADDBA request rejection after MLD link removal

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

Sorry, I've definitely fallen behind on some topics.

> With link reconfiguration, not handling the update requests sent on the 
> remaining links causes throughput drop, that's why went with handling 
> the update requests even-though the session is valid.

I'm not sure I see the relation right now, how does link reconfiguration
relate to AddBA re-negotiation? Are there implementations that
necessarily combine the two somehow?

> Please let us know if we need to clear the old session and then proceed 
> with the update request to avoid breaking the other drivers.

I'm not sure what you're trying to achieve, so I'm not entirely sure
what to say. I thought we were starting from "we should accept no-op
updates".

> > > Note: A similar fix has been proposed in [1]. This patch also fixes the
> > > issue mentioned there. The difference in approach is to keep a similar
> > > flow when the dialog_token matches. Previously only the timeout value
> > > is checked, updated that to check the timeout only for the hardware that
> > > doesn't set SUPPORT_REORDERING_BUFFER. In [1], it was changed to invoke
> > > driver unconditionally when SUPPORT_REORDERING_BUFFER is set.
> > > 
> > > https://lore.kernel.org/all/5806bab7e46506d3c300ab4eb66989d42936aeb0.1771323902.git.repk@triplefau.lt/
> > > 
> > What was wrong with the approach we discussed there, other than nobody
> > implementing it?
> > 
> > johannes
> 
> Not a major different between the other change, thought of keeping the 
> similar version that we validated in the internal tree.

I don't think it _is_ that similar though? Or maybe it is, but I didn't
think the other patch was correct (either).

Basically I'm concerned that calling drv_ampdu_action() with
IEEE80211_AMPDU_RX_START when the session is already active may break
things depending on the driver.

If you think there's a need to _really_ update parameters, then we
should probably add a IEEE80211_AMPDU_RX_UPDATE operation, and implement
it in drivers, and of course in some drivers that might just be
equivalent to IEEE80211_AMPDU_RX_START (though that seems unlikely in
general, given you may have to allocate reorder buffers etc.)

Regardless of that, if you think we need to accept no-op updates (I
don't think we've seen the need so far), then the code shouldn't really
(need to) call the driver at all, since things don't really change. For
that I'd argue we should go with the approached discussed in the thread
of [1], but not really that change itself.

[1] https://lore.kernel.org/all/5806bab7e46506d3c300ab4eb66989d42936aeb0.1771323902.git.repk@triplefau.lt/

I'm perfectly willing to believe the need for both, but I'd argue that
 - no-op updates can be handled by mac80211 entirely by itself, as
   discussed in that thread, just someone needs to implement it
 - updates with changes need a separate call to the driver, which might
   fail (either because of the change, or because it's not implemented)

As is, I don't think the code is safe in general. It may be that you
validated it against your driver, but then I'd argue that it worked for
your driver only because a new IEEE80211_AMPDU_RX_UPDATE would be
equivalent to IEEE80211_AMPDU_RX_START for that driver for some reason,
not because there's a general reason that this should work.

johannes