| Message ID | 20191002143426.20123-1-martin.wilck@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixup "qla2xxx: Optimize NPIV tear down process" | expand |
On 10/2/19 7:35 AM, Martin Wilck wrote: > From: Martin Wilck <mwilck@suse.com> > > Hello Martin, > > this patch fixes two issues in patch 02/14 in Himanshu's latest > qla2xxx series ("qla2xxx: Bug fixes for the driver") from > Sept. 12th, which you applied onto 5.4/scsi-fixes already. > See https://marc.info/?l=linux-scsi&m=156951704106671&w=2 > > I'm assuming that Himanshu and Quinn are working on another > series of fixes, in which case that should take precedence > over this patch. I just wanted to provide this so that the > already known problems are fixed in your tree. > > Commit message follows: > > Fix two issues with the previously submitted patch > "qla2xxx: Optimize NPIV tear down process": a missing negation > in a wait_event_timeout() condition, and a missing loop end > condition. > --- > drivers/scsi/qla2xxx/qla_mid.c | 2 +- > drivers/scsi/qla2xxx/qla_os.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c > index 6afad68e5ba2..b4e042c1bd2a 100644 > --- a/drivers/scsi/qla2xxx/qla_mid.c > +++ b/drivers/scsi/qla2xxx/qla_mid.c > @@ -78,7 +78,7 @@ qla24xx_deallocate_vp_id(scsi_qla_host_t *vha) > */ > for (i = 0; i < 10 && atomic_read(&vha->vref_count); i++) > wait_event_timeout(vha->vref_waitq, > - atomic_read(&vha->vref_count), HZ); > + !atomic_read(&vha->vref_count), HZ); > > spin_lock_irqsave(&ha->vport_slock, flags); > if (atomic_read(&vha->vref_count)) { > diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c > index 6e627e521562..ee8167517621 100644 > --- a/drivers/scsi/qla2xxx/qla_os.c > +++ b/drivers/scsi/qla2xxx/qla_os.c > @@ -1119,7 +1119,7 @@ qla2x00_wait_for_sess_deletion(scsi_qla_host_t *vha) > > qla2x00_mark_all_devices_lost(vha, 0); > > - for (i = 0; i < 10; i++) > + for (i = 0; !test_fcport_count(vha) && i < 10; i++) > wait_event_timeout(vha->fcport_waitQ, test_fcport_count(vha), > HZ); Hi Martin, Both loops check the loop termination condition twice. Has it been considered to write these loops such that the loop termination condition is only tested once, e.g. using the following pattern? for (i = 0; i < 10; i++) if (wait_event_timeout(...) > 0) break; Thanks, Bart.
On Wed, 2019-10-02 at 08:17 -0700, Bart Van Assche wrote: > > Both loops check the loop termination condition twice. Has it been > considered to write these loops such that the loop termination > condition > is only tested once, e.g. using the following pattern? > > for (i = 0; i < 10; i++) > if (wait_event_timeout(...) > 0) > break; > Right, that's probably better. This was just meant as a minimal, temporary fix for the already applied patch. I expect Himanshu or Quinn to follow up anyway. I also still think that it'd be better to get the wake_up() calls right and not have to loop over wait_event_timeout() at all. Thanks, Martin
On 10/2/19 8:25 AM, Martin Wilck wrote: > On Wed, 2019-10-02 at 08:17 -0700, Bart Van Assche wrote: >> >> Both loops check the loop termination condition twice. Has it been >> considered to write these loops such that the loop termination >> condition >> is only tested once, e.g. using the following pattern? >> >> for (i = 0; i < 10; i++) >> if (wait_event_timeout(...) > 0) >> break; >> > > Right, that's probably better. This was just meant as a minimal, > temporary fix for the already applied patch. I expect Himanshu or Quinn > to follow up anyway. > > I also still think that it'd be better to get the wake_up() calls > right and not have to loop over wait_event_timeout() at all. Such a loop is most useful if some status information is reported during every iteration. An example from the SCSI target code: do { ret = wait_event_timeout(se_sess->cmd_list_wq, percpu_ref_is_zero(&se_sess->cmd_count), 180 * HZ); list_for_each_entry(cmd, &se_sess->sess_cmd_list, se_cmd_list) target_show_cmd("session shutdown: still waiting for ", cmd); } while (ret <= 0); Bart.
diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c index 6afad68e5ba2..b4e042c1bd2a 100644 --- a/drivers/scsi/qla2xxx/qla_mid.c +++ b/drivers/scsi/qla2xxx/qla_mid.c @@ -78,7 +78,7 @@ qla24xx_deallocate_vp_id(scsi_qla_host_t *vha) */ for (i = 0; i < 10 && atomic_read(&vha->vref_count); i++) wait_event_timeout(vha->vref_waitq, - atomic_read(&vha->vref_count), HZ); + !atomic_read(&vha->vref_count), HZ); spin_lock_irqsave(&ha->vport_slock, flags); if (atomic_read(&vha->vref_count)) { diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c index 6e627e521562..ee8167517621 100644 --- a/drivers/scsi/qla2xxx/qla_os.c +++ b/drivers/scsi/qla2xxx/qla_os.c @@ -1119,7 +1119,7 @@ qla2x00_wait_for_sess_deletion(scsi_qla_host_t *vha) qla2x00_mark_all_devices_lost(vha, 0); - for (i = 0; i < 10; i++) + for (i = 0; !test_fcport_count(vha) && i < 10; i++) wait_event_timeout(vha->fcport_waitQ, test_fcport_count(vha), HZ);
From: Martin Wilck <mwilck@suse.com> Hello Martin, this patch fixes two issues in patch 02/14 in Himanshu's latest qla2xxx series ("qla2xxx: Bug fixes for the driver") from Sept. 12th, which you applied onto 5.4/scsi-fixes already. See https://marc.info/?l=linux-scsi&m=156951704106671&w=2 I'm assuming that Himanshu and Quinn are working on another series of fixes, in which case that should take precedence over this patch. I just wanted to provide this so that the already known problems are fixed in your tree. Commit message follows: Fix two issues with the previously submitted patch "qla2xxx: Optimize NPIV tear down process": a missing negation in a wait_event_timeout() condition, and a missing loop end condition. --- drivers/scsi/qla2xxx/qla_mid.c | 2 +- drivers/scsi/qla2xxx/qla_os.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)