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

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 <redacted>
---
This is version 2 of this patch.  I was told that eieio doesn't order
loads from main memory, so a sync is used instead.  Also, due to
objectons that iobarrier_* shouldn't be used for main memory, I used
eieio() instead (a wmb() would be unnecessarily heavy).

 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..b014d27 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;
 
+	eieio();
 	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 */
+	eieio();
 	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))) {
+		rmb();
 		skb = priv->rx_skbuff[priv->skb_currx];
 
 		if (!(bdp->status &

-- 
1.5.0.3

On May 2, 2007, at 2:57 PM, 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.

Signed-off-by: Scott Wood <redacted>
---
This is version 2 of this patch.  I was told that eieio doesn't order
loads from main memory, so a sync is used instead.  Also, due to
objectons that iobarrier_* shouldn't be used for main memory, I used
eieio() instead (a wmb() would be unnecessarily heavy).

I'd rather see a wmb() instead of eieio() to keep this code non-ppc  
specific.  (also, we implement wmb as eieio, so I don't keep the  
comment about it being too heavy, unless you mean generically).

- k

Kumar Gala wrote:

I'd rather see a wmb() instead of eieio() to keep this code non-ppc  
specific.  (also, we implement wmb as eieio, so I don't keep the  
comment about it being too heavy, unless you mean generically).

wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would not 
accept a sync in those spots.

And the driver is already ppc-specific; it uses in/out_be32.

-Scott

On May 2, 2007, at 3:12 PM, Scott Wood wrote:

Kumar Gala wrote:

quoted

I'd rather see a wmb() instead of eieio() to keep this code non- 
ppc  specific.  (also, we implement wmb as eieio, so I don't keep  
the  comment about it being too heavy, unless you mean generically).

wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would not  
accept a sync in those spots.

Sorry, was looking at the iobarrier code.

And the driver is already ppc-specific; it uses in/out_be32.

True, but its hidden behind the gfar_read/write accessors.

Your change is a bit more blatant.

- k

Kumar Gala wrote:

On May 2, 2007, at 3:12 PM, Scott Wood wrote:

quoted

wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would not  
accept a sync in those spots.

Sorry, was looking at the iobarrier code.

quoted

And the driver is already ppc-specific; it uses in/out_be32.

True, but its hidden behind the gfar_read/write accessors.

Your change is a bit more blatant.

Well, Segher doesn't want me to use iobarrier (because it's not I/O). 
Andy doesn't want me to use wmb() (because it's sync).  I don't think 
something like gfar_wmb() would be appropriate.  So the remaining 
options are either eieio(), or a new non-arch-specific, 
non-driver-specific mem_wmb() (or whatever).

While I like the latter option, I don't think this bugfix should have to 
wait for it.

-Scott

On May 2, 2007, at 3:40 PM, Scott Wood wrote:

Kumar Gala wrote:

quoted

On May 2, 2007, at 3:12 PM, Scott Wood wrote:

quoted

wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would  
not  accept a sync in those spots.

Sorry, was looking at the iobarrier code.

quoted

And the driver is already ppc-specific; it uses in/out_be32.

True, but its hidden behind the gfar_read/write accessors.
Your change is a bit more blatant.

Well, Segher doesn't want me to use iobarrier (because it's not I/ 
O). Andy doesn't want me to use wmb() (because it's sync).  I don't  
think something like gfar_wmb() would be appropriate.  So the  
remaining options are either eieio(), or a new non-arch-specific,  
non-driver-specific mem_wmb() (or whatever).

While I like the latter option, I don't think this bugfix should  
have to wait for it.

Ok, I've resigned to the eieio's.  If we end up respinning the patch  
again for any reason I'd like to see something in the commit comment  
to the fact that we are adding ppc specific sync operations.

Why doesn't marking the bdp pointer volatile resolve the issue in  
gfar_clean_rx_ring() to ensure load ordering?

- k

Kumar Gala wrote:

Why doesn't marking the bdp pointer volatile resolve the issue in  
gfar_clean_rx_ring() to ensure load ordering?

