On Thu, 2018-09-06 at 09:03 +0200, Hannes Reinecke wrote:
> On 09/05/2018 04:09 PM, Martin Wilck wrote:
> > On Wed, 2018-09-05 at 15:53 +0200, Hannes Reinecke wrote:
> > > test_bit() is atomic, test_bit() || test_bit() is not.
> > > So protect consecutive bit tests with a lock to avoid races.
> > >
> > > Signed-off-by: Hannes Reinecke <hare@xxxxxxxx>
> > > ---
> > > drivers/scsi/qedf/qedf_els.c | 9 +++++++--
> > > drivers/scsi/qedf/qedf_main.c | 5 ++++-
> > > 2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/scsi/qedf/qedf_els.c
> > > b/drivers/scsi/qedf/qedf_els.c
> > > index 04f0c4d2e256..acb8157a96a8 100644
> > > --- a/drivers/scsi/qedf/qedf_els.c
> > > +++ b/drivers/scsi/qedf/qedf_els.c
> > > @@ -335,20 +335,25 @@ void qedf_restart_rport(struct qedf_rport
> > > *fcport)
> > > struct fc_lport *lport;
> > > struct fc_rport_priv *rdata;
> > > u32 port_id;
> > > + unsigned long flags;
> > >
> > > if (!fcport)
> > > return;
> > >
> > > + spin_lock_irqsave(&fcport->rport_lock, flags);
> > > if (test_bit(QEDF_RPORT_IN_RESET, &fcport->flags) ||
> > > !test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags) ||
> > > test_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags))
> >
> > Could you avoid the need for locking like this?
> >
> > unsigned long flgs = atomic_read(&fcport->flags);
> > if (flgs
> > & (QEDF_RPORT_IN_RESET|QEDF_RPORT_UPLOADING_CONNECTION) ||
> > !flgs & QEDF_RPORT_SESSION_READY) {
> >
>
> No.
> There still is a race condition between those two tests.
> QEDF_RPORT_UPLOADING_CONNECTION might be set just after the first
> test,
> rendering the entire statement invalid.
... and you need to sync with the set_bit(QEDF_RPORT_IN_RESET) later
on. I overlooked that, sorry. But then, there's at least one more place
where QEDF_RPORT_SESSION_READY is cleared without taking the lock
(qedf_cleanup_fcport()). Perhaps you need to protect that, too?
Thanks,
Martin
