Re: [PATCH v2 2/3] wifi: ath11k: implement custom wake_tx_queue with flow control

[Tamizh Raja]

....
>
>  drivers/net/wireless/ath/ath11k/dp.c  |  1 +
>  drivers/net/wireless/ath/ath11k/dp.h  |  2 +
>  drivers/net/wireless/ath/ath11k/mac.c | 60 ++++++++++++++++++++++++++-
>  3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath11k/dp.c b/drivers/net/wireless/ath/ath11k/dp.c
> index f389b97acbdd..2e5978ec2b05 100644
> --- a/drivers/net/wireless/ath/ath11k/dp.c
> +++ b/drivers/net/wireless/ath/ath11k/dp.c
> @@ -1087,6 +1087,7 @@ int ath11k_dp_alloc(struct ath11k_base *ab)
>         for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) {
>                 idr_init(&dp->tx_ring[i].txbuf_idr);
>                 spin_lock_init(&dp->tx_ring[i].tx_idr_lock);
> +               spin_lock_init(&dp->tx_ring[i].wake_tx_lock);
>                 dp->tx_ring[i].tcl_data_ring_id = i;
>
>                 dp->tx_ring[i].tx_status_head = 0;
> diff --git a/drivers/net/wireless/ath/ath11k/dp.h b/drivers/net/wireless/ath/ath11k/dp.h
> index 84f66839f0c6..6d99501aa269 100644
> --- a/drivers/net/wireless/ath/ath11k/dp.h
> +++ b/drivers/net/wireless/ath/ath11k/dp.h
> @@ -87,6 +87,8 @@ struct dp_tx_ring {
>         struct idr txbuf_idr;
>         /* Protects txbuf_idr and num_pending */
>         spinlock_t tx_idr_lock;
> +       /* Serializes wake_tx_queue operations for this ring */
> +       spinlock_t wake_tx_lock;
>         struct hal_wbm_release_ring *tx_status;
>         int tx_status_head;
>         int tx_status_tail;
> diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
> index 2d55cdc4d165..046cefd53178 100644
> --- a/drivers/net/wireless/ath/ath11k/mac.c
> +++ b/drivers/net/wireless/ath/ath11k/mac.c
> @@ -10065,9 +10065,67 @@ static int ath11k_mac_op_sta_state(struct ieee80211_hw *hw,
>         return ret;
>  }
>
> +static void ath11k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
> +                                       struct ieee80211_txq *txq)
> +{
> +       struct ieee80211_tx_control control = {
> +               .sta = txq->sta,
> +       };
> +       const struct sk_buff *peek_skb;
> +       struct ath11k *ar = hw->priv;
> +       struct dp_tx_ring *tx_ring;
> +       struct hal_srng *tcl_ring;
> +       struct sk_buff *skb;
> +       u32 ring_selector;
> +       int num_free;
> +       u8 ring_id;
> +
> +       if (!ar)
> +               return;
> +
> +       while (1) {
> +               if (unlikely(test_bit(ATH11K_FLAG_CRASH_FLUSH,
> +                                     &ar->ab->dev_flags)))
> +                       break;
> +
> +               peek_skb = ieee80211_tx_peek(hw, txq);

in ieee80211_tx_peek() acquires fq->lock, peeks at the skb, releases
the lock, then returns the pointer.
Between spin_unlock_bh(&fq->lock) and using the pointer for
ring_selector. The same skb can also be returned for
the another CPU which calls this function. This may cause use after a
free scenario.

> +               if (!peek_skb)
> +                       break;
> +
> +               ring_selector = ar->ab->hw_params.hw_ops->get_ring_selector(
> +                                       (struct sk_buff *)peek_skb);
> +               ring_id = ring_selector %
> +                         ar->ab->hw_params.hal_params->num_tx_rings;
> +
> +               tx_ring = &ar->ab->dp.tx_ring[ring_id];
> +               tcl_ring = &ar->ab->hal.srng_list[tx_ring->tcl_data_ring.ring_id];
> +
> +               spin_lock_bh(&tx_ring->wake_tx_lock);
> +
> +               spin_lock(&tcl_ring->lock);
> +               num_free = ath11k_hal_srng_src_num_free(ar->ab, tcl_ring, true);
> +               spin_unlock(&tcl_ring->lock);
> +
> +               if (num_free == 0) {
> +                       spin_unlock_bh(&tx_ring->wake_tx_lock);
> +                       break;
> +               }
> +
> +               skb = ieee80211_tx_dequeue(hw, txq);
> +               if (!skb) {
> +                       spin_unlock_bh(&tx_ring->wake_tx_lock);
> +                       break;
> +               }
> +
> +               ath11k_mac_op_tx(hw, &control, skb);
> +
> +               spin_unlock_bh(&tx_ring->wake_tx_lock);
> +       }
> +}
> +
>  static const struct ieee80211_ops ath11k_ops = {
>         .tx                             = ath11k_mac_op_tx,
> -       .wake_tx_queue                  = ieee80211_handle_wake_tx_queue,
> +       .wake_tx_queue                  = ath11k_mac_op_wake_tx_queue,
>         .start                          = ath11k_mac_op_start,
>         .stop                           = ath11k_mac_op_stop,
>         .reconfig_complete              = ath11k_mac_op_reconfig_complete,
> --
> 2.54.0
>
>


-- 
- Tamizh.