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

[Manish Dharanenthiran <manish.dharanenthiran@oss.qualcomm.com>]

On 5/7/2026 11:56 PM, Johannes Berg wrote:
> Hi,
> 
> Sorry, I've definitely fallen behind on some topics.
> 
No problem :)

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

No, there is no implementations that combine these two, but there are 
cases where the update AddBA request can be received from the station 
with link reconfiguration.

For instance, if a station associated in 2 GHz, later with link 
reconfiguration station can either move to 5 GHz or it can add 5 GHz (as 
MLD). Station then can send a AddBA request to update the window size or 
other related parameters. Current change tries to address this by 
treating the request as START and ath12k driver internally takes care of 
updating the existing session. But, as you mentioned, we feel that it 
should be treated as an UPDATE instead of START.

>> 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.)
> 
Agree.

> 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/
> 
We believe that a no-op update is not required (or at-least we couldn't 
think of a case in which that is actually needed) as there can might be 
an actual change in the subsequent AddBA request.

> 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

Yes, based on the case that we encountered, IEEE80211_AMPDU_RX_UPDATE is 
the right approach for the handling those requests safely for other 
driver(s) as well instead of relying on IEEE80211_AMPDU_RX_START

Regards
Manish Dharanenthiran