[bug report] drm/i915/gvt: fix deadlock in workload_thread

[bug report] drm/i915/gvt: fix deadlock in workload_thread

[Dan Carpenter]

Hello Pei Zhang,

The patch 90d27a1b180e: "drm/i915/gvt: fix deadlock in
workload_thread" from Nov 14, 2016, leads to the following static
checker warning:

	drivers/gpu/drm/i915/gvt/scheduler.c:217 dispatch_workload()
	warn: inconsistent returns 'mutex:&dev_priv->drm.struct_mutex'.

drivers/gpu/drm/i915/gvt/scheduler.c
   161  static int dispatch_workload(struct intel_vgpu_workload *workload)
   162  {
   163          int ring_id = workload->ring_id;
   164          struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
   165          struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
   166          struct drm_i915_gem_request *rq;
   167          int ret;
   168  
   169          gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
   170                  ring_id, workload);
   171  
   172          shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
   173                                      GEN8_CTX_ADDRESSING_MODE_SHIFT;
   174  
   175          mutex_lock(&dev_priv->drm.struct_mutex);
   176  
   177          rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
   178          if (IS_ERR(rq)) {
   179                  gvt_err("fail to allocate gem request\n");
   180                  workload->status = PTR_ERR(rq);
   181                  return workload->status;

We're holding the lock here, which is obviously a bug.  But also should
we goto out?  I always thought that functions with an "out" label were
future proof?

   182          }
   183  
   184          gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
   185  
   186          workload->req = i915_gem_request_get(rq);
   187  
   188          ret = intel_gvt_scan_and_shadow_workload(workload);
   189          if (ret)
   190                  goto out;
   191  
   192          ret = intel_gvt_scan_and_shadow_wa_ctx(&workload->wa_ctx);
   193          if (ret)
   194                  goto out;
   195  

regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igvt-g-dev] [bug report] drm/i915/gvt: fix deadlock in workload_thread

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 3117 bytes --]

On 2016.11.24 01:17:06 +0300, Dan Carpenter wrote:
> Hello Pei Zhang,
> 
> The patch 90d27a1b180e: "drm/i915/gvt: fix deadlock in
> workload_thread" from Nov 14, 2016, leads to the following static
> checker warning:
> 
> 	drivers/gpu/drm/i915/gvt/scheduler.c:217 dispatch_workload()
> 	warn: inconsistent returns 'mutex:&dev_priv->drm.struct_mutex'.
> 
> drivers/gpu/drm/i915/gvt/scheduler.c
>    161  static int dispatch_workload(struct intel_vgpu_workload *workload)
>    162  {
>    163          int ring_id = workload->ring_id;
>    164          struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
>    165          struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
>    166          struct drm_i915_gem_request *rq;
>    167          int ret;
>    168  
>    169          gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
>    170                  ring_id, workload);
>    171  
>    172          shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
>    173                                      GEN8_CTX_ADDRESSING_MODE_SHIFT;
>    174  
>    175          mutex_lock(&dev_priv->drm.struct_mutex);
>    176  
>    177          rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
>    178          if (IS_ERR(rq)) {
>    179                  gvt_err("fail to allocate gem request\n");
>    180                  workload->status = PTR_ERR(rq);
>    181                  return workload->status;
> 
> We're holding the lock here, which is obviously a bug.  But also should
> we goto out?  I always thought that functions with an "out" label were
> future proof?
>

Thanks, Dan. Yes, missed alloc failure path. How about below one? Pei, is it fine for you?

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index f898df3..4db2422 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -177,8 +177,8 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
 	rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
 	if (IS_ERR(rq)) {
 		gvt_err("fail to allocate gem request\n");
-		workload->status = PTR_ERR(rq);
-		return workload->status;
+		ret = PTR_ERR(rq);
+		goto out;
 	}
 
 	gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq);
@@ -212,7 +212,8 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
 	if (ret)
 		workload->status = ret;
 
