diff mbox

[1/1,RFC] wcn36xx: fix buffer commit logic on TX path

Message ID 20180410174245.1116-2-daniel@zonque.org (mailing list archive)
State RFC
Delegated to: Kalle Valo
Headers show

Commit Message

Daniel Mack April 10, 2018, 5:42 p.m. UTC
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 <daniel@zonque.org>
---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 75 +++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 37 deletions(-)

Comments

Loic Poulain April 11, 2018, 1:30 p.m. UTC | #1
Hi Daniel,

>         /* 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();

Is this first memory barrier really needed? from what I understand, we
only need to ensure that the control descriptor is marked valid at the
end of the procedure and we don't really care about the paylod one.

> +       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

Otherwise, patch makes sense.

Regards,
Loic
Daniel Mack April 11, 2018, 1:37 p.m. UTC | #2
Hi Loic,

On Wednesday, April 11, 2018 03:30 PM, Loic Poulain wrote:
>>         /* 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();
> 
> Is this first memory barrier really needed? from what I understand, we
> only need to ensure that the control descriptor is marked valid at the
> end of the procedure and we don't really care about the paylod one.

Well, without documentation or the firmware sources, that's just
guesswork at this point. My assumption is only based on the weird
comments and workarounds in the downstream driver.

I added the second barrier to ensure that no descriptor is ever marked
valid unless all other bits are definitely in sync.


Thanks,
Daniel
Loic Poulain April 12, 2018, 12:45 p.m. UTC | #3
On 11 April 2018 at 15:37, Daniel Mack <daniel@zonque.org> wrote:
> Hi Loic,
>
> On Wednesday, April 11, 2018 03:30 PM, Loic Poulain wrote:
>>>         /* 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();
>>
>> Is this first memory barrier really needed? from what I understand, we
>> only need to ensure that the control descriptor is marked valid at the
>> end of the procedure and we don't really care about the paylod one.
>
> Well, without documentation or the firmware sources, that's just
> guesswork at this point. My assumption is only based on the weird
> comments and workarounds in the downstream driver.
>
> I added the second barrier to ensure that no descriptor is ever marked
> valid unless all other bits are definitely in sync.

Fair enough!
Daniel Mack April 24, 2018, 2:33 p.m. UTC | #4
Hi Loic,

On Wednesday, April 11, 2018 03:30 PM, Loic Poulain wrote:
>>         /* 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();
> 
> Is this first memory barrier really needed? from what I understand, we
> only need to ensure that the control descriptor is marked valid at the
> end of the procedure and we don't really care about the paylod one.
> 
>> +       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
> 
> Otherwise, patch makes sense.

This patch has somehow vanished from patchwork. I am running it since I
posted it and I can't see it causing any regressions, so I'd like to
repost. Given that you seem to be in favor of it, can I have your
Reviewed-by?


Thanks,
Daniel
Kalle Valo April 24, 2018, 3:17 p.m. UTC | #5
Daniel Mack <daniel@zonque.org> writes:

> Hi Loic,
>
> On Wednesday, April 11, 2018 03:30 PM, Loic Poulain wrote:
>>>         /* 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();
>> 
>> Is this first memory barrier really needed? from what I understand, we
>> only need to ensure that the control descriptor is marked valid at the
>> end of the procedure and we don't really care about the paylod one.
>> 
>>> +       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
>> 
>> Otherwise, patch makes sense.
>
> This patch has somehow vanished from patchwork.

It's still in patchwork, just in state RFC:

https://patchwork.kernel.org/patch/10333553/
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index 26e6c42f886a..cb93545a42ce 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -638,8 +638,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;
@@ -647,74 +647,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