[PATCH v4 0/7] staging: vchiq_core: Refactor vchiq_bulk_transfer() logic

[PATCH v4 0/7] staging: vchiq_core: Refactor vchiq_bulk_transfer() logic

[Umang Jain]

Series mostly refactors vchiq_bulk_transfer() code according
to Arnd's review comment from [1]:

> You can also wrap vchiq_bulk_transfer() in order to have
> four separate functions based on the different 'mode'
> values and have them only take the arguments they actually
> need.

I didn't wrap vchiq_bulk_transfer(), instead created four
differnet function, one for each mode.
This will pave the way to address his second comment:

> Ideally this should be cleaned up in a way that completely
> avoids passing both user and kernel data at the same time.

which is not part of this series and will be done on top as
arguments shuffling will have to fix the sparse warnings
that exists currently.

Patch 1/7 first moves the VCHIQ_BULK_MODE_WAITING logic out
of vchiq_bulk_transfer

Patch 2/7 then moves the core logic to vchiq_bulk_transfer()
which can be shared in subsequent patches

Patch 3/7 and 4/7 refactors remaining bulk transfer modes

Patch 5/7 finally drops the vchiq_bulk_transfer() as not needed

patch 6/7 and 7/7 are drive by patches, noticed during development.

[1]: https://lore.kernel.org/linux-staging/3d3b7368-93b2-4c0d-845e-4099c2de9dc1@app.fastmail.com/

Changes in v4:
- In 1/7 return 0 direcly instead of assigning status = 0
- In 3/7, fix null assignment of bulk waiter 

CHanges in v3:
- Directly return if service == NULL in 1/7 and 4/7
- Drop set-but-unused variable from 1/7

Changes in v2:
- Directly return if service == NULL in 3/7
- Prefix non-static functions with "vchiq_" (appropriate driver prefix)
- Use typed argument for 'userdata' in 1/7


Umang Jain (7):
  staging: vchiq: Factor out bulk transfer for VCHIQ_BULK_MODE_WAITING
  staging: vchiq_core: Simplify vchiq_bulk_transfer()
  staging: vchiq_core: Factor out bulk transfer for blocking mode
  staging: vchiq_core: Factor out bulk transfer for (no/)callback mode
  staging: vchiq_core: Drop vchiq_bulk_transfer()
  staging: vchiq_core: Remove unused function argument
  staging: vchiq_core: Pass enumerated flag instead of int

 .../interface/vchiq_arm/vchiq_arm.c           |  20 +-
 .../interface/vchiq_arm/vchiq_core.c          | 319 +++++++++++-------
 .../interface/vchiq_arm/vchiq_core.h          |  16 +-
 .../interface/vchiq_arm/vchiq_dev.c           |  15 +-
 4 files changed, 228 insertions(+), 142 deletions(-)

-- 
2.45.2

[PATCH v4 1/7] staging: vchiq: Factor out bulk transfer for VCHIQ_BULK_MODE_WAITING

[Umang Jain]

The bulk transfer is VCHIQ_BULK_MODE_WAITING is used by VCHIQ ioctl
interface. It is factored out to a separate function from
vchiq_bulk_transfer() to bulk_xfer_waiting_interruptible().

This is a part of vchiq_bulk_transfer refactoring. Each bulk mode
will have their dedicated functions to execute bulk transfers.
Each mode will be handled separately in subsequent patches.

bulk_xfer_waiting_interruptible() is suffixed with "_interruptible"
to denote that it can be interrupted when a signal is received.
-EAGAIN maybe returned in those cases, similar to what
vchiq_bulk_transfer() does.

Adjust the vchiq_irq_queue_bulk_tx_rx() in the vchiq-dev.c to call
bulk_xfer_waiting_interruptible() for waiting mode. A temporary
goto label has been introduced to jump the call execution over
vchiq_bulk_transfer() for waiting mode only. When all dedicated bulk
transfer calls are introduced, this label shall be dropped.

No function changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 53 ++++++++++++++++---
 .../interface/vchiq_arm/vchiq_core.h          |  4 ++
 .../interface/vchiq_arm/vchiq_dev.c           |  5 ++
 3 files changed, 54 insertions(+), 8 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 50af04b217f4..2239f59519be 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3023,10 +3023,6 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 		bulk_waiter->actual = 0;
 		bulk_waiter->bulk = NULL;
 		break;
-	case VCHIQ_BULK_MODE_WAITING:
-		bulk_waiter = userdata;
-		bulk = bulk_waiter->bulk;
-		goto waiting;
 	default:
 		goto error_exit;
 	}
@@ -3115,11 +3111,8 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 		state->id, service->localport, dir_char, queue->local_insert,
 		queue->remote_insert, queue->process);
 
-waiting:
 	vchiq_service_put(service);
 
-	status = 0;
-
 	if (bulk_waiter) {
 		bulk_waiter->bulk = bulk;
 		if (wait_for_completion_interruptible(&bulk_waiter->event))
@@ -3128,7 +3121,7 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 			status = -EINVAL;
 	}
 
-	return status;
+	return 0;
 
 unlock_both_error_exit:
 	mutex_unlock(&state->slot_mutex);
@@ -3143,6 +3136,50 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 	return status;
 }
 
