On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> The only functional change in this patch is the addition of a
> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++----------
> ----
> 1 file changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fced4528ee90..dfa5f127342b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
> {
> struct request_queue *q = hba->cmd_queue;
> DECLARE_COMPLETION_ONSTACK(wait);
> + struct scsi_cmnd *scmd;
> struct request *req;
> struct ufshcd_lrb *lrbp;
> int err;
> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
> * Even though we use wait_event() which sleeps indefinitely,
> * the maximum wait time is bounded by SCSI request timeout.
> */
> - req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> - if (IS_ERR(req)) {
> - err = PTR_ERR(req);
> + scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> + if (IS_ERR(scmd)) {
> + err = PTR_ERR(scmd);
> goto out_unlock;
> }
> + req = scsi_cmd_to_rq(scmd);
> tag = req->tag;
> WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> - /* Set the timeout such that the SCSI error handler is not
> activated. */
> - req->timeout = msecs_to_jiffies(2 * timeout);
> + /*
> + * Start the request such that blk_mq_tag_idle() is called when
> the
> + * device management request finishes.
> + */
> blk_mq_start_request(req);
>
> lrbp = &hba->lrb[tag];
> @@ -2972,7 +2976,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
> (struct utp_upiu_req *)lrbp-
> >ucd_rsp_ptr);
>
> out:
> - blk_mq_free_request(req);
> + scsi_put_internal_cmd(scmd);
> +
> out_unlock:
> up_read(&hba->clk_scaling_lock);
> return err;
> @@ -6573,17 +6578,16 @@ static int __ufshcd_issue_tm_cmd(struct
> ufs_hba *hba,
> struct request_queue *q = hba->tmf_queue;
> struct Scsi_Host *host = hba->host;
> DECLARE_COMPLETION_ONSTACK(wait);
> + struct scsi_cmnd *scmd;
> struct request *req;
> unsigned long flags;
> int task_tag, err;
>
> - /*
> - * blk_mq_alloc_request() is used here only to get a free tag.
> - */
> - req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> - if (IS_ERR(req))
> - return PTR_ERR(req);
> + scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> + if (IS_ERR(scmd))
> + return PTR_ERR(scmd);
>
> + req = scsi_cmd_to_rq(scmd);
> req->end_io_data = &wait;
> ufshcd_hold(hba, false);
>
> @@ -6636,7 +6640,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba
> *hba,
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> ufshcd_release(hba);
> - blk_mq_free_request(req);
> +
> + scsi_put_internal_cmd(scmd);
>
> return err;
> }
> @@ -6714,6 +6719,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
> {
> struct request_queue *q = hba->cmd_queue;
> DECLARE_COMPLETION_ONSTACK(wait);
> + struct scsi_cmnd *scmd;
> struct request *req;
> struct ufshcd_lrb *lrbp;
> int err = 0;
> @@ -6722,13 +6728,19 @@ static int
> ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>
> down_read(&hba->clk_scaling_lock);
>
> - req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> - if (IS_ERR(req)) {
> - err = PTR_ERR(req);
> + scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> + if (IS_ERR(scmd)) {
> + err = PTR_ERR(scmd);
> goto out_unlock;
> }
> + req = scsi_cmd_to_rq(scmd);
> tag = req->tag;
> WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> + /*
> + * Start the request such that blk_mq_tag_idle() is called when
> the
> + * device management request finishes.
> + */
> + blk_mq_start_request(req);
Bart,
Calling blk_mq_start_request() will inject the trace print of the block
issued, but we do not have its paired completion trace print.
In addition, blk_mq_tag_idle() will not be called after the device
management request is completed, it will be called after the timer
expires.
I remember that we used to not allow this kind of LLD internal commands
to be attached to the block layer. I now find that to be correct way.
Bean
