| Message ID | 20220512111236.109851-9-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: EH rework prep patches, part 1 | expand |
This patch looks like it will call ibmvfc_reset_device() w/IBMVFC_TARGET_RESET before it has called ibmvfc_cancel_all() on all the devices, the existing code calls ibmvfc_reset_device() w/IBMVFC_TARGET_RESET after the iterator. Since you have the starget, why change to use shost_for_each_device() and check the sdev->channel and sdev->id ? That's what starget_for_each_device() does. -Ewan On Thu, May 12, 2022 at 7:13 AM Hannes Reinecke <hare@suse.de> wrote: > > From: Hannes Reinecke <hare@suse.com> > > For target reset we need a device to send the target reset to, > so open-code the loop in target reset to send the target reset TMF > to the correct device. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Cc: Tyrel Datwyler <tyreld@linux.ibm.com> > --- > drivers/scsi/ibmvscsi/ibmvfc.c | 42 +++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 19 deletions(-) > > diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c > index d0eab5700dc5..721d965f4b0e 100644 > --- a/drivers/scsi/ibmvscsi/ibmvfc.c > +++ b/drivers/scsi/ibmvscsi/ibmvfc.c > @@ -2925,18 +2925,6 @@ static void ibmvfc_dev_cancel_all_noreset(struct scsi_device *sdev, void *data) > *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_SUPPRESS_ABTS); > } > > -/** > - * ibmvfc_dev_cancel_all_reset - Device iterated cancel all function > - * @sdev: scsi device struct > - * @data: return code > - * > - **/ > -static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data) > -{ > - unsigned long *rc = data; > - *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_TGT_RESET); > -} > - > /** > * ibmvfc_eh_target_reset_handler - Reset the target > * @cmd: scsi command struct > @@ -2946,22 +2934,38 @@ static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data) > **/ > static int ibmvfc_eh_target_reset_handler(struct scsi_cmnd *cmd) > { > - struct scsi_device *sdev = cmd->device; > - struct ibmvfc_host *vhost = shost_priv(sdev->host); > - struct scsi_target *starget = scsi_target(sdev); > + struct scsi_target *starget = scsi_target(cmd->device); > + struct fc_rport *rport = starget_to_rport(starget); > + struct Scsi_Host *shost = rport_to_shost(rport); > + struct ibmvfc_host *vhost = shost_priv(shost); > int block_rc; > int reset_rc = 0; > int rc = FAILED; > unsigned long cancel_rc = 0; > + bool tgt_reset = false; > > ENTER; > - block_rc = fc_block_scsi_eh(cmd); > + block_rc = fc_block_rport(rport); > ibmvfc_wait_while_resetting(vhost); > if (block_rc != FAST_IO_FAIL) { > - starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_reset); > - reset_rc = ibmvfc_reset_device(sdev, IBMVFC_TARGET_RESET, "target"); > + struct scsi_device *sdev; > + > + shost_for_each_device(sdev, shost) { > + if ((sdev->channel != starget->channel) || > + (sdev->id != starget->id)) > + continue; > + > + cancel_rc |= ibmvfc_cancel_all(sdev, > + IBMVFC_TMF_TGT_RESET); > + if (!tgt_reset) { > + reset_rc = ibmvfc_reset_device(sdev, > + IBMVFC_TARGET_RESET, "target"); > + tgt_reset = true; > + } > + } > } else > - starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_noreset); > + starget_for_each_device(starget, &cancel_rc, > + ibmvfc_dev_cancel_all_noreset); > > if (!cancel_rc && !reset_rc) > rc = ibmvfc_wait_for_ops(vhost, starget, ibmvfc_match_target); > -- > 2.29.2 >
On 5/19/22 13:05, Ewan Milne wrote: > This patch looks like it will call ibmvfc_reset_device() w/IBMVFC_TARGET_RESET > before it has called ibmvfc_cancel_all() on all the devices, the > existing code calls > ibmvfc_reset_device() w/IBMVFC_TARGET_RESET after the iterator. > > Since you have the starget, why change to use shost_for_each_device() > and check the > sdev->channel and sdev->id ? That's what starget_for_each_device() does. > > -Ewan > > On Thu, May 12, 2022 at 7:13 AM Hannes Reinecke <hare@suse.de> wrote: >> >> From: Hannes Reinecke <hare@suse.com> >> >> For target reset we need a device to send the target reset to, >> so open-code the loop in target reset to send the target reset TMF >> to the correct device. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> Cc: Tyrel Datwyler <tyreld@linux.ibm.com> >> --- >> drivers/scsi/ibmvscsi/ibmvfc.c | 42 +++++++++++++++++++--------------- >> 1 file changed, 23 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c >> index d0eab5700dc5..721d965f4b0e 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvfc.c >> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c >> @@ -2925,18 +2925,6 @@ static void ibmvfc_dev_cancel_all_noreset(struct scsi_device *sdev, void *data) >> *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_SUPPRESS_ABTS); >> } >> >> -/** >> - * ibmvfc_dev_cancel_all_reset - Device iterated cancel all function >> - * @sdev: scsi device struct >> - * @data: return code >> - * >> - **/ >> -static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data) >> -{ >> - unsigned long *rc = data; >> - *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_TGT_RESET); >> -} >> - >> /** >> * ibmvfc_eh_target_reset_handler - Reset the target >> * @cmd: scsi command struct >> @@ -2946,22 +2934,38 @@ static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data) >> **/ >> static int ibmvfc_eh_target_reset_handler(struct scsi_cmnd *cmd) >> { >> - struct scsi_device *sdev = cmd->device; >> - struct ibmvfc_host *vhost = shost_priv(sdev->host); >> - struct scsi_target *starget = scsi_target(sdev); >> + struct scsi_target *starget = scsi_target(cmd->device); >> + struct fc_rport *rport = starget_to_rport(starget); >> + struct Scsi_Host *shost = rport_to_shost(rport); >> + struct ibmvfc_host *vhost = shost_priv(shost); >> int block_rc; >> int reset_rc = 0; >> int rc = FAILED; >> unsigned long cancel_rc = 0; >> + bool tgt_reset = false; >> >> ENTER; >> - block_rc = fc_block_scsi_eh(cmd); >> + block_rc = fc_block_rport(rport); >> ibmvfc_wait_while_resetting(vhost); >> if (block_rc != FAST_IO_FAIL) { >> - starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_reset); >> - reset_rc = ibmvfc_reset_device(sdev, IBMVFC_TARGET_RESET, "target"); >> + struct scsi_device *sdev; >> + >> + shost_for_each_device(sdev, shost) { >> + if ((sdev->channel != starget->channel) || >> + (sdev->id != starget->id)) >> + continue; >> + >> + cancel_rc |= ibmvfc_cancel_all(sdev, >> + IBMVFC_TMF_TGT_RESET); >> + if (!tgt_reset) { >> + reset_rc = ibmvfc_reset_device(sdev, >> + IBMVFC_TARGET_RESET, "target"); >> + tgt_reset = true; >> + } >> + } >> } else >> - starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_noreset); >> + starget_for_each_device(starget, &cancel_rc, >> + ibmvfc_dev_cancel_all_noreset); >> >> if (!cancel_rc && !reset_rc) >> rc = ibmvfc_wait_for_ops(vhost, starget, ibmvfc_match_target); >> -- >> 2.29.2 >> > You are right; the call to ibmvfc_reset_device() needs to be moved out of the loop. Cheers, Hannes
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c index d0eab5700dc5..721d965f4b0e 100644 --- a/drivers/scsi/ibmvscsi/ibmvfc.c +++ b/drivers/scsi/ibmvscsi/ibmvfc.c @@ -2925,18 +2925,6 @@ static void ibmvfc_dev_cancel_all_noreset(struct scsi_device *sdev, void *data) *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_SUPPRESS_ABTS); } -/** - * ibmvfc_dev_cancel_all_reset - Device iterated cancel all function - * @sdev: scsi device struct - * @data: return code - * - **/ -static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data) -{ - unsigned long *rc = data; - *rc |= ibmvfc_cancel_all(sdev, IBMVFC_TMF_TGT_RESET); -} - /** * ibmvfc_eh_target_reset_handler - Reset the target * @cmd: scsi command struct @@ -2946,22 +2934,38 @@ static void ibmvfc_dev_cancel_all_reset(struct scsi_device *sdev, void *data) **/ static int ibmvfc_eh_target_reset_handler(struct scsi_cmnd *cmd) { - struct scsi_device *sdev = cmd->device; - struct ibmvfc_host *vhost = shost_priv(sdev->host); - struct scsi_target *starget = scsi_target(sdev); + struct scsi_target *starget = scsi_target(cmd->device); + struct fc_rport *rport = starget_to_rport(starget); + struct Scsi_Host *shost = rport_to_shost(rport); + struct ibmvfc_host *vhost = shost_priv(shost); int block_rc; int reset_rc = 0; int rc = FAILED; unsigned long cancel_rc = 0; + bool tgt_reset = false; ENTER; - block_rc = fc_block_scsi_eh(cmd); + block_rc = fc_block_rport(rport); ibmvfc_wait_while_resetting(vhost); if (block_rc != FAST_IO_FAIL) { - starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_reset); - reset_rc = ibmvfc_reset_device(sdev, IBMVFC_TARGET_RESET, "target"); + struct scsi_device *sdev; + + shost_for_each_device(sdev, shost) { + if ((sdev->channel != starget->channel) || + (sdev->id != starget->id)) + continue; + + cancel_rc |= ibmvfc_cancel_all(sdev, + IBMVFC_TMF_TGT_RESET); + if (!tgt_reset) { + reset_rc = ibmvfc_reset_device(sdev, + IBMVFC_TARGET_RESET, "target"); + tgt_reset = true; + } + } } else - starget_for_each_device(starget, &cancel_rc, ibmvfc_dev_cancel_all_noreset); + starget_for_each_device(starget, &cancel_rc, + ibmvfc_dev_cancel_all_noreset); if (!cancel_rc && !reset_rc) rc = ibmvfc_wait_for_ops(vhost, starget, ibmvfc_match_target);