[PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

The hardware must not see that is given ownership of a buffer until it is
completely written, and when the driver receives ownership of a buffer,
it must ensure that any other reads to the buffer reflect its final
state.  Thus, I/O barriers are added where required.

Without this patch, I have observed GCC reordering the setting of
bdp->length and bdp->status in gfar_new_skb.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 drivers/net/gianfar.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index b666a0c..f187dc3 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1025,6 +1025,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	dev->trans_start = jiffies;
 
+	iobarrier_w();
 	txbdp->status = status;
 
 	/* If this was the last BD in the ring, the next one */
@@ -1301,6 +1302,7 @@ struct sk_buff * gfar_new_skb(struct net_device *dev, struct rxbd8 *bdp)
 	bdp->length = 0;
 
 	/* Mark the buffer empty */
+	iobarrier_w();
 	bdp->status |= (RXBD_EMPTY | RXBD_INTERRUPT);
 
 	return skb;
@@ -1484,6 +1486,7 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 	bdp = priv->cur_rx;
 
 	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
+		iobarrier_r();
 		skb = priv->rx_skbuff[priv->skb_currx];
 
 		if (!(bdp->status &
-- 
1.5.0.3

Re: [PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Segher Boessenkool]

> The hardware must not see that is given ownership of a buffer until it 
> is
> completely written, and when the driver receives ownership of a buffer,
> it must ensure that any other reads to the buffer reflect its final
> state.  Thus, I/O barriers are added where required.
>
> Without this patch, I have observed GCC reordering the setting of
> bdp->length and bdp->status in gfar_new_skb.

The :::"memory" in the barriers you used prevent GCC
from reordering accesses around the barriers.

AFAICS you need stronger barriers though; {w,r,}mb(),
to prevent _any_ reordering of those memory accesses,
not just the compiler-generated ones.


Segher

Re: [PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

Segher Boessenkool wrote:
>> The hardware must not see that is given ownership of a buffer until it is
>> completely written, and when the driver receives ownership of a buffer,
>> it must ensure that any other reads to the buffer reflect its final
>> state.  Thus, I/O barriers are added where required.
>>
>> Without this patch, I have observed GCC reordering the setting of
>> bdp->length and bdp->status in gfar_new_skb.
> 
> 
> The :::"memory" in the barriers you used prevent GCC
> from reordering accesses around the barriers.

Sure... it was just an example to point out that it's actually 
happening, rather than a theoretical concern.

> AFAICS you need stronger barriers though; {w,r,}mb(),
> to prevent _any_ reordering of those memory accesses,
> not just the compiler-generated ones.

My impression was that the eieio used by iobarrier would be sufficient 
for that, as we're not trying to synchronize between accesses to 
different types of memory.  Is sync really required here?

-Scott

Re: [PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Segher Boessenkool]

>> AFAICS you need stronger barriers though; {w,r,}mb(),
>> to prevent _any_ reordering of those memory accesses,
>> not just the compiler-generated ones.
>
> My impression was that the eieio used by iobarrier would be sufficient 
> for that, as we're not trying to synchronize between accesses to 
> different types of memory.  Is sync really required here?

For accesses to main system memory, eieio only orders
writes, not reads, so iobarrier_r() doesn't do what
you want; and iobarrier_w() isn't meant to be used for
main memory access ordering either.

Also, it is better to not use powerpc-specific interfaces
in a device driver if you don't have a strong reason to.


Segher

[PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

The hardware must not see that is given ownership of a buffer until it is
completely written, and when the driver receives ownership of a buffer,
it must ensure that any other reads to the buffer reflect its final
state.  Thus, I/O barriers are added where required.

Without this patch, I have observed GCC reordering the setting of
bdp->length and bdp->status in gfar_new_skb.  Hardware reordering
was also theoretically possible.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
I've added the requested comments to the source code about
the use of eieio().  Jeff, please consider for 2.6.22, as it
fixes a bug that has been observed.

 drivers/net/gianfar.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index b666a0c..f5b3cba 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1025,6 +1025,15 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	dev->trans_start = jiffies;
 
+	/* The powerpc-specific eieio() is used, as wmb() has too strong
+	 * semantics (it requires synchronization between cacheable and
+	 * uncacheable mappings, which eieio doesn't provide and which we
+	 * don't need), thus requiring a more expensive sync instruction.  At
+	 * some point, the set of architecture-independent barrier functions
+	 * should be expanded to include weaker barriers.
+	 */
+
+	eieio();
 	txbdp->status = status;
 
 	/* If this was the last BD in the ring, the next one */
@@ -1301,6 +1310,7 @@ struct sk_buff * gfar_new_skb(struct net_device *dev, struct rxbd8 *bdp)
 	bdp->length = 0;
 
 	/* Mark the buffer empty */
+	eieio();
 	bdp->status |= (RXBD_EMPTY | RXBD_INTERRUPT);
 
 	return skb;
@@ -1484,6 +1494,7 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 	bdp = priv->cur_rx;
 
 	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
+		rmb();
 		skb = priv->rx_skbuff[priv->skb_currx];
 
 		if (!(bdp->status &
-- 
1.5.0.3

Re: [PATCH] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Jeff Garzik]

Scott Wood wrote:
> The hardware must not see that is given ownership of a buffer until it is
> completely written, and when the driver receives ownership of a buffer,
> it must ensure that any other reads to the buffer reflect its final
> state.  Thus, I/O barriers are added where required.
> 
> Without this patch, I have observed GCC reordering the setting of
> bdp->length and bdp->status in gfar_new_skb.  Hardware reordering
> was also theoretically possible.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> I've added the requested comments to the source code about
> the use of eieio().  Jeff, please consider for 2.6.22, as it
> fixes a bug that has been observed.
> 
>  drivers/net/gianfar.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)

applied