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.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +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)
