[PATCH 4/5] staging: vc04_services: Fix messages appearing twice

[Stefan Wahren]

From: Phil Elwell <phil at raspberrypi.org>

An issue was observed when flushing openmax components
which generate a large number of messages returning
buffers to host.

We occasionally found a duplicate message from 16
messages prior, resulting in a buffer returned twice.

So fix the issue by adding more memory barriers.

Signed-off-by: Phil Elwell <phil at raspberrypi.org>
Signed-off-by: Stefan Wahren <stefan.wahren at i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |   45 ++++++++++++--------
 .../vc04_services/interface/vchiq_arm/vchiq_core.c |   24 ++++++++---
 2 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 72945f9..b02dc4b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -208,10 +208,11 @@ struct vchiq_instance_struct {
 	void *bulk_userdata)
 {
 	VCHIQ_COMPLETION_DATA_T *completion;
+	int insert;
 	DEBUG_INITIALISE(g_state.local)
 
-	while (instance->completion_insert ==
-		(instance->completion_remove + MAX_COMPLETIONS)) {
+	insert = instance->completion_insert;
+	while ((insert - instance->completion_remove) >= MAX_COMPLETIONS) {
 		/* Out of space - wait for the client */
 		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 		vchiq_log_trace(vchiq_arm_log_level,
@@ -229,9 +230,7 @@ struct vchiq_instance_struct {
 		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 	}
 
-	completion =
-		 &instance->completions[instance->completion_insert &
-		 (MAX_COMPLETIONS - 1)];
+	completion = &instance->completions[insert & (MAX_COMPLETIONS - 1)];
 
 	completion->header = header;
 	completion->reason = reason;
@@ -252,9 +251,10 @@ struct vchiq_instance_struct {
 	wmb();
 
 	if (reason == VCHIQ_MESSAGE_AVAILABLE)
-		user_service->message_available_pos =
-			instance->completion_insert;
-	instance->completion_insert++;
+		user_service->message_available_pos = insert;
+
+	insert++;
+	instance->completion_insert = insert;
 
 	up(&instance->insert_event);
 
@@ -895,24 +895,27 @@ struct vchiq_io_copy_callback_context {
 		}
 		DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 
-		/* A read memory barrier is needed to stop prefetch of a stale
-		** completion record
-		*/
-		rmb();
-
 		if (ret == 0) {
 			int msgbufcount = args.msgbufcount;
+			int remove = instance->completion_remove;
+
 			for (ret = 0; ret < args.count; ret++) {
 				VCHIQ_COMPLETION_DATA_T *completion;
 				VCHIQ_SERVICE_T *service;
 				USER_SERVICE_T *user_service;
 				VCHIQ_HEADER_T *header;
-				if (instance->completion_remove ==
-					instance->completion_insert)
+
+				if (remove == instance->completion_insert)
 					break;
+
 				completion = &instance->completions[
-					instance->completion_remove &
-					(MAX_COMPLETIONS - 1)];
+					remove & (MAX_COMPLETIONS - 1)];
+
+				/*
+				 * A read memory barrier is needed to stop
+				 * prefetch of a stale completion record
+				 */
+				rmb();
 
 				service = completion->service_userdata;
 				user_service = service->base.userdata;
@@ -987,7 +990,13 @@ struct vchiq_io_copy_callback_context {
 					break;
 				}
 
-				instance->completion_remove++;
+				/*
+				 * Ensure that the above copy has completed
+				 * before advancing the remove pointer.
+				 */
+				mb();
+				remove++;
+				instance->completion_remove = remove;
 			}
 
 			if (msgbufcount != args.msgbufcount) {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 9867e64..d587097 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -607,15 +607,17 @@ static const char *msg_type_str(unsigned int msg_type)
 	BITSET_T service_found[BITSET_SIZE(VCHIQ_MAX_SERVICES)];
 	int slot_queue_available;
 
-	/* Use a read memory barrier to ensure that any state that may have
-	** been modified by another thread is not masked by stale prefetched
-	** values. */
-	rmb();
-
 	/* Find slots which have been freed by the other side, and return them
 	** to the available queue. */
 	slot_queue_available = state->slot_queue_available;
 
+	/*
+	 * Use a memory barrier to ensure that any state that may have been
+	 * modified by another thread is not masked by stale prefetched
+	 * values.
+	 */
+	mb();
+
 	while (slot_queue_available != local->slot_queue_recycle) {
 		unsigned int pos;
 		int slot_index = local->slot_queue[slot_queue_available++ &
@@ -623,6 +625,12 @@ static const char *msg_type_str(unsigned int msg_type)
 		char *data = (char *)SLOT_DATA_FROM_INDEX(state, slot_index);
 		int data_found = 0;
 
+		/*
+		 * Beware of the address dependency - data is calculated
+		 * using an index written by the other side.
+		 */
+		rmb();
+
 		vchiq_log_trace(vchiq_core_log_level, "%d: pfq %d=%pK %x %x",
 			state->id, slot_index, data,
 			local->slot_queue_recycle, slot_queue_available);
@@ -721,6 +729,12 @@ static const char *msg_type_str(unsigned int msg_type)
 				up(&state->data_quota_event);
 		}
 
+		/*
+		 * Don't allow the slot to be reused until we are no
+		 * longer interested in it.
+		 */
+		mb();
+
 		state->slot_queue_available = slot_queue_available;
 		up(&state->slot_available_event);
 	}
-- 
1.7.9.5