[PATCH 00/10] staging: vchiq_arm: address TODO list

[PATCH 00/10] staging: vchiq_arm: address TODO list

[Stefan Wahren]

This series mostly addresses the following points from the TODO list:
- Get rid of custom function return values
- Reformat core code with more sane indentations

Additionally this addresses some issues reported by checkpatch.pl 

Stefan Wahren (10):
  staging: vchiq_arm: avoid crashing the kernel
  staging: vchiq_core: break early in vchiq_close_service_internal
  staging: vchiq_core: introduce get_bulk_reason
  staging: vchiq_core: Drop unnecessary check in notify_bulks
  staging: vchiq_arm: drop return value of vchiq_arm_init_state
  staging: vchiq_2835_arm: drop enum vchiq_status
  staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal
  staging: vchiq_core: drop vchiq_status from vchiq_set_service_option
  staging: vchiq_core: drop vchiq_status from vchiq_initialise
  staging: vchiq_core: drop vchiq_status from vchiq_init_state

 .../interface/vchiq_arm/vchiq_2835_arm.c           |  23 ++-
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  75 ++++----
 .../vc04_services/interface/vchiq_arm/vchiq_arm.h  |   2 +-
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 191 +++++++++++----------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  12 +-
 5 files changed, 158 insertions(+), 145 deletions(-)

-- 
2.7.4

[PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel

[Stefan Wahren]

Using BUG_ON in a non-essential driver isn't recommend. So better
trigger a stacktrace and bailout.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c       | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 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 8b2b477..c51840a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -602,7 +602,9 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 
 	service = handle_to_service(handle);
-	BUG_ON(!service);
+	if (WARN_ON(!service))
+		return VCHIQ_SUCCESS;
+
 	user_service = (struct user_service *)service->base.userdata;
 	instance = user_service->instance;
 
@@ -918,8 +920,12 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 			goto out;
 	}
 
-	BUG_ON((int)(user_service->msg_insert -
-		user_service->msg_remove) < 0);
+	if (WARN_ON_ONCE((int)(user_service->msg_insert -
+			 user_service->msg_remove) < 0)) {
+		spin_unlock(&msg_queue_spinlock);
+		ret = -EINVAL;
+		goto out;
+	}
 
 	header = user_service->msg_queue[user_service->msg_remove &
 		(MSG_QUEUE_SIZE - 1)];
@@ -1937,7 +1943,10 @@ static int vchiq_release(struct inode *inode, struct file *file)
 
 		wait_for_completion(&service->remove_event);
 
-		BUG_ON(service->srvstate != VCHIQ_SRVSTATE_FREE);
+		if (WARN_ON(service->srvstate != VCHIQ_SRVSTATE_FREE)) {
+			unlock_service(service);
+			break;
+		}
 
 		spin_lock(&msg_queue_spinlock);
 
-- 
2.7.4

Re: [PATCH 01/10] staging: vchiq_arm: avoid crashing the kernel

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Using BUG_ON in a non-essential driver isn't recommend. So better
> trigger a stacktrace and bailout.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas

[PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal

[Stefan Wahren]

The logic in VCHIQ_SRVSTATE_OPEN* is unnecessary complex. Handle the error
case of queue_message() first makes it easier to read.

Btw we get the rid of the checkpatch warning:
WARNING: else is not generally useful after a break or return

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 26 +++++++++++-----------
 1 file changed, 13 insertions(+), 13 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 517a8c9..8a1e6f5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2763,21 +2763,21 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 				VCHIQ_MSG_DSTPORT(service->remoteport)),
 				NULL, NULL, 0, QMFLAGS_NO_MUTEX_UNLOCK);
 
-		if (status == VCHIQ_SUCCESS) {
-			if (!close_recvd) {
-				/* Change the state while the mutex is still held */
-				vchiq_set_service_state(service,
-							VCHIQ_SRVSTATE_CLOSESENT);
-				mutex_unlock(&state->slot_mutex);
-				if (service->sync)
-					mutex_unlock(&state->sync_mutex);
-				break;
-			}
-		} else if (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC) {
-			mutex_unlock(&state->sync_mutex);
+		if (status != VCHIQ_SUCCESS) {
+			if (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)
+				mutex_unlock(&state->sync_mutex);
 			break;
-		} else
+		}
+
+		if (!close_recvd) {
+			/* Change the state while the mutex is still held */
+			vchiq_set_service_state(service,
+						VCHIQ_SRVSTATE_CLOSESENT);
+			mutex_unlock(&state->slot_mutex);
+			if (service->sync)
+				mutex_unlock(&state->sync_mutex);
 			break;
+		}
 
 		/* Change the state while the mutex is still held */
 		vchiq_set_service_state(service, VCHIQ_SRVSTATE_CLOSERECVD);
-- 
2.7.4

Re: [PATCH 02/10] staging: vchiq_core: break early in vchiq_close_service_internal

[nicolas saenz julienne]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> The logic in VCHIQ_SRVSTATE_OPEN* is unnecessary complex. Handle the error
> case of queue_message() first makes it easier to read.
> 
> Btw we get the rid of the checkpatch warning:
> WARNING: else is not generally useful after a break or return
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks!
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason

[Stefan Wahren]

Nesting multiple ternary operators over multiple lines isn't easy to
read. Move this logic into a separate inline function.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 35 ++++++++++++++--------
 1 file changed, 22 insertions(+), 13 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 8a1e6f5..3a72c36 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1226,6 +1226,22 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
 	mutex_unlock(&state->recycle_mutex);
 }
 
