| Message ID | 20220929220021.247097-7-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix a deadlock in the UFS driver | expand |
On Thu, 2022-09-29 at 15:00 -0700, Bart Van Assche wrote: > Instead of only retrying the START STOP UNIT command if a unit > attention > is reported, repeat it if any SCSI error is reported by the device or > if > the command timed out. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Bean Huo <beanhuo@micron.com>
On 30/09/22 01:00, Bart Van Assche wrote: > Instead of only retrying the START STOP UNIT command if a unit attention > is reported, repeat it if any SCSI error is reported by the device or if > the command timed out. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 02e73208b921..e8c0504e9e83 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8784,9 +8784,9 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, > for (retries = 3; retries > 0; --retries) { > ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, > START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); > - if (!scsi_status_is_check_condition(ret) || > - !scsi_sense_valid(&sshdr) || > - sshdr.sense_key != UNIT_ATTENTION) > + if (ret < 0) > + break; Could use a comment to answer: why not retry if ret < 0 ? > + if (host_byte(ret) == 0 && scsi_status_is_good(ret)) > break; Also could use a comment: why not just "if (!ret)" ? > } > if (ret) {
On 10/2/22 23:10, Adrian Hunter wrote: > On 30/09/22 01:00, Bart Van Assche wrote: >> Instead of only retrying the START STOP UNIT command if a unit attention >> is reported, repeat it if any SCSI error is reported by the device or if >> the command timed out. >> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> --- >> drivers/ufs/core/ufshcd.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 02e73208b921..e8c0504e9e83 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -8784,9 +8784,9 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, >> for (retries = 3; retries > 0; --retries) { >> ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, >> START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); >> - if (!scsi_status_is_check_condition(ret) || >> - !scsi_sense_valid(&sshdr) || >> - sshdr.sense_key != UNIT_ATTENTION) >> + if (ret < 0) >> + break; > > Could use a comment to answer: why not retry if ret < 0 ? OK, I will add a comment above that if-test. >> + if (host_byte(ret) == 0 && scsi_status_is_good(ret)) >> break; > > Also could use a comment: why not just "if (!ret)" ? The UFS spec says "CONDITION MET - Not used for UFS" so changing the above test into "if (!ret)" would be valid. If nobody objects I will change the above test into "if (ret == 0)" instead of adding a comment. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 02e73208b921..e8c0504e9e83 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8784,9 +8784,9 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, for (retries = 3; retries > 0; --retries) { ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); - if (!scsi_status_is_check_condition(ret) || - !scsi_sense_valid(&sshdr) || - sshdr.sense_key != UNIT_ATTENTION) + if (ret < 0) + break; + if (host_byte(ret) == 0 && scsi_status_is_good(ret)) break; } if (ret) {
Instead of only retrying the START STOP UNIT command if a unit attention is reported, repeat it if any SCSI error is reported by the device or if the command timed out. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)