>-----Original Message----- >From: David Miller [mailto:da...@davemloft.net] >Sent: Monday, February 22, 2016 4:59 AM >To: claudiu.man...@freescale.com >Cc: netdev@vger.kernel.org >Subject: Re: [PATCH net-next 1/3] gianfar: Map head TxBD first to skip a >wmb() > >From: Claudiu Manoil <claudiu.man...@freescale.com> >Date: Fri, 19 Feb 2016 12:47:32 +0200 > >> The main point of this change is to update the buffer pointer of >> the first BD way before the status field is updated, so that a >> wmb() between these two operations is no longer needed. > >This is the completely bogus. > >The distance between two memory operations has no bearing upon >whether a memory barrier is necessary between them or not. > >There could be 1,000 loads and stores between two dependant >operations, you still need the memory barrier for full correctness >if the ordering between them is important. > >I am not applying this patch series because it's is doing things >and stating things which are entirely incorrect. >
I think it can be proven that the wmb() is not needed in this case. If it weren't so, then all the other eth drivers would require a wmb() in their xmit(), between buffer address update and status update for the last descriptor of a frame. Let's take a random driver - igb, there is no wmb() between: tx_desc->read.buffer_addr = cpu_to_le64(dma); and /* write last descriptor with RS and EOP bits */ cmd_type |= size | IGB_TXD_DCMD; tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type); Anyway, the patch series is far from being "entirely incorrect". Even with the contested gfar_wmb() statement left in place, the proposed code refactoring of xmit() improves execution time, and code maintainability.
