| Message ID | 20210819093534.17507-1-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: Fix ufshcd_request_sense_async() for Samsung KLUFG8RHDA-B2D1 | expand |
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.
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".
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.
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);
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(-)