* [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt @ 2016-04-05 14:36 Bastien Philbert 2016-04-05 17:43 ` Chad Dupuis 0 siblings, 1 reply; 4+ messages in thread From: Bastien Philbert @ 2016-04-05 14:36 UTC (permalink / raw) To: QLogic-Storage-Upstream; +Cc: jejb, martin.petersen, linux-scsi, linux-kernel 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 <bastienphilbert@gmail.com> --- 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); -- 2.5.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt 2016-04-05 14:36 [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt Bastien Philbert @ 2016-04-05 17:43 ` Chad Dupuis 2016-04-05 17:50 ` Bastien Bastien Philbert 0 siblings, 1 reply; 4+ messages in thread From: Chad Dupuis @ 2016-04-05 17:43 UTC (permalink / raw) To: Bastien Philbert Cc: QLogic-Storage-Upstream, jejb, martin.petersen, linux-scsi, linux-kernel 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 <bastienphilbert@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt 2016-04-05 17:43 ` Chad Dupuis @ 2016-04-05 17:50 ` Bastien Bastien Philbert 2016-04-06 1:16 ` James Bottomley 0 siblings, 1 reply; 4+ messages in thread From: Bastien Bastien Philbert @ 2016-04-05 17:50 UTC (permalink / raw) To: Chad Dupuis Cc: QLogic-Storage-Upstream, jejb, martin.petersen, linux-scsi, linux-kernel On Tue, Apr 5, 2016 at 1:43 PM, Chad Dupuis <chad.dupuis@qlogic.com> 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 <bastienphilbert@gmail.com> >> --- >> 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. Bastien ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt 2016-04-05 17:50 ` Bastien Bastien Philbert @ 2016-04-06 1:16 ` James Bottomley 0 siblings, 0 replies; 4+ messages in thread From: James Bottomley @ 2016-04-06 1:16 UTC (permalink / raw) To: Bastien Bastien Philbert, Chad Dupuis Cc: QLogic-Storage-Upstream, martin.petersen, linux-scsi, linux-kernel On Tue, 2016-04-05 at 13:50 -0400, Bastien Bastien Philbert wrote: > On Tue, Apr 5, 2016 at 1:43 PM, Chad Dupuis <chad.dupuis@qlogic.com> > 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 <bastienphilbert@gmail.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-04-06 1:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-05 14:36 [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt Bastien Philbert 2016-04-05 17:43 ` Chad Dupuis 2016-04-05 17:50 ` Bastien Bastien Philbert 2016-04-06 1:16 ` James Bottomley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox