Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755228AbcDFBQm (ORCPT ); Tue, 5 Apr 2016 21:16:42 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:55864 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752206AbcDFBQk (ORCPT ); Tue, 5 Apr 2016 21:16:40 -0400 X-IBM-Helo: d03dlp01.boulder.ibm.com X-IBM-MailFrom: jejb@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org;linux-scsi@vger.kernel.org Message-ID: <1459905386.2315.69.camel@linux.vnet.ibm.com> Subject: Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt From: James Bottomley To: Bastien Bastien Philbert , Chad Dupuis Cc: QLogic-Storage-Upstream@qlogic.com, martin.petersen@oracle.com, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org Date: Tue, 05 Apr 2016 21:16:26 -0400 In-Reply-To: References: <1459867006-19605-1-git-send-email-bastienphilbert@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16040601-0017-0000-0000-00001F5617B4 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2060 Lines: 58 On Tue, 2016-04-05 at 13:50 -0400, Bastien Bastien Philbert wrote: > On Tue, Apr 5, 2016 at 1:43 PM, Chad Dupuis > wrote: > > > > > > On Tue, 5 Apr 2016, Bastien Philbert wrote: > > > > > This fixes the locking around the call to bnx2fc_alloc_id to > > > comply > > > with the comments about this particular function's definition > > > about > > > requiring the need to hold the hba mutex before and after calling > > > it. > > > Signed-off-by: Bastien Philbert > > > --- > > > drivers/scsi/bnx2fc/bnx2fc_tgt.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/scsi/bnx2fc/bnx2fc_tgt.c > > > b/drivers/scsi/bnx2fc/bnx2fc_tgt.c > > > index 08ec318..f2988cd 100644 > > > --- a/drivers/scsi/bnx2fc/bnx2fc_tgt.c > > > +++ b/drivers/scsi/bnx2fc/bnx2fc_tgt.c > > > @@ -368,9 +368,13 @@ static int bnx2fc_init_tgt(struct > > > bnx2fc_rport *tgt, > > > return -1; > > > } > > > > > > + mutex_lock(&hba->hba_mutex); > > > tgt->fcoe_conn_id = bnx2fc_alloc_conn_id(hba, tgt); > > > - if (tgt->fcoe_conn_id == -1) > > > + if (tgt->fcoe_conn_id == -1) { > > > + mutex_unlock(&hba->hba_mutex); > > > return -1; > > > + } > > > + mutex_unlock(&hba->hba_mutex); > > > > > > BNX2FC_TGT_DBG(tgt, "init_tgt - conn_id = 0x%x\n", > > > tgt->fcoe_conn_id); > > > > > > > > > > Taking the mutex here is not needed as it is already taken in > > bnx2fc_rport_event_handler() in the call chain > > bnx2fc_rport_event_handler->bnx2fc_offload_session->bnx2fc_init_tgt > > ->bnx2fc_alloc_conn_id > Ok thanks and good to known. Since the mutex was already taken, the patch you propose would produce an immediate deadlock if applied, meaning you didn't actually test it out. If you're going to propose patches like this, please understand the driver you're patching, *test* the patches on actual hardware and describe the problem you're solving in the changelog. Thanks, James