[RFC PATCH v2 0/1] scsi: ufs: fix lrbp pointer incorrect initialization issue

[RFC PATCH v2 0/1] scsi: ufs: fix lrbp pointer incorrect initialization issue

[Bean Huo]

Hi, Martin and Bart

This is the patch for fixing the issue introduced by the patch

[PATCH v2 4/4] ufs: Let the SCSI core allocate per-command UFS data.

Bean Huo (1):
  scsi: ufs: fix lrbp pointer incorrect initialization issue

 drivers/scsi/ufs/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.17.1

[RFC PATCH v2 1/1] scsi: ufs: fix lrbp pointer incorrect initialization issue

[Bean Huo]

The parameter cmd of ufshcd_init_cmd_priv() wasn't given a correct tag
value while the SCSI layer calls back ufshcd_init_cmd_priv(), this results
in all pointers of lrbp in UFS driver point to first the lrbp.

As this is just observed, the patch is for reference so others
who want to use the latest UFS driver can avoid this issue. Any recommend
is welcomed.

Fixes: 34656dda81ac ("scsi: ufs: Let the SCSI core allocate per-command UFS data")
---
 drivers/scsi/ufs/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e987fa3a77c7..396512a9234f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2471,6 +2471,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		BUG();
 	}
 
+	ufshcd_init_lrb(hba, lrbp, tag);
+
 	WARN_ON_ONCE(!ufshcd_is_scsi(cmd->request));
 
 	if (!down_read_trylock(&hba->clk_scaling_lock))
@@ -2707,6 +2709,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 
 	init_completion(&wait);
 	lrbp = ufshcd_req_to_lrb(req);
+	ufshcd_init_lrb(hba, lrbp, tag);
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
 	if (unlikely(err))
 		goto out_put_tag;
@@ -5900,6 +5903,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 
 	init_completion(&wait);
 	lrbp = ufshcd_req_to_lrb(req);
+	ufshcd_init_lrb(hba, lrbp, tag);
 	lrbp->sense_bufflen = 0;
 	lrbp->sense_buffer = NULL;
 	lrbp->task_tag = tag;
-- 
2.17.1

Re: [RFC PATCH v2 1/1] scsi: ufs: fix lrbp pointer incorrect initialization issue

[Bart Van Assche]

On 2020-03-08 07:56, Bean Huo wrote:
> The parameter cmd of ufshcd_init_cmd_priv() wasn't given a correct tag
> value while the SCSI layer calls back ufshcd_init_cmd_priv(), this results
> in all pointers of lrbp in UFS driver point to first the lrbp.
> 
> As this is just observed, the patch is for reference so others
> who want to use the latest UFS driver can avoid this issue. Any recommend
> is welcomed.

How about also removing ufshcd_init_cmd_priv(), like in the untested patch
below?

Thanks,

Bart.

Subject: [PATCH] ufs: Fix LRB initialization

There are two issues with the ufshcd_init_lrb() call in
ufshcd_init_cmd_priv():
- cmd->tag is set from inside scsi_mq_prep_fn() and hence is not yet set
  when ufshcd_init_cmd_priv() is set.
- Inside ufshcd_init_cmd_priv() the tag can only be derived from the SCSI
  command pointer if no scheduler has been associated with the UFS block
  device. If no scheduler is associated with a block device, the
  relationship between hctx->tags->static_rqs[] and rq->tag is static.
  If a scheduler has been configured, a single tag can be associated
  with a different struct request if a request is reallocated.

Fixes: 34656dda81ac ("ufs: Let the SCSI core allocate per-command UFS data")
---
 drivers/scsi/ufs/ufshcd.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e987fa3a77c7..1589ba70672f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2473,6 +2473,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)

 	WARN_ON_ONCE(!ufshcd_is_scsi(cmd->request));

+	ufshcd_init_lrb(hba, lrbp, tag);
+
 	if (!down_read_trylock(&hba->clk_scaling_lock))
 		return SCSI_MLQUEUE_HOST_BUSY;

@@ -2707,6 +2709,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,

 	init_completion(&wait);
 	lrbp = ufshcd_req_to_lrb(req);
+	ufshcd_init_lrb(hba, lrbp, tag);
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
 	if (unlikely(err))
 		goto out_put_tag;
@@ -3504,14 +3507,6 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
 	}
 }

-static int ufshcd_init_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
-{
-	struct ufs_hba *hba = shost_priv(shost);
-
-	ufshcd_init_lrb(hba, scsi_cmd_priv(cmd), cmd->tag);
-	return 0;
-}
-
 /**
  * ufshcd_dme_link_startup - Notify Unipro to perform link startup
  * @hba: per adapter instance
@@ -5900,6 +5895,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,

 	init_completion(&wait);
 	lrbp = ufshcd_req_to_lrb(req);
+	ufshcd_init_lrb(hba, lrbp, tag);
 	lrbp->sense_bufflen = 0;
 	lrbp->sense_buffer = NULL;
 	lrbp->task_tag = tag;
@@ -7180,7 +7176,6 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= UFSHCD,
 	.proc_name		= UFSHCD,
-	.init_cmd_priv		= ufshcd_init_cmd_priv,
 	.queuecommand		= ufshcd_queuecommand,
 	.slave_alloc		= ufshcd_slave_alloc,
 	.slave_configure	= ufshcd_slave_configure,