[1/2] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl
diff mbox

Message ID 1436967532-13251-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 15, 2015, 1:38 p.m. UTC
Since David Herrmann's mmap vma manager rework we don't need to grab
dev->struct_mutex any more to prevent races when looking up the mmap
offset. Drop it and instead don't forget to use the unref_unlocked
variant (since the drm core still cares).

While at it also fix a leak when this ioctl is called on an imported
buffer.

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/tegra/gem.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Thierry Reding Aug. 10, 2015, 10:30 a.m. UTC | #1
On Wed, Jul 15, 2015 at 03:38:51PM +0200, Daniel Vetter wrote:
> Since David Herrmann's mmap vma manager rework we don't need to grab
> dev->struct_mutex any more to prevent races when looking up the mmap
> offset. Drop it and instead don't forget to use the unref_unlocked
> variant (since the drm core still cares).
> 
> While at it also fix a leak when this ioctl is called on an imported
> buffer.

I don't see where the leak's fixed, but other than that this looks good
to me. Shall I pick this up into the drm/tegra tree?

Thierry

> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/tegra/gem.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 01e16e146bfe..827838e64d6e 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
>  	struct drm_gem_object *gem;
>  	struct tegra_bo *bo;
>  
> -	mutex_lock(&drm->struct_mutex);
> -
>  	gem = drm_gem_object_lookup(drm, file, handle);
>  	if (!gem) {
>  		dev_err(drm->dev, "failed to lookup GEM object\n");
> -		mutex_unlock(&drm->struct_mutex);
>  		return -EINVAL;
>  	}
>  
> @@ -423,8 +420,6 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
>  
>  	drm_gem_object_unreference(gem);
>  
> -	mutex_unlock(&drm->struct_mutex);
> -
>  	return 0;
>  }
>  
> -- 
> 2.1.4
>
Daniel Vetter Aug. 10, 2015, 11:31 a.m. UTC | #2
On Mon, Aug 10, 2015 at 12:30:21PM +0200, Thierry Reding wrote:
> On Wed, Jul 15, 2015 at 03:38:51PM +0200, Daniel Vetter wrote:
> > Since David Herrmann's mmap vma manager rework we don't need to grab
> > dev->struct_mutex any more to prevent races when looking up the mmap
> > offset. Drop it and instead don't forget to use the unref_unlocked
> > variant (since the drm core still cares).
> > 
> > While at it also fix a leak when this ioctl is called on an imported
> > buffer.
> 
> I don't see where the leak's fixed, but other than that this looks good
> to me. Shall I pick this up into the drm/tegra tree?

Copypaste in the commit message from armada, doesn't apply to tegra. Do
you also plan to pick up "drm/tegra: Use
drm_gem_object_reference_unlocked" directly?

And thanks for all the review.
-Daniel

> 
> Thierry
> 
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/tegra/gem.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> > index 01e16e146bfe..827838e64d6e 100644
> > --- a/drivers/gpu/drm/tegra/gem.c
> > +++ b/drivers/gpu/drm/tegra/gem.c
> > @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
> >  	struct drm_gem_object *gem;
> >  	struct tegra_bo *bo;
> >  
> > -	mutex_lock(&drm->struct_mutex);
> > -
> >  	gem = drm_gem_object_lookup(drm, file, handle);
> >  	if (!gem) {
> >  		dev_err(drm->dev, "failed to lookup GEM object\n");
> > -		mutex_unlock(&drm->struct_mutex);
> >  		return -EINVAL;
> >  	}
> >  
> > @@ -423,8 +420,6 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
> >  
> >  	drm_gem_object_unreference(gem);
> >  
> > -	mutex_unlock(&drm->struct_mutex);
> > -
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.1.4
> >
Thierry Reding Aug. 10, 2015, 11:38 a.m. UTC | #3
On Mon, Aug 10, 2015 at 01:31:42PM +0200, Daniel Vetter wrote:
> On Mon, Aug 10, 2015 at 12:30:21PM +0200, Thierry Reding wrote:
> > On Wed, Jul 15, 2015 at 03:38:51PM +0200, Daniel Vetter wrote:
> > > Since David Herrmann's mmap vma manager rework we don't need to grab
> > > dev->struct_mutex any more to prevent races when looking up the mmap
> > > offset. Drop it and instead don't forget to use the unref_unlocked
> > > variant (since the drm core still cares).
> > > 
> > > While at it also fix a leak when this ioctl is called on an imported
> > > buffer.
> > 
> > I don't see where the leak's fixed, but other than that this looks good
> > to me. Shall I pick this up into the drm/tegra tree?
> 
> Copypaste in the commit message from armada, doesn't apply to tegra. Do
> you also plan to pick up "drm/tegra: Use
> drm_gem_object_reference_unlocked" directly?

I don't mind much either way. I don't think they'll conflict with
anything, but since you already said that they're all independent, I
don't see a reason why I shouldn't pull them into drm/tegra. If you
prefer to keep them together, that's fine with me too.

Thanks,
Thierry

Patch
diff mbox

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 01e16e146bfe..827838e64d6e 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -408,12 +408,9 @@  int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
 	struct drm_gem_object *gem;
 	struct tegra_bo *bo;
 
-	mutex_lock(&drm->struct_mutex);
-
 	gem = drm_gem_object_lookup(drm, file, handle);
 	if (!gem) {
 		dev_err(drm->dev, "failed to lookup GEM object\n");
-		mutex_unlock(&drm->struct_mutex);
 		return -EINVAL;
 	}
 
@@ -423,8 +420,6 @@  int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
 
 	drm_gem_object_unreference(gem);
 
-	mutex_unlock(&drm->struct_mutex);
-
 	return 0;
 }