| Message ID | 20180410173558.32425-1-daniel@zonque.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9ff6fde86fd27a0c733c01114eb0d46bd3a33fb7 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 10 April 2018 at 20:35, Daniel Mack <daniel@zonque.org> wrote: > 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. In this case, caching does make sense. I'm not sure regarding READ_ONCE in that context I'll have to do some reading regarding that. But I don't think this is right here. Thanks, Ramon > > 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 <daniel@zonque.org> > --- > drivers/net/wireless/ath/wcn36xx/dxe.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c > index 26e6c42f886a..c52f1c21ece9 100644 > --- a/drivers/net/wireless/ath/wcn36xx/dxe.c > +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c > @@ -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 >
On Tuesday, April 10, 2018 08:17 PM, Ramon Fried wrote: > On 10 April 2018 at 20:35, Daniel Mack <daniel@zonque.org> wrote: >> 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
On 11 April 2018 at 09:58, Daniel Mack <daniel@zonque.org> wrote: > On Tuesday, April 10, 2018 08:17 PM, Ramon Fried wrote: >> On 10 April 2018 at 20:35, Daniel Mack <daniel@zonque.org> wrote: >>> 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? > No, you're right. I missed that. > > Thanks, > Daniel
Daniel Mack <daniel@zonque.org> wrote: > 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 <daniel@zonque.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Patch applied to ath-next branch of ath.git, thanks. 9ff6fde86fd2 wcn36xx: use READ_ONCE() to access desc->ctrl
diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c index 26e6c42f886a..c52f1c21ece9 100644 --- a/drivers/net/wireless/ath/wcn36xx/dxe.c +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c @@ -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);
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 <daniel@zonque.org> --- drivers/net/wireless/ath/wcn36xx/dxe.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)