+static inline enum vchiq_reason
+get_bulk_reason(struct vchiq_bulk *bulk)
+{
+	if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
+		if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
+			return VCHIQ_BULK_TRANSMIT_ABORTED;
+
+		return VCHIQ_BULK_TRANSMIT_DONE;
+	}
+
+	if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
+		return VCHIQ_BULK_RECEIVE_ABORTED;
+
+	return VCHIQ_BULK_RECEIVE_DONE;
+}
+
 /* Called by the slot handler - don't hold the bulk mutex */
 static enum vchiq_status
 notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
@@ -1281,16 +1297,8 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
 					spin_unlock(&bulk_waiter_spinlock);
 				} else if (bulk->mode ==
 					VCHIQ_BULK_MODE_CALLBACK) {
-					enum vchiq_reason reason = (bulk->dir ==
-						VCHIQ_BULK_TRANSMIT) ?
-						((bulk->actual ==
-						VCHIQ_BULK_ACTUAL_ABORTED) ?
-						VCHIQ_BULK_TRANSMIT_ABORTED :
-						VCHIQ_BULK_TRANSMIT_DONE) :
-						((bulk->actual ==
-						VCHIQ_BULK_ACTUAL_ABORTED) ?
-						VCHIQ_BULK_RECEIVE_ABORTED :
-						VCHIQ_BULK_RECEIVE_DONE);
+					enum vchiq_reason reason =
+							get_bulk_reason(bulk);
 					status = make_service_callback(service,
 						reason,	NULL, bulk->userdata);
 					if (status == VCHIQ_RETRY)
@@ -2622,9 +2630,10 @@ do_abort_bulks(struct vchiq_service *service)
 	mutex_unlock(&service->bulk_mutex);
 
 	status = notify_bulks(service, &service->bulk_tx, 0/*!retry_poll*/);
-	if (status == VCHIQ_SUCCESS)
-		status = notify_bulks(service, &service->bulk_rx,
-			0/*!retry_poll*/);
+	if (status != VCHIQ_SUCCESS)
+		return 0;
+
+	status = notify_bulks(service, &service->bulk_rx, 0/*!retry_poll*/);
 	return (status == VCHIQ_SUCCESS);
 }
 
-- 
2.7.4

Re: [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason

[Fabio Aiuto]

On Thu, Apr 22, 2021 at 10:07:49PM +0200, Stefan Wahren wrote:
> Nesting multiple ternary operators over multiple lines isn't easy to
> read. Move this logic into a separate inline function.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_core.c | 35 ++++++++++++++--------
>  1 file changed, 22 insertions(+), 13 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 8a1e6f5..3a72c36 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1226,6 +1226,22 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
>  	mutex_unlock(&state->recycle_mutex);
>  }
>  
> +static inline enum vchiq_reason
> +get_bulk_reason(struct vchiq_bulk *bulk)
> +{
> +	if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
> +		if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
> +			return VCHIQ_BULK_TRANSMIT_ABORTED;
> +
> +		return VCHIQ_BULK_TRANSMIT_DONE;
> +	}
> +
> +	if (bulk->actual == VCHIQ_BULK_ACTUAL_ABORTED)
> +		return VCHIQ_BULK_RECEIVE_ABORTED;
> +
> +	return VCHIQ_BULK_RECEIVE_DONE;
> +}
> +
>  /* Called by the slot handler - don't hold the bulk mutex */
>  static enum vchiq_status
>  notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
> @@ -1281,16 +1297,8 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
>  					spin_unlock(&bulk_waiter_spinlock);
>  				} else if (bulk->mode ==
>  					VCHIQ_BULK_MODE_CALLBACK) {
> -					enum vchiq_reason reason = (bulk->dir ==
> -						VCHIQ_BULK_TRANSMIT) ?
> -						((bulk->actual ==
> -						VCHIQ_BULK_ACTUAL_ABORTED) ?
> -						VCHIQ_BULK_TRANSMIT_ABORTED :
> -						VCHIQ_BULK_TRANSMIT_DONE) :
> -						((bulk->actual ==
> -						VCHIQ_BULK_ACTUAL_ABORTED) ?
> -						VCHIQ_BULK_RECEIVE_ABORTED :
> -						VCHIQ_BULK_RECEIVE_DONE);
> +					enum vchiq_reason reason =
> +							get_bulk_reason(bulk);
>  					status = make_service_callback(service,
>  						reason,	NULL, bulk->userdata);
>  					if (status == VCHIQ_RETRY)
> @@ -2622,9 +2630,10 @@ do_abort_bulks(struct vchiq_service *service)
>  	mutex_unlock(&service->bulk_mutex);
>  
>  	status = notify_bulks(service, &service->bulk_tx, 0/*!retry_poll*/);
> -	if (status == VCHIQ_SUCCESS)
> -		status = notify_bulks(service, &service->bulk_rx,
> -			0/*!retry_poll*/);
> +	if (status != VCHIQ_SUCCESS)
> +		return 0;
> +
> +	status = notify_bulks(service, &service->bulk_rx, 0/*!retry_poll*/);
>  	return (status == VCHIQ_SUCCESS);
>  }

Hi Stefan,

this change seem to be unrelated ti what's pointed out in changelog.
Should it be put in a separate patch?

>  
> -- 
> 2.7.4
> 
> 

thank you,

fabio

