Return-path: Received: from mail-qg0-f53.google.com ([209.85.192.53]:36163 "EHLO mail-qg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965096AbbJ1IZK (ORCPT ); Wed, 28 Oct 2015 04:25:10 -0400 Received: by qgad10 with SMTP id d10so100232qga.3 for ; Wed, 28 Oct 2015 01:25:10 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <562C454B.4070307@linaro.org> References: <1445708535-20169-1-git-send-email-me@bobcopeland.com> <562C454B.4070307@linaro.org> Date: Wed, 28 Oct 2015 08:25:09 +0000 Message-ID: (sfid-20151028_092517_814278_9BA30F54) Subject: Re: [PATCH] wcn36xx: introduce per-channel ring buffer locks From: Eugene Krasnikov To: yfw Cc: Bob Copeland , linux-wireless , wcn36xx Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: Looks good to me! 2015-10-25 2:58 GMT+00:00 yfw : > 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 >> --- >> 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