Re: [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Dec 16, 2013 at 06:58:20PM +0000, Chris Wilson wrote:
> On Mon, Dec 16, 2013 at 12:01:07PM +0000, Chris Wilson wrote:
> > If i915_driver_load fails before i915_gem_load we try to unlink the ggtt
> > prior to initialising the list and triggering an oops.
> 
> This is incomplete.
>  
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index df1410ecbb8b..6364c503f97d 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1683,11 +1683,11 @@ out_gem_unload:
> >  	intel_teardown_gmbus(dev);
> >  	intel_teardown_mchbar(dev);
> >  	destroy_workqueue(dev_priv->wq);
> > +	list_del(&dev_priv->gtt.base.global_link);

This is correct.

> +	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);

I'm not convinced here. It should be safe [ideal] to call cleanup after
we've setup the function pointer, which would be out_gtt; it's done in
i915_gem_gtt_init(). If it's not safe to call for some reason, we should
fix cleanup()

> >  out_mtrrfree:
> >  	arch_phys_wc_del(dev_priv->gtt.mtrr);
> >  	io_mapping_free(dev_priv->gtt.mappable);
> >  out_gtt:
> > -	list_del(&dev_priv->gtt.base.global_link);
> -  	drm_mm_takedown(&dev_priv->gtt.base.mm);

This is correct.

> -  	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center