Re: [PATCH 2/2] drm: Serialise multiple event readers

[Thomas Hellstrom <thellstrom@vmware.com>]

Do you need to take the mutex around other event pullers as well?
So that no such process thinks it has pulled all events and then
suddenly an event reappears?

I think there was some event pulling code in one of the drivers, but I
might be wrong.
The close() code should be safe against this.

/Thomas


On 11/25/2015 03:39 PM, Chris Wilson wrote:
> The previous patch reintroduced a race condition whereby a failure in
> one reader may allow a second reader to see out-of-order events.
> Introduce a mutex to serialise readers so that an event is completed in
> its entirety before another reader may process an event. The two readers
> may race against each other, but the events each retrieves are in the
> correct order.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fops.c | 18 +++++++++++++-----
>  include/drm/drmP.h         |  2 ++
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index eb8702d39e7d..81df9ae95e2e 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -172,6 +172,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>  	init_waitqueue_head(&priv->event_wait);
>  	priv->event_space = 4096; /* set aside 4k for event buffer */
>  
> +	mutex_init(&priv->event_read_lock);
> +
>  	if (drm_core_check_feature(dev, DRIVER_GEM))
>  		drm_gem_open(dev, priv);
>  
> @@ -483,11 +485,15 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
>  {
>  	struct drm_file *file_priv = filp->private_data;
>  	struct drm_device *dev = file_priv->minor->dev;
> -	ssize_t ret = 0;
> +	ssize_t ret;
>  
>  	if (!access_ok(VERIFY_WRITE, buffer, count))
>  		return -EFAULT;
>  
> +	ret = mutex_lock_interruptible(&file_priv->event_read_lock);
> +	if (ret)
> +		return ret;
> +
>  	for (;;) {
>  		struct drm_pending_event *e = NULL;
>  
> @@ -509,12 +515,13 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
>  				break;
>  			}
>  
> +			mutex_unlock(&file_priv->event_read_lock);
>  			ret = wait_event_interruptible(file_priv->event_wait,
>  						       !list_empty(&file_priv->event_list));
> -			if (ret < 0)
> -				break;
> -
> -			ret = 0;
> +			if (ret >= 0)
> +				ret = mutex_lock_interruptible(&file_priv->event_read_lock);
> +			if (ret)
> +				return ret;
>  		} else {
>  			unsigned length = e->event->length;
>  
> @@ -537,6 +544,7 @@ put_back_event:
>  			e->destroy(e);
>  		}
>  	}
> +	mutex_unlock(&file_priv->event_read_lock);
>  
>  	return ret;
>  }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 30d4a5a495e2..8e1df1f7057c 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -344,6 +344,8 @@ struct drm_file {
>  	struct list_head event_list;
>  	int event_space;
>  
> +	struct mutex event_read_lock;
> +
>  	struct drm_prime_file_private prime;
>  };
>  

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel