[PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt

[PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt

[Bastien Philbert]

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

Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt

[Chad Dupuis]

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

Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt

[Bastien Bastien Philbert]

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

Re: [PATCH] bnx2fc: Fix locking requirements in bnx2fc_init_tgt

[James Bottomley]

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