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 |
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.
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.
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 --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",