Message ID | 1891546521.01636066202065.JavaMail.epsvc@epcpadp3 (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | scsi: ufs: Improve SCSI abort handling | expand |
On 11/4/21 3:39 PM, Daejun Park wrote: > I found similar code in the ufshcd_err_handler(). I think the following > patch will required to fix another warning. > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index f5ba8f953b87..cce9abc6b879 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work) > } > dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag, > hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1); > + hba->lrb[tag].cmd = NULL; > } > > /* Clear pending task management requests */ Hmm ... since the error handler calls ufshcd_complete_requests(), shouldn't the completion function clear the 'cmd' member? I'm concerned that the above change would break the completion handler. Thanks, Bart.
>On 11/4/21 3:39 PM, Daejun Park wrote: >> I found similar code in the ufshcd_err_handler(). I think the following >> patch will required to fix another warning. >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index f5ba8f953b87..cce9abc6b879 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work) >> } >> dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag, >> hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1); >> + hba->lrb[tag].cmd = NULL; >> } >> >> /* Clear pending task management requests */ > >Hmm ... since the error handler calls ufshcd_complete_requests(), >shouldn't the completion function clear the 'cmd' member? I'm concerned >that the above change would break the completion handler. I missed that the error handler calls ufshcd_complete_requests(). Please ignore my suggestion. By the way, I give my reviewed-by tag. Reviewed-by: Daejun Park <daejun7.park@samsung.com> Thanks, Daejun
On 11/4/21 4:39 PM, Daejun Park wrote: >> On 11/4/21 3:39 PM, Daejun Park wrote: >>> I found similar code in the ufshcd_err_handler(). I think the following >>> patch will required to fix another warning. >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index f5ba8f953b87..cce9abc6b879 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work) >>> } >>> dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag, >>> hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1); >>> + hba->lrb[tag].cmd = NULL; >>> } >>> >>> /* Clear pending task management requests */ >> >> Hmm ... since the error handler calls ufshcd_complete_requests(), >> shouldn't the completion function clear the 'cmd' member? I'm concerned >> that the above change would break the completion handler. > > I missed that the error handler calls ufshcd_complete_requests(). Please > ignore my suggestion. > > By the way, I give my reviewed-by tag. > > Reviewed-by: Daejun Park <daejun7.park@samsung.com> Thanks Daejun! However, your question made me wonder whether ufshcd_abort() should clear the 'tag' bit from hba->outstanding_reqs. Although the SCSI standard requires that a command that is aborted is not completed, the UFSHCI specification requires that writing into the UTRLCLR register clears the corresponding bit(s) in the UTRLDBR register. I think bit 'tag' will have to be cleared from hba->outstanding_reqs to prevent that the aborted request is completed while the SCSI core is resubmitting it. Thanks, Bart.
>On 11/4/21 4:39 PM, Daejun Park wrote: >>> On 11/4/21 3:39 PM, Daejun Park wrote: >>>> I found similar code in the ufshcd_err_handler(). I think the following >>>> patch will required to fix another warning. >>>> >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>>> index f5ba8f953b87..cce9abc6b879 100644 >>>> --- a/drivers/scsi/ufs/ufshcd.c >>>> +++ b/drivers/scsi/ufs/ufshcd.c >>>> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work) >>>> } >>>> dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag, >>>> hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1); >>>> + hba->lrb[tag].cmd = NULL; >>>> } >>>> >>>> /* Clear pending task management requests */ >>> >>> Hmm ... since the error handler calls ufshcd_complete_requests(), >>> shouldn't the completion function clear the 'cmd' member? I'm concerned >>> that the above change would break the completion handler. >> >> I missed that the error handler calls ufshcd_complete_requests(). Please >> ignore my suggestion. >> >> By the way, I give my reviewed-by tag. >> >> Reviewed-by: Daejun Park <daejun7.park@samsung.com> > >Thanks Daejun! However, your question made me wonder whether ufshcd_abort() >should clear the 'tag' bit from hba->outstanding_reqs. Although the SCSI >standard requires that a command that is aborted is not completed, the UFSHCI >specification requires that writing into the UTRLCLR register clears the >corresponding bit(s) in the UTRLDBR register. I think bit 'tag' will have to >be cleared from hba->outstanding_reqs to prevent that the aborted request is >completed while the SCSI core is resubmitting it. My understanding is the completion of aborted request can be duplicated because SCSI core will resubmit the request. Therefore the clearing of the bit can avoid duplicated completion of the request. Thanks, Daejun
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index f5ba8f953b87..cce9abc6b879 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work) } dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag, hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1); + hba->lrb[tag].cmd = NULL; } /* Clear pending task management requests */