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

[PATCH 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/

To: Florian Fainelli <florian.fainelli@broadcom.com>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-staging@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Cc: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Stefan Wahren <wahrenst@gmx.net>
Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Phil Elwell <phil@raspberrypi.com>

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
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

 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  20 +-
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 324 +++++++++++++--------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  16 +-
 .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  13 +-
 4 files changed, 236 insertions(+), 137 deletions(-)
---
base-commit: 3676d01bda1ed48ff15fcf9534897dc4640ec826
change-id: 20240823-to_sent2-75b3b8b8c156

Best regards,
-- 
Umang Jain <umang.jain@ideasonboard.com>

[PATCH 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>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 52 +++++++++++++++++++---
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  4 ++
 .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  5 +++
 3 files changed, 56 insertions(+), 5 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..228a41ecf90c 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,7 +3111,6 @@ 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;
@@ -3143,6 +3138,53 @@ 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
+bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, unsigned int handle,
+				void *userdata)
+{
+	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 (!userdata)
+		goto error_exit;
+
+	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
+		goto error_exit;
+
+	if (vchiq_check_service(service))
+		goto error_exit;
+
+	bulk_waiter = userdata;
+	bulk = bulk_waiter->bulk;
+
+	vchiq_service_put(service);
+
+	status = 0;
+
+	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 status;
+
+error_exit:
+	if (service)
+		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..1fda8631d1b6 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
+bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, unsigned int handle,
+				void *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..445b7938eea3 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 = 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.0

[PATCH 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>
---
 .../vc04_services/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 228a41ecf90c..c31f5d773fa5 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);
 
@@ -3125,13 +3047,6 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 
 	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);
-
 error_exit:
 	if (service)
 		vchiq_service_put(service);
@@ -3294,6 +3209,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.0

[PATCH 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>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  5 +--
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 48 +++++++++++++++++++---
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  5 +++
 .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  6 +++
 4 files changed, 55 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..e9b9c76381dc 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 = 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 c31f5d773fa5..341a06997848 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,48 @@ vchiq_remove_service(struct vchiq_instance *instance, unsigned int handle)
 	return status;
 }
 
+int
+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 = NULL;
+	enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
+	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;
+
+	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);
+	if (status)
+		goto error_exit;
+
+	vchiq_service_put(service);
+
+	return 0;
+
+error_exit:
+	if (service)
+		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 +3060,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 1fda8631d1b6..ff3559c3d1ba 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
 bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, unsigned int handle,
 				void *userdata);
 
+extern int
+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 445b7938eea3..1bff97ad28fa 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 = 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.0

[PATCH 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>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 15 ++++----
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 40 ++++++++++++++++++++++
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  6 ++++
 .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  6 ++++
 4 files changed, 60 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 e9b9c76381dc..5210ce8d269e 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 = 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 = 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 341a06997848..0606561fd3d0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3027,6 +3027,46 @@ bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int h
 	return status;
 }
 
+int
+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)
+		goto error_exit;
+
+	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);
+	if (status)
+		goto error_exit;
+
+	vchiq_service_put(service);
+
+	return 0;
+
+error_exit:
+	if (service)
+		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 ff3559c3d1ba..8aaf3c9d3dbe 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 @@ bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int h
 				 void *offset, void __user *uoffset, int size,
 				 void __user *userdata, enum vchiq_bulk_dir dir);
 
+extern int
+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 1bff97ad28fa..7ecfcaa85569 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 = 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.0

[PATCH 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>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 62 ----------------------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  5 --
 .../vc04_services/interface/vchiq_arm/vchiq_dev.c  | 10 ----
 3 files changed, 77 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 0606561fd3d0..43c6a214be86 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3067,68 +3067,6 @@ bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned int h
 	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);
-
-	status = 0;
-
-	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 status;
-
-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 8aaf3c9d3dbe..be85a15df9a7 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 @@ bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned int h
 				 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 7ecfcaa85569..4d8432158259 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -308,8 +308,6 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		status = 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,
@@ -332,22 +330,14 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		userdata = &waiter->bulk_waiter;
 
 		status = bulk_xfer_waiting_interruptible(instance, args->handle, userdata);
-
-		goto bulk_transfer_handled;
 	} else {
 		userdata = args->userdata;
 
 		status = 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,
-				     userdata, args->mode, dir);
-
-bulk_transfer_handled:
 	if (!waiter) {
 		ret = 0;
 		goto out;

-- 
2.45.0

[PATCH 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>
---
 drivers/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 43c6a214be86..84631878f77d 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 */
@@ -3150,7 +3150,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.0

[PATCH 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>
---
 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 84631878f77d..54e21a4273b8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3146,7 +3146,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.0

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

[Arnd Bergmann]

On Fri, Aug 23, 2024, at 15:14, Umang Jain wrote:
> 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>

This looks reasonable, just one think I would change:

> +int
> +bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, 
> unsigned int handle,
> +				void *userdata)
> +{
> +	struct vchiq_service *service = find_service_by_handle(instance, 
> handle);
> +	struct bulk_waiter *bulk_waiter = NULL;

> +
> +	bulk_waiter = userdata;

Since you always pass a 'struct bulk_waiter' here, please
replace the 'void *userdata' with the typed argument directly.

       Arnd

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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Aug 23, 2024 at 08:44:24PM +0530, Umang Jain wrote:
> 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>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 15 ++++----
>  .../vc04_services/interface/vchiq_arm/vchiq_core.c | 40 ++++++++++++++++++++++
>  .../vc04_services/interface/vchiq_arm/vchiq_core.h |  6 ++++
>  .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  6 ++++
>  4 files changed, 60 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 e9b9c76381dc..5210ce8d269e 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 = 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 = 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 341a06997848..0606561fd3d0 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -3027,6 +3027,46 @@ bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int h
>  	return status;
>  }
>  
> +int
> +bulk_xfer_callback_interruptible(struct vchiq_instance *instance, unsigned int handle,

Please prefix the name of all functions with an appropriate driver
prefix, especially for functions that are not static.

> +				 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)
> +		goto error_exit;
> +
> +	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);
> +	if (status)
> +		goto error_exit;
> +
> +	vchiq_service_put(service);
> +
> +	return 0;
> +
> +error_exit:
> +	if (service)
> +		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 ff3559c3d1ba..8aaf3c9d3dbe 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 @@ bulk_xfer_blocking_interruptible(struct vchiq_instance *instance, unsigned int h
>  				 void *offset, void __user *uoffset, int size,
>  				 void __user *userdata, enum vchiq_bulk_dir dir);
>  
> +extern int
> +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 1bff97ad28fa..7ecfcaa85569 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 = 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,
> 

-- 
Regards,

Laurent Pinchart

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

[Arnd Bergmann]

On Fri, Aug 23, 2024, at 15:14, Umang Jain wrote:

> 
> +int
> +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)

