[PATCH] drm/i915/overlay: Fix unpinning along init error paths

[PATCH] drm/i915/overlay: Fix unpinning along init error paths

[Chris Wilson]

As pointed out by Dan Carpenter, it was seemingly possible to hit an error
whilst mapping the buffer for the regs (except the only likely error
returns should not happen during init) and so leak a pin count on the
bo. To handle this we would need to reacquire the struct mutex, so for
simplicity rearrange for the lock to be held for the entire function.
For extra pedagogy, test that we only call init once.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_overlay.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index cffd3ed..d360380 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1405,6 +1405,11 @@ void intel_setup_overlay(struct drm_device *dev)
 	overlay = kzalloc(sizeof(struct intel_overlay), GFP_KERNEL);
 	if (!overlay)
 		return;
+
+	mutex_lock(&dev->struct_mutex);
+	if (WARN_ON(dev_priv->overlay))
+		goto out_free;
+
 	overlay->dev = dev;
 
 	reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE);
@@ -1412,8 +1417,6 @@ void intel_setup_overlay(struct drm_device *dev)
 		goto out_free;
 	overlay->reg_bo = reg_bo;
 
-	mutex_lock(&dev->struct_mutex);
-
 	if (OVERLAY_NEEDS_PHYSICAL(dev)) {
 		ret = i915_gem_attach_phys_object(dev, reg_bo,
 						  I915_GEM_PHYS_OVERLAY_REGS,
@@ -1438,8 +1441,6 @@ void intel_setup_overlay(struct drm_device *dev)
                 }
 	}
 
-	mutex_unlock(&dev->struct_mutex);
-
 	/* init all values */
 	overlay->color_key = 0x0101fe;
 	overlay->brightness = -19;
@@ -1448,7 +1449,7 @@ void intel_setup_overlay(struct drm_device *dev)
 
 	regs = intel_overlay_map_regs(overlay);
 	if (!regs)
-		goto out_free_bo;
+		goto out_unpin_bo;
 
 	memset(regs, 0, sizeof(struct overlay_registers));
 	update_polyphase_filter(regs);
@@ -1457,15 +1458,17 @@ void intel_setup_overlay(struct drm_device *dev)
 	intel_overlay_unmap_regs(overlay, regs);
 
 	dev_priv->overlay = overlay;
+	mutex_unlock(&dev->struct_mutex);
 	DRM_INFO("initialized overlay support\n");
 	return;
 
 out_unpin_bo:
-	i915_gem_object_unpin(reg_bo);
+	if (!OVERLAY_NEEDS_PHYSICAL(dev))
+		i915_gem_object_unpin(reg_bo);
 out_free_bo:
 	drm_gem_object_unreference(&reg_bo->base);
-	mutex_unlock(&dev->struct_mutex);
 out_free:
+	mutex_unlock(&dev->struct_mutex);
 	kfree(overlay);
 	return;
 }
-- 
1.7.5.4

Re: [PATCH] drm/i915/overlay: Fix unpinning along init error paths

[Ben Widawsky]

On Tue, Jun 28, 2011 at 11:27:47AM +0100, Chris Wilson wrote:
> As pointed out by Dan Carpenter, it was seemingly possible to hit an error
> whilst mapping the buffer for the regs (except the only likely error
> returns should not happen during init) and so leak a pin count on the
> bo. To handle this we would need to reacquire the struct mutex, so for
> simplicity rearrange for the lock to be held for the entire function.
> For extra pedagogy, test that we only call init once.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index cffd3ed..d360380 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1405,6 +1405,11 @@ void intel_setup_overlay(struct drm_device *dev)
>  	overlay = kzalloc(sizeof(struct intel_overlay), GFP_KERNEL);
>  	if (!overlay)
>  		return;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	if (WARN_ON(dev_priv->overlay))
> +		goto out_free;
> +
>  	overlay->dev = dev;
>  
>  	reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE);

For my curiosity, was there a reason you chose to acquire the lock after
the kzalloc?

Re: [PATCH] drm/i915/overlay: Fix unpinning along init error paths

[Chris Wilson]

On Tue, 28 Jun 2011 08:57:14 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > index cffd3ed..d360380 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -1405,6 +1405,11 @@ void intel_setup_overlay(struct drm_device *dev)
> >  	overlay = kzalloc(sizeof(struct intel_overlay), GFP_KERNEL);
> >  	if (!overlay)
> >  		return;
> > +
> > +	mutex_lock(&dev->struct_mutex);
> > +	if (WARN_ON(dev_priv->overlay))
> > +		goto out_free;
> > +
> >  	overlay->dev = dev;
> >  
> >  	reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE);
> 
> For my curiosity, was there a reason you chose to acquire the lock after
> the kzalloc?

I was just trying to keep the error paths as simple as possible. Overtime,
we probably want to lift the mutex out of this function and higher into
the init paths for simplicity.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/overlay: Fix unpinning along init error paths

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 753 bytes --]

On Tue, 28 Jun 2011 11:27:47 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> As pointed out by Dan Carpenter, it was seemingly possible to hit an error
> whilst mapping the buffer for the regs (except the only likely error
> returns should not happen during init) and so leak a pin count on the
> bo. To handle this we would need to reacquire the struct mutex, so for
> simplicity rearrange for the lock to be held for the entire function.
> For extra pedagogy, test that we only call init once.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Keith Packard <keithp@keithp.com>

I'll get this into drm-intel-fixes once I can fast forward and pick up
Hugh's original patch.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx