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
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
