[Intel-gfx] [PATCH v1] drm/i915/guc: Fix a memory leak where guc->execbuf_client is not freed

[Yu Dai]

On 01/13/2016 10:15 AM, Dave Gordon wrote:
> On 12/01/16 23:17, yu.dai at intel.com wrote:
> > From: Alex Dai <yu.dai at intel.com>
> >
> > During driver unloading, the guc_client created for command submission
> > needs to be released to avoid memory leak.
> >
> > The struct_mutex needs to be held before tearing down GuC.
> >
> > v1: Move i915_guc_submission_disable out of i915_guc_submission_fini and
> >      take struct_mutex lock before release GuC client. (Dave Gordon)
>
> You don't seem to have implemented all the points I mentioned? I think
> you want:
>
> drivers/gpu/drm/i915/intel_guc_loader.c:
> @@ -445,6 +445,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>
>           direct_interrupts_to_host(dev_priv);
>           i915_guc_submission_disable(dev);
> +       i915_guc_submission_fini(dev);
>
> Optional, but cleaner. We called i915_guc_submission_init() earlier in
> this function, so we should call i915_guc_submission_fini() in the
> failure path. That way, we either succeed, or leave the system state
> unchanged, NOT leaving extra objects allocated.
>
>           return err;
>    }

I don't want this because struct_mutex is held by caller already while 
the fini() will acquire it too.
> @@ -561,10 +562,12 @@ static void guc_fw_fetch(struct drm_device *dev,
> struct intel_guc_fw *guc_fw)
>    	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
>    		  guc_fw->guc_fw_path, err);
>
> +	mutex_lock(&dev->struct_mutex);
>    	obj = guc_fw->guc_fw_obj;
>    	if (obj)
>    		drm_gem_object_unreference(&obj->base);
>    	guc_fw->guc_fw_obj = NULL;
> +	mutex_unlock(&dev->struct_mutex);
>
> This is the locking that needs to be added to the failure path.
> This is required *in addition to* the locking reorganisation below.

I missed this part.
> > Signed-off-by: Alex Dai <yu.dai at intel.com>
> >
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index d20788f..70fa8f5 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -631,10 +631,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> >   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >
> > +	mutex_lock(&dev->struct_mutex);
> >   	direct_interrupts_to_host(dev_priv);
> > +	i915_guc_submission_disable(dev);
> >   	i915_guc_submission_fini(dev);
> >
> > -	mutex_lock(&dev->struct_mutex);
> >   	if (guc_fw->guc_fw_obj)
> >   		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
> >   	guc_fw->guc_fw_obj = NULL;
>
> This bit is fine, but incomplete without the other changes above.
>
> .Dave.