[net-next,1/4] via-rhine: commit receive buffer address before descriptor status update.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 1 +
 1 file changed, 1 insertion(+)

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