[PATCH 0/3] staging: vchiq: fix completion routines & semaphores

[PATCH 0/3] staging: vchiq: fix completion routines & semaphores

[Nicolas Saenz Julienne]

As pointed out by Arnd Bergman[1] there where some issues on how I
converted semaphores to completion routines. I wasn't aware of a
macro override happening in vchiq_killable.h, which was changing
down_interruptible()'s meaning.

This patch changes all completions so they use the proper APIs and gets
rid of vchiq_killable.h. I took into account Arnd's commit to avoid
merge conflicts.

[1] https://www.spinics.net/lists/arm-kernel/msg694422.html
===

Nicolas Saenz Julienne (3):
  staging: vchiq: switch to wait_for_completion_killable
  staging: vchiq_2835_arm: quit using custom down_interruptible()
  staging: vchiq: delete vchiq_killable.h

 .../interface/vchiq_arm/vchiq_2835_arm.c      |  3 +-
 .../interface/vchiq_arm/vchiq_arm.c           | 22 ++++----
 .../interface/vchiq_arm/vchiq_connected.c     |  1 -
 .../interface/vchiq_arm/vchiq_core.c          | 22 ++++----
 .../interface/vchiq_arm/vchiq_killable.h      | 55 -------------------
 .../interface/vchiq_arm/vchiq_util.c          |  7 +--
 6 files changed, 24 insertions(+), 86 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h

-- 
2.19.2

[PATCH 1/3] staging: vchiq: switch to wait_for_completion_killable

[Nicolas Saenz Julienne]

