* [bug report] drm/i915/gvt: fix deadlock in workload_thread
@ 2016-11-23 22:17 Dan Carpenter
2016-11-24 2:40 ` [igvt-g-dev] " Zhenyu Wang
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2016-11-23 22:17 UTC (permalink / raw)
To: pei.zhang; +Cc: intel-gfx, igvt-g-dev
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [igvt-g-dev] [bug report] drm/i915/gvt: fix deadlock in workload_thread 2016-11-23 22:17 [bug report] drm/i915/gvt: fix deadlock in workload_thread Dan Carpenter @ 2016-11-24 2:40 ` Zhenyu Wang 2016-11-24 7:09 ` Zhang, Pei 0 siblings, 1 reply; 3+ messages in thread From: Zhenyu Wang @ 2016-11-24 2:40 UTC (permalink / raw) To: Dan Carpenter; +Cc: intel-gfx, pei.zhang, igvt-g-dev [-- 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [igvt-g-dev] [bug report] drm/i915/gvt: fix deadlock in workload_thread 2016-11-24 2:40 ` [igvt-g-dev] " Zhenyu Wang @ 2016-11-24 7:09 ` Zhang, Pei 0 siblings, 0 replies; 3+ messages in thread From: Zhang, Pei @ 2016-11-24 7:09 UTC (permalink / raw) To: Zhenyu Wang, Dan Carpenter Cc: intel-gfx@lists.freedesktop.org, igvt-g-dev@lists.01.org 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-24 7:10 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-23 22:17 [bug report] drm/i915/gvt: fix deadlock in workload_thread Dan Carpenter 2016-11-24 2:40 ` [igvt-g-dev] " Zhenyu Wang 2016-11-24 7:09 ` Zhang, Pei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox