diff mbox series

[37/51] aha152x: look for stuck command when resetting device

Message ID 20210817091456.73342-38-hare@suse.de (mailing list archive)
State Changes Requested
Headers show
Series SCSI EH argument reshuffle part II | expand

Commit Message

Hannes Reinecke Aug. 17, 2021, 9:14 a.m. UTC
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(-)

Comments

Finn Thain Aug. 18, 2021, 12:43 a.m. UTC | #1
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 mbox series

Patch

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);