[PATCH] scsi: ufs: Fix ufshcd_request_sense_async() for Samsung KLUFG8RHDA-B2D1

[PATCH] scsi: ufs: Fix ufshcd_request_sense_async() for Samsung KLUFG8RHDA-B2D1

[Adrian Hunter]

Samsung KLUFG8RHDA-B2D1 does not clear the unit attention condition if the
length is zero. So go back to requesting all the sense data, as it was
before patch "scsi: ufs: Request sense data asynchronously". That is
simpler than creating and maintaining a quirk for affected devices.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufshcd.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a3b419848f0a..c7a130bc782d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7937,7 +7937,8 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
 {
 	if (error != BLK_STS_OK)
-		pr_err("%s: REQUEST SENSE failed (%d)", __func__, error);
+		pr_err("%s: REQUEST SENSE failed (%d)\n", __func__, error);
+	kfree(rq->end_io_data);
 	blk_put_request(rq);
 }
 
@@ -7946,22 +7947,38 @@ ufshcd_request_sense_async(struct ufs_hba *hba, struct scsi_device *sdev)
 {
 	/*
 	 * From SPC-6: the REQUEST SENSE command with any allocation length
-	 * clears the sense data.
+	 * clears the sense data, but not all UFS devices behave that way.
 	 */
-	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, 0, 0};
+	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, UFS_SENSE_SIZE, 0};
 	struct scsi_request *rq;
 	struct request *req;
+	char *buffer;
+	int ret;
+
+	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
 
-	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN, /*flags=*/0);
+	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
+			      /*flags=*/BLK_MQ_REQ_PM);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
+	ret = blk_rq_map_kern(sdev->request_queue, req,
+			      buffer, UFS_SENSE_SIZE, GFP_NOIO);
+	if (ret) {
+		blk_put_request(req);
+		kfree(buffer);
+		return ret;
+	}
+
 	rq = scsi_req(req);
 	rq->cmd_len = ARRAY_SIZE(cmd);
 	memcpy(rq->cmd, cmd, rq->cmd_len);
 	rq->retries = 3;
 	req->timeout = 1 * HZ;
 	req->rq_flags |= RQF_PM | RQF_QUIET;
+	req->end_io_data = buffer;
 
 	blk_execute_rq_nowait(/*bd_disk=*/NULL, req, /*at_head=*/true,
 			      ufshcd_request_sense_done);
-- 
2.17.1

Re: [PATCH] scsi: ufs: Fix ufshcd_request_sense_async() for Samsung KLUFG8RHDA-B2D1

[Bart Van Assche]

On 8/19/21 2:35 AM, Adrian Hunter wrote:
>   	 * From SPC-6: the REQUEST SENSE command with any allocation length
> -	 * clears the sense data.
> +	 * clears the sense data, but not all UFS devices behave that way.
>   	 */

How about removing the comment entirely? Comprehending the above comment 
is not possible without reviewing the git history so I think it's better 
to remove it.

> -	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, 0, 0};
> +	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, UFS_SENSE_SIZE, 0};
>   	struct scsi_request *rq;
>   	struct request *req;
> +	char *buffer;
> +	int ret;
> +
> +	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
>   
> -	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN, /*flags=*/0);
> +	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
> +			      /*flags=*/BLK_MQ_REQ_PM);

Why has the flags argument been changed from 0 into BLK_MQ_REQ_PM? MODE 
SENSE is not a power management command.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: Fix ufshcd_request_sense_async() for Samsung KLUFG8RHDA-B2D1

[Adrian Hunter]

On 19/08/21 9:14 pm, Bart Van Assche wrote:
> On 8/19/21 2:35 AM, Adrian Hunter wrote:
>>        * From SPC-6: the REQUEST SENSE command with any allocation length
>> -     * clears the sense data.
>> +     * clears the sense data, but not all UFS devices behave that way.
>>        */
> 
> How about removing the comment entirely? Comprehending the above comment is not possible without reviewing the git history so I think it's better to remove it.

Perhaps a comment might stop someone tempted to remove the sense size in the future.  What about:

	/*
	 * Some UFS devices clear unit attention condition only if the sense
	 * size used (UFS_SENSE_SIZE in this case) is non-zero.
	 */

> 
>> -    static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, 0, 0};
>> +    static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, UFS_SENSE_SIZE, 0};
>>       struct scsi_request *rq;
>>       struct request *req;
>> +    char *buffer;
>> +    int ret;
>> +
>> +    buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
>> +    if (!buffer)
>> +        return -ENOMEM;
>>   -    req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN, /*flags=*/0);
>> +    req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
>> +                  /*flags=*/BLK_MQ_REQ_PM);
> 
> Why has the flags argument been changed from 0 into BLK_MQ_REQ_PM? MODE SENSE is not a power management command.

It is used in a PM path, it is consistent with RQF_PM also used by ufshcd_request_sense_async(), it is what __scsi_execute() does with RQF_PM, so it is what was used before "scsi: ufs: Request sense data asynchronously".

Re: [PATCH] scsi: ufs: Fix ufshcd_request_sense_async() for Samsung KLUFG8RHDA-B2D1

[Bart Van Assche]

On 8/19/21 10:37 PM, Adrian Hunter wrote:
> On 19/08/21 9:14 pm, Bart Van Assche wrote:
>> On 8/19/21 2:35 AM, Adrian Hunter wrote:
>>>         * From SPC-6: the REQUEST SENSE command with any allocation length
>>> -     * clears the sense data.
>>> +     * clears the sense data, but not all UFS devices behave that way.
>>>         */
>>
>> How about removing the comment entirely? Comprehending the above comment is not possible without reviewing the git history so I think it's better to remove it.
> 
> Perhaps a comment might stop someone tempted to remove the sense size in the future.  What about:
> 
> 	/*
> 	 * Some UFS devices clear unit attention condition only if the sense
> 	 * size used (UFS_SENSE_SIZE in this case) is non-zero.
> 	 */

That sounds good to me.

PS: we are working with the team that depends on this behavior (clearing 
the unit attention condition) to implement a retry loop on top of 
ioctl(SG_IO). Once that loop has been added it will be possible to drop 
the code that clears the unit attention condition after a resume.

Thanks,

Bart.