On 24/11/2021 13:02, Adrian Hunter wrote:
> On 19/11/2021 21:57, 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);
>
> We do not need the block layer, nor SCSI commands which begs the question,
> why involve them at all?
>
> For example, the following is much simpler and seems to work:
Applied on top of patches 7 - 10, where patch 10 has the unintended
removal of blk_mq_free_request() that would have been removed here
instead.
>
>
> drivers/scsi/ufs/ufshcd.c | 52 +++++++--------------------------------
> drivers/scsi/ufs/ufshcd.h | 1 +
> 2 files changed, 10 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d125d8792ace5..bdfac3e9991ee 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -128,8 +128,9 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
> enum {
> UFSHCD_MAX_CHANNEL = 0,
> UFSHCD_MAX_ID = 1,
> - UFSHCD_CMD_PER_LUN = 32,
> - UFSHCD_CAN_QUEUE = 32,
> + UFSHCD_NUM_RESERVED = 1,
> + UFSHCD_CMD_PER_LUN = 32 - UFSHCD_NUM_RESERVED,
> + UFSHCD_CAN_QUEUE = 32 - UFSHCD_NUM_RESERVED,
> };
>
> static const char *const ufshcd_state_name[] = {
> @@ -2189,6 +2190,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
> hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
> hba->nutmrs =
> ((hba->capabilities & MASK_TASK_MANAGEMENT_REQUEST_SLOTS) >> 16) + 1;
> + hba->reserved_slot = hba->nutrs - 1;
>
> /* Read crypto capabilities */
> err = ufshcd_hba_init_crypto_capabilities(hba);
> @@ -2931,31 +2933,13 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
> static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
> enum dev_cmd_type cmd_type, int timeout)
> {
> - struct request_queue *q = hba->cmd_queue;
> DECLARE_COMPLETION_ONSTACK(wait);
> - struct request *req;
> struct ufshcd_lrb *lrbp;
> int err;
> - int tag;
> + int tag = hba->reserved_slot;
>
> down_read(&hba->clk_scaling_lock);
>
> - /*
> - * Get free slot, sleep if slots are unavailable.
> - * 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);
> - goto out_unlock;
> - }
> - 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);
> - blk_mq_start_request(req);
> -
> lrbp = &hba->lrb[tag];
> WARN_ON(lrbp->cmd);
> err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
> @@ -2970,10 +2954,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
> err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
> ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
> (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
> -
> out:
> - blk_mq_free_request(req);
> -out_unlock:
> up_read(&hba->clk_scaling_lock);
> return err;
> }
> @@ -4955,11 +4936,7 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
> */
> static int ufshcd_change_queue_depth(struct scsi_device *sdev, int depth)
> {
> - struct ufs_hba *hba = shost_priv(sdev->host);
> -
> - if (depth > hba->nutrs)
> - depth = hba->nutrs;
> - return scsi_change_queue_depth(sdev, depth);
> + return scsi_change_queue_depth(sdev, min(depth, sdev->host->can_queue));
> }
>
> static void ufshcd_hpb_destroy(struct ufs_hba *hba, struct scsi_device *sdev)
> @@ -6706,24 +6683,14 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
> enum dev_cmd_type cmd_type,
> enum query_opcode desc_op)
> {
> - struct request_queue *q = hba->cmd_queue;
> DECLARE_COMPLETION_ONSTACK(wait);
> - struct request *req;
> struct ufshcd_lrb *lrbp;
> int err = 0;
> - int tag;
> + int tag = hba->reserved_slot;
> u8 upiu_flags;
>
> 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);
> - goto out_unlock;
> - }
> - tag = req->tag;
> - WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> -
> lrbp = &hba->lrb[tag];
> WARN_ON(lrbp->cmd);
> lrbp->cmd = NULL;
> @@ -6791,7 +6758,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
> ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
> (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
>
> -out_unlock:
> up_read(&hba->clk_scaling_lock);
> return err;
> }
> @@ -9516,8 +9482,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> /* Configure LRB */
> ufshcd_host_memory_configure(hba);
>
> - host->can_queue = hba->nutrs;
> - host->cmd_per_lun = hba->nutrs;
> + host->can_queue = hba->nutrs - UFSHCD_NUM_RESERVED;
> + host->cmd_per_lun = hba->nutrs - UFSHCD_NUM_RESERVED;
> host->max_id = UFSHCD_MAX_ID;
> host->max_lun = UFS_MAX_LUNS;
> host->max_channel = UFSHCD_MAX_CHANNEL;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e9bc07c69a801..1addb2c906bae 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -836,6 +836,7 @@ struct ufs_hba {
> u32 capabilities;
> int nutrs;
> int nutmrs;
> + int reserved_slot; /* Protected by dev_cmd.lock */
> u32 ufs_version;
> const struct ufs_hba_variant_ops *vops;
> struct ufs_hba_variant_params *vps;
>
