From c7884aec1fb4f43436931220c4da2b5e1a33b41a Mon Sep 17 00:00:00 2001 From: Phil Elwell Date: Mon, 7 Mar 2016 15:05:11 +0000 Subject: [PATCH 1/5] vchiq_arm: Tweak the logging output Signed-off-by: Phil Elwell --- .../interface/vchiq_arm/vchiq_core.c | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) 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 2c98da4307dff..160db24aeea33 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c @@ -891,16 +891,14 @@ queue_message(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T *service, error_count); return VCHIQ_ERROR; } - if (i == 0) { - if (SRVTRACE_ENABLED(service, - VCHIQ_LOG_INFO)) - vchiq_log_dump_mem("Sent", 0, - header->data + pos, - min(64u, - elements[0].size)); - } } + if (SRVTRACE_ENABLED(service, + VCHIQ_LOG_INFO)) + vchiq_log_dump_mem("Sent", 0, + header->data, + min(16, pos)); + spin_lock("a_spinlock); service_quota->message_use_count++; @@ -1039,16 +1037,13 @@ queue_message_sync(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T *service, error_count); return VCHIQ_ERROR; } - if (i == 0) { - if (vchiq_sync_log_level >= - VCHIQ_LOG_TRACE) - vchiq_log_dump_mem("Sent Sync", - 0, header->data + pos, - min(64u, - elements[0].size)); - } } + if (vchiq_sync_log_level >= VCHIQ_LOG_TRACE) + vchiq_log_dump_mem("Sent Sync", + 0, header->data, + min(16, pos)); + VCHIQ_SERVICE_STATS_INC(service, ctrl_tx_count); VCHIQ_SERVICE_STATS_ADD(service, ctrl_tx_bytes, size); } else { @@ -1720,7 +1715,7 @@ parse_rx_slots(VCHIQ_STATE_T *state) remoteport, localport, size); if (size > 0) vchiq_log_dump_mem("Rcvd", 0, header->data, - min(64, size)); + min(16, size)); } if (((unsigned int)header & VCHIQ_SLOT_MASK) + calc_stride(size) @@ -2187,7 +2182,7 @@ sync_func(void *v) remoteport, localport, size); if (size > 0) vchiq_log_dump_mem("Rcvd", 0, header->data, - min(64, size)); + min(16, size)); } switch (type) { From 1bc256a520f2b99b8f81c208eaef1a201b906398 Mon Sep 17 00:00:00 2001 From: Phil Elwell Date: Wed, 23 Mar 2016 14:16:25 +0000 Subject: [PATCH 2/5] vchiq_arm: Access the dequeue_pending flag locked Reading through this code looking for another problem (now found in userland) the use of dequeue_pending outside a lock didn't seem safe. Signed-off-by: Phil Elwell --- .../interface/vchiq_arm/vchiq_arm.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 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 47df1af2219d8..45399a3029e78 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -279,6 +279,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header, USER_SERVICE_T *user_service; VCHIQ_SERVICE_T *service; VCHIQ_INSTANCE_T instance; + int skip_completion = 0; DEBUG_INITIALISE(g_state.local) DEBUG_TRACE(SERVICE_CALLBACK_LINE); @@ -345,9 +346,6 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header, user_service->msg_queue[user_service->msg_insert & (MSG_QUEUE_SIZE - 1)] = header; user_service->msg_insert++; - spin_unlock(&msg_queue_spinlock); - - up(&user_service->insert_event); /* If there is a thread waiting in DEQUEUE_MESSAGE, or if ** there is a MESSAGE_AVAILABLE in the completion queue then @@ -356,13 +354,22 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header, if (((user_service->message_available_pos - instance->completion_remove) >= 0) || user_service->dequeue_pending) { - DEBUG_TRACE(SERVICE_CALLBACK_LINE); user_service->dequeue_pending = 0; - return VCHIQ_SUCCESS; + skip_completion = 1; } + spin_unlock(&msg_queue_spinlock); + + up(&user_service->insert_event); + header = NULL; } + + if (skip_completion) { + DEBUG_TRACE(SERVICE_CALLBACK_LINE); + return VCHIQ_SUCCESS; + } + DEBUG_TRACE(SERVICE_CALLBACK_LINE); return add_completion(instance, reason, header, user_service, From e5afc1b4668074b0e157492dc2b9e93afcea7605 Mon Sep 17 00:00:00 2001 From: Phil Elwell Date: Wed, 23 Mar 2016 20:53:47 +0000 Subject: [PATCH 3/5] vchiq_arm: Service callbacks must not fail Service callbacks are not allowed to return an error. The internal callback that delivers events and messages to user tasks does not enqueue them if the service is closing, but this is not an error and should not be reported as such. Signed-off-by: Phil Elwell --- drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 45399a3029e78..a76060c57cbbc 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -224,7 +224,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, } else if (instance->closing) { vchiq_log_info(vchiq_arm_log_level, "service_callback closing"); - return VCHIQ_ERROR; + return VCHIQ_SUCCESS; } DEBUG_TRACE(SERVICE_CALLBACK_LINE); } From 474b5106085db295a31c455c0457d6a4b587ca1a Mon Sep 17 00:00:00 2001 From: Phil Elwell Date: Thu, 21 Apr 2016 13:49:32 +0100 Subject: [PATCH 4/5] vchiq_arm: Add completion records under the mutex 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. While only one thread adds completions, without the mutex you don't get the protection of the automatic memory barrier you get with synchronisation objects. Signed-off-by: Phil Elwell --- .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 a76060c57cbbc..51e6018f413ca 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -210,6 +210,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, VCHIQ_COMPLETION_DATA_T *completion; DEBUG_INITIALISE(g_state.local) + mutex_lock(&instance->completion_mutex); + while (instance->completion_insert == (instance->completion_remove + MAX_COMPLETIONS)) { /* Out of space - wait for the client */ @@ -217,11 +219,17 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, vchiq_log_trace(vchiq_arm_log_level, "add_completion - completion queue full"); DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT); + + mutex_unlock(&instance->completion_mutex); if (down_interruptible(&instance->remove_event) != 0) { vchiq_log_info(vchiq_arm_log_level, "service_callback interrupted"); return VCHIQ_RETRY; - } else if (instance->closing) { + } + + mutex_lock(&instance->completion_mutex); + if (instance->closing) { + mutex_unlock(&instance->completion_mutex); vchiq_log_info(vchiq_arm_log_level, "service_callback closing"); return VCHIQ_SUCCESS; @@ -254,8 +262,11 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, if (reason == VCHIQ_MESSAGE_AVAILABLE) user_service->message_available_pos = instance->completion_insert; + instance->completion_insert++; + mutex_unlock(&instance->completion_mutex); + up(&instance->insert_event); return VCHIQ_SUCCESS; From 76f2eaf87dda5fec52c144f002e7b31527c902a1 Mon Sep 17 00:00:00 2001 From: Phil Elwell Date: Mon, 20 Jun 2016 13:51:44 +0100 Subject: [PATCH 5/5] vchiq_arm: Avoid use of mutex in add_completion Claiming the completion_mutex within add_completion did prevent some messages appearing twice, but provokes a deadlock caused by vcsm using vchiq within a page fault handler. Revert the use of completion_mutex, and instead fix the original problem using more memory barriers. Signed-off-by: Phil Elwell --- .../interface/vchiq_arm/vchiq_arm.c | 55 ++++++++++--------- .../interface/vchiq_arm/vchiq_core.c | 14 +++-- 2 files changed, 37 insertions(+), 32 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 51e6018f413ca..2dfccafc765b2 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -64,10 +64,10 @@ #define VCHIQ_MINOR 0 /* Some per-instance constants */ -#define MAX_COMPLETIONS 16 +#define MAX_COMPLETIONS 128 #define MAX_SERVICES 64 #define MAX_ELEMENTS 8 -#define MSG_QUEUE_SIZE 64 +#define MSG_QUEUE_SIZE 128 #define KEEPALIVE_VER 1 #define KEEPALIVE_VER_MIN KEEPALIVE_VER @@ -208,28 +208,24 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, void *bulk_userdata) { VCHIQ_COMPLETION_DATA_T *completion; + int insert; DEBUG_INITIALISE(g_state.local) - mutex_lock(&instance->completion_mutex); - - 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, "add_completion - completion queue full"); DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT); - mutex_unlock(&instance->completion_mutex); if (down_interruptible(&instance->remove_event) != 0) { vchiq_log_info(vchiq_arm_log_level, "service_callback interrupted"); return VCHIQ_RETRY; } - mutex_lock(&instance->completion_mutex); if (instance->closing) { - mutex_unlock(&instance->completion_mutex); vchiq_log_info(vchiq_arm_log_level, "service_callback closing"); return VCHIQ_SUCCESS; @@ -237,9 +233,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, 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; @@ -260,12 +254,9 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason, wmb(); if (reason == VCHIQ_MESSAGE_AVAILABLE) - user_service->message_available_pos = - instance->completion_insert; + user_service->message_available_pos = insert; - instance->completion_insert++; - - mutex_unlock(&instance->completion_mutex); + instance->completion_insert = ++insert; up(&instance->insert_event); @@ -795,6 +786,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg) instance->completion_insert) && !instance->closing) { int rc; + DEBUG_TRACE(AWAIT_COMPLETION_LINE); mutex_unlock(&instance->completion_mutex); rc = down_interruptible(&instance->insert_event); @@ -809,24 +801,29 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg) } 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; + + 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 prevent + ** the prefetch of a stale completion record + */ + rmb(); service = completion->service_userdata; user_service = service->base.userdata; @@ -903,7 +900,11 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break; } - instance->completion_remove++; + /* Ensure that the above copy has completed + ** before advancing the remove pointer. */ + mb(); + + 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 160db24aeea33..71a3bedc55314 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c @@ -610,15 +610,15 @@ process_free_queue(VCHIQ_STATE_T *state) 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++ & @@ -626,6 +626,8 @@ process_free_queue(VCHIQ_STATE_T *state) char *data = (char *)SLOT_DATA_FROM_INDEX(state, slot_index); int data_found = 0; + rmb(); + vchiq_log_trace(vchiq_core_log_level, "%d: pfq %d=%x %x %x", state->id, slot_index, (unsigned int)data, local->slot_queue_recycle, slot_queue_available); @@ -741,6 +743,8 @@ process_free_queue(VCHIQ_STATE_T *state) up(&state->data_quota_event); } + mb(); + state->slot_queue_available = slot_queue_available; up(&state->slot_available_event); }