diff mbox series

[v10,28/33] scsi: ufs: Have scsi-ml retry start stop errors

Message ID 20230714213419.95492-29-michael.christie@oracle.com (mailing list archive)
State Changes Requested
Headers show
Series scsi: Allow scsi_execute users to control retries | expand

Commit Message

Mike Christie July 14, 2023, 9:34 p.m. UTC 
This has scsi-ml retry errors instead of driving them itself.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ufs/core/ufshcd.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Bart Van Assche July 17, 2023, 5:05 p.m. UTC | #1 
On 7/14/23 14:34, Mike Christie wrote:
> This has scsi-ml retry errors instead of driving them itself.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/ufs/core/ufshcd.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 983fae84d9e8..267c24c57396 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9291,7 +9291,14 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
>   				     struct scsi_sense_hdr *sshdr)
>   {
>   	const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
> +	struct scsi_failure failures[] = {
> +		{
> +			.allowed = 2,
> +			.result = SCMD_FAILURE_RESULT_ANY,
> +		},
> +	};
>   	const struct scsi_exec_args args = {
> +		.failures = failures,
>   		.sshdr = sshdr,
>   		.req_flags = BLK_MQ_REQ_PM,
>   		.scmd_flags = SCMD_FAIL_IF_RECOVERING,
> @@ -9317,7 +9324,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>   	struct scsi_sense_hdr sshdr;
>   	struct scsi_device *sdp;
>   	unsigned long flags;
> -	int ret, retries;
> +	int ret;
>   
>   	spin_lock_irqsave(hba->host->host_lock, flags);
>   	sdp = hba->ufs_device_wlun;
> @@ -9343,15 +9350,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>   	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
>   	 * already suspended childs.
>   	 */
> -	for (retries = 3; retries > 0; --retries) {
> -		ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
> -		/*
> -		 * scsi_execute() only returns a negative value if the request
> -		 * queue is dying.
> -		 */
> -		if (ret <= 0)
> -			break;
> -	}
> +	ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
>   	if (ret) {
>   		sdev_printk(KERN_WARNING, sdp,
>   			    "START_STOP failed for power mode: %d, result %x\n",

The original code only retries if ->result > 0. Is my understanding 
correct that the new code retries SCSI command execution whether 
->result is < 0 or > 0? If so, I think this patch introduces an 
unintended behavior change.

Thanks,

Bart.
Mike Christie July 17, 2023, 6:29 p.m. UTC | #2 
On 7/17/23 12:05 PM, Bart Van Assche wrote:
> On 7/14/23 14:34, Mike Christie wrote:
>> This has scsi-ml retry errors instead of driving them itself.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/ufs/core/ufshcd.c | 19 +++++++++----------
>>   1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 983fae84d9e8..267c24c57396 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -9291,7 +9291,14 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
>>                        struct scsi_sense_hdr *sshdr)
>>   {
>>       const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
>> +    struct scsi_failure failures[] = {
>> +        {
>> +            .allowed = 2,
>> +            .result = SCMD_FAILURE_RESULT_ANY,
>> +        },
>> +    };
>>       const struct scsi_exec_args args = {
>> +        .failures = failures,
>>           .sshdr = sshdr,
>>           .req_flags = BLK_MQ_REQ_PM,
>>           .scmd_flags = SCMD_FAIL_IF_RECOVERING,
>> @@ -9317,7 +9324,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>>       struct scsi_sense_hdr sshdr;
>>       struct scsi_device *sdp;
>>       unsigned long flags;
>> -    int ret, retries;
>> +    int ret;
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>       sdp = hba->ufs_device_wlun;
>> @@ -9343,15 +9350,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>>        * callbacks hence set the RQF_PM flag so that it doesn't resume the
>>        * already suspended childs.
>>        */
>> -    for (retries = 3; retries > 0; --retries) {
>> -        ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
>> -        /*
>> -         * scsi_execute() only returns a negative value if the request
>> -         * queue is dying.
>> -         */
>> -        if (ret <= 0)
>> -            break;
>> -    }
>> +    ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
>>       if (ret) {
>>           sdev_printk(KERN_WARNING, sdp,
>>                   "START_STOP failed for power mode: %d, result %x\n",
> 
> The original code only retries if ->result > 0. Is my understanding correct that the new code retries SCSI command execution whether ->result is < 0 or > 0? If so, I think this patch introduces an unintended behavior change.

The new code does not retry when result is < 0.

SCMD_FAILURE_RESULT_ANY is for cases where result > 0.
Bart Van Assche July 17, 2023, 7:26 p.m. UTC | #3 
On 7/17/23 11:29, Mike Christie wrote:
> On 7/17/23 12:05 PM, Bart Van Assche wrote:
>> The original code only retries if ->result > 0. Is my understanding
>> correct that the new code retries SCSI command execution whether
>> ->result is < 0 or > 0? If so, I think this patch introduces an
>> unintended behavior change.
> 
> The new code does not retry when result is < 0.
> 
> SCMD_FAILURE_RESULT_ANY is for cases where result > 0.

Right. Hence:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 983fae84d9e8..267c24c57396 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9291,7 +9291,14 @@  static int ufshcd_execute_start_stop(struct scsi_device *sdev,
 				     struct scsi_sense_hdr *sshdr)
 {
 	const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
+	struct scsi_failure failures[] = {
+		{
+			.allowed = 2,
+			.result = SCMD_FAILURE_RESULT_ANY,
+		},
+	};
 	const struct scsi_exec_args args = {
+		.failures = failures,
 		.sshdr = sshdr,
 		.req_flags = BLK_MQ_REQ_PM,
 		.scmd_flags = SCMD_FAIL_IF_RECOVERING,
@@ -9317,7 +9324,7 @@  static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	struct scsi_sense_hdr sshdr;
 	struct scsi_device *sdp;
 	unsigned long flags;
-	int ret, retries;
+	int ret;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	sdp = hba->ufs_device_wlun;
@@ -9343,15 +9350,7 @@  static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
 	 * already suspended childs.
 	 */
-	for (retries = 3; retries > 0; --retries) {
-		ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
-		/*
-		 * scsi_execute() only returns a negative value if the request
-		 * queue is dying.
-		 */
-		if (ret <= 0)
-			break;
-	}
+	ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
 	if (ret) {
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",