+/*
+ * This function is called by VCHIQ ioctl interface and is interruptible.
+ * It may receive -EAGAIN to indicate that a signal has been received
+ * and the call should be retried after being returned to user context.
+ */
+int
+vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance *instance,
+				      unsigned int handle, struct bulk_waiter *userdata)
+{
+	struct vchiq_service *service = find_service_by_handle(instance, handle);
+	struct bulk_waiter *bulk_waiter;
+	int status = -EINVAL;
+
+	if (!service)
+		return -EINVAL;
+
+	if (!userdata)
+		goto error_exit;
+
+	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
+		goto error_exit;
+
+	if (vchiq_check_service(service))
+		goto error_exit;
+
+	bulk_waiter = userdata;
+
+	vchiq_service_put(service);
+
+	status = 0;
+
+	if (wait_for_completion_interruptible(&bulk_waiter->event))
+		return -EAGAIN;
+	else if (bulk_waiter->actual == VCHIQ_BULK_ACTUAL_ABORTED)
+		return -EINVAL;
+
+	return status;
+
+error_exit:
+	vchiq_service_put(service);
+
+	return status;
+}
+
 int
 vchiq_queue_message(struct vchiq_instance *instance, unsigned int handle,
 		    ssize_t (*copy_callback)(void *context, void *dest,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 77cc4d7ac077..985d9ea3a06a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -470,6 +470,10 @@ vchiq_shutdown_internal(struct vchiq_state *state, struct vchiq_instance *instan
 extern void
 remote_event_pollall(struct vchiq_state *state);
 
+extern int
+vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance *instance,
+				      unsigned int handle, struct bulk_waiter *userdata);
+
 extern int
 vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
 		    void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 9cd2a64dce5e..550838d2863b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -324,6 +324,10 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		dev_dbg(service->state->dev, "arm: found bulk_waiter %pK for pid %d\n",
 			waiter, current->pid);
 		userdata = &waiter->bulk_waiter;
+
+		status = vchiq_bulk_xfer_waiting_interruptible(instance, args->handle, userdata);
+
+		goto bulk_transfer_handled;
 	} else {
 		userdata = args->userdata;
 	}
@@ -331,6 +335,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 	status = vchiq_bulk_transfer(instance, args->handle, NULL, args->data, args->size,
 				     userdata, args->mode, dir);
 
+bulk_transfer_handled:
 	if (!waiter) {
 		ret = 0;
 		goto out;
-- 
2.45.2

[PATCH v4 2/7] staging: vchiq_core: Simplify vchiq_bulk_transfer()

[Umang Jain]

Factor out core logic for preparing bulk data transfer(mutex locking,
waits on vchiq_bulk_queue wait-queue, initialising the bulk transfer)
out of the vchiq_bulk_transfer(). This simplifies the existing
vchiq_bulk_transfer() and makes it more readable since all the core
logic is handled in vchiq_bulk_xfer_queue_msg_interruptible(). It
will also help us to refactor vchiq_bulk_transfer() easily for different
vchiq bulk transfer modes.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 217 ++++++++++--------
 1 file changed, 121 insertions(+), 96 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 2239f59519be..f36044bab194 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -189,6 +189,13 @@ static const char *const conn_state_names[] = {
 static void
 release_message_sync(struct vchiq_state *state, struct vchiq_header *header);
 
+static int
+vchiq_bulk_xfer_queue_msg_interruptible(struct vchiq_service *service,
+					void *offset, void __user *uoffset,
+					int size, void *userdata,
+					enum vchiq_bulk_mode mode,
+					enum vchiq_bulk_dir dir);
+
 static const char *msg_type_str(unsigned int msg_type)
 {
 	switch (msg_type) {
@@ -2991,15 +2998,9 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 			enum vchiq_bulk_mode mode, enum vchiq_bulk_dir dir)
 {
 	struct vchiq_service *service = find_service_by_handle(instance, handle);
-	struct vchiq_bulk_queue *queue;
-	struct vchiq_bulk *bulk;
-	struct vchiq_state *state;
 	struct bulk_waiter *bulk_waiter = NULL;
-	const char dir_char = (dir == VCHIQ_BULK_TRANSMIT) ? 't' : 'r';
-	const int dir_msgtype = (dir == VCHIQ_BULK_TRANSMIT) ?
-		VCHIQ_MSG_BULK_TX : VCHIQ_MSG_BULK_RX;
+	struct vchiq_bulk *bulk;
 	int status = -EINVAL;
-	int payload[2];
 
 	if (!service)
 		goto error_exit;
@@ -3027,89 +3028,10 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 		goto error_exit;
 	}
 
-	state = service->state;
-
-	queue = (dir == VCHIQ_BULK_TRANSMIT) ?
-		&service->bulk_tx : &service->bulk_rx;
-
-	if (mutex_lock_killable(&service->bulk_mutex)) {
-		status = -EAGAIN;
-		goto error_exit;
-	}
-
-	if (queue->local_insert == queue->remove + VCHIQ_NUM_SERVICE_BULKS) {
-		VCHIQ_SERVICE_STATS_INC(service, bulk_stalls);
-		do {
-			mutex_unlock(&service->bulk_mutex);
-			if (wait_for_completion_interruptible(&service->bulk_remove_event)) {
-				status = -EAGAIN;
-				goto error_exit;
-			}
-			if (mutex_lock_killable(&service->bulk_mutex)) {
-				status = -EAGAIN;
-				goto error_exit;
-			}
-		} while (queue->local_insert == queue->remove +
-				VCHIQ_NUM_SERVICE_BULKS);
-	}
-
-	bulk = &queue->bulks[BULK_INDEX(queue->local_insert)];
-
-	bulk->mode = mode;
-	bulk->dir = dir;
-	bulk->userdata = userdata;
-	bulk->size = size;
-	bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
-
-	if (vchiq_prepare_bulk_data(instance, bulk, offset, uoffset, size, dir))
-		goto unlock_error_exit;
-
-	/*
-	 * Ensure that the bulk data record is visible to the peer
-	 * before proceeding.
-	 */
-	wmb();
-
-	dev_dbg(state->dev, "core: %d: bt (%d->%d) %cx %x@%pad %pK\n",
-		state->id, service->localport, service->remoteport,
-		dir_char, size, &bulk->data, userdata);
-
-	/*
-	 * The slot mutex must be held when the service is being closed, so
-	 * claim it here to ensure that isn't happening
-	 */
-	if (mutex_lock_killable(&state->slot_mutex)) {
-		status = -EAGAIN;
-		goto cancel_bulk_error_exit;
-	}
-
-	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
-		goto unlock_both_error_exit;
-
-	payload[0] = lower_32_bits(bulk->data);
-	payload[1] = bulk->size;
-	status = queue_message(state,
-			       NULL,
-			       VCHIQ_MAKE_MSG(dir_msgtype,
-					      service->localport,
-					      service->remoteport),
-			       memcpy_copy_callback,
-			       &payload,
-			       sizeof(payload),
-			       QMFLAGS_IS_BLOCKING |
-			       QMFLAGS_NO_MUTEX_LOCK |
-			       QMFLAGS_NO_MUTEX_UNLOCK);
+	status = vchiq_bulk_xfer_queue_msg_interruptible(service, offset, uoffset,
+							 size, userdata, mode, dir);
 	if (status)
-		goto unlock_both_error_exit;
-
-	queue->local_insert++;
-
-	mutex_unlock(&state->slot_mutex);
-	mutex_unlock(&service->bulk_mutex);
-
-	dev_dbg(state->dev, "core: %d: bt:%d %cx li=%x ri=%x p=%x\n",
-		state->id, service->localport, dir_char, queue->local_insert,
-		queue->remote_insert, queue->process);
+		goto error_exit;
 
 	vchiq_service_put(service);
 
@@ -3123,13 +3045,6 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 
 	return 0;
 
-unlock_both_error_exit:
-	mutex_unlock(&state->slot_mutex);
-cancel_bulk_error_exit:
-	vchiq_complete_bulk(service->instance, bulk);
-unlock_error_exit:
-	mutex_unlock(&service->bulk_mutex);
-
 error_exit:
 	if (service)
 		vchiq_service_put(service);
@@ -3289,6 +3204,116 @@ vchiq_release_message(struct vchiq_instance *instance, unsigned int handle,
 }
 EXPORT_SYMBOL(vchiq_release_message);
 
+/*
+ * Prepares a bulk transfer to be queued. The function is interruptible and is
+ * intended to be called from user threads. It may return -EAGAIN to indicate
+ * that a signal has been received and the call should be retried after being
+ * returned to user context.
+ */
+static int
+vchiq_bulk_xfer_queue_msg_interruptible(struct vchiq_service *service,
+					void *offset, void __user *uoffset,
+					int size, void *userdata,
+					enum vchiq_bulk_mode mode,
+					enum vchiq_bulk_dir dir)
+{
+	struct vchiq_bulk_queue *queue;
+	struct vchiq_bulk *bulk;
+	struct vchiq_state *state = service->state;
+	const char dir_char = (dir == VCHIQ_BULK_TRANSMIT) ? 't' : 'r';
+	const int dir_msgtype = (dir == VCHIQ_BULK_TRANSMIT) ?
+		VCHIQ_MSG_BULK_TX : VCHIQ_MSG_BULK_RX;
+	int status = -EINVAL;
+	int payload[2];
+
+	queue = (dir == VCHIQ_BULK_TRANSMIT) ?
+		&service->bulk_tx : &service->bulk_rx;
+
+	if (mutex_lock_killable(&service->bulk_mutex))
+		return -EAGAIN;
+
+	if (queue->local_insert == queue->remove + VCHIQ_NUM_SERVICE_BULKS) {
+		VCHIQ_SERVICE_STATS_INC(service, bulk_stalls);
+		do {
+			mutex_unlock(&service->bulk_mutex);
+			if (wait_for_completion_interruptible(&service->bulk_remove_event))
+				return -EAGAIN;
+			if (mutex_lock_killable(&service->bulk_mutex))
+				return -EAGAIN;
+		} while (queue->local_insert == queue->remove +
+				VCHIQ_NUM_SERVICE_BULKS);
+	}
+
+	bulk = &queue->bulks[BULK_INDEX(queue->local_insert)];
+
+	bulk->mode = mode;
+	bulk->dir = dir;
+	bulk->userdata = userdata;
+	bulk->size = size;
+	bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
+
+	if (vchiq_prepare_bulk_data(service->instance, bulk, offset, uoffset, size, dir))
+		goto unlock_error_exit;
+
+	/*
+	 * Ensure that the bulk data record is visible to the peer
+	 * before proceeding.
+	 */
+	wmb();
+
+	dev_dbg(state->dev, "core: %d: bt (%d->%d) %cx %x@%pad %pK\n",
+		state->id, service->localport, service->remoteport,
+		dir_char, size, &bulk->data, userdata);
+
+	/*
+	 * The slot mutex must be held when the service is being closed, so
+	 * claim it here to ensure that isn't happening
+	 */
+	if (mutex_lock_killable(&state->slot_mutex)) {
+		status = -EAGAIN;
+		goto cancel_bulk_error_exit;
+	}
+
+	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
+		goto unlock_both_error_exit;
+
+	payload[0] = lower_32_bits(bulk->data);
+	payload[1] = bulk->size;
+	status = queue_message(state,
+			       NULL,
+			       VCHIQ_MAKE_MSG(dir_msgtype,
+					      service->localport,
+					      service->remoteport),
+			       memcpy_copy_callback,
+			       &payload,
+			       sizeof(payload),
+			       QMFLAGS_IS_BLOCKING |
+			       QMFLAGS_NO_MUTEX_LOCK |
+			       QMFLAGS_NO_MUTEX_UNLOCK);
+	if (status)
+		goto unlock_both_error_exit;
+
+	queue->local_insert++;
+
+	mutex_unlock(&state->slot_mutex);
+	mutex_unlock(&service->bulk_mutex);
+
+	dev_dbg(state->dev, "core: %d: bt:%d %cx li=%x ri=%x p=%x\n",
+		state->id, service->localport, dir_char, queue->local_insert,
+		queue->remote_insert, queue->process);
+
+	return status;
+
+unlock_both_error_exit:
+	mutex_unlock(&state->slot_mutex);
+cancel_bulk_error_exit:
+	vchiq_complete_bulk(service->instance, bulk);
+unlock_error_exit:
+	mutex_unlock(&service->bulk_mutex);
+
+	return status;
+}
+
 static void
 release_message_sync(struct vchiq_state *state, struct vchiq_header *header)
 {
-- 
2.45.2

[PATCH v4 3/7] staging: vchiq_core: Factor out bulk transfer for blocking mode

[Umang Jain]

Factor out bulk transfer for blocking mode into a separate dedicated
function bulk_xfer_blocking_interruptible(). It is suffixed by
"_interruptible" to denote that it can be interrupted and -EAGAIN
can be returned. It would be up to the users of the function to retry
the call in those cases.

Adjust the calls to vchiq-dev.c ioctl interface and vchiq_arm.c
for blocking bulk transfers.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  5 +--
 .../interface/vchiq_arm/vchiq_core.c          | 42 ++++++++++++++++---
 .../interface/vchiq_arm/vchiq_core.h          |  5 +++
 .../interface/vchiq_arm/vchiq_dev.c           |  6 +++
 4 files changed, 49 insertions(+), 9 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 c4d97dbf6ba8..688c9b1be868 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -968,9 +968,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 			return -ENOMEM;
 	}
 
-	ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
-				  &waiter->bulk_waiter,
-				  VCHIQ_BULK_MODE_BLOCKING, dir);
+	ret = vchiq_bulk_xfer_blocking_interruptible(instance, handle, data, NULL, size,
+						     &waiter->bulk_waiter, dir);
 	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
 		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
 
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 f36044bab194..43f951fa4b89 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2985,6 +2985,42 @@ vchiq_remove_service(struct vchiq_instance *instance, unsigned int handle)
 	return status;
 }
 
+int
+vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int handle,
+				       void *offset, void __user *uoffset, int size,
+				       void __user *userdata, enum vchiq_bulk_dir dir)
+{
+	struct vchiq_service *service = find_service_by_handle(instance, handle);
+	struct bulk_waiter *bulk_waiter;
+	enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
+	int status = -EINVAL;
+
+	if (!service)
+		return -EINVAL;
+
+	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
+		goto error_exit;
+
+	if (!offset && !uoffset)
+		goto error_exit;
+
+	if (vchiq_check_service(service))
+		goto error_exit;
+
+	bulk_waiter = userdata;
+	init_completion(&bulk_waiter->event);
+	bulk_waiter->actual = 0;
+	bulk_waiter->bulk = NULL;
+
+	status = vchiq_bulk_xfer_queue_msg_interruptible(service, offset, uoffset, size,
+							 userdata, mode, dir);
+
+error_exit:
+	vchiq_service_put(service);
+
+	return status;
+}
+
 /*
  * This function may be called by kernel threads or user threads.
  * User threads may receive -EAGAIN to indicate that a signal has been
@@ -3018,12 +3054,6 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 	case VCHIQ_BULK_MODE_NOCALLBACK:
 	case VCHIQ_BULK_MODE_CALLBACK:
 		break;
-	case VCHIQ_BULK_MODE_BLOCKING:
-		bulk_waiter = userdata;
-		init_completion(&bulk_waiter->event);
-		bulk_waiter->actual = 0;
-		bulk_waiter->bulk = NULL;
-		break;
 	default:
 		goto error_exit;
 	}
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 985d9ea3a06a..2dd89101c1c6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -474,6 +474,11 @@ extern int
 vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance *instance,
 				      unsigned int handle, struct bulk_waiter *userdata);
 
+extern int
+vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int handle,
+				       void *offset, void __user *uoffset, int size,
+				       void __user *userdata, enum vchiq_bulk_dir dir);
+
 extern int
 vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
 		    void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 550838d2863b..830633f2326b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -304,6 +304,12 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		}
 
 		userdata = &waiter->bulk_waiter;
+
+		status = vchiq_bulk_xfer_blocking_interruptible(instance, args->handle,
+								NULL, args->data, args->size,
+								userdata, dir);
+
+		goto bulk_transfer_handled;
 	} else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
 		mutex_lock(&instance->bulk_waiter_list_mutex);
 		list_for_each_entry(iter, &instance->bulk_waiter_list,
-- 
2.45.2

Re: [PATCH v4 3/7] staging: vchiq_core: Factor out bulk transfer for blocking mode

[Stefan Wahren]

Hi Umang,

Am 06.09.24 um 09:25 schrieb Umang Jain:
> Factor out bulk transfer for blocking mode into a separate dedicated
> function bulk_xfer_blocking_interruptible(). It is suffixed by
> "_interruptible" to denote that it can be interrupted and -EAGAIN
> can be returned. It would be up to the users of the function to retry
> the call in those cases.
>
> Adjust the calls to vchiq-dev.c ioctl interface and vchiq_arm.c
> for blocking bulk transfers.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
i applied this series on top of recent linux-next and the vchiq ping
test on Raspberry Pi 3 B Plus (multi_v7_defconfig) crashes or hanges
strangly.

I bisected the issue to this patch, but it's possible the root cause
might be introduced before.

But i'm pretty sure that the series introduced the regression.

Sorry, i don't have the time analyse this issue deeper.

Best regards
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           |  5 +--
>   .../interface/vchiq_arm/vchiq_core.c          | 42 ++++++++++++++++---
>   .../interface/vchiq_arm/vchiq_core.h          |  5 +++
>   .../interface/vchiq_arm/vchiq_dev.c           |  6 +++
>   4 files changed, 49 insertions(+), 9 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 c4d97dbf6ba8..688c9b1be868 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -968,9 +968,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>   			return -ENOMEM;
>   	}
>
> -	ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
> -				  &waiter->bulk_waiter,
> -				  VCHIQ_BULK_MODE_BLOCKING, dir);
> +	ret = vchiq_bulk_xfer_blocking_interruptible(instance, handle, data, NULL, size,
> +						     &waiter->bulk_waiter, dir);
>   	if ((ret != -EAGAIN) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
>   		struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
>
> 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 f36044bab194..43f951fa4b89 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2985,6 +2985,42 @@ vchiq_remove_service(struct vchiq_instance *instance, unsigned int handle)
>   	return status;
>   }
>
> +int
> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int handle,
> +				       void *offset, void __user *uoffset, int size,
> +				       void __user *userdata, enum vchiq_bulk_dir dir)
> +{
> +	struct vchiq_service *service = find_service_by_handle(instance, handle);
> +	struct bulk_waiter *bulk_waiter;
> +	enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
> +	int status = -EINVAL;
> +
> +	if (!service)
> +		return -EINVAL;
> +
> +	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
> +		goto error_exit;
> +
> +	if (!offset && !uoffset)
> +		goto error_exit;
> +
> +	if (vchiq_check_service(service))
> +		goto error_exit;
> +
> +	bulk_waiter = userdata;
> +	init_completion(&bulk_waiter->event);
> +	bulk_waiter->actual = 0;
> +	bulk_waiter->bulk = NULL;
> +
> +	status = vchiq_bulk_xfer_queue_msg_interruptible(service, offset, uoffset, size,
> +							 userdata, mode, dir);
> +
> +error_exit:
> +	vchiq_service_put(service);
> +
> +	return status;
> +}
> +
>   /*
>    * This function may be called by kernel threads or user threads.
>    * User threads may receive -EAGAIN to indicate that a signal has been
> @@ -3018,12 +3054,6 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
>   	case VCHIQ_BULK_MODE_NOCALLBACK:
>   	case VCHIQ_BULK_MODE_CALLBACK:
>   		break;
> -	case VCHIQ_BULK_MODE_BLOCKING:
> -		bulk_waiter = userdata;
> -		init_completion(&bulk_waiter->event);
> -		bulk_waiter->actual = 0;
> -		bulk_waiter->bulk = NULL;
> -		break;
>   	default:
>   		goto error_exit;
>   	}
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 985d9ea3a06a..2dd89101c1c6 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -474,6 +474,11 @@ extern int
>   vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance *instance,
>   				      unsigned int handle, struct bulk_waiter *userdata);
>
> +extern int
> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int handle,
> +				       void *offset, void __user *uoffset, int size,
> +				       void __user *userdata, enum vchiq_bulk_dir dir);
> +
>   extern int
>   vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
>   		    void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> index 550838d2863b..830633f2326b 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -304,6 +304,12 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
>   		}
>
>   		userdata = &waiter->bulk_waiter;
> +
> +		status = vchiq_bulk_xfer_blocking_interruptible(instance, args->handle,
> +								NULL, args->data, args->size,
> +								userdata, dir);
> +
> +		goto bulk_transfer_handled;
>   	} else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
>   		mutex_lock(&instance->bulk_waiter_list_mutex);
>   		list_for_each_entry(iter, &instance->bulk_waiter_list,

Re: [PATCH v4 3/7] staging: vchiq_core: Factor out bulk transfer for blocking mode

[Umang Jain]

Hi Stefan

On 07/09/24 5:19 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 06.09.24 um 09:25 schrieb Umang Jain:
>> Factor out bulk transfer for blocking mode into a separate dedicated
>> function bulk_xfer_blocking_interruptible(). It is suffixed by
>> "_interruptible" to denote that it can be interrupted and -EAGAIN
>> can be returned. It would be up to the users of the function to retry
>> the call in those cases.
>>
>> Adjust the calls to vchiq-dev.c ioctl interface and vchiq_arm.c
>> for blocking bulk transfers.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> i applied this series on top of recent linux-next and the vchiq ping
> test on Raspberry Pi 3 B Plus (multi_v7_defconfig) crashes or hanges
> strangly.

This is really annoying. The behavior is non-deterministic

I tested this repeatedly and I see the hang on my 3rd ping test.
The first 2 ping tests completed successfully.

Here is the output for all 3 tries:

https://paste.debian.net/plain/1328739

I'll take a look and fix it in next version.

>
> I bisected the issue to this patch, but it's possible the root cause
> might be introduced before.
>
> But i'm pretty sure that the series introduced the regression.
>
> Sorry, i don't have the time analyse this issue deeper.
>
> Best regards
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           |  5 +--
>>   .../interface/vchiq_arm/vchiq_core.c          | 42 ++++++++++++++++---
>>   .../interface/vchiq_arm/vchiq_core.h          |  5 +++
>>   .../interface/vchiq_arm/vchiq_dev.c           |  6 +++
>>   4 files changed, 49 insertions(+), 9 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 c4d97dbf6ba8..688c9b1be868 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -968,9 +968,8 @@ vchiq_blocking_bulk_transfer(struct 
>> vchiq_instance *instance, unsigned int handl
>>               return -ENOMEM;
>>       }
>>
>> -    ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
>> -                  &waiter->bulk_waiter,
>> -                  VCHIQ_BULK_MODE_BLOCKING, dir);
>> +    ret = vchiq_bulk_xfer_blocking_interruptible(instance, handle, 
>> data, NULL, size,
>> +                             &waiter->bulk_waiter, dir);
>>       if ((ret != -EAGAIN) || fatal_signal_pending(current) || 
>> !waiter->bulk_waiter.bulk) {
>>           struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
>>
>> 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 f36044bab194..43f951fa4b89 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -2985,6 +2985,42 @@ vchiq_remove_service(struct vchiq_instance 
>> *instance, unsigned int handle)
>>       return status;
>>   }
>>
>> +int
>> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance 
>> *instance, unsigned int handle,
>> +                       void *offset, void __user *uoffset, int size,
>> +                       void __user *userdata, enum vchiq_bulk_dir dir)
>> +{
>> +    struct vchiq_service *service = find_service_by_handle(instance, 
>> handle);
>> +    struct bulk_waiter *bulk_waiter;
>> +    enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
>> +    int status = -EINVAL;
>> +
>> +    if (!service)
>> +        return -EINVAL;
>> +
>> +    if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
>> +        goto error_exit;
>> +
>> +    if (!offset && !uoffset)
>> +        goto error_exit;
>> +
>> +    if (vchiq_check_service(service))
>> +        goto error_exit;
>> +
>> +    bulk_waiter = userdata;
>> +    init_completion(&bulk_waiter->event);
>> +    bulk_waiter->actual = 0;
>> +    bulk_waiter->bulk = NULL;
>> +
>> +    status = vchiq_bulk_xfer_queue_msg_interruptible(service, 
>> offset, uoffset, size,
>> +                             userdata, mode, dir);
>> +
>> +error_exit:
>> +    vchiq_service_put(service);
>> +
>> +    return status;
>> +}
>> +
>>   /*
>>    * This function may be called by kernel threads or user threads.
>>    * User threads may receive -EAGAIN to indicate that a signal has been
>> @@ -3018,12 +3054,6 @@ int vchiq_bulk_transfer(struct vchiq_instance 
>> *instance, unsigned int handle,
>>       case VCHIQ_BULK_MODE_NOCALLBACK:
>>       case VCHIQ_BULK_MODE_CALLBACK:
>>           break;
>> -    case VCHIQ_BULK_MODE_BLOCKING:
>> -        bulk_waiter = userdata;
>> -        init_completion(&bulk_waiter->event);
>> -        bulk_waiter->actual = 0;
>> -        bulk_waiter->bulk = NULL;
>> -        break;
>>       default:
>>           goto error_exit;
>>       }
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> index 985d9ea3a06a..2dd89101c1c6 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> @@ -474,6 +474,11 @@ extern int
>>   vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance *instance,
>>                         unsigned int handle, struct bulk_waiter 
>> *userdata);
>>
>> +extern int
>> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance 
>> *instance, unsigned int handle,
>> +                       void *offset, void __user *uoffset, int size,
>> +                       void __user *userdata, enum vchiq_bulk_dir dir);
>> +
>>   extern int
>>   vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int 
>> handle, void *offset,
>>               void __user *uoffset, int size, void *userdata, enum 
>> vchiq_bulk_mode mode,
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> index 550838d2863b..830633f2326b 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> @@ -304,6 +304,12 @@ static int vchiq_irq_queue_bulk_tx_rx(struct 
>> vchiq_instance *instance,
>>           }
>>
>>           userdata = &waiter->bulk_waiter;
>> +
>> +        status = vchiq_bulk_xfer_blocking_interruptible(instance, 
>> args->handle,
>> +                                NULL, args->data, args->size,
>> +                                userdata, dir);
>> +
>> +        goto bulk_transfer_handled;
>>       } else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
>>           mutex_lock(&instance->bulk_waiter_list_mutex);
>>           list_for_each_entry(iter, &instance->bulk_waiter_list,
>

Re: [PATCH v4 3/7] staging: vchiq_core: Factor out bulk transfer for blocking mode

[Stefan Wahren]

Hi Umang,

Am 07.09.24 um 20:26 schrieb Umang Jain:
> Hi Stefan
>
> On 07/09/24 5:19 pm, Stefan Wahren wrote:
>> Hi Umang,
>>
>> Am 06.09.24 um 09:25 schrieb Umang Jain:
>>> Factor out bulk transfer for blocking mode into a separate dedicated
>>> function bulk_xfer_blocking_interruptible(). It is suffixed by
>>> "_interruptible" to denote that it can be interrupted and -EAGAIN
>>> can be returned. It would be up to the users of the function to retry
>>> the call in those cases.
>>>
>>> Adjust the calls to vchiq-dev.c ioctl interface and vchiq_arm.c
>>> for blocking bulk transfers.
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> i applied this series on top of recent linux-next and the vchiq ping
>> test on Raspberry Pi 3 B Plus (multi_v7_defconfig) crashes or hanges
>> strangly.
>
> This is really annoying. The behavior is non-deterministic
>
> I tested this repeatedly and I see the hang on my 3rd ping test.
> The first 2 ping tests completed successfully.
>
> Here is the output for all 3 tries:
>
> https://paste.debian.net/plain/1328739
>
> I'll take a look and fix it in next version.
I was too imprecise with my information.

Just try "vchiq_test -p" which makes a big difference in reproducibility.

Best regards
>
>>
>> I bisected the issue to this patch, but it's possible the root cause
>> might be introduced before.
>>
>> But i'm pretty sure that the series introduced the regression.
>>
>> Sorry, i don't have the time analyse this issue deeper.
>>
>> Best regards
>>> ---
>>>   .../interface/vchiq_arm/vchiq_arm.c           |  5 +--
>>>   .../interface/vchiq_arm/vchiq_core.c          | 42
>>> ++++++++++++++++---
>>>   .../interface/vchiq_arm/vchiq_core.h          |  5 +++
>>>   .../interface/vchiq_arm/vchiq_dev.c           |  6 +++
>>>   4 files changed, 49 insertions(+), 9 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 c4d97dbf6ba8..688c9b1be868 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> @@ -968,9 +968,8 @@ vchiq_blocking_bulk_transfer(struct
>>> vchiq_instance *instance, unsigned int handl
>>>               return -ENOMEM;
>>>       }
>>>
>>> -    ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
>>> -                  &waiter->bulk_waiter,
>>> -                  VCHIQ_BULK_MODE_BLOCKING, dir);
>>> +    ret = vchiq_bulk_xfer_blocking_interruptible(instance, handle,
>>> data, NULL, size,
>>> +                             &waiter->bulk_waiter, dir);
>>>       if ((ret != -EAGAIN) || fatal_signal_pending(current) ||
>>> !waiter->bulk_waiter.bulk) {
>>>           struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
>>>
>>> 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 f36044bab194..43f951fa4b89 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> @@ -2985,6 +2985,42 @@ vchiq_remove_service(struct vchiq_instance
>>> *instance, unsigned int handle)
>>>       return status;
>>>   }
>>>
>>> +int
>>> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance
>>> *instance, unsigned int handle,
>>> +                       void *offset, void __user *uoffset, int size,
>>> +                       void __user *userdata, enum vchiq_bulk_dir dir)
>>> +{
>>> +    struct vchiq_service *service =
>>> find_service_by_handle(instance, handle);
>>> +    struct bulk_waiter *bulk_waiter;
>>> +    enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
>>> +    int status = -EINVAL;
>>> +
>>> +    if (!service)
>>> +        return -EINVAL;
>>> +
>>> +    if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
>>> +        goto error_exit;
>>> +
>>> +    if (!offset && !uoffset)
>>> +        goto error_exit;
>>> +
>>> +    if (vchiq_check_service(service))
>>> +        goto error_exit;
>>> +
>>> +    bulk_waiter = userdata;
>>> +    init_completion(&bulk_waiter->event);
>>> +    bulk_waiter->actual = 0;
>>> +    bulk_waiter->bulk = NULL;
>>> +
>>> +    status = vchiq_bulk_xfer_queue_msg_interruptible(service,
>>> offset, uoffset, size,
>>> +                             userdata, mode, dir);
>>> +
>>> +error_exit:
>>> +    vchiq_service_put(service);
>>> +
>>> +    return status;
>>> +}
>>> +
>>>   /*
>>>    * This function may be called by kernel threads or user threads.
>>>    * User threads may receive -EAGAIN to indicate that a signal has
>>> been
>>> @@ -3018,12 +3054,6 @@ int vchiq_bulk_transfer(struct vchiq_instance
>>> *instance, unsigned int handle,
>>>       case VCHIQ_BULK_MODE_NOCALLBACK:
>>>       case VCHIQ_BULK_MODE_CALLBACK:
>>>           break;
>>> -    case VCHIQ_BULK_MODE_BLOCKING:
>>> -        bulk_waiter = userdata;
>>> -        init_completion(&bulk_waiter->event);
>>> -        bulk_waiter->actual = 0;
>>> -        bulk_waiter->bulk = NULL;
>>> -        break;
>>>       default:
>>>           goto error_exit;
>>>       }
>>> diff --git
>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>>> index 985d9ea3a06a..2dd89101c1c6 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>>> @@ -474,6 +474,11 @@ extern int
>>>   vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance
>>> *instance,
>>>                         unsigned int handle, struct bulk_waiter
>>> *userdata);
>>>
>>> +extern int
>>> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance
>>> *instance, unsigned int handle,
>>> +                       void *offset, void __user *uoffset, int size,
>>> +                       void __user *userdata, enum vchiq_bulk_dir
>>> dir);
>>> +
>>>   extern int
>>>   vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int
>>> handle, void *offset,
>>>               void __user *uoffset, int size, void *userdata, enum
>>> vchiq_bulk_mode mode,
>>> diff --git
>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>>> index 550838d2863b..830633f2326b 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>>> @@ -304,6 +304,12 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
>>> vchiq_instance *instance,
>>>           }
>>>
>>>           userdata = &waiter->bulk_waiter;
>>> +
>>> +        status = vchiq_bulk_xfer_blocking_interruptible(instance,
>>> args->handle,
>>> +                                NULL, args->data, args->size,
>>> +                                userdata, dir);
>>> +
>>> +        goto bulk_transfer_handled;
>>>       } else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
>>> mutex_lock(&instance->bulk_waiter_list_mutex);
>>>           list_for_each_entry(iter, &instance->bulk_waiter_list,
>>
>

[PATCH v4 4/7] staging: vchiq_core: Factor out bulk transfer for (no/)callback mode

[Umang Jain]

Factor out bulk transfer for VCHIQ_BULK_MODE_NOCALLBACK and
VCHIQ_BULK_MODE_CALLBACK mode into a separate dedicated function
bulk_xfer_callback_interruptible(). It is suffixed by "_interruptible"
to denote that it can be interrupted and -EAGAIN can be returned. It
would be up to the users of the function to retry the call in those cases.

bulk_xfer_callback_interruptible() also takes in 'mode' parameter to
differentiate between VCHIQ_BULK_MODE_NOCALLBACK and
VCHIQ_BULK_MODE_CALLBACK, which then is directly passed to
vchiq_bulk_xfer_queue_msg_interruptible() inside the function.

Adjust the calls to vchiq-dev.c ioctl interface and vchiq_arm.c
for the respective bulk transfers.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 15 ++++----
 .../interface/vchiq_arm/vchiq_core.c          | 34 +++++++++++++++++++
 .../interface/vchiq_arm/vchiq_core.h          |  6 ++++
 .../interface/vchiq_arm/vchiq_dev.c           |  6 ++++
 4 files changed, 54 insertions(+), 7 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 688c9b1be868..3dbeffc650d3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -857,10 +857,10 @@ vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const
 		switch (mode) {
 		case VCHIQ_BULK_MODE_NOCALLBACK:
 		case VCHIQ_BULK_MODE_CALLBACK:
-			ret = vchiq_bulk_transfer(instance, handle,
-						  (void *)data, NULL,
-						  size, userdata, mode,
-						  VCHIQ_BULK_TRANSMIT);
+			ret = vchiq_bulk_xfer_callback_interruptible(instance, handle,
+								     (void *)data, NULL,
+								     size, mode, userdata,
+								     VCHIQ_BULK_TRANSMIT);
 			break;
 		case VCHIQ_BULK_MODE_BLOCKING:
 			ret = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
@@ -895,9 +895,10 @@ int vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
 		switch (mode) {
 		case VCHIQ_BULK_MODE_NOCALLBACK:
 		case VCHIQ_BULK_MODE_CALLBACK:
-			ret = vchiq_bulk_transfer(instance, handle, data, NULL,
-						  size, userdata,
-						  mode, VCHIQ_BULK_RECEIVE);
+			ret = vchiq_bulk_xfer_callback_interruptible(instance, handle,
+								     (void *)data, NULL,
+								     size, mode, userdata,
+								     VCHIQ_BULK_RECEIVE);
 			break;
 		case VCHIQ_BULK_MODE_BLOCKING:
 			ret = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
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 43f951fa4b89..573dad5c7893 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3021,6 +3021,40 @@ vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned
 	return status;
 }
 
+int
+vchiq_bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned int handle,
+				       void *offset, void __user *uoffset, int size,
+				       enum vchiq_bulk_mode mode, void *userdata,
+				       enum vchiq_bulk_dir dir)
+{
+	struct vchiq_service *service = find_service_by_handle(instance, handle);
+	int status = -EINVAL;
+
+	if (!service)
+		return -EINVAL;
+
+	if (mode != VCHIQ_BULK_MODE_CALLBACK &&
+	    mode != VCHIQ_BULK_MODE_NOCALLBACK)
+		goto error_exit;
+
+	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
+		goto error_exit;
+
+	if (!offset && !uoffset)
+		goto error_exit;
+
+	if (vchiq_check_service(service))
+		goto error_exit;
+
+	status = vchiq_bulk_xfer_queue_msg_interruptible(service, offset, uoffset,
+							 size, userdata, mode, dir);
+
+error_exit:
+	vchiq_service_put(service);
+
+	return status;
+}
+
 /*
  * This function may be called by kernel threads or user threads.
  * User threads may receive -EAGAIN to indicate that a signal has been
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 2dd89101c1c6..9c8c076eaaeb 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -479,6 +479,12 @@ vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned
 				       void *offset, void __user *uoffset, int size,
 				       void __user *userdata, enum vchiq_bulk_dir dir);
 
+extern int
+vchiq_bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned int handle,
+				       void *offset, void __user *uoffset, int size,
+				       enum vchiq_bulk_mode mode, void *userdata,
+				       enum vchiq_bulk_dir dir);
+
 extern int
 vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
 		    void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 830633f2326b..169a2ffda996 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -336,6 +336,12 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		goto bulk_transfer_handled;
 	} else {
 		userdata = args->userdata;
+
+		status = vchiq_bulk_xfer_callback_interruptible(instance, args->handle, NULL,
+								args->data, args->size,
+								args->mode, userdata, dir);
+
+		goto bulk_transfer_handled;
 	}
 
 	status = vchiq_bulk_transfer(instance, args->handle, NULL, args->data, args->size,
-- 
2.45.2

[PATCH v4 5/7] staging: vchiq_core: Drop vchiq_bulk_transfer()

[Umang Jain]

Drop vchiq_bulk_transfer() as every VCHIQ_BULK_MODE_* mode
now have their own dedicated functions to execute bulk transfers.

Also, drop the temporary label we introduced earlier in vchiq-dev.c
to jump over the vchiq_bulk_transfer() call when each separate mode
helper was being developed.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 60 -------------------
 .../interface/vchiq_arm/vchiq_core.h          |  5 --
 .../interface/vchiq_arm/vchiq_dev.c           |  8 ---
 3 files changed, 73 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 573dad5c7893..9b0009d1906e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3055,66 +3055,6 @@ vchiq_bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned
 	return status;
 }
 
-/*
- * This function may be called by kernel threads or user threads.
- * User threads may receive -EAGAIN to indicate that a signal has been
- * received and the call should be retried after being returned to user
- * context.
- * When called in blocking mode, the userdata field points to a bulk_waiter
- * structure.
- */
-int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
-			void *offset, void __user *uoffset, int size, void *userdata,
-			enum vchiq_bulk_mode mode, enum vchiq_bulk_dir dir)
-{
-	struct vchiq_service *service = find_service_by_handle(instance, handle);
-	struct bulk_waiter *bulk_waiter = NULL;
-	struct vchiq_bulk *bulk;
-	int status = -EINVAL;
-
-	if (!service)
-		goto error_exit;
-
-	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
-		goto error_exit;
-
-	if (!offset && !uoffset)
-		goto error_exit;
-
-	if (vchiq_check_service(service))
-		goto error_exit;
-
-	switch (mode) {
-	case VCHIQ_BULK_MODE_NOCALLBACK:
-	case VCHIQ_BULK_MODE_CALLBACK:
-		break;
-	default:
-		goto error_exit;
-	}
-
-	status = vchiq_bulk_xfer_queue_msg_interruptible(service, offset, uoffset,
-							 size, userdata, mode, dir);
-	if (status)
-		goto error_exit;
-
-	vchiq_service_put(service);
-
-	if (bulk_waiter) {
-		bulk_waiter->bulk = bulk;
-		if (wait_for_completion_interruptible(&bulk_waiter->event))
-			status = -EAGAIN;
-		else if (bulk_waiter->actual == VCHIQ_BULK_ACTUAL_ABORTED)
-			status = -EINVAL;
-	}
-
-	return 0;
-
-error_exit:
-	if (service)
-		vchiq_service_put(service);
-	return status;
-}
-
 /*
  * This function is called by VCHIQ ioctl interface and is interruptible.
  * It may receive -EAGAIN to indicate that a signal has been received
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 9c8c076eaaeb..468463f31801 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -485,11 +485,6 @@ vchiq_bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned
 				       enum vchiq_bulk_mode mode, void *userdata,
 				       enum vchiq_bulk_dir dir);
 
-extern int
-vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
-		    void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
-		    enum vchiq_bulk_dir dir);
-
 extern void
 vchiq_dump_state(struct seq_file *f, struct vchiq_state *state);
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 169a2ffda996..d41a4624cc92 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -309,7 +309,6 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 								NULL, args->data, args->size,
 								userdata, dir);
 
-		goto bulk_transfer_handled;
 	} else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
 		mutex_lock(&instance->bulk_waiter_list_mutex);
 		list_for_each_entry(iter, &instance->bulk_waiter_list,
@@ -332,8 +331,6 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		userdata = &waiter->bulk_waiter;
 
 		status = vchiq_bulk_xfer_waiting_interruptible(instance, args->handle, userdata);
-
-		goto bulk_transfer_handled;
 	} else {
 		userdata = args->userdata;
 
@@ -341,13 +338,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 								args->data, args->size,
 								args->mode, userdata, dir);
 
-		goto bulk_transfer_handled;
 	}
 
-	status = vchiq_bulk_transfer(instance, args->handle, NULL, args->data, args->size,
-				     userdata, args->mode, dir);
-
-bulk_transfer_handled:
 	if (!waiter) {
 		ret = 0;
 		goto out;
-- 
2.45.2

[PATCH v4 6/7] staging: vchiq_core: Remove unused function argument

[Umang Jain]

The argument 'is_blocking' in queue_message_sync() is not
used in the function. Drop it.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c  | 6 +++---
 1 file changed, 3 insertions(+), 3 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 9b0009d1906e..497c09d991b3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1146,7 +1146,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 		   int msgid,
 		   ssize_t (*copy_callback)(void *context, void *dest,
 					    size_t offset, size_t maxsize),
-		   void *context, int size, int is_blocking)
+		   void *context, int size)
 {
 	struct vchiq_shared_state *local;
 	struct vchiq_header *header;
@@ -1524,7 +1524,7 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
 		/* Acknowledge the OPEN */
 		if (service->sync) {
 			if (queue_message_sync(state, NULL, openack_id, memcpy_copy_callback,
-					       &ack_payload, sizeof(ack_payload), 0) == -EAGAIN)
+					       &ack_payload, sizeof(ack_payload)) == -EAGAIN)
 				goto bail_not_ready;
 
 			/* The service is now open */
@@ -3135,7 +3135,7 @@ vchiq_queue_message(struct vchiq_instance *instance, unsigned int handle,
 		break;
 	case VCHIQ_SRVSTATE_OPENSYNC:
 		status = queue_message_sync(service->state, service, data_id,
-					    copy_callback, context, size, 1);
+					    copy_callback, context, size);
 		break;
 	default:
 		status = -EINVAL;
-- 
2.45.2

[PATCH v4 7/7] staging: vchiq_core: Pass enumerated flag instead of int

[Umang Jain]

Pass proper enumerated flag which exists, instead of an integer while
calling queue_message(). It helps with readability of the code.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 497c09d991b3..e9041a20d0b2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3131,7 +3131,8 @@ vchiq_queue_message(struct vchiq_instance *instance, unsigned int handle,
 	switch (service->srvstate) {
 	case VCHIQ_SRVSTATE_OPEN:
 		status = queue_message(service->state, service, data_id,
-				       copy_callback, context, size, 1);
+				       copy_callback, context, size,
+				       QMFLAGS_IS_BLOCKING);
 		break;
 	case VCHIQ_SRVSTATE_OPENSYNC:
 		status = queue_message_sync(service->state, service, data_id,
-- 
2.45.2