Skip to contentSkip to conversation

ibmveth bug fixes 4/4

PATCH In review Santiago Leon 2 participants 4 messages Aug 10, 2004 – Aug 15, 2004 Network drivers

lore
Share & tools
lore

Santiago Leon

author
#0 lore raw

PATCH 1/1 ibmveth bug fixes 4/4

Andrew,

This patch adds a memory barrier to ensure synchronization with the hypervisor (and avoid a panic when the hypervisor is halfway through writing to the descriptor). It also removes an unnecessary check that is flawed anyway because the value can change between the atomic_inc() and the assert. Please apply.

Signed-off-by Santiago Leon <[email protected]>
Signature · 3 lines
Santiago A. Leon
Power Linux Development
IBM Linux Technology Center

===== drivers/net/ibmveth.c 1.16 vs edited =====

drivers/net/ibmveth.c +2 -1
@@ -271,7 +271,6 @@
 	adapter->rx_no_buffer = *(u64*)(((char*)adapter->buffer_list_addr) + 4096 - 8);
 
 	atomic_inc(&adapter->not_replenishing);
-	ibmveth_assert(atomic_read(&adapter->not_replenishing) == 1);
 }
 
 /* kick the replenish tasklet if we need replenishing and it isn't already running */

@@ -733,6 +732,8 @@
 
 		if(ibmveth_rxq_pending_buffer(adapter)) {
 			struct sk_buff *skb;
+
+			rmb();
 
 			if(!ibmveth_rxq_buffer_valid(adapter)) {
 				wmb(); /* suggested by larson1 */
author
#1 lore raw

Andrew...

I had discussion over email with Dave Hansen about this patch and after I explained the bug and the fix, he recommended that I include a comment in the patch. So here's an updated patch. Please apply.

Signature · 3 lines
Santiago A. Leon
Power Linux Development
IBM Linux Technology Center

===== drivers/net/ibmveth.c 1.14 vs edited =====

drivers/net/ibmveth.c +8 -1
@@ -271,7 +271,6 @@
 	adapter->rx_no_buffer = *(u64*)(((char*)adapter->buffer_list_addr) + 4096 - 8);
 
 	atomic_inc(&adapter->not_replenishing);
-	ibmveth_assert(atomic_read(&adapter->not_replenishing) == 1);
 }
 
 /* kick the replenish tasklet if we need replenishing and it isn't already running */

@@ -733,6 +732,14 @@
 
 		if(ibmveth_rxq_pending_buffer(adapter)) {
 			struct sk_buff *skb;
+
+			/* This barrier is necessary because we might be 
+			   reading a control dword and then reading an old 
+			   cached correlator dword. Also, the hypervisor
+			   guarantees that the control field will be globally
+			   visible after all the other fields are visible, 
+			*/
+			rmb();
 
 			if(!ibmveth_rxq_buffer_valid(adapter)) {
 				wmb(); /* suggested by larson1 */
author
#2 lore raw
Quoted 1 line

I had discussion over email with Dave Hansen about this patch and after I explained the bug and the fix, he recommended that I include a comment in the patch. So here's an updated patch. Please apply.

Ooops.. forgot the signed stuff... Here it goes again...

Signed-off-by Santiago Leon <[email protected]>
Signature · 3 lines
Santiago A. Leon
Power Linux Development
IBM Linux Technology Center

===== drivers/net/ibmveth.c 1.14 vs edited =====

drivers/net/ibmveth.c +8 -1
@@ -271,7 +271,6 @@
 	adapter->rx_no_buffer = *(u64*)(((char*)adapter->buffer_list_addr) + 4096 - 8);
 
 	atomic_inc(&adapter->not_replenishing);
-	ibmveth_assert(atomic_read(&adapter->not_replenishing) == 1);
 }
 
 /* kick the replenish tasklet if we need replenishing and it isn't already running */

@@ -733,6 +732,14 @@
 
 		if(ibmveth_rxq_pending_buffer(adapter)) {
 			struct sk_buff *skb;
+
+			/* This barrier is necessary because we might be 
+			   reading a control dword and then reading an old 
+			   cached correlator dword. Also, the hypervisor
+			   guarantees that the control field will be globally
+			   visible after all the other fields are visible, 
+			*/
+			rmb();
 
 			if(!ibmveth_rxq_buffer_valid(adapter)) {
 				wmb(); /* suggested by larson1 */
#3 lore raw
In reply to Santiago Leon
Quoted 7 lines · Santiago Leon

his patch adds a memory barrier to ensure synchronization with the
hypervisor (and avoid a panic when the hypervisor is halfway through writing to the descriptor). It also removes an unnecessary check that is flawed anyway because the value can change between the atomic_inc() and the assert. Please apply.

Quoted 7 lines

...
@@ -733,6 +732,8 @@

if(ibmveth_rxq_pending_buffer(adapter)) {
struct sk_buff *skb;
+
+ rmb();

Please always add a comment when adding such a barrier - it is otherwise quite impossible for the reader to work out what it is doing in there.

Also, please in future avoid giving your patches subjects such as "ibmveth bug fixes 4/4". Remember that the Subject: in your email will be carried through into kernel bitkeeper and it should be meaningful.

I relabelled these four patches as

ibmveth: module tag fixes
ibmveth: race fixes
ibmveth: hypervisor return value fix
ibmveth: add memory barrier for hypervisor synchronisation

Thanks.