Re: [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason

[Stefan Wahren]

Am 23.04.21 um 09:23 schrieb Fabio Aiuto:
> On Thu, Apr 22, 2021 at 10:07:49PM +0200, Stefan Wahren wrote:
>> Nesting multiple ternary operators over multiple lines isn't easy to
>> read. Move this logic into a separate inline function.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  .../vc04_services/interface/vchiq_arm/vchiq_core.c | 35 ++++++++++++++--------
>>  1 file changed, 22 insertions(+), 13 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 8a1e6f5..3a72c36 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -2622,9 +2630,10 @@ do_abort_bulks(struct vchiq_service *service)
>>  	mutex_unlock(&service->bulk_mutex);
>>  
>>  	status = notify_bulks(service, &service->bulk_tx, 0/*!retry_poll*/);
>> -	if (status == VCHIQ_SUCCESS)
>> -		status = notify_bulks(service, &service->bulk_rx,
>> -			0/*!retry_poll*/);
>> +	if (status != VCHIQ_SUCCESS)
>> +		return 0;
>> +
>> +	status = notify_bulks(service, &service->bulk_rx, 0/*!retry_poll*/);
>>  	return (status == VCHIQ_SUCCESS);
>>  }
> Hi Stefan,
>
> this change seem to be unrelated ti what's pointed out in changelog.
> Should it be put in a separate patch?
Oops, yes this should be a separate patch.
>
>>  
>> -- 
>> 2.7.4
>>
>>
> thank you,
>
> fabio

Re: [PATCH 03/10] staging: vchiq_core: introduce get_bulk_reason

[nicolassaenzj]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Nesting multiple ternary operators over multiple lines isn't easy to
> read. Move this logic into a separate inline function.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Looks good to me modulo Fabio's comment.

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks

[Stefan Wahren]

There is no modification to the vchiq_status before the first if, so drop
this unnecessary check.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.c | 92 +++++++++++-----------
 1 file changed, 45 insertions(+), 47 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 3a72c36..9dc9a6a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1257,61 +1257,59 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
 
 	queue->remote_notify = queue->process;
 
-	if (status == VCHIQ_SUCCESS) {
-		while (queue->remove != queue->remote_notify) {
-			struct vchiq_bulk *bulk =
-				&queue->bulks[BULK_INDEX(queue->remove)];
+	while (queue->remove != queue->remote_notify) {
+		struct vchiq_bulk *bulk =
+			&queue->bulks[BULK_INDEX(queue->remove)];
 
-			/*
-			 * Only generate callbacks for non-dummy bulk
-			 * requests, and non-terminated services
-			 */
-			if (bulk->data && service->instance) {
-				if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
-					if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
-						VCHIQ_SERVICE_STATS_INC(service,
-							bulk_tx_count);
-						VCHIQ_SERVICE_STATS_ADD(service,
-							bulk_tx_bytes,
-							bulk->actual);
-					} else {
-						VCHIQ_SERVICE_STATS_INC(service,
-							bulk_rx_count);
-						VCHIQ_SERVICE_STATS_ADD(service,
-							bulk_rx_bytes,
-							bulk->actual);
-					}
+		/*
+		 * Only generate callbacks for non-dummy bulk
+		 * requests, and non-terminated services
+		 */
+		if (bulk->data && service->instance) {
+			if (bulk->actual != VCHIQ_BULK_ACTUAL_ABORTED) {
+				if (bulk->dir == VCHIQ_BULK_TRANSMIT) {
+					VCHIQ_SERVICE_STATS_INC(service,
+						bulk_tx_count);
+					VCHIQ_SERVICE_STATS_ADD(service,
+						bulk_tx_bytes,
+						bulk->actual);
 				} else {
 					VCHIQ_SERVICE_STATS_INC(service,
-						bulk_aborted_count);
+						bulk_rx_count);
+					VCHIQ_SERVICE_STATS_ADD(service,
+						bulk_rx_bytes,
+						bulk->actual);
 				}
-				if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
-					struct bulk_waiter *waiter;
-
-					spin_lock(&bulk_waiter_spinlock);
-					waiter = bulk->userdata;
-					if (waiter) {
-						waiter->actual = bulk->actual;
-						complete(&waiter->event);
-					}
-					spin_unlock(&bulk_waiter_spinlock);
-				} else if (bulk->mode ==
-					VCHIQ_BULK_MODE_CALLBACK) {
-					enum vchiq_reason reason =
-							get_bulk_reason(bulk);
-					status = make_service_callback(service,
-						reason,	NULL, bulk->userdata);
-					if (status == VCHIQ_RETRY)
-						break;
+			} else {
+				VCHIQ_SERVICE_STATS_INC(service,
+					bulk_aborted_count);
+			}
+			if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
+				struct bulk_waiter *waiter;
+
+				spin_lock(&bulk_waiter_spinlock);
+				waiter = bulk->userdata;
+				if (waiter) {
+					waiter->actual = bulk->actual;
+					complete(&waiter->event);
 				}
+				spin_unlock(&bulk_waiter_spinlock);
+			} else if (bulk->mode ==
+				VCHIQ_BULK_MODE_CALLBACK) {
+				enum vchiq_reason reason =
+						get_bulk_reason(bulk);
+				status = make_service_callback(service,
+					reason,	NULL, bulk->userdata);
+				if (status == VCHIQ_RETRY)
+					break;
 			}
-
-			queue->remove++;
-			complete(&service->bulk_remove_event);
 		}
-		if (!retry_poll)
-			status = VCHIQ_SUCCESS;
+
+		queue->remove++;
+		complete(&service->bulk_remove_event);
 	}
