[PATCH -next] drm/i915: Fix missing unlock on error in i915_gem_init_hw()

[PATCH -next] drm/i915: Fix missing unlock on error in i915_gem_init_hw()

[weiyj_lk]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

Add the missing unlock before return from function i915_gem_init_hw()
in the error handling case.

Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d2ba315..3eeb2d0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4879,8 +4879,10 @@ i915_gem_init_hw(struct drm_device *dev)
 	i915_gem_init_swizzling(dev);
 
 	ret = dev_priv->gt.init_rings(dev);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&dev->struct_mutex);
 		return ret;
+	}
 
 	for (i = 0; i < NUM_L3_SLICES(dev); i++)
 		i915_gem_l3_remap(&dev_priv->ring[RCS], i);

Re: [PATCH -next] drm/i915: Fix missing unlock on error in i915_gem_init_hw()

[Jeremiah Mahler]

Wei Yongjun,

On Fri, Dec 05, 2014 at 08:55:59AM +0800, weiyj_lk@163.com wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> Add the missing unlock before return from function i915_gem_init_hw()
> in the error handling case.
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d2ba315..3eeb2d0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4879,8 +4879,10 @@ i915_gem_init_hw(struct drm_device *dev)
>  	i915_gem_init_swizzling(dev);
>  
>  	ret = dev_priv->gt.init_rings(dev);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&dev->struct_mutex);
>  		return ret;
> +	}
>  

There are other places in i915_gem_init_hw() where it returns without
unlocking the mutex.  Why is it only necessary here and not any of the
other places?

>  	for (i = 0; i < NUM_L3_SLICES(dev); i++)
>  		i915_gem_l3_remap(&dev_priv->ring[RCS], i);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
- Jeremiah Mahler

Re: drm/i915: Fix missing unlock on error in i915_gem_init_hw()

[Jani Nikula]

On Fri, 05 Dec 2014, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> There are other places in i915_gem_init_hw() where it returns without
> unlocking the mutex.  Why is it only necessary here and not any of the
> other places?

There's probably some rebase/merge confusion going on. The following
patch is against drm-intel-next-queued where the problem is
present. Thanks for reporting this.

BR,
Jani.


Jani Nikula (1):
  drm/i915: release struct_mutex on the i915_gem_init_hw fail path

 drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
2.1.3

Re: drm/i915: Fix missing unlock on error in i915_gem_init_hw()

[Jani Nikula]

On Fri, 05 Dec 2014, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> There are other places in i915_gem_init_hw() where it returns without
> unlocking the mutex.  Why is it only necessary here and not any of the
> other places?

There's probably some rebase/merge confusion going on. The following
patch is against drm-intel-next-queued where the problem is
present. Thanks for reporting this.

BR,
Jani.


Jani Nikula (1):
  drm/i915: release struct_mutex on the i915_gem_init_hw fail path

 drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
2.1.3

[PATCH] drm/i915: release struct_mutex on the i915_gem_init_hw fail path

[Jani Nikula]

Release struct_mutex if init_rings() fails.

This is a regression introduced in
commit 35a57ffbb10840af219eeaf64718434242bb7c76
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Nov 20 00:33:07 2014 +0100

    drm/i915: Only init engines once

Reported-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c1c11418231b..e3ce4bef22a3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4860,22 +4860,18 @@ int i915_gem_init(struct drm_device *dev)
 	}
 
 	ret = i915_gem_init_userptr(dev);
-	if (ret) {
-		mutex_unlock(&dev->struct_mutex);
-		return ret;
-	}
+	if (ret)
+		goto out_unlock;
 
 	i915_gem_init_global_gtt(dev);
 
 	ret = i915_gem_context_init(dev);
-	if (ret) {
-		mutex_unlock(&dev->struct_mutex);
-		return ret;
-	}
+	if (ret)
+		goto out_unlock;
 
 	ret = dev_priv->gt.init_rings(dev);
 	if (ret)
-		return ret;
+		goto out_unlock;
 
 	ret = i915_gem_init_hw(dev);
 	if (ret == -EIO) {
@@ -4887,6 +4883,8 @@ int i915_gem_init(struct drm_device *dev)
 		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
 		ret = 0;
 	}
+
+out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
-- 
2.1.3

Re: [PATCH] drm/i915: release struct_mutex on the i915_gem_init_hw fail path

[Jeremiah Mahler]

Jani,

