Re: Regarding .wake_tx_queue() model

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Maxime Bizon <mbizon@freebox.fr> writes:

> On Tuesday 05 May 2020 à 14:06:18 (+0200), Toke Høiland-Jørgensen wrote:
>
> Hello Toke,
>
> thanks for your comments
>
>> I don't think ieee80211_stop_queue() is meant to be used this way at all
>> in the wake_tx_queue case. Rather, when you get a wake_tx_queue()
>> callback, you just queue as many frames as you feel like (see '2
>> aggregate' limit above), and then return.  Then, on a TX completion you
>> just call your internal driver function that tries to pull more frames
>> from the mac80211 TXQs.
>
> alright, indeed .wake_tx_queue() does not have to result in any actual
> frame sent.
>
> but what about .release_buffered_frames then ?
>
> .release_buffered_frames() and .drv_tx() are both "push" oriented
> interface. When they are called, you have to push a frame to the
> hardware, so if they are called when hardware FIFO is full, you need
> to drop the frame (or add yet another intermediate level of queuing)
>
> from what I can see, .release_buffered_frames() will happily be called
> even if queue is stopped, and you have to at least send one frame.
>
> I'm just trying to avoid any additionnal intermediate queing in the
> driver between what mac80211 gives me and the HW fifo which has a
> fixed size (well actually you can choose the size, but only at init
> time).
>
> my current workaround is to pre-size the hw fifo queue to handle what
> I think is the worst case

Well, I think that should be fine? Having a longer HW queue is fine, as
long as you have some other logic to not fill it all the time (like the
"max two aggregates" logic I mentioned before). I think this is what
ath9k does too. At least it looks like both drv_tx() and
release_buffered_frames() will just abort (and drop in the former case)
if there is no HW buffer space left.

> Also .release_buffered_frames() codepath is difficult to test, how do
> you trigger that reliably ? assuming VIF is an AP, then you need the
> remote STA to go to sleep even though you have traffic waiting for it
> in the txqi. For now I patch the stack to introduce artificial delay.
>
> Since my hardware has a sticky per-STA PS filter with good status
> reporting, I'm considering using ieee80211_sta_block_awake() and only
> unblock STA when all its txqi are empty to get rid of
> .release_buffered_frames() complexity.

I'm probably not the right person to answer that; never did have a good
grip on the details of PS support.


What hardware is it you're writing a driver for, BTW, and are you
planning to upstream it? :)

-Toke