-	i915_add_request_no_flush(rq);
+	if (!IS_ERR_OR_NULL(rq))
+		i915_add_request_no_flush(rq);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 	return ret;
 }
@@ -460,7 +461,8 @@ static int workload_thread(void *priv)
 
 		complete_current_workload(gvt, ring_id);
 
-		i915_gem_request_put(fetch_and_zero(&workload->req));
+		if (workload->req)
+			i915_gem_request_put(fetch_and_zero(&workload->req));
 
 		if (need_force_wake)
 			intel_uncore_forcewake_put(gvt->dev_priv,


-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igvt-g-dev] [bug report] drm/i915/gvt: fix deadlock in workload_thread

[Zhang, Pei]

Thanks Dan for pointing this code error. Zhenyu, your change looks fine to me.

-----Original Message-----
From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com] 
Sent: Thursday, November 24, 2016 10:41
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Zhang, Pei <pei.zhang@intel.com>; intel-gfx@lists.freedesktop.org; igvt-g-dev@lists.01.org
Subject: Re: [igvt-g-dev] [bug report] drm/i915/gvt: fix deadlock in workload_thread

On 2016.11.24 01:17:06 +0300, Dan Carpenter wrote:
> Hello Pei Zhang,
> 
> The patch 90d27a1b180e: "drm/i915/gvt: fix deadlock in 
> workload_thread" from Nov 14, 2016, leads to the following static 
> checker warning:
> 
> 	drivers/gpu/drm/i915/gvt/scheduler.c:217 dispatch_workload()
> 	warn: inconsistent returns 'mutex:&dev_priv->drm.struct_mutex'.
> 
> drivers/gpu/drm/i915/gvt/scheduler.c
>    161  static int dispatch_workload(struct intel_vgpu_workload *workload)
>    162  {
>    163          int ring_id = workload->ring_id;
>    164          struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
>    165          struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
>    166          struct drm_i915_gem_request *rq;
>    167          int ret;
>    168  
>    169          gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
>    170                  ring_id, workload);
>    171  
>    172          shadow_ctx->desc_template = workload->ctx_desc.addressing_mode <<
>    173                                      GEN8_CTX_ADDRESSING_MODE_SHIFT;
>    174  
>    175          mutex_lock(&dev_priv->drm.struct_mutex);
>    176  
>    177          rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
>    178          if (IS_ERR(rq)) {
>    179                  gvt_err("fail to allocate gem request\n");
>    180                  workload->status = PTR_ERR(rq);
>    181                  return workload->status;
> 
> We're holding the lock here, which is obviously a bug.  But also 
> should we goto out?  I always thought that functions with an "out" 
> label were future proof?
>

Thanks, Dan. Yes, missed alloc failure path. How about below one? Pei, is it fine for you?

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index f898df3..4db2422 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -177,8 +177,8 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
 	rq = i915_gem_request_alloc(dev_priv->engine[ring_id], shadow_ctx);
 	if (IS_ERR(rq)) {
 		gvt_err("fail to allocate gem request\n");
-		workload->status = PTR_ERR(rq);
-		return workload->status;
+		ret = PTR_ERR(rq);
+		goto out;
 	}
 
 	gvt_dbg_sched("ring id %d get i915 gem request %p\n", ring_id, rq); @@ -212,7 +212,8 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
 	if (ret)
 		workload->status = ret;
 
-	i915_add_request_no_flush(rq);
+	if (!IS_ERR_OR_NULL(rq))
+		i915_add_request_no_flush(rq);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 	return ret;
 }
@@ -460,7 +461,8 @@ static int workload_thread(void *priv)
 
 		complete_current_workload(gvt, ring_id);
 
-		i915_gem_request_put(fetch_and_zero(&workload->req));
+		if (workload->req)
+			i915_gem_request_put(fetch_and_zero(&workload->req));
 
 		if (need_force_wake)
 			intel_uncore_forcewake_put(gvt->dev_priv,


--
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx