[PATCH] wcn36xx: introduce per-channel ring buffer locks

[Eugene Krasnikov]

Looks good to me!

2015-10-25 2:58 GMT+00:00 yfw <fengwei.yin at linaro.org>:
> Hi Bob,
>
>
> On 2015/10/25 1:42, Bob Copeland wrote:
>>
>> wcn36xx implements a ring buffer for transmitted frames for each
>> (high and low priority) DMA channel.  The ring buffers are lockless:
>> new frames are inserted at the head of the queue, while finished
>> packets are reaped from the tail.
>>
>> Unfortunately, the list manipulations are missing any kind of barriers
>> so are susceptible to various races: for example, a TX completion
>> handler might read an updated desc->ctrl before the head has actually
>> advanced, and then null out the ctl->skb pointer while it is still
>> being used in the TX path.
>>
>> Simplify things here by adding a spin lock when traversing the ring.
>> This change increased stability for me without adding any noticeable
>> overhead on my platform (xperia z).
>>
>> Signed-off-by: Bob Copeland <me at bobcopeland.com>
>> ---
>>   drivers/net/wireless/ath/wcn36xx/dxe.c | 27 +++++++++++++++++++--------
>>   drivers/net/wireless/ath/wcn36xx/dxe.h |  1 +
>>   2 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c
>> b/drivers/net/wireless/ath/wcn36xx/dxe.c
>> index 086549b..26085f7 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/dxe.c
>> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
>> @@ -79,6 +79,7 @@ static int wcn36xx_dxe_allocate_ctl_block(struct
>> wcn36xx_dxe_ch *ch)
>>         struct wcn36xx_dxe_ctl *cur_ctl = NULL;
>>         int i;
>>
>> +       spin_lock_init(&ch->lock);
>>         for (i = 0; i < ch->desc_num; i++) {
>>                 cur_ctl = kzalloc(sizeof(*cur_ctl), GFP_KERNEL);
>>                 if (!cur_ctl)
>> @@ -345,7 +346,7 @@ void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32
>> status)
>>
>>   static void reap_tx_dxes(struct wcn36xx *wcn, struct wcn36xx_dxe_ch *ch)
>>   {
>> -       struct wcn36xx_dxe_ctl *ctl = ch->tail_blk_ctl;
>> +       struct wcn36xx_dxe_ctl *ctl;
>>         struct ieee80211_tx_info *info;
>>         unsigned long flags;
>>
>> @@ -354,6 +355,8 @@ static void reap_tx_dxes(struct wcn36xx *wcn, struct
>> wcn36xx_dxe_ch *ch)
>>          * completely full head and tail are pointing to the same element
>>          * and while-do will not make any cycles.
>>          */
>> +       spin_lock_irqsave(&ch->lock, flags);
>> +       ctl = ch->tail_blk_ctl;
>>         do {
>>                 if (ctl->desc->ctrl & WCN36XX_DXE_CTRL_VALID_MASK)
>>                         break;
>> @@ -365,12 +368,12 @@ static void reap_tx_dxes(struct wcn36xx *wcn, struct
>> wcn36xx_dxe_ch *ch)
>>                                 /* Keep frame until TX status comes */
>>                                 ieee80211_free_txskb(wcn->hw, ctl->skb);
>>                         }
>> -                       spin_lock_irqsave(&ctl->skb_lock, flags);
>> +                       spin_lock(&ctl->skb_lock);
>>                         if (wcn->queues_stopped) {
>>                                 wcn->queues_stopped = false;
>>                                 ieee80211_wake_queues(wcn->hw);
>>                         }
>> -                       spin_unlock_irqrestore(&ctl->skb_lock, flags);
>> +                       spin_unlock(&ctl->skb_lock);
>>
>>                         ctl->skb = NULL;
>>                 }
>> @@ -379,6 +382,7 @@ static void reap_tx_dxes(struct wcn36xx *wcn, struct
>> wcn36xx_dxe_ch *ch)
>>                !(ctl->desc->ctrl & WCN36XX_DXE_CTRL_VALID_MASK));
>>
>>         ch->tail_blk_ctl = ctl;
>> +       spin_unlock_irqrestore(&ch->lock, flags);
>>   }
>>
>>   static irqreturn_t wcn36xx_irq_tx_complete(int irq, void *dev)
>> @@ -596,12 +600,14 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>>         struct wcn36xx_dxe_desc *desc = NULL;
>>         struct wcn36xx_dxe_ch *ch = NULL;
>>         unsigned long flags;
>> +       int ret;
>>
>>         ch = is_low ? &wcn->dxe_tx_l_ch : &wcn->dxe_tx_h_ch;
>>
>> +       spin_lock_irqsave(&ch->lock, flags);
>>         ctl = ch->head_blk_ctl;
>>
>> -       spin_lock_irqsave(&ctl->next->skb_lock, flags);
>> +       spin_lock(&ctl->next->skb_lock);
>>
>>         /*
>>          * If skb is not null that means that we reached the tail of the
>> ring
>> @@ -611,10 +617,11 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>>         if (NULL != ctl->next->skb) {
>>                 ieee80211_stop_queues(wcn->hw);
>>                 wcn->queues_stopped = true;
>> -               spin_unlock_irqrestore(&ctl->next->skb_lock, flags);
>> +               spin_unlock(&ctl->next->skb_lock);
>> +               spin_unlock_irqrestore(&ch->lock, flags);
>>                 return -EBUSY;
>>         }
>> -       spin_unlock_irqrestore(&ctl->next->skb_lock, flags);
>> +       spin_unlock(&ctl->next->skb_lock);
>>
>>         ctl->skb = NULL;
>>         desc = ctl->desc;
>> @@ -640,7 +647,8 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>>         desc = ctl->desc;
>>         if (ctl->bd_cpu_addr) {
>>                 wcn36xx_err("bd_cpu_addr cannot be NULL for skb DXE\n");
>> -               return -EINVAL;
>> +               ret = -EINVAL;
>> +               goto unlock;
>>         }
>>
>>         desc->src_addr_l = dma_map_single(NULL,
>> @@ -679,7 +687,10 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>>                         ch->reg_ctrl, ch->def_ctrl);
>>         }
>>
>> -       return 0;
>> +       ret = 0;
>> +unlock:
>> +       spin_unlock_irqrestore(&ch->lock, flags);
>> +       return ret;
>>   }
>>
>>   int wcn36xx_dxe_init(struct wcn36xx *wcn)
>> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.h
>> b/drivers/net/wireless/ath/wcn36xx/dxe.h
>> index 35ee7e9..3eca4f9 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/dxe.h
>> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.h
>> @@ -243,6 +243,7 @@ struct wcn36xx_dxe_ctl {
>>   };
>>
>>   struct wcn36xx_dxe_ch {
>> +       spinlock_t                      lock;   /* protects head/tail ptrs
>> */
>>         enum wcn36xx_dxe_ch_type        ch_type;
>>         void                            *cpu_addr;
>>         dma_addr_t                      dma_addr;
>>
>
> The patch looks great to me.
>
> Regards
> Yin, Fengwei



-- 
Best regards,
Eugene