This fixes f27e47bc6b8b ("staging: vchiq: use completions instead of
semaphores") as it neglected the subtle down_interruptible() macro
override in vchiq_killable.h. Hence all completions should be killable
instead of interruptible.

Fixes: f27e47bc6b8b ("staging: vchiq: use completions instead of semaphores")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 21 +++++++++----------
 .../interface/vchiq_arm/vchiq_core.c          | 21 +++++++++----------
 .../interface/vchiq_arm/vchiq_util.c          |  6 +++---
 3 files changed, 23 insertions(+), 25 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 f28f681192dd..227c208dd122 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -559,8 +559,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
 		vchiq_log_trace(vchiq_arm_log_level,
 			"%s - completion queue full", __func__);
 		DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
-		if (wait_for_completion_interruptible(
-					&instance->remove_event)) {
+		if (wait_for_completion_killable( &instance->remove_event)) {
 			vchiq_log_info(vchiq_arm_log_level,
 				"service_callback interrupted");
 			return VCHIQ_RETRY;
@@ -671,7 +670,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
 			}
 
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-			if (wait_for_completion_interruptible(
+			if (wait_for_completion_killable(
 						&user_service->remove_event)
 				!= 0) {
 				vchiq_log_info(vchiq_arm_log_level,
@@ -1006,7 +1005,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		   has been closed until the client library calls the
 		   CLOSE_DELIVERED ioctl, signalling close_event. */
 		if (user_service->close_pending &&
-			wait_for_completion_interruptible(
+			wait_for_completion_killable(
 				&user_service->close_event))
 			status = VCHIQ_RETRY;
 		break;
@@ -1182,7 +1181,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 			DEBUG_TRACE(AWAIT_COMPLETION_LINE);
 			mutex_unlock(&instance->completion_mutex);
-			rc = wait_for_completion_interruptible(
+			rc = wait_for_completion_killable(
 						&instance->insert_event);
 			mutex_lock(&instance->completion_mutex);
 			if (rc != 0) {
@@ -1351,7 +1350,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			do {
 				spin_unlock(&msg_queue_spinlock);
 				DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
-				if (wait_for_completion_interruptible(
+				if (wait_for_completion_killable(
 					&user_service->insert_event)) {
 					vchiq_log_info(vchiq_arm_log_level,
 						"DEQUEUE_MESSAGE interrupted");
@@ -2356,7 +2355,7 @@ vchiq_keepalive_thread_func(void *v)
 	while (1) {
 		long rc = 0, uc = 0;
 
-		if (wait_for_completion_interruptible(&arm_state->ka_evt)
+		if (wait_for_completion_killable(&arm_state->ka_evt)
 				!= 0) {
 			vchiq_log_error(vchiq_susp_log_level,
 				"%s interrupted", __func__);
@@ -2606,7 +2605,7 @@ block_resume(VCHIQ_ARM_STATE_T *arm_state)
 		write_unlock_bh(&arm_state->susp_res_lock);
 		vchiq_log_info(vchiq_susp_log_level, "%s wait for previously "
 			"blocked clients", __func__);
-		if (wait_for_completion_interruptible_timeout(
+		if (wait_for_completion_killable_timeout(
 				&arm_state->blocked_blocker, timeout_val)
 					<= 0) {
 			vchiq_log_error(vchiq_susp_log_level, "%s wait for "
@@ -2632,7 +2631,7 @@ block_resume(VCHIQ_ARM_STATE_T *arm_state)
 		write_unlock_bh(&arm_state->susp_res_lock);
 		vchiq_log_info(vchiq_susp_log_level, "%s wait for resume",
 			__func__);
-		if (wait_for_completion_interruptible_timeout(
+		if (wait_for_completion_killable_timeout(
 				&arm_state->vc_resume_complete, timeout_val)
 					<= 0) {
 			vchiq_log_error(vchiq_susp_log_level, "%s wait for "
@@ -2839,7 +2838,7 @@ vchiq_arm_force_suspend(VCHIQ_STATE_T *state)
 	do {
 		write_unlock_bh(&arm_state->susp_res_lock);
 
-		rc = wait_for_completion_interruptible_timeout(
+		rc = wait_for_completion_killable_timeout(
 				&arm_state->vc_suspend_complete,
 				msecs_to_jiffies(FORCE_SUSPEND_TIMEOUT_MS));
 
@@ -2935,7 +2934,7 @@ vchiq_arm_allow_resume(VCHIQ_STATE_T *state)
 	write_unlock_bh(&arm_state->susp_res_lock);
 
 	if (resume) {
-		if (wait_for_completion_interruptible(
+		if (wait_for_completion_killable(
 			&arm_state->vc_resume_complete) < 0) {
 			vchiq_log_error(vchiq_susp_log_level,
 				"%s interrupted", __func__);
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 5ee667d46eb5..afe2aa5ac3e9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -592,7 +592,7 @@ reserve_space(VCHIQ_STATE_T *state, size_t space, int is_blocking)
 			remote_event_signal(&state->remote->trigger);
 
 			if (!is_blocking ||
-				(wait_for_completion_interruptible(
+				(wait_for_completion_killable(
 				&state->slot_available_event)))
 				return NULL; /* No space available */
 		}
@@ -862,7 +862,7 @@ queue_message(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T *service,
 			spin_unlock(&quota_spinlock);
 			mutex_unlock(&state->slot_mutex);
 
-			if (wait_for_completion_interruptible(
+			if (wait_for_completion_killable(
 						&state->data_quota_event))
 				return VCHIQ_RETRY;
 
@@ -893,7 +893,7 @@ queue_message(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T *service,
 				service_quota->slot_use_count);
 			VCHIQ_SERVICE_STATS_INC(service, quota_stalls);
 			mutex_unlock(&state->slot_mutex);
-			if (wait_for_completion_interruptible(
+			if (wait_for_completion_killable(
 						&service_quota->quota_event))
 				return VCHIQ_RETRY;
 			if (service->closing)
@@ -1742,8 +1742,7 @@ parse_rx_slots(VCHIQ_STATE_T *state)
 					&service->bulk_rx : &service->bulk_tx;
 
 				DEBUG_TRACE(PARSE_LINE);
-				if (mutex_lock_killable(
-					&service->bulk_mutex) != 0) {
+				if (mutex_lock_killable(&service->bulk_mutex)) {
 					DEBUG_TRACE(PARSE_LINE);
 					goto bail_not_ready;
 				}
@@ -2468,7 +2467,7 @@ vchiq_open_service_internal(VCHIQ_SERVICE_T *service, int client_id)
 			       QMFLAGS_IS_BLOCKING);
 	if (status == VCHIQ_SUCCESS) {
 		/* Wait for the ACK/NAK */
-		if (wait_for_completion_interruptible(&service->remove_event)) {
+		if (wait_for_completion_killable(&service->remove_event)) {
 			status = VCHIQ_RETRY;
 			vchiq_release_service_internal(service);
 		} else if ((service->srvstate != VCHIQ_SRVSTATE_OPEN) &&
@@ -2835,7 +2834,7 @@ vchiq_connect_internal(VCHIQ_STATE_T *state, VCHIQ_INSTANCE_T instance)
 	}
 
 	if (state->conn_state == VCHIQ_CONNSTATE_CONNECTING) {
-		if (wait_for_completion_interruptible(&state->connect))
+		if (wait_for_completion_killable(&state->connect))
 			return VCHIQ_RETRY;
 
 		vchiq_set_conn_state(state, VCHIQ_CONNSTATE_CONNECTED);
@@ -2934,7 +2933,7 @@ vchiq_close_service(VCHIQ_SERVICE_HANDLE_T handle)
 	}
 
 	while (1) {
-		if (wait_for_completion_interruptible(&service->remove_event)) {
+		if (wait_for_completion_killable(&service->remove_event)) {
 			status = VCHIQ_RETRY;
 			break;
 		}
@@ -2995,7 +2994,7 @@ vchiq_remove_service(VCHIQ_SERVICE_HANDLE_T handle)
 		request_poll(service->state, service, VCHIQ_POLL_REMOVE);
 	}
 	while (1) {
-		if (wait_for_completion_interruptible(&service->remove_event)) {
+		if (wait_for_completion_killable(&service->remove_event)) {
 			status = VCHIQ_RETRY;
 			break;
 		}
@@ -3078,7 +3077,7 @@ VCHIQ_STATUS_T vchiq_bulk_transfer(VCHIQ_SERVICE_HANDLE_T handle,
 		VCHIQ_SERVICE_STATS_INC(service, bulk_stalls);
 		do {
 			mutex_unlock(&service->bulk_mutex);
-			if (wait_for_completion_interruptible(
+			if (wait_for_completion_killable(
 						&service->bulk_remove_event)) {
 				status = VCHIQ_RETRY;
 				goto error_exit;
@@ -3155,7 +3154,7 @@ VCHIQ_STATUS_T vchiq_bulk_transfer(VCHIQ_SERVICE_HANDLE_T handle,
 
 	if (bulk_waiter) {
 		bulk_waiter->bulk = bulk;
-		if (wait_for_completion_interruptible(&bulk_waiter->event))
+		if (wait_for_completion_killable(&bulk_waiter->event))
 			status = VCHIQ_RETRY;
 		else if (bulk_waiter->actual == VCHIQ_BULK_ACTUAL_ABORTED)
 			status = VCHIQ_ERROR;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
index 4b8554bc647e..42e10d7d97a4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
@@ -80,7 +80,7 @@ void vchiu_queue_push(VCHIU_QUEUE_T *queue, VCHIQ_HEADER_T *header)
 		return;
 
 	while (queue->write == queue->read + queue->size) {
-		if (wait_for_completion_interruptible(&queue->pop))
+		if (wait_for_completion_killable(&queue->pop))
 			flush_signals(current);
 	}
 
@@ -93,7 +93,7 @@ void vchiu_queue_push(VCHIU_QUEUE_T *queue, VCHIQ_HEADER_T *header)
 VCHIQ_HEADER_T *vchiu_queue_peek(VCHIU_QUEUE_T *queue)
 {
 	while (queue->write == queue->read) {
-		if (wait_for_completion_interruptible(&queue->push))
+		if (wait_for_completion_killable(&queue->push))
 			flush_signals(current);
 	}
 
@@ -107,7 +107,7 @@ VCHIQ_HEADER_T *vchiu_queue_pop(VCHIU_QUEUE_T *queue)
 	VCHIQ_HEADER_T *header;
 
 	while (queue->write == queue->read) {
-		if (wait_for_completion_interruptible(&queue->push))
+		if (wait_for_completion_killable(&queue->push))
 			flush_signals(current);
 	}
 
-- 
2.19.2

[PATCH 2/3] staging: vchiq_2835_arm: quit using custom down_interruptible()

[Nicolas Saenz Julienne]

vchi_killable.h overrides down_interruptible() by implementing a
function similar to down_killable(). To make things simpler we turn
calls to down_interruptible() into kernel's implementation of
down_killable().

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index ecee54a31f8d..691038cdfdab 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -543,7 +543,7 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
 		(g_cache_line_size - 1)))) {
 		char *fragments;
 
-		if (down_interruptible(&g_free_fragments_sema) != 0) {
+		if (down_killable(&g_free_fragments_sema)) {
 			cleanup_pagelistinfo(pagelistinfo);
 			return NULL;
 		}
-- 
2.19.2

[PATCH 3/3] staging: vchiq: delete vchiq_killable.h

[Nicolas Saenz Julienne]

There are no users for that header file.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../interface/vchiq_arm/vchiq_2835_arm.c      |  1 -
 .../interface/vchiq_arm/vchiq_arm.c           |  1 -
 .../interface/vchiq_arm/vchiq_connected.c     |  1 -
 .../interface/vchiq_arm/vchiq_core.c          |  1 -
 .../interface/vchiq_arm/vchiq_killable.h      | 55 -------------------
 .../interface/vchiq_arm/vchiq_util.c          |  1 -
 6 files changed, 60 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 691038cdfdab..66fbb9ff551a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -48,7 +48,6 @@
 
 #include "vchiq_arm.h"
 #include "vchiq_connected.h"
-#include "vchiq_killable.h"
 #include "vchiq_pagelist.h"
 
 #define MAX_FRAGMENTS (VCHIQ_NUM_CURRENT_BULKS * 2)
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 227c208dd122..acc7b07c7291 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -55,7 +55,6 @@
 #include "vchiq_ioctl.h"
 #include "vchiq_arm.h"
 #include "vchiq_debugfs.h"
-#include "vchiq_killable.h"
 
 #define DEVICE_NAME "vchiq"
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index 7ea29665bd0c..7d64e2ed7b42 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -33,7 +33,6 @@
 
 #include "vchiq_connected.h"
 #include "vchiq_core.h"
-#include "vchiq_killable.h"
 #include <linux/module.h>
 #include <linux/mutex.h>
 
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 afe2aa5ac3e9..31e69b445b4c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -32,7 +32,6 @@
  */
 
 #include "vchiq_core.h"
-#include "vchiq_killable.h"
 
 #define VCHIQ_SLOT_HANDLER_STACK 8192
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h
deleted file mode 100644
index 778063ba312a..000000000000
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h
+++ /dev/null
@@ -1,55 +0,0 @@
-/**
- * Copyright (c) 2010-2012 Broadcom. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions, and the following disclaimer,
- *    without modification.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- * 3. The names of the above-listed copyright holders may not be used
- *    to endorse or promote products derived from this software without
- *    specific prior written permission.
- *
- * ALTERNATIVELY, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2, as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
- * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#ifndef VCHIQ_KILLABLE_H
-#define VCHIQ_KILLABLE_H
-
-#include <linux/mutex.h>
-#include <linux/semaphore.h>
-
-#define SHUTDOWN_SIGS   (sigmask(SIGKILL) | sigmask(SIGINT) | sigmask(SIGQUIT) | sigmask(SIGTRAP) | sigmask(SIGSTOP) | sigmask(SIGCONT))
-
-static inline int __must_check down_interruptible_killable(struct semaphore *sem)
-{
-	/* Allow interception of killable signals only. We don't want to be interrupted by harmless signals like SIGALRM */
-	int ret;
-	sigset_t blocked, oldset;
-	siginitsetinv(&blocked, SHUTDOWN_SIGS);
-	sigprocmask(SIG_SETMASK, &blocked, &oldset);
-	ret = down_interruptible(sem);
-	sigprocmask(SIG_SETMASK, &oldset, NULL);
-	return ret;
-}
-#define down_interruptible down_interruptible_killable
-
-#endif
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
index 42e10d7d97a4..248a3170b42c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c
@@ -32,7 +32,6 @@
  */
 
 #include "vchiq_util.h"
-#include "vchiq_killable.h"
 
 static inline int is_pow2(int i)
 {
-- 
2.19.2

Re: [PATCH 0/3] staging: vchiq: fix completion routines & semaphores

[Arnd Bergmann]

On Wed, Dec 12, 2018 at 7:54 PM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> As pointed out by Arnd Bergman[1] there where some issues on how I
> converted semaphores to completion routines. I wasn't aware of a
> macro override happening in vchiq_killable.h, which was changing
> down_interruptible()'s meaning.
>
> This patch changes all completions so they use the proper APIs and gets
> rid of vchiq_killable.h. I took into account Arnd's commit to avoid
> merge conflicts.
>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Thanks for the follow-up!

      Arnd