+	if (!retry_poll)
+		status = VCHIQ_SUCCESS;
 
 	if (status == VCHIQ_RETRY)
 		request_poll(service->state, service,
-- 
2.7.4

Re: [PATCH 04/10] staging: vchiq_core: Drop unnecessary check in notify_bulks

[nicolassaenzj]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> There is no modification to the vchiq_status before the first if, so drop
> this unnecessary check.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state

[Stefan Wahren]

The function vchiq_arm_init_state() cannot fail. So drop the return
value and the unnecessary code.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c    | 8 ++------
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c     | 3 +--
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h     | 2 +-
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 8782ebe..7c336d0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -172,7 +172,6 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
 enum vchiq_status
 vchiq_platform_init_state(struct vchiq_state *state)
 {
-	enum vchiq_status status = VCHIQ_SUCCESS;
 	struct vchiq_2835_state *platform_state;
 
 	state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
@@ -182,12 +181,9 @@ vchiq_platform_init_state(struct vchiq_state *state)
 	platform_state = (struct vchiq_2835_state *)state->platform_state;
 
 	platform_state->inited = 1;
-	status = vchiq_arm_init_state(state, &platform_state->arm_state);
+	vchiq_arm_init_state(state, &platform_state->arm_state);
 
-	if (status != VCHIQ_SUCCESS)
-		platform_state->inited = 0;
-
-	return status;
+	return VCHIQ_SUCCESS;
 }
 
 struct vchiq_arm_state*
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 c51840a..db10d83 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2312,7 +2312,7 @@ vchiq_keepalive_thread_func(void *v)
 	return 0;
 }
 
-enum vchiq_status
+void
 vchiq_arm_init_state(struct vchiq_state *state,
 		     struct vchiq_arm_state *arm_state)
 {
@@ -2328,7 +2328,6 @@ vchiq_arm_init_state(struct vchiq_state *state,
 		arm_state->first_connect = 0;
 
 	}
-	return VCHIQ_SUCCESS;
 }
 
 enum vchiq_status
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 24c331c..c7d2cf1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -67,7 +67,7 @@ int vchiq_platform_init(struct platform_device *pdev,
 extern struct vchiq_state *
 vchiq_get_state(void);
 
-extern enum vchiq_status
+extern void
 vchiq_arm_init_state(struct vchiq_state *state,
 		     struct vchiq_arm_state *arm_state);
 
-- 
2.7.4

Re: [PATCH 05/10] staging: vchiq_arm: drop return value of vchiq_arm_init_state

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> The function vchiq_arm_init_state() cannot fail. So drop the return
> value and the unnecessary code.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas

[PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status

[Stefan Wahren]

Replace the custom set of return values with proper Linux error codes.
As a result we need to initialize vchiq_status in vchiq_init_state.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c       | 12 ++++++------
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c   | 11 +++++------
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h   |  4 ++--
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 7c336d0..a644fe6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -169,21 +169,21 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
 	return 0;
 }
 
-enum vchiq_status
+int
 vchiq_platform_init_state(struct vchiq_state *state)
 {
 	struct vchiq_2835_state *platform_state;
 
 	state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
 	if (!state->platform_state)
-		return VCHIQ_ERROR;
+		return -ENOMEM;
 
 	platform_state = (struct vchiq_2835_state *)state->platform_state;
 
 	platform_state->inited = 1;
 	vchiq_arm_init_state(state, &platform_state->arm_state);
 
-	return VCHIQ_SUCCESS;
+	return 0;
 }
 
 struct vchiq_arm_state*
@@ -211,7 +211,7 @@ remote_event_signal(struct remote_event *event)
 		writel(0, g_regs + BELL2); /* trigger vc interrupt */
 }
 
-enum vchiq_status
+int
 vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
 			void __user *uoffset, int size, int dir)
 {
@@ -223,7 +223,7 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
 				       : PAGELIST_WRITE);
 
 	if (!pagelistinfo)
-		return VCHIQ_ERROR;
+		return -ENOMEM;
 
 	bulk->data = pagelistinfo->dma_addr;
 
@@ -233,7 +233,7 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
 	 */
 	bulk->remote_data = pagelistinfo;
 
