[Intel-gfx] [PATCH] drm/i915: Clear unpin_work along error path.

[Jesse Barnes]

On Fri,  8 Jan 2010 14:08:25 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> The locally allocated unpin_work was being freed along the error path,
> but we forgot that we had already set the dev_priv->unpin_work to the
> now freed structure. So remember to clear that pointer along the error
> path as well.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index 42e8c03..cb32f18 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4027,8 +4027,6 @@ static int intel_crtc_page_flip(struct drm_crtc
> *crtc, if (work == NULL)
>  		return -ENOMEM;
>  
> -	mutex_lock(&dev->struct_mutex);
> -
>  	work->event = event;
>  	work->dev = crtc->dev;
>  	intel_fb = to_intel_framebuffer(crtc->fb);
> @@ -4040,7 +4038,6 @@ static int intel_crtc_page_flip(struct drm_crtc
> *crtc, if (intel_crtc->unpin_work) {
>  		spin_unlock_irqrestore(&dev->event_lock, flags);
>  		kfree(work);
> -		mutex_unlock(&dev->struct_mutex);
>  		return -EBUSY;
>  	}
>  	intel_crtc->unpin_work = work;
> @@ -4049,10 +4046,16 @@ static int intel_crtc_page_flip(struct
> drm_crtc *crtc, intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> +	mutex_lock(&dev->struct_mutex);
>  	ret = intel_pin_and_fence_fb_obj(dev, obj);
>  	if (ret != 0) {
> -		kfree(work);
>  		mutex_unlock(&dev->struct_mutex);
> +
> +		spin_lock_irqsave(&dev->event_lock, flags);
> +		intel_crtc->unpin_work = NULL;
> +		spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +		kfree(work);
>  		return ret;
>  	}
>  

My last patchset had the NULL set, but I forgot to take the spinlock,
can you do an incremental one on top of those?  You should probably
mention something in the changelog about how safe it is to take the
mutex later too. :)

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center