Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:52986 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751858AbeEQJD4 (ORCPT ); Thu, 17 May 2018 05:03:56 -0400 Received: by mail-wm0-f68.google.com with SMTP id w194-v6so7020963wmf.2 for ; Thu, 17 May 2018 02:03:55 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180516140820.1636-2-daniel@zonque.org> References: <20180516140820.1636-1-daniel@zonque.org> <20180516140820.1636-2-daniel@zonque.org> From: Ramon Fried Date: Thu, 17 May 2018 12:03:54 +0300 Message-ID: (sfid-20180517_110412_845030_671621FF) Subject: Re: [PATCH 01/10] wcn36xx: fix buffer commit logic on TX path To: Daniel Mack Cc: linux-wireless@vger.kernel.org, loic.poulain@linaro.org, bjorn.andersson@linaro.org, nicolas.dechesne@linaro.org, wcn36xx@lists.infradead.org, rfried@codeaurora.org, kvalo@codeaurora.org Content-Type: text/plain; charset="UTF-8" Sender: linux-wireless-owner@vger.kernel.org List-ID: On Wed, May 16, 2018 at 5:08 PM, Daniel Mack wrote: > When wcn36xx_dxe_tx_frame() is entered while the device is still processing > the queue asyncronously, we are racing against the firmware code with > updates to the buffer descriptors. Presumably, the firmware scans the ring > buffer that holds the descriptors and scans for a valid control descriptor, > and then assumes that the next descriptor contains the payload. If, however, > the control descriptor is marked valid, but the payload descriptor isn't, > the packet is not sent out. > > Another issue with the current code is that is lacks memory barriers before > descriptors are marked valid. This is important because the CPU may reorder > writes to memory, even if it is allocated as coherent DMA area, and hence > the device may see incompletely written data. > > To fix this, the code in wcn36xx_dxe_tx_frame() was restructured a bit so > that the payload descriptor is made valid before the control descriptor. > Memory barriers are added to ensure coherency of shared memory areas. > > Signed-off-by: Daniel Mack > --- > drivers/net/wireless/ath/wcn36xx/dxe.c | 75 +++++++++++++++++----------------- > 1 file changed, 38 insertions(+), 37 deletions(-) > > diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c > index bd2b946a65c9..3d1cf7dd1f8e 100644 > --- a/drivers/net/wireless/ath/wcn36xx/dxe.c > +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c > @@ -642,8 +642,8 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, > struct sk_buff *skb, > bool is_low) > { > - struct wcn36xx_dxe_ctl *ctl = NULL; > - struct wcn36xx_dxe_desc *desc = NULL; > + struct wcn36xx_dxe_desc *desc_bd, *desc_skb; > + struct wcn36xx_dxe_ctl *ctl_bd, *ctl_skb; > struct wcn36xx_dxe_ch *ch = NULL; > unsigned long flags; > int ret; > @@ -651,74 +651,75 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, > ch = is_low ? &wcn->dxe_tx_l_ch : &wcn->dxe_tx_h_ch; > > spin_lock_irqsave(&ch->lock, flags); > - ctl = ch->head_blk_ctl; > + ctl_bd = ch->head_blk_ctl; > + ctl_skb = ctl_bd->next; > > /* > * If skb is not null that means that we reached the tail of the ring > * hence ring is full. Stop queues to let mac80211 back off until ring > * has an empty slot again. > */ > - if (NULL != ctl->next->skb) { > + if (NULL != ctl_skb->skb) { > ieee80211_stop_queues(wcn->hw); > wcn->queues_stopped = true; > spin_unlock_irqrestore(&ch->lock, flags); > return -EBUSY; > } > > - ctl->skb = NULL; > - desc = ctl->desc; > + if (unlikely(ctl_skb->bd_cpu_addr)) { > + wcn36xx_err("bd_cpu_addr cannot be NULL for skb DXE\n"); > + ret = -EINVAL; > + goto unlock; > + } > + > + desc_bd = ctl_bd->desc; > + desc_skb = ctl_skb->desc; > + > + ctl_bd->skb = NULL; > > /* write buffer descriptor */ > - memcpy(ctl->bd_cpu_addr, bd, sizeof(*bd)); > + memcpy(ctl_bd->bd_cpu_addr, bd, sizeof(*bd)); > > /* Set source address of the BD we send */ > - desc->src_addr_l = ctl->bd_phy_addr; > - > - desc->dst_addr_l = ch->dxe_wq; > - desc->fr_len = sizeof(struct wcn36xx_tx_bd); > - desc->ctrl = ch->ctrl_bd; > + desc_bd->src_addr_l = ctl_bd->bd_phy_addr; > + desc_bd->dst_addr_l = ch->dxe_wq; > + desc_bd->fr_len = sizeof(struct wcn36xx_tx_bd); > > wcn36xx_dbg(WCN36XX_DBG_DXE, "DXE TX\n"); > > wcn36xx_dbg_dump(WCN36XX_DBG_DXE_DUMP, "DESC1 >>> ", > - (char *)desc, sizeof(*desc)); > + (char *)desc_bd, sizeof(*desc_bd)); > wcn36xx_dbg_dump(WCN36XX_DBG_DXE_DUMP, > - "BD >>> ", (char *)ctl->bd_cpu_addr, > + "BD >>> ", (char *)ctl_bd->bd_cpu_addr, > sizeof(struct wcn36xx_tx_bd)); > > - /* Set source address of the SKB we send */ > - ctl = ctl->next; > - desc = ctl->desc; > - if (ctl->bd_cpu_addr) { > - wcn36xx_err("bd_cpu_addr cannot be NULL for skb DXE\n"); > - ret = -EINVAL; > - goto unlock; > - } > - > - desc->src_addr_l = dma_map_single(wcn->dev, > - skb->data, > - skb->len, > - DMA_TO_DEVICE); > - if (dma_mapping_error(wcn->dev, desc->src_addr_l)) { > + desc_skb->src_addr_l = dma_map_single(wcn->dev, > + skb->data, > + skb->len, > + DMA_TO_DEVICE); > + if (dma_mapping_error(wcn->dev, desc_skb->src_addr_l)) { > dev_err(wcn->dev, "unable to DMA map src_addr_l\n"); > ret = -ENOMEM; > goto unlock; > } > > - ctl->skb = skb; > - desc->dst_addr_l = ch->dxe_wq; > - desc->fr_len = ctl->skb->len; > - > - /* set dxe descriptor to VALID */ > - desc->ctrl = ch->ctrl_skb; > + ctl_skb->skb = skb; > + desc_skb->dst_addr_l = ch->dxe_wq; > + desc_skb->fr_len = ctl_skb->skb->len; > > wcn36xx_dbg_dump(WCN36XX_DBG_DXE_DUMP, "DESC2 >>> ", > - (char *)desc, sizeof(*desc)); > + (char *)desc_skb, sizeof(*desc_skb)); > wcn36xx_dbg_dump(WCN36XX_DBG_DXE_DUMP, "SKB >>> ", > - (char *)ctl->skb->data, ctl->skb->len); > + (char *)ctl_skb->skb->data, ctl_skb->skb->len); > > /* Move the head of the ring to the next empty descriptor */ > - ch->head_blk_ctl = ctl->next; > + ch->head_blk_ctl = ctl_skb->next; > + > + /* Commit all previous writes and set descriptors to VALID */ > + wmb(); > + desc_skb->ctrl = ch->ctrl_skb; > + wmb(); > + desc_bd->ctrl = ch->ctrl_bd; > > /* > * When connected and trying to send data frame chip can be in sleep > -- > 2.14.3 > > > _______________________________________________ > wcn36xx mailing list > wcn36xx@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/wcn36xx Acked-by: Ramon Fried