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 <[email protected]> >> --- >> 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.
Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
