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

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

[Chris Wilson]

If i915_driver_load fails before i915_gem_load we try to unlink the ggtt
prior to initialising the list and triggering an oops.

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);
 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);
 	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
 out_regs:
-- 
1.8.5.1

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

[Chris Wilson]

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);
+	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
>  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);
-  	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Ben Widawsky]

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

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

[Chris Wilson]

On Fri, Jan 10, 2014 at 12:22:44PM -0800, Ben Widawsky wrote:
> 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()

It blew up, hence the amendment, iirc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre