From patchwork Thu Nov 24 02:40:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zhenyu Wang X-Patchwork-Id: 9444713 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 73F9860235 for ; Thu, 24 Nov 2016 02:44:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 61FBB27CEA for ; Thu, 24 Nov 2016 02:44:38 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5416927DF9; Thu, 24 Nov 2016 02:44:38 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.2 required=2.0 tests=BAYES_00,BIGNUM_EMAILS, RCVD_IN_DNSWL_MED autolearn=no version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A639927CEA for ; Thu, 24 Nov 2016 02:44:37 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2170E6E9D1; Thu, 24 Nov 2016 02:44:37 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3FC7C6E9D1 for ; Thu, 24 Nov 2016 02:44:35 +0000 (UTC) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP; 23 Nov 2016 18:44:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,689,1473145200"; d="asc'?scan'208";a="33949730" Received: from zhen-hp.sh.intel.com (HELO zhen-hp) ([10.239.159.132]) by orsmga004.jf.intel.com with ESMTP; 23 Nov 2016 18:44:33 -0800 Date: Thu, 24 Nov 2016 10:40:34 +0800 From: Zhenyu Wang To: Dan Carpenter Message-ID: <20161124024034.nbuuv3fu4g32bpxg@zhen-hp.sh.intel.com> References: <20161123135504.GA3304@mwanda> MIME-Version: 1.0 In-Reply-To: <20161123135504.GA3304@mwanda> User-Agent: NeoMutt/20161014 (1.7.1) Cc: intel-gfx@lists.freedesktop.org, pei.zhang@intel.com, igvt-g-dev@lists.01.org Subject: Re: [Intel-gfx] [igvt-g-dev] [bug report] drm/i915/gvt: fix deadlock in workload_thread X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Zhenyu Wang Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP 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,