wcn36xx: use READ_ONCE() to access desc->ctrl

When accessing shared memory to check for the stat of submitted descriptors, make sure to use READ_ONCE(). This will guarantee the compiler treats these memory locations as volatile and doesn't apply any caching.

While this doesn't fix any particular problem I ran into, it's best practice to do it this way.

Note that this patch also removes the superflous extra condition check in the do-while loop in reap_tx_dxes(), as the loop will break instantly anyway in that case.

Signed-off-by Daniel Mack <[email protected]>

---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/net/wireless/ath/wcn36xx/dxe.c +3 -5

index 26e6c42f886a..c52f1c21ece9 100644
@@ -363,7 +363,7 @@ static void reap_tx_dxes(struct wcn36xx *wcn, struct wcn36xx_dxe_ch *ch)
        spin_lock_irqsave(&ch->lock, flags);
        ctl = ch->tail_blk_ctl;
        do {
-               if (ctl->desc->ctrl & WCN36xx_DXE_CTRL_VLD)
+               if (READ_ONCE(ctl->desc->ctrl) & WCN36xx_DXE_CTRL_VLD)
                        break;
                if (ctl->skb) {
                        dma_unmap_single(wcn->dev, ctl->desc->src_addr_l,

@@ -382,8 +382,7 @@ static void reap_tx_dxes(struct wcn36xx *wcn, struct wcn36xx_dxe_ch *ch)
                        ctl->skb = NULL;
                }
                ctl = ctl->next;
-       } while (ctl != ch->head_blk_ctl &&
-              !(ctl->desc->ctrl & WCN36xx_DXE_CTRL_VLD));
+       } while (ctl != ch->head_blk_ctl);
 
        ch->tail_blk_ctl = ctl;
        spin_unlock_irqrestore(&ch->lock, flags);

@@ -525,7 +524,7 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
                int_mask = WCN36XX_DXE_INT_CH3_MASK;
        }
 
-       while (!(dxe->ctrl & WCN36xx_DXE_CTRL_VLD)) {
+       while (!(READ_ONCE(dxe->ctrl) & WCN36xx_DXE_CTRL_VLD)) {
                skb = ctl->skb;
                dma_addr = dxe->dst_addr_l;
                ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl, GFP_ATOMIC);
--

2.14.3

Quoted 4 lines · Daniel Mack

When accessing shared memory to check for the stat of submitted descriptors, make sure to use READ_ONCE(). This will guarantee the compiler treats these memory locations as volatile and doesn't apply any caching.

The structure that is tested is not shared memory. It's accessed only by the apps processor.

Quoted 8 lines · Ramon Fried

Quoted 4 lines · Daniel Mack

When accessing shared memory to check for the stat of submitted descriptors, make sure to use READ_ONCE(). This will guarantee the compiler treats these memory locations as volatile and doesn't apply any caching.

The structure that is tested is not shared memory. It's accessed only by the apps processor.

Hmm? ctl->desc is of type struct wcn36xx_dxe_desc, which is packed and shared with the firmware. The WCN36xx_DXE_CTRL_VLD bit in ctrl bitfield is set by the firmware when a frame is valid, and before asserting the RX interrupt. So the host CPU must treat it as volatile and expect it to change.

Am I reading this wrong?

Thanks,
Daniel

When accessing shared memory to check for the stat of submitted descriptors, make sure to use READ_ONCE(). This will guarantee the compiler treats these memory locations as volatile and doesn't apply any caching.

While this doesn't fix any particular problem I ran into, it's best practice to do it this way.

Note that this patch also removes the superflous extra condition check in the do-while loop in reap_tx_dxes(), as the loop will break instantly anyway in that case.

Signed-off-by Daniel Mack <[email protected]>

Signed-off-by Kalle Valo <[email protected]>