Re: [PATCH 1/4] drm/vkms: Add functions to map GEM backing storage

[Haneen Mohammed <hamohammed.sa@gmail.com>]

On Tue, Jul 10, 2018 at 09:12:36AM +0100, Chris Wilson wrote:
> Quoting Haneen Mohammed (2018-07-09 16:44:26)
> > +struct page **get_pages(struct vkms_gem_object *vkms_obj)
> > +{
> > +       struct drm_gem_object *gem_obj = &vkms_obj->gem;
> > +       struct page **pages = vkms_obj->pages;
> > +
> > +       if (!pages) {
> > +               mutex_lock(&vkms_obj->pages_lock);
> > +               pages = drm_gem_get_pages(gem_obj);
> > +               if (IS_ERR(pages)) {
> > +                       mutex_unlock(&vkms_obj->pages_lock);
> > +                       return pages;
> > +               }
> > +
> > +               vkms_obj->pages = pages;
> > +               mutex_unlock(&vkms_obj->pages_lock);
> 
> You have a race here with two callers setting pages. Trivially you fix
> it by checking if (!pages) again inside the lock, but the lock is
> superfluous in this case:
> 	if (!vkms_obj->pages) {
> 		srtuct pages **pages;
> 
> 		pages = drm_gem_get_pages(gem_obj);
> 		if (IS_ERR(pages))
> 			return pages;
> 		
> 		if (cmpxchg(&vkms_obj->pages, NULL, pages))
> 			put_pages(pages);
> 
> 		/* barrier() is implied */
> 	}
> 
> 	return vkms_obj->pages;
> -Chris

Thank you for the feedback!

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