Re: [bug report] scsi: ufs: Optimize host lock on transfer requests send/compl paths

[Can Guo <cang@codeaurora.org>]

Hi Dan,

On 2021-06-09 19:01, Dan Carpenter wrote:
> Hello Can Guo,
> 
> The patch a45f937110fa: "scsi: ufs: Optimize host lock on transfer
> requests send/compl paths" from May 24, 2021, leads to the following
> static checker warning:
> 
> 	drivers/scsi/ufs/ufshcd.c:2998 ufshcd_exec_dev_cmd()
> 	error: potentially dereferencing uninitialized 'lrbp'.
> 

I uploaded a fix yesterday - 
https://lore.kernel.org/patchwork/patch/1443774/
Thanks for reporting it and sorry for the disturb.

Regards,
Can Guo.

> drivers/scsi/ufs/ufshcd.c
>   2948  static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>   2949                  enum dev_cmd_type cmd_type, int timeout)
>   2950  {
>   2951          struct request_queue *q = hba->cmd_queue;
>   2952          struct request *req;
>   2953          struct ufshcd_lrb *lrbp;
>                                    ^^^^
> 
>   2954          int err;
>   2955          int tag;
>   2956          struct completion wait;
>   2957
>   2958          down_read(&hba->clk_scaling_lock);
>   2959
>   2960          /*
>   2961           * Get free slot, sleep if slots are unavailable.
>   2962           * Even though we use wait_event() which sleeps 
> indefinitely,
>   2963           * the maximum wait time is bounded by SCSI request 
> timeout.
>   2964           */
>   2965          req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
>   2966          if (IS_ERR(req)) {
>   2967                  err = PTR_ERR(req);
>   2968                  goto out_unlock;
>   2969          }
>   2970          tag = req->tag;
>   2971          WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
>   2972          /* Set the timeout such that the SCSI error handler is
> not activated. */
>   2973          req->timeout = msecs_to_jiffies(2 * timeout);
>   2974          blk_mq_start_request(req);
>   2975
>   2976          if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
>   2977                  err = -EBUSY;
>   2978                  goto out;
>                         ^^^^^^^^
> 
>   2979          }
>   2980
>   2981          init_completion(&wait);
>   2982          lrbp = &hba->lrb[tag];
> 
> This used to be initialized before the goto
> 
>   2983          WARN_ON(lrbp->cmd);
>   2984          err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
>   2985          if (unlikely(err))
>   2986                  goto out_put_tag;
>   2987
>   2988          hba->dev_cmd.complete = &wait;
>   2989
>   2990          ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND,
> lrbp->ucd_req_ptr);
>   2991          /* Make sure descriptors are ready before ringing the
> doorbell */
>   2992          wmb();
>   2993
>   2994          ufshcd_send_command(hba, tag);
>   2995          err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
>   2996  out:
>   2997          ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> UFS_QUERY_COMP,
>   2998                                      (struct utp_upiu_req
> *)lrbp->ucd_rsp_ptr);
> 
> ^^^^^^^^^^^^^^^^^
> 
>   2999
>   3000  out_put_tag:
>   3001          blk_put_request(req);
>   3002  out_unlock:
>   3003          up_read(&hba->clk_scaling_lock);
>   3004          return err;
>   3005  }
> 
> regards,
> dan carpenter