-	return VCHIQ_SUCCESS;
+	return 0;
 }
 
 void
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 9dc9a6a..9f9677a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2162,9 +2162,9 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 {
 	struct vchiq_shared_state *local;
 	struct vchiq_shared_state *remote;
-	enum vchiq_status status;
+	enum vchiq_status status = VCHIQ_SUCCESS;
 	char threadname[16];
-	int i;
+	int i, ret;
 
 	if (vchiq_states[0]) {
 		pr_err("%s: VCHIQ state already initialized\n", __func__);
@@ -2246,8 +2246,8 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 
 	local->debug[DEBUG_ENTRIES] = DEBUG_MAX;
 
-	status = vchiq_platform_init_state(state);
-	if (status != VCHIQ_SUCCESS)
+	ret = vchiq_platform_init_state(state);
+	if (ret)
 		return VCHIQ_ERROR;
 
 	/*
@@ -3141,8 +3141,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
 	bulk->size = size;
 	bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
 
-	if (vchiq_prepare_bulk_data(bulk, offset, uoffset, size, dir)
-			!= VCHIQ_SUCCESS)
+	if (vchiq_prepare_bulk_data(bulk, offset, uoffset, size, dir))
 		goto unlock_error_exit;
 
 	wmb();
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 b817097..6b3a907 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -644,7 +644,7 @@ vchiq_queue_message(unsigned int handle,
  * implementations must be provided.
  */
 
-extern enum vchiq_status
+extern int
 vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
 			void __user *uoffset, int size, int dir);
 
@@ -679,7 +679,7 @@ vchiq_on_remote_use(struct vchiq_state *state);
 extern void
 vchiq_on_remote_release(struct vchiq_state *state);
 
-extern enum vchiq_status
+extern int
 vchiq_platform_init_state(struct vchiq_state *state);
 
 extern enum vchiq_status
-- 
2.7.4

Re: [PATCH 06/10] staging: vchiq_2835_arm: drop enum vchiq_status

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes.
> As a result we need to initialize vchiq_status in vchiq_init_state.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas

[PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal

[Stefan Wahren]

Replace the custom set of return values with proper Linux error codes
for the following functions:

vchiq_use_internal()
vchiq_release_internal()
vchiq_use_service_internal()
vchiq_release_service_internal()

Now we can use the result directly as return value for vchiq_ioctl().

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 31 +++++++++++-----------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h |  4 +--
 2 files changed, 17 insertions(+), 18 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 db10d83..3395201 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1385,21 +1385,20 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		service = find_service_for_instance(instance, handle);
 		if (service) {
-			status = (cmd == VCHIQ_IOC_USE_SERVICE)	?
+			ret = (cmd == VCHIQ_IOC_USE_SERVICE) ?
 				vchiq_use_service_internal(service) :
 				vchiq_release_service_internal(service);
-			if (status != VCHIQ_SUCCESS) {
+			if (ret) {
 				vchiq_log_error(vchiq_susp_log_level,
-					"%s: cmd %s returned error %d for service %c%c%c%c:%03d",
+					"%s: cmd %s returned error %ld for service %c%c%c%c:%03d",
 					__func__,
 					(cmd == VCHIQ_IOC_USE_SERVICE) ?
 						"VCHIQ_IOC_USE_SERVICE" :
 						"VCHIQ_IOC_RELEASE_SERVICE",
-					status,
+					ret,
 					VCHIQ_FOURCC_AS_4CHARS(
 						service->base.fourcc),
 					service->client_id);
-				ret = -EINVAL;
 			}
 		} else
 			ret = -EINVAL;
@@ -2330,18 +2329,18 @@ vchiq_arm_init_state(struct vchiq_state *state,
 	}
 }
 
-enum vchiq_status
+int
 vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 		   enum USE_TYPE_E use_type)
 {
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
-	enum vchiq_status ret = VCHIQ_SUCCESS;
+	int ret = 0;
 	char entity[16];
 	int *entity_uc;
 	int local_uc;
 
 	if (!arm_state) {
-		ret = VCHIQ_ERROR;
+		ret = -EINVAL;
 		goto out;
 	}
 
@@ -2357,7 +2356,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 		entity_uc = &service->service_use_count;
 	} else {
 		vchiq_log_error(vchiq_susp_log_level, "%s null service ptr", __func__);
-		ret = VCHIQ_ERROR;
+		ret = -EINVAL;
 		goto out;
 	}
 
@@ -2371,7 +2370,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 
 	write_unlock_bh(&arm_state->susp_res_lock);
 
-	if (ret == VCHIQ_SUCCESS) {
+	if (!ret) {
 		enum vchiq_status status = VCHIQ_SUCCESS;
 		long ack_cnt = atomic_xchg(&arm_state->ka_use_ack_count, 0);
 
@@ -2391,16 +2390,16 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 	return ret;
 }
 
-enum vchiq_status
+int
 vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
 {
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
-	enum vchiq_status ret = VCHIQ_SUCCESS;
+	int ret = 0;
 	char entity[16];
 	int *entity_uc;
 
 	if (!arm_state) {
-		ret = VCHIQ_ERROR;
+		ret = -EINVAL;
 		goto out;
 	}
 
@@ -2421,7 +2420,7 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
 		/* Don't use BUG_ON - don't allow user thread to crash kernel */
 		WARN_ON(!arm_state->videocore_use_count);
 		WARN_ON(!(*entity_uc));
-		ret = VCHIQ_ERROR;
+		ret = -EINVAL;
 		goto unlock;
 	}
 	--arm_state->videocore_use_count;
@@ -2460,13 +2459,13 @@ vchiq_on_remote_release(struct vchiq_state *state)
 	complete(&arm_state->ka_evt);
 }
 
-enum vchiq_status
+int
 vchiq_use_service_internal(struct vchiq_service *service)
 {
 	return vchiq_use_internal(service->state, service, USE_TYPE_SERVICE);
 }
 
-enum vchiq_status
+int
 vchiq_release_service_internal(struct vchiq_service *service)
 {
 	return vchiq_release_internal(service->state, service);
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 6b3a907..aad1c5c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -667,10 +667,10 @@ extern int
 vchiq_dump_platform_service_state(void *dump_context,
 	struct vchiq_service *service);
 
-extern enum vchiq_status
+extern int
 vchiq_use_service_internal(struct vchiq_service *service);
 
-extern enum vchiq_status
+extern int
 vchiq_release_service_internal(struct vchiq_service *service);
 
 extern void
-- 
2.7.4

Re: [PATCH 07/10] staging: vchiq_arm: drop enum vchiq_status from vchiq_*_internal

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes
> for the following functions:
> 
> vchiq_use_internal()
> vchiq_release_internal()
> vchiq_use_service_internal()
> vchiq_release_service_internal()
> 
> Now we can use the result directly as return value for vchiq_ioctl().
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas

[PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option

[Stefan Wahren]

Replace the custom set of return values with proper Linux error codes for
vchiq_set_service_option(). Now we can use the result directly as return
value for vchiq_ioctl().

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c      |  4 ++--
 .../vc04_services/interface/vchiq_arm/vchiq_core.c     | 18 +++++++++---------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h     |  2 +-
 3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			break;
 		}
 
-		status = vchiq_set_service_option(
-				args.handle, args.option, args.value);
+		ret = vchiq_set_service_option(args.handle, args.option,
+					       args.value);
 	} break;
 
 	case VCHIQ_IOC_LIB_VERSION: {
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 9f9677a..31c1c1a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
 	config->version_min            = VCHIQ_VERSION_MIN;
 }
 
-enum vchiq_status
+int
 vchiq_set_service_option(unsigned int handle,
 	enum vchiq_service_option option, int value)
 {
 	struct vchiq_service *service = find_service_by_handle(handle);
-	enum vchiq_status status = VCHIQ_ERROR;
 	struct vchiq_service_quota *quota;
+	int ret = -EIO;
 
 	if (!service)
-		return VCHIQ_ERROR;
+		return -EINVAL;
 
 	switch (option) {
 	case VCHIQ_SERVICE_OPTION_AUTOCLOSE:
 		service->auto_close = value;
-		status = VCHIQ_SUCCESS;
+		ret = 0;
 		break;
 
 	case VCHIQ_SERVICE_OPTION_SLOT_QUOTA:
@@ -3402,7 +3402,7 @@ vchiq_set_service_option(unsigned int handle,
 				 * dropped below its quota
 				 */
 				complete(&quota->quota_event);
-			status = VCHIQ_SUCCESS;
+			ret = 0;
 		}
 		break;
 
@@ -3420,7 +3420,7 @@ vchiq_set_service_option(unsigned int handle,
 				 * dropped below its quota
 				 */
 				complete(&quota->quota_event);
-			status = VCHIQ_SUCCESS;
+			ret = 0;
 		}
 		break;
 
@@ -3428,13 +3428,13 @@ vchiq_set_service_option(unsigned int handle,
 		if ((service->srvstate == VCHIQ_SRVSTATE_HIDDEN) ||
 		    (service->srvstate == VCHIQ_SRVSTATE_LISTENING)) {
 			service->sync = value;
-			status = VCHIQ_SUCCESS;
+			ret = 0;
 		}
 		break;
 
 	case VCHIQ_SERVICE_OPTION_TRACE:
 		service->trace = value;
-		status = VCHIQ_SUCCESS;
+		ret = 0;
 		break;
 
 	default:
@@ -3442,7 +3442,7 @@ vchiq_set_service_option(unsigned int handle,
 	}
 	unlock_service(service);
 
-	return status;
+	return ret;
 }
 
 static int
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 aad1c5c..bd1f590 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -712,7 +712,7 @@ extern int vchiq_get_client_id(unsigned int service);
 
 extern void vchiq_get_config(struct vchiq_config *config);
 
-extern enum vchiq_status
+extern int
 vchiq_set_service_option(unsigned int service, enum vchiq_service_option option,
 			 int value);
 
-- 
2.7.4

Re: [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes for
> vchiq_set_service_option(). Now we can use the result directly as return
> value for vchiq_ioctl().
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_arm.c      |  4 ++--
>  .../vc04_services/interface/vchiq_arm/vchiq_core.c     | 18 +++++++++---------
>  .../vc04_services/interface/vchiq_arm/vchiq_core.h     |  2 +-
>  3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  			break;
>  		}
>  
> 
> -		status = vchiq_set_service_option(
> -				args.handle, args.option, args.value);
> +		ret = vchiq_set_service_option(args.handle, args.option,
> +					       args.value);
>  	} break;
>  
> 
>  	case VCHIQ_IOC_LIB_VERSION: {
> 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 9f9677a..31c1c1a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
>  	config->version_min            = VCHIQ_VERSION_MIN;
>  }
>  
> 
> -enum vchiq_status
> +int
>  vchiq_set_service_option(unsigned int handle,
>  	enum vchiq_service_option option, int value)
>  {
>  	struct vchiq_service *service = find_service_by_handle(handle);
> -	enum vchiq_status status = VCHIQ_ERROR;
>  	struct vchiq_service_quota *quota;
> +	int ret = -EIO;

Due to the nature of the driver it's hard to make a clear distinction, but is
-EIO really the best error for the function? I don't see error paths that
really depend on communication with VC4. I see it more aligned with -EINVAL.

Regards,
Nicolas

Re: [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option

[Stefan Wahren]

Am 23.04.21 um 11:59 schrieb nicolas saenz julienne:
> On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
>> Replace the custom set of return values with proper Linux error codes for
>> vchiq_set_service_option(). Now we can use the result directly as return
>> value for vchiq_ioctl().
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  .../vc04_services/interface/vchiq_arm/vchiq_arm.c      |  4 ++--
>>  .../vc04_services/interface/vchiq_arm/vchiq_core.c     | 18 +++++++++---------
>>  .../vc04_services/interface/vchiq_arm/vchiq_core.h     |  2 +-
>>  3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>  			break;
>>  		}
>>  
>>
>> -		status = vchiq_set_service_option(
>> -				args.handle, args.option, args.value);
>> +		ret = vchiq_set_service_option(args.handle, args.option,
>> +					       args.value);
>>  	} break;
>>  
>>
>>  	case VCHIQ_IOC_LIB_VERSION: {
>> 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 9f9677a..31c1c1a 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
>>  	config->version_min            = VCHIQ_VERSION_MIN;
>>  }
>>  
>>
>> -enum vchiq_status
>> +int
>>  vchiq_set_service_option(unsigned int handle,
>>  	enum vchiq_service_option option, int value)
>>  {
>>  	struct vchiq_service *service = find_service_by_handle(handle);
>> -	enum vchiq_status status = VCHIQ_ERROR;
>>  	struct vchiq_service_quota *quota;
>> +	int ret = -EIO;
> Due to the nature of the driver it's hard to make a clear distinction, but is
> -EIO really the best error for the function? I don't see error paths that
> really depend on communication with VC4. I see it more aligned with -EINVAL.

In general i totally agree with you. I just wanted to the keep the
o(l)dd behavior, because vchiq_ioctl() has a catch all for VCHIQ_ERROR
-> -EIO

I'm not sure if any user application relies on the error code of
vchiq_ioctl(). Otherwise this isn't argument for all the other patches ;-)

I will change it to -EINVAL.

>
> Regards,
> Nicolas
>

Re: [PATCH 08/10] staging: vchiq_core: drop vchiq_status from vchiq_set_service_option

[nicolas saenz julienne]

On Fri, 2021-04-23 at 13:03 +0200, Stefan Wahren wrote:
> Am 23.04.21 um 11:59 schrieb nicolas saenz julienne:
> > On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> > > Replace the custom set of return values with proper Linux error codes for
> > > vchiq_set_service_option(). Now we can use the result directly as return
> > > value for vchiq_ioctl().
> > > 
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > ---
> > >  .../vc04_services/interface/vchiq_arm/vchiq_arm.c      |  4 ++--
> > >  .../vc04_services/interface/vchiq_arm/vchiq_core.c     | 18 +++++++++---------
> > >  .../vc04_services/interface/vchiq_arm/vchiq_core.h     |  2 +-
> > >  3 files changed, 12 insertions(+), 12 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 3395201..ab2ce07 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > >  			break;
> > >  		}
> > >  
> > > 
> > > -		status = vchiq_set_service_option(
> > > -				args.handle, args.option, args.value);
> > > +		ret = vchiq_set_service_option(args.handle, args.option,
> > > +					       args.value);
> > >  	} break;
> > >  
> > > 
> > >  	case VCHIQ_IOC_LIB_VERSION: {
> > > 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 9f9677a..31c1c1a 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
> > >  	config->version_min            = VCHIQ_VERSION_MIN;
> > >  }
> > >  
> > > 
> > > -enum vchiq_status
> > > +int
> > >  vchiq_set_service_option(unsigned int handle,
> > >  	enum vchiq_service_option option, int value)
> > >  {
> > >  	struct vchiq_service *service = find_service_by_handle(handle);
> > > -	enum vchiq_status status = VCHIQ_ERROR;
> > >  	struct vchiq_service_quota *quota;
> > > +	int ret = -EIO;
> > Due to the nature of the driver it's hard to make a clear distinction, but is
> > -EIO really the best error for the function? I don't see error paths that
> > really depend on communication with VC4. I see it more aligned with -EINVAL.
> 
> In general i totally agree with you. I just wanted to the keep the
> o(l)dd behavior, because vchiq_ioctl() has a catch all for VCHIQ_ERROR
> -> -EIO
> 
> I'm not sure if any user application relies on the error code of
> vchiq_ioctl(). Otherwise this isn't argument for all the other patches ;-)

This is something that bugged me the last time I tried to do a vchiq cleanup.
At some point we'll start affecting the IOCTL interface in subtle ways. It's a
PITA as we don't really know if there are users out there that wrote their own
VCHIQ applications. RPi ones we could fix.

I remember thinking that the best course of action would be to completely
decouple the core part from the char device. As in, for it to live under a
completely different directory, and registered the way we do it with the camera
interface. We'd get a way more concise VCHIQ core. But IIRC there are lots of
quirks and hacks to accomodate to the IOCTl behavior we'd have to iron out.

Regards,
Nicolas

[PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise

[Stefan Wahren]

Replace the custom set of return values with proper Linux error codes for
vchiq_initialise().

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c    | 20 +++++++++++---------
 1 file changed, 11 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 ab2ce07..04a72d4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -147,12 +147,11 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data,
 	unsigned int size, enum vchiq_bulk_dir dir);
 
 #define VCHIQ_INIT_RETRIES 10
-enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
+int vchiq_initialise(struct vchiq_instance **instance_out)
 {
-	enum vchiq_status status = VCHIQ_ERROR;
 	struct vchiq_state *state;
 	struct vchiq_instance *instance = NULL;
-	int i;
+	int i, ret;
 
 	vchiq_log_trace(vchiq_core_log_level, "%s called", __func__);
 
@@ -170,6 +169,7 @@ enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
 	if (i == VCHIQ_INIT_RETRIES) {
 		vchiq_log_error(vchiq_core_log_level,
 			"%s: videocore not initialized\n", __func__);
+		ret = -ENOTCONN;
 		goto failed;
 	} else if (i > 0) {
 		vchiq_log_warning(vchiq_core_log_level,
@@ -181,6 +181,7 @@ enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
 	if (!instance) {
 		vchiq_log_error(vchiq_core_log_level,
 			"%s: error allocating vchiq instance\n", __func__);
+		ret = -ENOMEM;
 		goto failed;
 	}
 
@@ -191,13 +192,13 @@ enum vchiq_status vchiq_initialise(struct vchiq_instance **instance_out)
 
 	*instance_out = instance;
 
-	status = VCHIQ_SUCCESS;
+	ret = 0;
 
 failed:
 	vchiq_log_trace(vchiq_core_log_level,
-		"%s(%p): returning %d", __func__, instance, status);
+		"%s(%p): returning %d", __func__, instance, ret);
 
-	return status;
+	return ret;
 }
 EXPORT_SYMBOL(vchiq_initialise);
 
@@ -2236,6 +2237,7 @@ vchiq_keepalive_thread_func(void *v)
 	enum vchiq_status status;
 	struct vchiq_instance *instance;
 	unsigned int ka_handle;
+	int ret;
 
 	struct vchiq_service_params_kernel params = {
 		.fourcc      = VCHIQ_MAKE_FOURCC('K', 'E', 'E', 'P'),
@@ -2244,10 +2246,10 @@ vchiq_keepalive_thread_func(void *v)
 		.version_min = KEEPALIVE_VER_MIN
 	};
 
-	status = vchiq_initialise(&instance);
-	if (status != VCHIQ_SUCCESS) {
+	ret = vchiq_initialise(&instance);
+	if (ret) {
 		vchiq_log_error(vchiq_susp_log_level,
-			"%s vchiq_initialise failed %d", __func__, status);
+			"%s vchiq_initialise failed %d", __func__, ret);
 		goto exit;
 	}
 
-- 
2.7.4

Re: [PATCH 09/10] staging: vchiq_core: drop vchiq_status from vchiq_initialise

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes for
> vchiq_initialise().
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas

[PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state

[Stefan Wahren]

Replace the custom set of return values with proper Linux error codes for
vchiq_init_state().

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c    |  5 +++--
 .../vc04_services/interface/vchiq_arm/vchiq_core.c        | 15 ++++++++-------
 .../vc04_services/interface/vchiq_arm/vchiq_core.h        |  2 +-
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index a644fe6..c3fbb29 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -132,8 +132,9 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
 	*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
 	sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
 
-	if (vchiq_init_state(state, vchiq_slot_zero) != VCHIQ_SUCCESS)
-		return -EINVAL;
+	err = vchiq_init_state(state, vchiq_slot_zero);
+	if (err)
+		return err;
 
 	g_regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(g_regs))
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 31c1c1a..4a4ce3c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2162,13 +2162,12 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 {
 	struct vchiq_shared_state *local;
 	struct vchiq_shared_state *remote;
-	enum vchiq_status status = VCHIQ_SUCCESS;
 	char threadname[16];
 	int i, ret;
 
 	if (vchiq_states[0]) {
 		pr_err("%s: VCHIQ state already initialized\n", __func__);
-		return VCHIQ_ERROR;
+		return -EINVAL;
 	}
 
 	local = &slot_zero->slave;
@@ -2181,7 +2180,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 		else
 			vchiq_loud_error("master/slave mismatch two slaves");
 		vchiq_loud_error_footer();
-		return VCHIQ_ERROR;
+		return -EINVAL;
 	}
 
 	memset(state, 0, sizeof(struct vchiq_state));
@@ -2248,7 +2247,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 
 	ret = vchiq_platform_init_state(state);
 	if (ret)
-		return VCHIQ_ERROR;
+		return ret;
 
 	/*
 	 * bring up slot handler thread
@@ -2262,7 +2261,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 		vchiq_loud_error_header();
 		vchiq_loud_error("couldn't create thread %s", threadname);
 		vchiq_loud_error_footer();
-		return VCHIQ_ERROR;
+		return PTR_ERR(state->slot_handler_thread);
 	}
 	set_user_nice(state->slot_handler_thread, -19);
 
@@ -2274,6 +2273,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 		vchiq_loud_error_header();
 		vchiq_loud_error("couldn't create thread %s", threadname);
 		vchiq_loud_error_footer();
+		ret = PTR_ERR(state->recycle_thread);
 		goto fail_free_handler_thread;
 	}
 	set_user_nice(state->recycle_thread, -19);
@@ -2286,6 +2286,7 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 		vchiq_loud_error_header();
 		vchiq_loud_error("couldn't create thread %s", threadname);
 		vchiq_loud_error_footer();
+		ret = PTR_ERR(state->sync_thread);
 		goto fail_free_recycle_thread;
 	}
 	set_user_nice(state->sync_thread, -20);
@@ -2299,14 +2300,14 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
 	/* Indicate readiness to the other side */
 	local->initialised = 1;
 
-	return status;
+	return 0;
 
 fail_free_recycle_thread:
 	kthread_stop(state->recycle_thread);
 fail_free_handler_thread:
 	kthread_stop(state->slot_handler_thread);
 
-	return VCHIQ_ERROR;
+	return ret;
 }
 
 void vchiq_msg_queue_push(unsigned int handle, struct vchiq_header *header)
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 bd1f590..89f898ce 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -539,7 +539,7 @@ get_conn_state_name(enum vchiq_connstate conn_state);
 extern struct vchiq_slot_zero *
 vchiq_init_slots(void *mem_base, int mem_size);
 
-extern enum vchiq_status
+extern int
 vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero);
 
 extern enum vchiq_status
-- 
2.7.4

Re: [PATCH 10/10] staging: vchiq_core: drop vchiq_status from vchiq_init_state

[nicolas saenz julienne]

On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> Replace the custom set of return values with proper Linux error codes for
> vchiq_init_state().
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Thanks,
Nicolas