Because that only addresses compiler reordering (and does so in a rather 
clumsy way -- not all accesses need to be strongly ordered), not 
hardware reordering.

-Scott

quoted

quoted

And the driver is already ppc-specific; it uses in/out_be32.

True, but its hidden behind the gfar_read/write accessors.

Your change is a bit more blatant.

Well, Segher doesn't want me to use iobarrier (because it's not I/O).
Andy doesn't want me to use wmb() (because it's sync).

You should use wmb(), but unfortunately too strong
semantics are required for that (ordering wrt I/O)
so it's a full sync on PowerPC.  I don't believe
a priori that that would be notably slower, but if
actually is, you could use eieio() I suppose since
you say the driver is powerpc specific -- but please
put a comment in the source code then saying why you
don't use wmb() there.


Segher

On Wed, 2 May 2007, Scott Wood wrote:

Kumar Gala wrote:

quoted

Why doesn't marking the bdp pointer volatile resolve the issue in
gfar_clean_rx_ring() to ensure load ordering?

Because that only addresses compiler reordering (and does so in a rather
clumsy way -- not all accesses need to be strongly ordered), not hardware
reordering.

-Scott

So what about some thing like this where we do the read only once?

- k

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index a06d8d1..9cd7d1e 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c

@@ -1438,31 +1438,35 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 {
 	struct rxbd8 *bdp;
 	struct sk_buff *skb;
-	u16 pkt_len;
+	u16 pkt_len, status;
+	u32 bd_info;
 	int howmany = 0;
 	struct gfar_private *priv = netdev_priv(dev);

 	/* Get the first full descriptor */
 	bdp = priv->cur_rx;

-	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
+	bd_info = *bdp;
+
+	status = bd_info >> 16;
+	/* Remove the FCS from the packet length */
+	pkt_len = (bd_info & 0xffff) - 4;
+
+	while (!((status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
 		skb = priv->rx_skbuff[priv->skb_currx];

-		if (!(bdp->status &
+		if (!(status &
 		      (RXBD_LARGE | RXBD_SHORT | RXBD_NONOCTET
 		       | RXBD_CRCERR | RXBD_OVERRUN | RXBD_TRUNCATED))) {
 			/* Increment the number of packets */
 			priv->stats.rx_packets++;
 			howmany++;

-			/* Remove the FCS from the packet length */
-			pkt_len = bdp->length - 4;
-
 			gfar_process_frame(dev, skb, pkt_len);

 			priv->stats.rx_bytes += pkt_len;
 		} else {
-			count_errors(bdp->status, priv);
+			count_errors(status, priv);

 			if (skb)
 				dev_kfree_skb_any(skb);

@@ -1480,7 +1484,7 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 		priv->rx_skbuff[priv->skb_currx] = skb;

 		/* Update to the next pointer */
-		if (bdp->status & RXBD_WRAP)
+		if (status & RXBD_WRAP)
 			bdp = priv->rx_bd_base;
 		else
 			bdp++;

@@ -1490,6 +1494,11 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 		    (priv->skb_currx +
 		     1) & RX_RING_MOD_MASK(priv->rx_ring_size);

+		bd_info = *bdp;
+
+		status = bd_info >> 16;
+		/* Remove the FCS from the packet length */
+		pkt_len = (bd_info & 0xffff) - 4;
 	}

 	/* Update the current rxbd pointer to be the next one */

Kumar Gala wrote:

I suggested that on IRC yesterday, and Segher was concerned that the 
compiler might, in theory, "optimize" it into to two lhz instructions. 
I'm rather skeptical that it would actually do so (even if it needs to 
load twice due to register pressure, why not just use lwz both times?), 
and there's probably many other places that would break if it did, but I 
wasn't up for digging around GCC to prove otherwise.

Plus, that wouldn't synchronize the bd_info read with the buffer data reads.

-Scott

quoted

So what about some thing like this where we do the read only once?

- k

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index a06d8d1..9cd7d1e 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c

@@ -1438,31 +1438,35 @@ int gfar_clean_rx_ring(struct net_device 

*dev, int rx_work_limit)
 {
 	struct rxbd8 *bdp;
 	struct sk_buff *skb;
-	u16 pkt_len;
+	u16 pkt_len, status;
+	u32 bd_info;

I suggested that on IRC yesterday, and Segher was concerned that the
compiler might, in theory, "optimize" it into to two lhz instructions.

Yes.  The same is true of the original code btw, but since
you test only one bit there, all is fine.

I'm rather skeptical that it would actually do so (even if it needs to
load twice due to register pressure, why not just use lwz both times?),

Sure.  That doesn't make this code correct though.

and there's probably many other places that would break if it did,

Most other network drivers read from an MMIO reg to see
which RX ring entries are kernel owned AFAICS.

but I wasn't up for digging around GCC to prove otherwise.

It doesn't matter what current GCC does -- simply look
at what it is *allowed* to do instead.

If you want a 32-bit read to be atomic, you should
do the read via a (volatile u32 *).  Doing this with
a cast in the places where you need the atomic access
makes sure you don't get unnecessary rereads.

Plus, that wouldn't synchronize the bd_info read with the buffer data 
reads.

Yes, you still need the rmb().


Segher

On Wed, May 02, 2007 at 03:40:20PM -0500, Scott Wood wrote:

Well, Segher doesn't want me to use iobarrier (because it's not I/O). 
Andy doesn't want me to use wmb() (because it's sync).  I don't think 
something like gfar_wmb() would be appropriate.  So the remaining 
options are either eieio(), 

? Just curious... the original intent of eieio was to order I/O, 
such as MMIO; it has no effect on memory that isn't marked 
cache-inhibited or write-trhough or guarded. Has this changed?
I guess I haven't kept up with the times ... is eieio now
being used to provide some other kind of barrier?
Is eieio providing some sort of SMP synchronization side-effect?

Point being: if Segher doesn't let you "use iobarrier (because 
it's not I/O)", then I don't understand why eieio would work (since
that's for io only).  

--linas

On Fri, May 04, 2007 at 05:13:09PM -0500, Linas Vepstas wrote:

On Wed, May 02, 2007 at 03:40:20PM -0500, Scott Wood wrote:

quoted

Well, Segher doesn't want me to use iobarrier (because it's not I/O). 
Andy doesn't want me to use wmb() (because it's sync).  I don't think 
something like gfar_wmb() would be appropriate.  So the remaining 
options are either eieio(), 

? Just curious... the original intent of eieio was to order I/O, 
such as MMIO; it has no effect on memory that isn't marked 
cache-inhibited or write-trhough or guarded. Has this changed?
I guess I haven't kept up with the times ... is eieio now
being used to provide some other kind of barrier?
Is eieio providing some sort of SMP synchronization side-effect?

Point being: if Segher doesn't let you "use iobarrier (because 
it's not I/O)", then I don't understand why eieio would work (since
that's for io only).  

Eieio has always worked for regular cachable memory as well, it just never
orders _between_ cache inhibited/guarded and cachable memory.

Book II 2.03 has a pretty good description of it on page 367.



-Olof

quoted

Well, Segher doesn't want me to use iobarrier (because it's not I/O).
Andy doesn't want me to use wmb() (because it's sync).  I don't think
something like gfar_wmb() would be appropriate.  So the remaining
options are either eieio(),

? Just curious... the original intent of eieio was to order I/O,
such as MMIO; it has no effect on memory that isn't marked
cache-inhibited or write-trhough or guarded. Has this changed?

eieio orders all accesses to address space that is WIMG=1xxx
or WIMG=x1x1; separately, it orders stores to address space
that is WIMG=001x.

I guess I haven't kept up with the times ... is eieio now
being used to provide some other kind of barrier?

Nothing changed.

Is eieio providing some sort of SMP synchronization side-effect?

It orders stores to "well-behaved" memory yes.

Point being: if Segher doesn't let you "use iobarrier (because
it's not I/O)", then I don't understand why eieio would work (since
that's for io only).

iobarrier() is a kernel-level primitive, meant for ordering
I/O only, as its name indicates.  eieio is a CPU insn that
orders stores to main memory (amongst other things), not that
its name would tell you.


Segher