On Tue, Nov 6, 2018 at 2:32 PM Emmanuel Grumbach
<emmanuel.grumbach@xxxxxxxxx> wrote:
>
> The iTXQs stop/wake queue mechanism involves a whole bunch
> of locks and this is probably why the call to
> ieee80211_wake_txqs is deferred to a tasklet when called from
> __ieee80211_wake_queue.
>
> Another advantage of that is that ieee80211_wake_txqs might
> call the wake_tx_queue() callback and then the driver may
> call mac80211 which will call it back in the same context.
>
> The bug I saw is that when we send a deauth frame as a
> station we do:
>
> flush(drop=1)
> tx deauth
> flush(drop=0)
>
> While we flush we stop the queues and wake them up
> immediately after we finished flushing. The problem here is
> that the tasklet that de-facto enables the queue may not have
> run until we send the deauth. Then the deauth frame is sent
> to the driver (which is surprising by itself), but the driver
> won't get anything useful from ieee80211_tx_dequeue because
> the queue is stopped (or more precisely because
> vif->txqs_stopped[0] is true).
> Then the deauth is not sent. Later on, the tasklet will run,
> but that'll be too late. We'll already have removed all the
> vif etc...
>
> There are 2 options I see here:
> 1) do what I did in this patch (which is pretty awful because
> of the if (lock) thing. If we wake up the queue because of
> a reason which is internal to mac80211, call
> ieee80211_wake_txqs synchronously since we know we are not
> coming from the driver.
> 2) we could set vif->txqs_stopped to false synchrously and do
> the rest of the job in a tasklet. This would allow the code
> that sends the Tx to put the frame in the queue and the
> driver would actually get it.
>
> Maybe others have better ideas?
>
> Change-Id: I5d3bd20ec765becb91944741c35e35f6e3888981
> Cc: Manikanta Pubbisetty <mpubbise@xxxxxxxxxxxxxx>
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx>
> ---
> net/mac80211/util.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index c9cb00a..c731ddb 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -299,16 +299,16 @@ out:
> spin_unlock_bh(&fq->lock);
> }
>
> -void ieee80211_wake_txqs(unsigned long data)
> +static void _ieee80211_wake_txqs(struct ieee80211_local *local, bool lock)
> {
> - struct ieee80211_local *local = (struct ieee80211_local *)data;
> struct ieee80211_sub_if_data *sdata;
> int n_acs = IEEE80211_NUM_ACS;
> unsigned long flags;
> int i;
>
> rcu_read_lock();
> - spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> + if (lock)
> + spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
>
This is buggy of course... If lock is false, then we'll call
spin_unlock_irqrestore
further down in the function, but flags won't be initialized.
I don't really know why we have this strange pattern of a
lock
loop
unlock
wake_tx_queue()
lock
unlock
If we want not to call wake_tx_queue with the lock taken, we can
prepare a bitmap of ACs
and then call wake_tx_queue() for all the ACs, bu then of course, we
may have races.
OTOH, we may have races here as well. Someone could change queue_stop_reasons
after we release the lock and before we call wake_tx_queue and because
of that, we'd end up
waking a queue that is supposed to be stopped.
> if (local->hw.queues < IEEE80211_NUM_ACS)
> n_acs = 1;
> @@ -332,10 +332,18 @@ void ieee80211_wake_txqs(unsigned long data)
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> }
>
> - spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> + if (lock)
> + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> rcu_read_unlock();
> }
>
> +void ieee80211_wake_txqs(unsigned long data)
> +{
> + struct ieee80211_local *local = (struct ieee80211_local *)data;
> +
> + _ieee80211_wake_txqs(local, true);
> +}
> +
> void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue)
> {
> struct ieee80211_sub_if_data *sdata;
> @@ -405,8 +413,12 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
> } else
> tasklet_schedule(&local->tx_pending_tasklet);
>
> - if (local->ops->wake_tx_queue)
> - tasklet_schedule(&local->wake_txqs_tasklet);
> + if (local->ops->wake_tx_queue) {
> + if (reason == IEEE80211_QUEUE_STOP_REASON_DRIVER)
> + tasklet_schedule(&local->wake_txqs_tasklet);
> + else
> + _ieee80211_wake_txqs(local, false);
> + }
> }
>
> void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue,
> --
> 2.7.4
>
