Message ID | 87c90b27962c818239073d3a65341054922bd563.1428340371.git.romieu@fr.zoreil.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
From: Francois Romieu <romieu@fr.zoreil.com> Date: Mon, 6 Apr 2015 20:01:49 +0200 > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> > --- > drivers/net/ethernet/via/via-rhine.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c > index a191afc..00fea3d 100644 > --- a/drivers/net/ethernet/via/via-rhine.c > +++ b/drivers/net/ethernet/via/via-rhine.c > @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit) > break; > } > rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]); > + wmb(); dma_wmb() perhaps? I think this is exactly the situation that interface was added for. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2015 11:01 AM, Francois Romieu wrote: > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> > --- > drivers/net/ethernet/via/via-rhine.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c > index a191afc..00fea3d 100644 > --- a/drivers/net/ethernet/via/via-rhine.c > +++ b/drivers/net/ethernet/via/via-rhine.c > @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit) > break; > } > rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]); > + wmb(); > } > rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn); > } Do you need a full wmb() here or would a dma_wmb() be enough? It could make a difference on some processor architectures. - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller <davem@davemloft.net> : > From: Francois Romieu <romieu@fr.zoreil.com> [...] > > @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit) > > break; > > } > > rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]); > > + wmb(); > > dma_wmb() perhaps? I think this is exactly the situation that interface was > added for. I need the buffer address to be written in the receive descriptor before the descriptor status is. The cpu does W1, W2 and the nic mustn't see W2, W1.
On 04/07/2015 02:02 PM, Francois Romieu wrote: > David Miller <davem@davemloft.net> : >> From: Francois Romieu <romieu@fr.zoreil.com> > [...] >>> @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit) >>> break; >>> } >>> rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]); >>> + wmb(); >> dma_wmb() perhaps? I think this is exactly the situation that interface was >> added for. > I need the buffer address to be written in the receive descriptor before > the descriptor status is. The cpu does W1, W2 and the nic mustn't see W2, W1. That is the point of the dma_wmb(). If you are writing both W1 and W2 to system memory then dma_wmb should be enough, if W1 is system memory and W2 is device memory (MMIO) then you need wmb(). You can think of dma_wmb as being something similar to smp_wmb w/o the SMP processor requirement. On architectures that are strong ordered such as most x86 the wmb() translates to a barrier. On other architectures it is something usually a little lighter than a full wmb() so for example on PowerPC wmb is sync() while dma_wmb() is lwsync(). - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Francois Romieu <romieu@fr.zoreil.com> Date: Tue, 7 Apr 2015 23:02:48 +0200 > David Miller <davem@davemloft.net> : >> From: Francois Romieu <romieu@fr.zoreil.com> > [...] >> > @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit) >> > break; >> > } >> > rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]); >> > + wmb(); >> >> dma_wmb() perhaps? I think this is exactly the situation that interface was >> added for. > > I need the buffer address to be written in the receive descriptor before > the descriptor status is. The cpu does W1, W2 and the nic mustn't see W2, W1. That's exactly dma_wmb(). It barriers cpu writes so that the device sees things in a certain order. It's what all the most common ethernet chip drivers use in their descriptor handling routines now. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-04-07 at 17:27 -0400, David Miller wrote: > That's exactly dma_wmb(). > > It barriers cpu writes so that the device sees things in a certain > order. > > It's what all the most common ethernet chip drivers use in their > descriptor handling routines now. To be fair, only 2 drivers currently use dma_wmb() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/07/2015 02:54 PM, Eric Dumazet wrote: > On Tue, 2015-04-07 at 17:27 -0400, David Miller wrote: > >> That's exactly dma_wmb(). >> >> It barriers cpu writes so that the device sees things in a certain >> order. >> >> It's what all the most common ethernet chip drivers use in their >> descriptor handling routines now. > To be fair, only 2 drivers currently use dma_wmb() Hey, I got at least 4.. :-) I only got around to patching 3 Intel drivers and one RealTek since that is what I had to test with. I was honestly hoping there would be more interest from other developers to pick this up and update their drivers to avoid unnecessary barriers but it doesn't look like I have had much luck on that front. Maybe what I can do is submit a set of patches over the next couple of weeks to try and update all the spots that either need to have a barrier added, such as what is being addressed here, or can have a barrier weakened as I have already done for a few other drivers. - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-04-07 at 15:12 -0700, Alexander Duyck wrote: > On 04/07/2015 02:54 PM, Eric Dumazet wrote: > > To be fair, only 2 drivers currently use dma_wmb() > > Hey, I got at least 4.. :-) I only got around to patching 3 Intel > drivers and one RealTek since that is what I had to test with. I was > honestly hoping there would be more interest from other developers to > pick this up and update their drivers to avoid unnecessary barriers but > it doesn't look like I have had much luck on that front. Strange I see only 2 drivers here, and no Intel ones : # git grep -l dma_wmb drivers/net drivers/net/ethernet/amd/xgbe/xgbe-dev.c drivers/net/ethernet/realtek/r8169.c I probably missed something ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/07/2015 03:22 PM, Eric Dumazet wrote: > On Tue, 2015-04-07 at 15:12 -0700, Alexander Duyck wrote: >> On 04/07/2015 02:54 PM, Eric Dumazet wrote: >>> To be fair, only 2 drivers currently use dma_wmb() >> Hey, I got at least 4.. :-) I only got around to patching 3 Intel >> drivers and one RealTek since that is what I had to test with. I was >> honestly hoping there would be more interest from other developers to >> pick this up and update their drivers to avoid unnecessary barriers but >> it doesn't look like I have had much luck on that front. > Strange I see only 2 drivers here, and no Intel ones : > > # git grep -l dma_wmb drivers/net > drivers/net/ethernet/amd/xgbe/xgbe-dev.c > drivers/net/ethernet/realtek/r8169.c > > I probably missed something ;) Yeah, I was thinking of the dma_rmb more than the dma_wmb, they are essentially just two sides to the same coin. dma_rmb - read descriptor status for ownership, then dma_rmb, then if we own it process rest of descriptor dma_wmb - write descriptor fields, then dma_wmb, then update status bit to release ownership A wmb is still needed if a PIO doorbell must be used to notify the device of additional descriptors. - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Alexander Duyck > Sent: 08 April 2015 00:21 ... > dma_wmb - write descriptor fields, then dma_wmb, then update status bit > to release ownership > > A wmb is still needed if a PIO doorbell must be used to notify the > device of additional descriptors. That is needed after the write that releases ownership. So you are likely to need a dma_wmb() before the status bit update and a full wmb() just after it. If you are adding a lot of frames then you should be able to update the status for the first fragment last - and only need the barriers on the final update. David
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c index a191afc..00fea3d 100644 --- a/drivers/net/ethernet/via/via-rhine.c +++ b/drivers/net/ethernet/via/via-rhine.c @@ -2063,6 +2063,7 @@ static int rhine_rx(struct net_device *dev, int limit) break; } rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]); + wmb(); } rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn); }
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> --- drivers/net/ethernet/via/via-rhine.c | 1 + 1 file changed, 1 insertion(+)