| Message ID | 20210817091456.73342-38-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SCSI EH argument reshuffle part II | expand |
On Tue, 17 Aug 2021, Hannes Reinecke wrote: > From: Hannes Reinecke <hare@suse.com> > > The LLDD needs a command to send the reset with, so look at the > list of outstanding commands to get one. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/aha152x.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c > index 6fbdb6f3c996..3f96b38b7b56 100644 > --- a/drivers/scsi/aha152x.c > +++ b/drivers/scsi/aha152x.c > @@ -1045,24 +1045,28 @@ static int aha152x_abort(struct scsi_cmnd *SCpnt) > */ > static int aha152x_device_reset(struct scsi_cmnd * SCpnt) > { > - struct Scsi_Host *shpnt = SCpnt->device->host; > + struct scsi_device *sdev = SCpnt->device; > + struct Scsi_Host *shpnt = sdev->host; > DECLARE_COMPLETION(done); > int ret, issued, disconnected; > - unsigned char old_cmd_len = SCpnt->cmd_len; > + unsigned char old_cmd_len; > unsigned long flags; > unsigned long timeleft; > > - if(CURRENT_SC==SCpnt) { > - scmd_printk(KERN_ERR, SCpnt, "cannot reset current device\n"); > - return FAILED; > - } > - > DO_LOCK(flags); > - issued = remove_SC(&ISSUE_SC, SCpnt) == NULL; > - disconnected = issued && remove_SC(&DISCONNECTED_SC, SCpnt); > + /* Look for the stuck command */ > + SCpnt = remove_lun_SC(&ISSUE_SC, sdev->id, sdev->lun); > + if (SCpnt) > + issued = 1; > + else > + SCpnt = remove_lun_SC(&DISCONNECTED_SC, sdev->id, sdev->lun); > + if (!issued && SCpnt) > + disconnected = 1; It looks like 'issued' is left uninitialized in the !SCpnt case. Similar problem for 'disconnected'. Personally, I prefer the original style for being more readable i.e. unconditional assignments. Also, it looks like you've added some logic bugs. The values of 'disconnected' and 'issued' have been changed here such that commands never issued will now end up on the DISCONNECTED_SC list, and commands that were issued already will now end up on the ISSUE_SC list. > DO_UNLOCK(flags); > - > - SCpnt->cmd_len = 0; > + if (!SCpnt) > + return FAILED; If a command was removed from a list, it used to get re-added in the FAILED case (later on). If you 'return' here, that won't happen and EH escallation won't lead to free_hard_reset_SCs(). That looks like a memory leak. > + old_cmd_len = SCpnt->cmd_len; > + SCpnt->cmd_len = 0; > > aha152x_internal_queue(SCpnt, &done, resetting, reset_done); > >
diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c index 6fbdb6f3c996..3f96b38b7b56 100644 --- a/drivers/scsi/aha152x.c +++ b/drivers/scsi/aha152x.c @@ -1045,24 +1045,28 @@ static int aha152x_abort(struct scsi_cmnd *SCpnt) */ static int aha152x_device_reset(struct scsi_cmnd * SCpnt) { - struct Scsi_Host *shpnt = SCpnt->device->host; + struct scsi_device *sdev = SCpnt->device; + struct Scsi_Host *shpnt = sdev->host; DECLARE_COMPLETION(done); int ret, issued, disconnected; - unsigned char old_cmd_len = SCpnt->cmd_len; + unsigned char old_cmd_len; unsigned long flags; unsigned long timeleft; - if(CURRENT_SC==SCpnt) { - scmd_printk(KERN_ERR, SCpnt, "cannot reset current device\n"); - return FAILED; - } - DO_LOCK(flags); - issued = remove_SC(&ISSUE_SC, SCpnt) == NULL; - disconnected = issued && remove_SC(&DISCONNECTED_SC, SCpnt); + /* Look for the stuck command */ + SCpnt = remove_lun_SC(&ISSUE_SC, sdev->id, sdev->lun); + if (SCpnt) + issued = 1; + else + SCpnt = remove_lun_SC(&DISCONNECTED_SC, sdev->id, sdev->lun); + if (!issued && SCpnt) + disconnected = 1; DO_UNLOCK(flags); - - SCpnt->cmd_len = 0; + if (!SCpnt) + return FAILED; + old_cmd_len = SCpnt->cmd_len; + SCpnt->cmd_len = 0; aha152x_internal_queue(SCpnt, &done, resetting, reset_done);