diff mbox series

[7/7] scsi_error: streamline scsi_eh_bus_device_reset()

Message ID 20231002155915.109359-8-hare@suse.de
State New
Headers show
Series scsi: EH rework, main part | expand

Commit Message

Hannes Reinecke Oct. 2, 2023, 3:59 p.m. UTC
Streamline to use a similar code flow as the other reset functions.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_error.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

Comments

Bart Van Assche Oct. 3, 2023, 5:45 p.m. UTC | #1
On 10/2/23 08:59, Hannes Reinecke wrote:
> Streamline to use a similar code flow as the other reset functions.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/scsi_error.c | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 21d84940c9cb..81b38f5da3b6 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
>   {
>   	struct scsi_cmnd *scmd, *bdr_scmd, *next;
>   	struct scsi_device *sdev;
> +	LIST_HEAD(check_list);
>   	enum scsi_disposition rtn;
>   
>   	shost_for_each_device(sdev, shost) {
> @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
>   			sdev_printk(KERN_INFO, sdev,
>   				     "%s: Sending BDR\n", current->comm));
>   		rtn = scsi_try_bus_device_reset(sdev);
> -		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
> -			if (!scsi_device_online(sdev) ||
> -			    rtn == FAST_IO_FAIL ||
> -			    !scsi_eh_tur(bdr_scmd)) {
> -				list_for_each_entry_safe(scmd, next,
> -							 work_q, eh_entry) {
> -					if (scmd->device == sdev &&
> -					    scsi_eh_action(scmd, rtn) != FAILED)
> -						__scsi_eh_finish_cmd(scmd,
> -								     done_q,
> -								     DID_RESET);
> -				}
> -			}
> -		} else {
> +		if (rtn != SUCCESS && rtn != FAST_IO_FAIL)
>   			SCSI_LOG_ERROR_RECOVERY(3,
>   				sdev_printk(KERN_INFO, sdev,
>   					    "%s: BDR failed\n", current->comm));
> +		list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
> +			if (scmd->device != sdev)
> +				continue;
> +			if (rtn == SUCCESS)
> +				list_move_tail(&scmd->eh_entry, &check_list);
> +			else if (rtn == FAST_IO_FAIL)
> +				__scsi_eh_finish_cmd(scmd, done_q,
> +						     DID_TRANSPORT_DISRUPTED);
>   		}
>   	}
>   
> -	return list_empty(work_q);
> +	return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
>   }

I think the description of this patch is too brief. The following 
information is missing from the patch description:
- Why the scsi_device_online() and scsi_eh_tur() checks have been left
   out.
- Why DID_RESET has been changed into DID_TRANSPORT_DISRUPTED.
- Why the list_move_tail() call has been introduced.
- Why the scsi_eh_test_devices() call has been introduced.

Thanks,

Bart.
Hannes Reinecke Oct. 4, 2023, 6:50 a.m. UTC | #2
On 10/3/23 19:45, Bart Van Assche wrote:
> On 10/2/23 08:59, Hannes Reinecke wrote:
>> Streamline to use a similar code flow as the other reset functions.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/scsi_error.c | 26 +++++++++++---------------
>>   1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 21d84940c9cb..81b38f5da3b6 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct 
>> Scsi_Host *shost,
>>   {
>>       struct scsi_cmnd *scmd, *bdr_scmd, *next;
>>       struct scsi_device *sdev;
>> +    LIST_HEAD(check_list);
>>       enum scsi_disposition rtn;
>>       shost_for_each_device(sdev, shost) {
>> @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct 
>> Scsi_Host *shost,
>>               sdev_printk(KERN_INFO, sdev,
>>                        "%s: Sending BDR\n", current->comm));
>>           rtn = scsi_try_bus_device_reset(sdev);
>> -        if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
>> -            if (!scsi_device_online(sdev) ||
>> -                rtn == FAST_IO_FAIL ||
>> -                !scsi_eh_tur(bdr_scmd)) {
>> -                list_for_each_entry_safe(scmd, next,
>> -                             work_q, eh_entry) {
>> -                    if (scmd->device == sdev &&
>> -                        scsi_eh_action(scmd, rtn) != FAILED)
>> -                        __scsi_eh_finish_cmd(scmd,
>> -                                     done_q,
>> -                                     DID_RESET);
>> -                }
>> -            }
>> -        } else {
>> +        if (rtn != SUCCESS && rtn != FAST_IO_FAIL)
>>               SCSI_LOG_ERROR_RECOVERY(3,
>>                   sdev_printk(KERN_INFO, sdev,
>>                           "%s: BDR failed\n", current->comm));
>> +        list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
>> +            if (scmd->device != sdev)
>> +                continue;
>> +            if (rtn == SUCCESS)
>> +                list_move_tail(&scmd->eh_entry, &check_list);
>> +            else if (rtn == FAST_IO_FAIL)
>> +                __scsi_eh_finish_cmd(scmd, done_q,
>> +                             DID_TRANSPORT_DISRUPTED);
>>           }
>>       }
>> -    return list_empty(work_q);
>> +    return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
>>   }
> 
> I think the description of this patch is too brief. The following 
> information is missing from the patch description:
> - Why the scsi_device_online() and scsi_eh_tur() checks have been left
>    out.
> - Why DID_RESET has been changed into DID_TRANSPORT_DISRUPTED.
> - Why the list_move_tail() call has been introduced.
> - Why the scsi_eh_test_devices() call has been introduced.
>
Okay, will be doing so.

Cheers,

Hannes
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 21d84940c9cb..81b38f5da3b6 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1581,6 +1581,7 @@  static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
 {
 	struct scsi_cmnd *scmd, *bdr_scmd, *next;
 	struct scsi_device *sdev;
+	LIST_HEAD(check_list);
 	enum scsi_disposition rtn;
 
 	shost_for_each_device(sdev, shost) {
@@ -1606,27 +1607,22 @@  static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
 			sdev_printk(KERN_INFO, sdev,
 				     "%s: Sending BDR\n", current->comm));
 		rtn = scsi_try_bus_device_reset(sdev);
-		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
-			if (!scsi_device_online(sdev) ||
-			    rtn == FAST_IO_FAIL ||
-			    !scsi_eh_tur(bdr_scmd)) {
-				list_for_each_entry_safe(scmd, next,
-							 work_q, eh_entry) {
-					if (scmd->device == sdev &&
-					    scsi_eh_action(scmd, rtn) != FAILED)
-						__scsi_eh_finish_cmd(scmd,
-								     done_q,
-								     DID_RESET);
-				}
-			}
-		} else {
+		if (rtn != SUCCESS && rtn != FAST_IO_FAIL)
 			SCSI_LOG_ERROR_RECOVERY(3,
 				sdev_printk(KERN_INFO, sdev,
 					    "%s: BDR failed\n", current->comm));
+		list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
+			if (scmd->device != sdev)
+				continue;
+			if (rtn == SUCCESS)
+				list_move_tail(&scmd->eh_entry, &check_list);
+			else if (rtn == FAST_IO_FAIL)
+				__scsi_eh_finish_cmd(scmd, done_q,
+						     DID_TRANSPORT_DISRUPTED);
 		}
 	}
 
-	return list_empty(work_q);
+	return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
 }
 
 /**