Re: [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value

[Stefan Wahren <wahrenst@gmx.net>]

Hi Umang,

Am 03.07.24 um 15:10 schrieb Umang Jain:
> wait_event_interruptible() returns if the condition evaluates to true
> it receives a signal. However, the current code always assume that the
> wait_event_interruptible() returns only when the event is fired.
> This should not be the case as wait_event_interruptible() can
> return on receiving a signal (with -ERESTARTSYS as return value).
>
> We should consider this and bubble up the return value of
> wait_event_interruptible() to exactly know if the wait has failed
> and error out. This will also help to properly stop kthreads in the
> subsequent patch.
>
> Meanwhile at it, remote_wait_event() is modified to return 0 on success,
> and an error code (from wait_event_interruptible()) on failure. The
> return value is now checked for remote_wait_event() calls.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_core.c          | 31 ++++++++++++++-----
>   1 file changed, 24 insertions(+), 7 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 4f65e4021c4d..dd70f2881eca 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -501,16 +501,21 @@ remote_event_create(wait_queue_head_t *wq, struct remote_event *event)
>    * routines where switched to the "interruptible" family of functions, as the
>    * former was deemed unjustified and the use "killable" set all VCHIQ's
>    * threads in D state.
> + *
> + * Returns: 0 on success, a negative error code on failure
>    */
>   static inline int
>   remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
>   {
> +	int ret = 0;
> +
>   	if (!event->fired) {
>   		event->armed = 1;
>   		dsb(sy);
> -		if (wait_event_interruptible(*wq, event->fired)) {
> +		ret = wait_event_interruptible(*wq, event->fired);
> +		if (ret) {
>   			event->armed = 0;
> -			return 0;
> +			return ret;
>   		}
>   		event->armed = 0;
>   		/* Ensure that the peer sees that we are not waiting (armed == 0). */
> @@ -518,7 +523,7 @@ remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
>   	}
>
>   	event->fired = 0;
> -	return 1;
> +	return ret;
in general this patch looks good to me. But maybe we better return 0
directly and reduce the scope of ret.
>   }
>
>   /*
> @@ -1140,6 +1145,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>   	struct vchiq_header *header;
>   	ssize_t callback_result;
>   	int svc_fourcc;
> +	int ret;
>
>   	local = state->local;
>
> @@ -1147,7 +1153,9 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>   	    mutex_lock_killable(&state->sync_mutex))
>   		return -EAGAIN;
>
> -	remote_event_wait(&state->sync_release_event, &local->sync_release);
> +	ret = remote_event_wait(&state->sync_release_event, &local->sync_release);
> +	if (ret)
> +		return ret;
>
>   	/* Ensure that reads don't overtake the remote_event_wait. */
>   	rmb();
> @@ -1929,13 +1937,16 @@ slot_handler_func(void *v)
>   {
>   	struct vchiq_state *state = v;
>   	struct vchiq_shared_state *local = state->local;
> +	int ret;
>
>   	DEBUG_INITIALISE(local);
>
>   	while (1) {
>   		DEBUG_COUNT(SLOT_HANDLER_COUNT);
>   		DEBUG_TRACE(SLOT_HANDLER_LINE);
> -		remote_event_wait(&state->trigger_event, &local->trigger);
> +		ret = remote_event_wait(&state->trigger_event, &local->trigger);
> +		if (ret)
> +			return ret;
>
>   		/* Ensure that reads don't overtake the remote_event_wait. */
>   		rmb();
> @@ -1966,6 +1977,7 @@ recycle_func(void *v)
>   	struct vchiq_shared_state *local = state->local;
>   	u32 *found;
>   	size_t length;
> +	int ret;
>
>   	length = sizeof(*found) * BITSET_SIZE(VCHIQ_MAX_SERVICES);
>
> @@ -1975,7 +1987,9 @@ recycle_func(void *v)
>   		return -ENOMEM;
>
>   	while (1) {
> -		remote_event_wait(&state->recycle_event, &local->recycle);
> +		ret = remote_event_wait(&state->recycle_event, &local->recycle);
> +		if (ret)
> +			return ret;
>
>   		process_free_queue(state, found, length);
>   	}
> @@ -1992,6 +2006,7 @@ sync_func(void *v)
>   		(struct vchiq_header *)SLOT_DATA_FROM_INDEX(state,
>   			state->remote->slot_sync);
>   	int svc_fourcc;
> +	int ret;
>
>   	while (1) {
>   		struct vchiq_service *service;
> @@ -1999,7 +2014,9 @@ sync_func(void *v)
>   		int type;
>   		unsigned int localport, remoteport;
>
> -		remote_event_wait(&state->sync_trigger_event, &local->sync_trigger);
> +		ret = remote_event_wait(&state->sync_trigger_event, &local->sync_trigger);
> +		if (ret)
> +			return ret;
>
>   		/* Ensure that reads don't overtake the remote_event_wait. */
>   		rmb();