It seems you can also tighten the argument types here a bit more:

- uoffset is always NULL and can be left out
- userdata is either NULL or a struct mmal_msg_context, so you can use
  that type directly.

     Arnd

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

[Stefan Wahren]

Hi Umang,

Am 23.08.24 um 17:14 schrieb 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>
> ---
>   .../vc04_services/interface/vchiq_arm/vchiq_core.c | 52 +++++++++++++++++++---
>   .../vc04_services/interface/vchiq_arm/vchiq_core.h |  4 ++
>   .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  5 +++
>   3 files changed, 56 insertions(+), 5 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..228a41ecf90c 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,7 +3111,6 @@ 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;
> @@ -3143,6 +3138,53 @@ 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
> +bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, unsigned int handle,
> +				void *userdata)
> +{
> +	struct vchiq_service *service = find_service_by_handle(instance, handle);
> +	struct bulk_waiter *bulk_waiter = NULL;
I think there is no need to init bulk_waiter with NULL
> +	struct vchiq_bulk *bulk;
> +	int status = -EINVAL;
> +
> +	if (!service)
> +		goto error_exit;
> +
> +	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;
> +	bulk = bulk_waiter->bulk;
> +
> +	vchiq_service_put(service);
> +
> +	status = 0;
> +
> +	bulk_waiter->bulk = bulk;
Maybe i missed something, but before vchiq_service_put() we already
assigned the other way around.
> +	if (wait_for_completion_interruptible(&bulk_waiter->event))
> +		status = -EAGAIN;
> +	else if (bulk_waiter->actual == VCHIQ_BULK_ACTUAL_ABORTED)
> +		status = -EINVAL;
> +
> +	return status;
How about return the return code directly at these 3 places? This would
improve readability.

Best regards
> +
> +error_exit:
> +	if (service)
> +		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..1fda8631d1b6 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
> +bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, unsigned int handle,
> +				void *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..445b7938eea3 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 = 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;
>

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

[Stefan Wahren]

Hi Umang,

Am 23.08.24 um 17:14 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>
> ---
>   .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  5 +--
>   .../vc04_services/interface/vchiq_arm/vchiq_core.c | 48 +++++++++++++++++++---
>   .../vc04_services/interface/vchiq_arm/vchiq_core.h |  5 +++
>   .../vc04_services/interface/vchiq_arm/vchiq_dev.c  |  6 +++
>   4 files changed, 55 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..e9b9c76381dc 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 = 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 c31f5d773fa5..341a06997848 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,48 @@ vchiq_remove_service(struct vchiq_instance *instance, unsigned int handle)
>   	return status;
>   }
>
> +int
> +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 = NULL;
No need to init bulk_waiter here
> +	enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
> +	int status = -EINVAL;
> +
> +	if (!service)
> +		goto error_exit;
Please return the error directly
> +
> +	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);
> +	if (status)
> +		goto error_exit;
> +
> +	vchiq_service_put(service);
> +
> +	return 0;
> +
> +error_exit:
> +	if (service)
> +		vchiq_service_put(service);
Since we return directly if service == NULL, we can drop the if statement
> +	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 +3060,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 1fda8631d1b6..ff3559c3d1ba 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
>   bulk_xfer_waiting_interruptible(struct vchiq_instance *instance, unsigned int handle,
>   				void *userdata);
>
> +extern int
> +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 445b7938eea3..1bff97ad28fa 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 = 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 6/7] staging: vchiq_core: Remove unused function argument

[Stefan Wahren]

Am 23.08.24 um 17:14 schrieb 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>
Good catch!

Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>   drivers/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 43c6a214be86..84631878f77d 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 */
> @@ -3150,7 +3150,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;
>

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

[Stefan Wahren]

Am 23.08.24 um 17:14 schrieb 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>

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

[Umang Jain]

Hi Arnd

On 23/08/24 9:11 pm, Arnd Bergmann wrote:
> On Fri, Aug 23, 2024, at 15:14, Umang Jain wrote:
>
>> +int
>> +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)
> It seems you can also tighten the argument types here a bit more:
>
> - uoffset is always NULL and can be left out

Not true.

in vchiq_dev.c. it is not NULL

This should ideally be split off even more, for in-kernel usage (using 
the kernel pointers only) and for ioctl interface (that uses __user pointer)
> - userdata is either NULL or a struct mmal_msg_context, so you can use
>    that type directly.

It can be NULL, struct mmal_msg_context or a integer pointer - if coming 
through ioctl interface (vchiq_dev.c)

(Checked via checking vchiq_test code)

>
>       Arnd