Thanks Dan for pointing this code error. Zhenyu, your change looks fine to me. -----Original Message----- From: Zhenyu Wang [mailto:zhenyuw@xxxxxxxxxxxxxxx] Sent: Thursday, November 24, 2016 10:41 To: Dan Carpenter <dan.carpenter@xxxxxxxxxx> Cc: Zhang, Pei <pei.zhang@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; igvt-g-dev@xxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
