RE: [PATCH] scsi: ufs: Improve SCSI abort handling

From: Daejun Park <daejun7.park@samsung.com>
To: Bart Van Assche <bvanassche@acm.org>,
	Daejun Park <daejun7.park@samsung.com>,
	"Martin K . Petersen" <martin.petersen@oracle.com>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	Can Guo <cang@codeaurora.org>,
	Stanley Chu <stanley.chu@mediatek.com>,
	Asutosh Das <asutoshd@codeaurora.org>,
	James Bottomley <James.Bottomley@HansenPartnership.com>,
	Vinayak Holikatti <vinholikatti@gmail.com>,
	VISHAK G <vishak.g@samsung.com>,
	Girish K S <girish.shivananjappa@linaro.org>,
	Santosh Yaraganavi <santoshsy@gmail.com>,
	"huobean@gmail.com" <huobean@gmail.com>
Subject: RE: [PATCH] scsi: ufs: Improve SCSI abort handling
Date: Fri, 05 Nov 2021 10:53:15 +0900	[thread overview]
Message-ID: <2038148563.21636078682214.JavaMail.epsvc@epcpadp4> (raw)
In-Reply-To: <cb0982d2-1124-a8ee-d129-e2e4975ef1c4@acm.org>

>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