On Fri, Dec 05, 2014 at 02:17:42PM +0200, Jani Nikula wrote:
> Release struct_mutex if init_rings() fails.
> 
> This is a regression introduced in
> commit 35a57ffbb10840af219eeaf64718434242bb7c76
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Nov 20 00:33:07 2014 +0100
> 
>     drm/i915: Only init engines once
> 
> Reported-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c1c11418231b..e3ce4bef22a3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4860,22 +4860,18 @@ int i915_gem_init(struct drm_device *dev)
>  	}
>  
>  	ret = i915_gem_init_userptr(dev);
> -	if (ret) {
> -		mutex_unlock(&dev->struct_mutex);
> -		return ret;
> -	}
> +	if (ret)
> +		goto out_unlock;
>  
>  	i915_gem_init_global_gtt(dev);
>  
>  	ret = i915_gem_context_init(dev);
> -	if (ret) {
> -		mutex_unlock(&dev->struct_mutex);
> -		return ret;
> -	}
> +	if (ret)
> +		goto out_unlock;
>  
>  	ret = dev_priv->gt.init_rings(dev);
>  	if (ret)
> -		return ret;
> +		goto out_unlock;
>  
>  	ret = i915_gem_init_hw(dev);
>  	if (ret == -EIO) {
> @@ -4887,6 +4883,8 @@ int i915_gem_init(struct drm_device *dev)
>  		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
>  		ret = 0;
>  	}
> +
> +out_unlock:
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	return ret;
> -- 
> 2.1.3
> 

Yes, this looks much better.

-- 
- Jeremiah Mahler

Re: [Intel-gfx] [PATCH -next] drm/i915: Fix missing unlock on error in i915_gem_init_hw()

[Daniel Vetter]

On Fri, Dec 05, 2014 at 08:55:59AM +0800, weiyj_lk@163.com wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> Add the missing unlock before return from function i915_gem_init_hw()
> in the error handling case.
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

Applied, thanks for the patch. Two minor comments:
- Please mention the commit that introduced the issue next time around.
  I've added that while applying.

- The usual patter is

	if (ret)
		goto out;


	/* more code */

out:
	mutex_unlock();
	return ret;

This would work really well in i915_gem_init_hw and besides the
code-cleanup also prevents such a fumble in the future. If you feel like
please submit that patch to convert init_hw to this shared unlock code
pattern, too.

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d2ba315..3eeb2d0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4879,8 +4879,10 @@ i915_gem_init_hw(struct drm_device *dev)
>  	i915_gem_init_swizzling(dev);
>  
>  	ret = dev_priv->gt.init_rings(dev);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&dev->struct_mutex);
>  		return ret;
> +	}
>  
>  	for (i = 0; i < NUM_L3_SLICES(dev); i++)
>  		i915_gem_l3_remap(&dev_priv->ring[RCS], i);
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: release struct_mutex on the i915_gem_init_hw fail path

[Daniel Vetter]

On Fri, Dec 05, 2014 at 05:03:14AM -0800, Jeremiah Mahler wrote:
> Jani,
> 
> On Fri, Dec 05, 2014 at 02:17:42PM +0200, Jani Nikula wrote:
> > Release struct_mutex if init_rings() fails.
> > 
> > This is a regression introduced in
> > commit 35a57ffbb10840af219eeaf64718434242bb7c76
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Thu Nov 20 00:33:07 2014 +0100
> > 
> >     drm/i915: Only init engines once
> > 
> > Reported-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c1c11418231b..e3ce4bef22a3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4860,22 +4860,18 @@ int i915_gem_init(struct drm_device *dev)
> >  	}
> >  
> >  	ret = i915_gem_init_userptr(dev);
> > -	if (ret) {
> > -		mutex_unlock(&dev->struct_mutex);
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		goto out_unlock;
> >  
> >  	i915_gem_init_global_gtt(dev);
> >  
> >  	ret = i915_gem_context_init(dev);
> > -	if (ret) {
> > -		mutex_unlock(&dev->struct_mutex);
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		goto out_unlock;
> >  
> >  	ret = dev_priv->gt.init_rings(dev);
> >  	if (ret)
> > -		return ret;
> > +		goto out_unlock;
> >  
> >  	ret = i915_gem_init_hw(dev);
> >  	if (ret == -EIO) {
> > @@ -4887,6 +4883,8 @@ int i915_gem_init(struct drm_device *dev)
> >  		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
> >  		ret = 0;
> >  	}
> > +
> > +out_unlock:
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> >  	return ret;
> > -- 
> > 2.1.3
> > 
> 
> Yes, this looks much better.

Oh well confusion in my mailbox. I've dropped Wei's patch and merged this
one instead.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch