Re: [PATCH] staging: vchiq: rework remove_event handling

[Nicolas Saenz Julienne]

Hi Arnd, thanks for the patch!

On Mon, 2018-12-10 at 22:11 +0100, Arnd Bergmann wrote:
> I had started the removal of semaphores in this driver without
> knowing
> that Nicolas Saenz Julienne also worked on this. In case of the
> "remote
> event" infrastructure, my solution seemed significantly better, so
> I'm
> proposing this as a change on top.
> 
> The problem with using either semaphores or completions here is that
> it's an overly complex way of waking up a thread, and it looks like
> the
> 'count' of the semaphore can easily get out of sync, even though I
> found
> it hard to come up with a specific example.
> 
> Changing it to a 'wait_queue_head_t' instead of a completion
> simplifies
> this by letting us wait directly on the 'event->fired' variable that
> is
> set by the videocore.
> 
> Another simplification is passing the wait queue directly into the
> helper
> functions instead of going through the fragile logic of recording the
> offset inside of a structure as part of a shared memory variable.
> This
> also avoids one uncached memory read and should be faster.
> 
> Note that I'm changing it back to 'killable' after the previous patch
> changed 'killable' to 'interruptible', apparently based on a
> misunderstanding
> of the subtle down_interruptible() macro override in
> vchiq_killable.h.
> 
> Fixes: f27e47bc6b8b ("staging: vchiq: use completions instead of
> semaphores")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  .../interface/vchiq_arm/vchiq_core.c          | 63 +++++++--------
> ----
>  .../interface/vchiq_arm/vchiq_core.h          | 12 ++--
>  2 files changed, 30 insertions(+), 45 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 482b5daf6c0c..eda3004a0c6a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -417,26 +417,23 @@ vchiq_set_conn_state(VCHIQ_STATE_T *state,
> VCHIQ_CONNSTATE_T newstate)
>  }
>  
>  static inline void
> -remote_event_create(REMOTE_EVENT_T *event)
> +remote_event_create(wait_queue_head_t *wq, REMOTE_EVENT_T *event)
>  {
>  	event->armed = 0;
>  	/* Don't clear the 'fired' flag because it may already have
> been set
>  	** by the other side. */
> +	init_waitqueue_head(wq);
>  }
>  
>  static inline int
> -remote_event_wait(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
> +remote_event_wait(wait_queue_head_t *wq, REMOTE_EVENT_T *event)
>  {
>  	if (!event->fired) {
>  		event->armed = 1;
>  		dsb(sy);
> -		if (!event->fired) {
> -			if (wait_for_completion_interruptible(
> -					(struct completion *)
> -					((char *)state + event-
> >event))) {
> -				event->armed = 0;
> -				return 0;
> -			}
> +		if (wait_event_killable(*wq, event->fired)) {
> +			event->armed = 0;
> +			return 0;
>  		}
>  		event->armed = 0;
>  		wmb();
> @@ -447,26 +444,26 @@ remote_event_wait(VCHIQ_STATE_T *state,
> REMOTE_EVENT_T *event)
>  }
>  
>  static inline void
> -remote_event_signal_local(VCHIQ_STATE_T *state, REMOTE_EVENT_T
> *event)
> +remote_event_signal_local(wait_queue_head_t *wq, REMOTE_EVENT_T
> *event)
>  {
>  	event->armed = 0;
> -	complete((struct completion *)((char *)state + event->event));
> +	wake_up_all(wq);

Shouldn't this just be "wake_up(wq)"? 

Regards,
Nicolas

Attachment: signature.asc
Description: This is a digitally signed message part