On Mon, 2022-09-26 at 18:13 +0200, Alexander Wetzel wrote:
> - trace_drv_wake_tx_queue(local, sdata, txq);
Technically, I guess we could keep both tracepoints, but it'd be kind of
pointless since we know statically which driver does which...
> @@ -596,21 +598,18 @@ __sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>
> sta->last_connected = ktime_get_seconds();
>
> - if (local->ops->wake_tx_queue) {
> - void *txq_data;
> - int size = sizeof(struct txq_info) +
> - ALIGN(hw->txq_data_size, sizeof(void *));
> + size = sizeof(struct txq_info) +
> + ALIGN(hw->txq_data_size, sizeof(void *));
>
> - txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> - if (!txq_data)
> - goto free;
> + txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> + if (!txq_data)
> + goto free;
>
> - for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> - struct txq_info *txq = txq_data + i * size;
> + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> + struct txq_info *txq = txq_data + i * size;
>
> - /* might not do anything for the bufferable MMPDU TXQ */
> - ieee80211_txq_init(sdata, sta, txq, i);
> - }
> + /* might not do anything for the bufferable MMPDU TXQ */
> + ieee80211_txq_init(sdata, sta, txq, i);
Is that comment still true?
> +++ b/net/mac80211/util.c
> @@ -288,6 +288,64 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw,
> }
> EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>
> +static void wake_tx_push_queue(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + struct ieee80211_txq *queue)
> +{
> + int q = sdata->vif.hw_queue[queue->ac];
> + struct ieee80211_tx_control control = {};
> + struct sk_buff *skb;
> + unsigned long flags;
> + bool q_stopped;
> +
> + control.sta = queue->sta;
> +
> + while (1) {
> + spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> + q_stopped = local->queue_stop_reasons[q];
> + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> +
> + if (q_stopped)
> + break;
> +
> + skb = ieee80211_tx_dequeue(&local->hw, queue);
> + if (!skb)
> + break;
> +
> + drv_tx(local, &control, skb);
> + }
> +}
> +
> +void wake_tx_queue(struct ieee80211_local *local, struct txq_info *txq)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(txq->txq.vif);
> + struct ieee80211_txq *queue;
> +
> + /* In reconfig don't transmit now, but mark for waking later */
> + if (local->in_reconfig) {
> + set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txq->flags);
> + return;
> + }
> +
> + if (!check_sdata_in_driver(sdata))
> + return;
> +
> + trace_wake_tx_queue(local, sdata, txq);
> +
> + if (local->ops->wake_tx_queue) {
> + drv_wake_tx_queue(local, txq);
> + return;
> + }
> +
> + /* Driver has no native support for iTXQ, handle the queues */
> + ieee80211_txq_schedule_start(&local->hw, txq->txq.ac);
> + while ((queue = ieee80211_next_txq(&local->hw, txq->txq.ac))) {
> + wake_tx_push_queue(local, sdata, queue);
> + ieee80211_return_txq(&local->hw, queue, false);
> + }
> + ieee80211_txq_schedule_end(&local->hw, txq->txq.ac);
> +}
Here's another thought:
Since this code is basically all moved from the original
drv_wake_tx_queue(), except for the "else" portion (after the if/return)
of it, another thing we could do is to just have an exported function
that does this:
void ieee80211_handle_wake_tx_queue(struct ieee80211_hw *hw,
struct ieee80211_txq *txq)
{
... *local = from_hw(hw);
... *sdata = from_vif(txq->vif);
wake_tx_push_queue(local, sdata, txq);
}
Actually ... I wonder why you'd here - in waking a single TXQ - use
ieee80211_next_txq() at all, Toke, what do you think?
Anyway, then we could require drivers set wake_txq to
ieee80211_handle_wake_tx_queue and make sure in main.c that
wake_tx_queue is non-NULL.
That's a bit more churn in drivers, but:
* it's not really that hard to do
* it avoids an extra function call to then jump to the op
* it avoids the tracing changes since now it does look like a driver
wake_tx_queue callback
What do you think?
johannes
