Also, the patch fails to do what it's message describes, i.e. the calls _bnx2fc_enable() and _bnx2fc_disable() are outside the lock/unlock.
-----Original Message----- From: Chad Dupuis Sent: Tuesday, December 15, 2015 8:43 AM To: Nicholas Krause <[email protected]> Cc: Dept-Eng QLogic Storage Upstream <[email protected]>; [email protected]; [email protected]; linux-scsi <[email protected]>; linux-kernel <[email protected]> Subject: Re: [PATCH] bnx2fc:Add proper locking protection in bnx2fc_ctrlr_enabled On Sat, 12 Dec 2015, Nicholas Krause wrote: > This adds proper locking protection in bnx2fc_ctrl_enabled around the > calls to the functions, _bnx2fc_enable and _bnx2fc_disable in order to > avoid concurrent access on these functions accessing global referenced > data structures in their internal intended work. > > Signed-off-by: Nicholas Krause <[email protected]> > --- > drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c > b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c > index 67405c6..e43648f 100644 > --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c > +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c > @@ -2177,13 +2177,21 @@ static int bnx2fc_ctlr_enabled(struct > fcoe_ctlr_device *cdev) { > struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev); > > + rtnl_lock(); > + mutex_lock(&bnx2fc_dev_lock); > switch (cdev->enabled) { > case FCOE_CTLR_ENABLED: > + rtnl_unlock(); > + mutex_unlock(&bnx2fc_dev_lock); > return __bnx2fc_enable(ctlr); > case FCOE_CTLR_DISABLED: > + rtnl_unlock(); > + mutex_unlock(&bnx2fc_dev_lock); > return __bnx2fc_disable(ctlr); > case FCOE_CTLR_UNUSED: > default: > + rtnl_unlock(); > + mutex_unlock(&bnx2fc_dev_lock); > return -ENOTSUPP; > }; > } > Nack. All we end up protecting is the check of cdev->enabled and I do not believe taking two mutexes is required for that.
<<attachment: winmail.dat>>
