[PATCH 0/6] Six UFS patches

[PATCH 0/6] Six UFS patches

[Bart Van Assche]

Hi Martin,

This patch series includes patches that clean up the UFS driver code somewhat.
Please consider these patches for kernel v5.6.

Thanks,

Bart.

Bart Van Assche (6):
  ufs: Fix indentation in ufshcd_query_attr_retry()
  ufs: Make ufshcd_add_command_trace() easier to read
  ufs: Make ufshcd_prepare_utp_scsi_cmd_upiu() easier to read
  ufs: Fix a race condition in the tracing code
  ufs: Remove superfluous memory barriers
  ufs: Remove the SCSI timeout handler

 drivers/scsi/ufs/ufshcd.c | 64 ++++++++-------------------------------
 1 file changed, 12 insertions(+), 52 deletions(-)

[PATCH 1/6] ufs: Fix indentation in ufshcd_query_attr_retry()

[Bart Van Assche]

Remove a space that occurs after a tab.

Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d6d0f83c9044..48f2f94d51bc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2869,7 +2869,7 @@ static int ufshcd_query_attr_retry(struct ufs_hba *hba,
 	int ret = 0;
 	u32 retries;
 
-	 for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
+	for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
 		ret = ufshcd_query_attr(hba, opcode, idn, index,
 						selector, attr_val);
 		if (ret)

[PATCH 2/6] ufs: Make ufshcd_add_command_trace() easier to read

[Bart Van Assche]

Since the lrbp->cmd expression occurs multiple times, introduce a new
local variable to hold that pointer. This patch does not change any
functionality.

Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 48f2f94d51bc..acc84e964e8f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -327,27 +327,27 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
 	u8 opcode = 0;
 	u32 intr, doorbell;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
+	struct scsi_cmnd *cmd = lrbp->cmd;
 	int transfer_len = -1;
 
 	if (!trace_ufshcd_command_enabled()) {
 		/* trace UPIU W/O tracing command */
-		if (lrbp->cmd)
+		if (cmd)
 			ufshcd_add_cmd_upiu_trace(hba, tag, str);
 		return;
 	}
 
-	if (lrbp->cmd) { /* data phase exists */
+	if (cmd) { /* data phase exists */
 		/* trace UPIU also */
 		ufshcd_add_cmd_upiu_trace(hba, tag, str);
-		opcode = (u8)(*lrbp->cmd->cmnd);
+		opcode = cmd->cmnd[0];
 		if ((opcode == READ_10) || (opcode == WRITE_10)) {
 			/*
 			 * Currently we only fully trace read(10) and write(10)
 			 * commands
 			 */
-			if (lrbp->cmd->request && lrbp->cmd->request->bio)
-				lba =
-				  lrbp->cmd->request->bio->bi_iter.bi_sector;
+			if (cmd->request && cmd->request->bio)
+				lba = cmd->request->bio->bi_iter.bi_sector;
 			transfer_len = be32_to_cpu(
 				lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
 		}

[PATCH 3/6] ufs: Make ufshcd_prepare_utp_scsi_cmd_upiu() easier to read

[Bart Van Assche]

Since the lrbp->cmd expression occurs multiple times, introduce a new
local variable to hold that pointer. This patch does not change any
functionality.

Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index acc84e964e8f..80b028ba39e9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2233,6 +2233,7 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
 static
 void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u32 upiu_flags)
 {
+	struct scsi_cmnd *cmd = lrbp->cmd;
 	struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
 	unsigned short cdb_len;
 
@@ -2246,12 +2247,11 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u32 upiu_flags)
 	/* Total EHS length and Data segment length will be zero */
 	ucd_req_ptr->header.dword_2 = 0;
 
-	ucd_req_ptr->sc.exp_data_transfer_len =
-		cpu_to_be32(lrbp->cmd->sdb.length);
+	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);
 
-	cdb_len = min_t(unsigned short, lrbp->cmd->cmd_len, UFS_CDB_SIZE);
+	cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
 	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
-	memcpy(ucd_req_ptr->sc.cdb, lrbp->cmd->cmnd, cdb_len);
+	memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
 
 	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 }

[PATCH 4/6] ufs: Fix a race condition in the tracing code

[Bart Van Assche]

Starting execution of a command before tracing a command may cause the
completion handler to free data while it is being traced. Fix this race
by tracing a command before it is submitted.

Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 80b028ba39e9..4d9bb1932b39 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1875,12 +1875,12 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 {
 	hba->lrb[task_tag].issue_time_stamp = ktime_get();
 	hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0);
+	ufshcd_add_command_trace(hba, task_tag, "send");
 	ufshcd_clk_scaling_start_busy(hba);
 	__set_bit(task_tag, &hba->outstanding_reqs);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	/* Make sure that doorbell is committed immediately */
 	wmb();
-	ufshcd_add_command_trace(hba, task_tag, "send");
 }
 
 /**

[PATCH 5/6] ufs: Remove superfluous memory barriers

[Bart Van Assche]

Calling wmb() after having written to a doorbell slows down code and does
not help to commit the doorbell write faster. Hence remove such wmb()
calls. Note: detailed information about the semantics of writel() is
available in Documentation/driver-api/device-io.rst.

Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4d9bb1932b39..edcc137c436b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1879,8 +1879,6 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 	ufshcd_clk_scaling_start_busy(hba);
 	__set_bit(task_tag, &hba->outstanding_reqs);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	/* Make sure that doorbell is committed immediately */
-	wmb();
 }
 
 /**
@@ -5766,8 +5764,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	wmb();
 
 	ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
-	/* Make sure that doorbell is committed immediately */
-	wmb();
 
 	spin_unlock_irqrestore(host->host_lock, flags);
 

[PATCH 6/6] ufs: Remove the SCSI timeout handler

[Bart Van Assche]

The UFS SCSI timeout handler was needed to compensate that
ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by eliminating
tag conflicts") fixed this so the timeout handler is no longer necessary.

See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout handler").

Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 36 ------------------------------------
 1 file changed, 36 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index edcc137c436b..763e41286a4b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7092,41 +7092,6 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	ufshcd_probe_hba(hba);
 }
 
-static enum blk_eh_timer_return ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
-{
-	unsigned long flags;
-	struct Scsi_Host *host;
-	struct ufs_hba *hba;
-	int index;
-	bool found = false;
-
-	if (!scmd || !scmd->device || !scmd->device->host)
-		return BLK_EH_DONE;
-
-	host = scmd->device->host;
-	hba = shost_priv(host);
-	if (!hba)
-		return BLK_EH_DONE;
-
-	spin_lock_irqsave(host->host_lock, flags);
-
-	for_each_set_bit(index, &hba->outstanding_reqs, hba->nutrs) {
-		if (hba->lrb[index].cmd == scmd) {
-			found = true;
-			break;
-		}
-	}
-
-	spin_unlock_irqrestore(host->host_lock, flags);
-
-	/*
-	 * Bypass SCSI error handling and reset the block layer timer if this
-	 * SCSI command was not actually dispatched to UFS driver, otherwise
-	 * let SCSI layer handle the error as usual.
-	 */
-	return found ? BLK_EH_DONE : BLK_EH_RESET_TIMER;
-}
-
 static const struct attribute_group *ufshcd_driver_groups[] = {
 	&ufs_sysfs_unit_descriptor_group,
 	&ufs_sysfs_lun_attributes_group,
@@ -7145,7 +7110,6 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.eh_abort_handler	= ufshcd_abort,
 	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
 	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
-	.eh_timed_out		= ufshcd_eh_timed_out,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,

Re: [PATCH 1/6] ufs: Fix indentation in ufshcd_query_attr_retry()

[Stanley Chu]

Hi Bart,

On Tue, 2019-12-24 at 14:02 -0800, Bart Van Assche wrote:
> Remove a space that occurs after a tab.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d6d0f83c9044..48f2f94d51bc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2869,7 +2869,7 @@ static int ufshcd_query_attr_retry(struct ufs_hba *hba,
>  	int ret = 0;
>  	u32 retries;
>  
> -	 for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> +	for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
>  		ret = ufshcd_query_attr(hba, opcode, idn, index,
>  						selector, attr_val);
>  		if (ret)

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH 2/6] ufs: Make ufshcd_add_command_trace() easier to read

[Stanley Chu]

Hi Bart,

On Tue, 2019-12-24 at 14:02 -0800, Bart Van Assche wrote:
> Since the lrbp->cmd expression occurs multiple times, introduce a new
> local variable to hold that pointer. This patch does not change any
> functionality.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH 3/6] ufs: Make ufshcd_prepare_utp_scsi_cmd_upiu() easier to read

[Stanley Chu]

Hi Bart,

On Tue, 2019-12-24 at 14:02 -0800, Bart Van Assche wrote:
> Since the lrbp->cmd expression occurs multiple times, introduce a new
> local variable to hold that pointer. This patch does not change any
> functionality.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH 1/6] ufs: Fix indentation in ufshcd_query_attr_retry()

[Can Guo]

On 2019-12-25 06:02, Bart Van Assche wrote:
> Remove a space that occurs after a tab.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d6d0f83c9044..48f2f94d51bc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2869,7 +2869,7 @@ static int ufshcd_query_attr_retry(struct ufs_hba 
> *hba,
>  	int ret = 0;
>  	u32 retries;
> 
> -	 for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> +	for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
>  		ret = ufshcd_query_attr(hba, opcode, idn, index,
>  						selector, attr_val);
>  		if (ret)

Re: [PATCH 2/6] ufs: Make ufshcd_add_command_trace() easier to read

[Can Guo]

On 2019-12-25 06:02, Bart Van Assche wrote:
> Since the lrbp->cmd expression occurs multiple times, introduce a new
> local variable to hold that pointer. This patch does not change any
> functionality.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  drivers/scsi/ufs/ufshcd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 48f2f94d51bc..acc84e964e8f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -327,27 +327,27 @@ static void ufshcd_add_command_trace(struct 
> ufs_hba *hba,
>  	u8 opcode = 0;
>  	u32 intr, doorbell;
>  	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
> +	struct scsi_cmnd *cmd = lrbp->cmd;
>  	int transfer_len = -1;
> 
>  	if (!trace_ufshcd_command_enabled()) {
>  		/* trace UPIU W/O tracing command */
> -		if (lrbp->cmd)
> +		if (cmd)
>  			ufshcd_add_cmd_upiu_trace(hba, tag, str);
>  		return;
>  	}
> 
> -	if (lrbp->cmd) { /* data phase exists */
> +	if (cmd) { /* data phase exists */
>  		/* trace UPIU also */
>  		ufshcd_add_cmd_upiu_trace(hba, tag, str);
> -		opcode = (u8)(*lrbp->cmd->cmnd);
> +		opcode = cmd->cmnd[0];
>  		if ((opcode == READ_10) || (opcode == WRITE_10)) {
>  			/*
>  			 * Currently we only fully trace read(10) and write(10)
>  			 * commands
>  			 */
> -			if (lrbp->cmd->request && lrbp->cmd->request->bio)
> -				lba =
> -				  lrbp->cmd->request->bio->bi_iter.bi_sector;
> +			if (cmd->request && cmd->request->bio)
> +				lba = cmd->request->bio->bi_iter.bi_sector;
>  			transfer_len = be32_to_cpu(
>  				lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
>  		}

Re: [PATCH 3/6] ufs: Make ufshcd_prepare_utp_scsi_cmd_upiu() easier to read

[Can Guo]

On 2019-12-25 06:02, Bart Van Assche wrote:
> Since the lrbp->cmd expression occurs multiple times, introduce a new
> local variable to hold that pointer. This patch does not change any
> functionality.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  drivers/scsi/ufs/ufshcd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index acc84e964e8f..80b028ba39e9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2233,6 +2233,7 @@ static void ufshcd_prepare_req_desc_hdr(struct
> ufshcd_lrb *lrbp,
>  static
>  void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u32 
> upiu_flags)
>  {
> +	struct scsi_cmnd *cmd = lrbp->cmd;
>  	struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
>  	unsigned short cdb_len;
> 
> @@ -2246,12 +2247,11 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct
> ufshcd_lrb *lrbp, u32 upiu_flags)
>  	/* Total EHS length and Data segment length will be zero */
>  	ucd_req_ptr->header.dword_2 = 0;
> 
> -	ucd_req_ptr->sc.exp_data_transfer_len =
> -		cpu_to_be32(lrbp->cmd->sdb.length);
> +	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);
> 
> -	cdb_len = min_t(unsigned short, lrbp->cmd->cmd_len, UFS_CDB_SIZE);
> +	cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
>  	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
> -	memcpy(ucd_req_ptr->sc.cdb, lrbp->cmd->cmnd, cdb_len);
> +	memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
> 
>  	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
>  }

Re: [PATCH 5/6] ufs: Remove superfluous memory barriers

[Can Guo]

On 2019-12-25 06:02, Bart Van Assche wrote:
> Calling wmb() after having written to a doorbell slows down code and 
> does
> not help to commit the doorbell write faster. Hence remove such wmb()
> calls. Note: detailed information about the semantics of writel() is
> available in Documentation/driver-api/device-io.rst.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 4d9bb1932b39..edcc137c436b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1879,8 +1879,6 @@ void ufshcd_send_command(struct ufs_hba *hba,
> unsigned int task_tag)
>  	ufshcd_clk_scaling_start_busy(hba);
>  	__set_bit(task_tag, &hba->outstanding_reqs);
>  	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -	/* Make sure that doorbell is committed immediately */
> -	wmb();
>  }
> 
>  /**
> @@ -5766,8 +5764,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba 
> *hba,
>  	wmb();
> 
>  	ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
> -	/* Make sure that doorbell is committed immediately */
> -	wmb();
> 
>  	spin_unlock_irqrestore(host->host_lock, flags);

Hi Bart,

Three wmb()s were added in commit ad1a1b9cd because we did see instances 
on
which OCS=3(MISMATCH_DATA_BUFFER_SIZE) error were observed in large 
scale
test. Commit ad1a1b9cd fixed the error and we had confirmed it through
large amount of tests. I am not sure removing the 2 wmb()s here would 
cause
regression or not.

Thanks,

Can Guo.

Re: [PATCH 4/6] ufs: Fix a race condition in the tracing code

[Can Guo]

On 2019-12-25 06:02, Bart Van Assche wrote:
> Starting execution of a command before tracing a command may cause the
> completion handler to free data while it is being traced. Fix this race
> by tracing a command before it is submitted.
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 80b028ba39e9..4d9bb1932b39 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1875,12 +1875,12 @@ void ufshcd_send_command(struct ufs_hba *hba,
> unsigned int task_tag)
>  {
>  	hba->lrb[task_tag].issue_time_stamp = ktime_get();
>  	hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0);
> +	ufshcd_add_command_trace(hba, task_tag, "send");

How about moving it after __set_bit(task_tag, &hba->outstanding_reqs);?

Thanks,

Can Guo.

>  	ufshcd_clk_scaling_start_busy(hba);
>  	__set_bit(task_tag, &hba->outstanding_reqs);
>  	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  	/* Make sure that doorbell is committed immediately */
>  	wmb();
> -	ufshcd_add_command_trace(hba, task_tag, "send");
>  }
> 
>  /**

Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler

[Can Guo]

On 2019-12-25 06:02, Bart Van Assche wrote:
> The UFS SCSI timeout handler was needed to compensate that
> ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
> time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by 
> eliminating
> tag conflicts") fixed this so the timeout handler is no longer 
> necessary.
> 
> See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout 
> handler").
> 
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 36 ------------------------------------
>  1 file changed, 36 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index edcc137c436b..763e41286a4b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7092,41 +7092,6 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>  	ufshcd_probe_hba(hba);
>  }
> 
> -static enum blk_eh_timer_return ufshcd_eh_timed_out(struct scsi_cmnd 
> *scmd)
> -{
> -	unsigned long flags;
> -	struct Scsi_Host *host;
> -	struct ufs_hba *hba;
> -	int index;
> -	bool found = false;
> -
> -	if (!scmd || !scmd->device || !scmd->device->host)
> -		return BLK_EH_DONE;
> -
> -	host = scmd->device->host;
> -	hba = shost_priv(host);
> -	if (!hba)
> -		return BLK_EH_DONE;
> -
> -	spin_lock_irqsave(host->host_lock, flags);
> -
> -	for_each_set_bit(index, &hba->outstanding_reqs, hba->nutrs) {
> -		if (hba->lrb[index].cmd == scmd) {
> -			found = true;
> -			break;
> -		}
> -	}
> -
> -	spin_unlock_irqrestore(host->host_lock, flags);
> -
> -	/*
> -	 * Bypass SCSI error handling and reset the block layer timer if this
> -	 * SCSI command was not actually dispatched to UFS driver, otherwise
> -	 * let SCSI layer handle the error as usual.
> -	 */
> -	return found ? BLK_EH_DONE : BLK_EH_RESET_TIMER;
> -}
> -
>  static const struct attribute_group *ufshcd_driver_groups[] = {
>  	&ufs_sysfs_unit_descriptor_group,
>  	&ufs_sysfs_lun_attributes_group,
> @@ -7145,7 +7110,6 @@ static struct scsi_host_template
> ufshcd_driver_template = {
>  	.eh_abort_handler	= ufshcd_abort,
>  	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>  	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
> -	.eh_timed_out		= ufshcd_eh_timed_out,
>  	.this_id		= -1,
>  	.sg_tablesize		= SG_ALL,
>  	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,

Reviewed-by: Can Guo <cang@codeaurora.org>

Re: [PATCH 4/6] ufs: Fix a race condition in the tracing code

[Bart Van Assche]

On 12/25/19 2:59 AM, Can Guo wrote:
> On 2019-12-25 06:02, Bart Van Assche wrote:
>> Starting execution of a command before tracing a command may cause the
>> completion handler to free data while it is being traced. Fix this race
>> by tracing a command before it is submitted.
>>
>> Cc: Bean Huo <beanhuo@micron.com>
>> Cc: Can Guo <cang@codeaurora.org>
>> Cc: Avri Altman <avri.altman@wdc.com>
>> Cc: Stanley Chu <stanley.chu@mediatek.com>
>> Cc: Tomas Winkler <tomas.winkler@intel.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 80b028ba39e9..4d9bb1932b39 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1875,12 +1875,12 @@ void ufshcd_send_command(struct ufs_hba *hba,
>> unsigned int task_tag)
>>  {
>>      hba->lrb[task_tag].issue_time_stamp = ktime_get();
>>      hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0);
>> +    ufshcd_add_command_trace(hba, task_tag, "send");
> 
> How about moving it after __set_bit(task_tag, &hba->outstanding_reqs);?
> 
> Thanks,
> 
> Can Guo.
> 
>>      ufshcd_clk_scaling_start_busy(hba);
>>      __set_bit(task_tag, &hba->outstanding_reqs);
>>      ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>      /* Make sure that doorbell is committed immediately */
>>      wmb();
>> -    ufshcd_add_command_trace(hba, task_tag, "send");
>>  }

Hi Can,

As far as I can see ufshcd_add_command_trace() does not read 
hba->outstanding_reqs so calling ufshcd_add_command_trace() before 
changing outstanding_reqs should be fine.

The order chosen in the posted patch should minimize energy usage of the 
UFS controller, namely by calling the tracing function before starting 
clock scaling.

Thanks,

Bart.

Re: [PATCH 5/6] ufs: Remove superfluous memory barriers

[Bart Van Assche]

On 12/25/19 2:31 AM, Can Guo wrote:
> On 2019-12-25 06:02, Bart Van Assche wrote:
>> Calling wmb() after having written to a doorbell slows down code and does
>> not help to commit the doorbell write faster. Hence remove such wmb()
>> calls. Note: detailed information about the semantics of writel() is
>> available in Documentation/driver-api/device-io.rst.
>>
>> Cc: Bean Huo <beanhuo@micron.com>
>> Cc: Can Guo <cang@codeaurora.org>
>> Cc: Avri Altman <avri.altman@wdc.com>
>> Cc: Stanley Chu <stanley.chu@mediatek.com>
>> Cc: Tomas Winkler <tomas.winkler@intel.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 4d9bb1932b39..edcc137c436b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1879,8 +1879,6 @@ void ufshcd_send_command(struct ufs_hba *hba,
>> unsigned int task_tag)
>>      ufshcd_clk_scaling_start_busy(hba);
>>      __set_bit(task_tag, &hba->outstanding_reqs);
>>      ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> -    /* Make sure that doorbell is committed immediately */
>> -    wmb();
>>  }
>>
>>  /**
>> @@ -5766,8 +5764,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba 
>> *hba,
>>      wmb();
>>
>>      ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
>> -    /* Make sure that doorbell is committed immediately */
>> -    wmb();
>>
>>      spin_unlock_irqrestore(host->host_lock, flags);
> 
> Hi Bart,
> 
> Three wmb()s were added in commit ad1a1b9cd because we did see instances on
> which OCS=3(MISMATCH_DATA_BUFFER_SIZE) error were observed in large scale
> test. Commit ad1a1b9cd fixed the error and we had confirmed it through
> large amount of tests. I am not sure removing the 2 wmb()s here would cause
> regression or not.

Hi Can,

Thank you for having reported this. I will drop this patch.

Bart.

Re: [PATCH 1/6] ufs: Fix indentation in ufshcd_query_attr_retry()

[Alim Akhtar]

Hi Bart

On Wed, Dec 25, 2019 at 3:33 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Remove a space that occurs after a tab.
>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Reviewed-by: Alim Akhar <alim.akhtar@samsung.com>
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d6d0f83c9044..48f2f94d51bc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2869,7 +2869,7 @@ static int ufshcd_query_attr_retry(struct ufs_hba *hba,
>         int ret = 0;
>         u32 retries;
>
> -        for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> +       for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
>                 ret = ufshcd_query_attr(hba, opcode, idn, index,
>                                                 selector, attr_val);
>                 if (ret)



-- 
Regards,
Alim

Re: [PATCH 2/6] ufs: Make ufshcd_add_command_trace() easier to read

[Alim Akhtar]

On Wed, Dec 25, 2019 at 3:34 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Since the lrbp->cmd expression occurs multiple times, introduce a new
> local variable to hold that pointer. This patch does not change any
> functionality.
>
FYMI, Any Advantage of doing this? or it just we don't want to fetch
cmd from lrbp every time? I mean in terms of execution speed.

> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
>  drivers/scsi/ufs/ufshcd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 48f2f94d51bc..acc84e964e8f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -327,27 +327,27 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
>         u8 opcode = 0;
>         u32 intr, doorbell;
>         struct ufshcd_lrb *lrbp = &hba->lrb[tag];
> +       struct scsi_cmnd *cmd = lrbp->cmd;
>         int transfer_len = -1;
>
>         if (!trace_ufshcd_command_enabled()) {
>                 /* trace UPIU W/O tracing command */
> -               if (lrbp->cmd)
> +               if (cmd)
>                         ufshcd_add_cmd_upiu_trace(hba, tag, str);
>                 return;
>         }
>
> -       if (lrbp->cmd) { /* data phase exists */
> +       if (cmd) { /* data phase exists */
>                 /* trace UPIU also */
>                 ufshcd_add_cmd_upiu_trace(hba, tag, str);
> -               opcode = (u8)(*lrbp->cmd->cmnd);
> +               opcode = cmd->cmnd[0];
>                 if ((opcode == READ_10) || (opcode == WRITE_10)) {
>                         /*
>                          * Currently we only fully trace read(10) and write(10)
>                          * commands
>                          */
> -                       if (lrbp->cmd->request && lrbp->cmd->request->bio)
> -                               lba =
> -                                 lrbp->cmd->request->bio->bi_iter.bi_sector;
> +                       if (cmd->request && cmd->request->bio)
> +                               lba = cmd->request->bio->bi_iter.bi_sector;
>                         transfer_len = be32_to_cpu(
>                                 lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
>                 }



-- 
Regards,
Alim

Re: [PATCH 4/6] ufs: Fix a race condition in the tracing code

[Alim Akhtar]

On Wed, Dec 25, 2019 at 3:34 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Starting execution of a command before tracing a command may cause the
> completion handler to free data while it is being traced. Fix this race
> by tracing a command before it is submitted.
>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 80b028ba39e9..4d9bb1932b39 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1875,12 +1875,12 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
>  {
>         hba->lrb[task_tag].issue_time_stamp = ktime_get();
>         hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0);
> +       ufshcd_add_command_trace(hba, task_tag, "send");
>         ufshcd_clk_scaling_start_busy(hba);
>         __set_bit(task_tag, &hba->outstanding_reqs);
>         ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>         /* Make sure that doorbell is committed immediately */
>         wmb();
> -       ufshcd_add_command_trace(hba, task_tag, "send");
>  }
>
>  /**



-- 
Regards,
Alim

Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler

[Alim Akhtar]

On Wed, Dec 25, 2019 at 3:35 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> The UFS SCSI timeout handler was needed to compensate that
> ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
> time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by eliminating
> tag conflicts") fixed this so the timeout handler is no longer necessary.
>
> See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout handler").
>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/scsi/ufs/ufshcd.c | 36 ------------------------------------
>  1 file changed, 36 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index edcc137c436b..763e41286a4b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7092,41 +7092,6 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>         ufshcd_probe_hba(hba);
>  }
>
> -static enum blk_eh_timer_return ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
> -{
> -       unsigned long flags;
> -       struct Scsi_Host *host;
> -       struct ufs_hba *hba;
> -       int index;
> -       bool found = false;
> -
> -       if (!scmd || !scmd->device || !scmd->device->host)
> -               return BLK_EH_DONE;
> -
> -       host = scmd->device->host;
> -       hba = shost_priv(host);
> -       if (!hba)
> -               return BLK_EH_DONE;
> -
> -       spin_lock_irqsave(host->host_lock, flags);
> -
> -       for_each_set_bit(index, &hba->outstanding_reqs, hba->nutrs) {
> -               if (hba->lrb[index].cmd == scmd) {
> -                       found = true;
> -                       break;
> -               }
> -       }
> -
> -       spin_unlock_irqrestore(host->host_lock, flags);
> -
> -       /*
> -        * Bypass SCSI error handling and reset the block layer timer if this
> -        * SCSI command was not actually dispatched to UFS driver, otherwise
> -        * let SCSI layer handle the error as usual.
> -        */
> -       return found ? BLK_EH_DONE : BLK_EH_RESET_TIMER;
> -}
> -
>  static const struct attribute_group *ufshcd_driver_groups[] = {
>         &ufs_sysfs_unit_descriptor_group,
>         &ufs_sysfs_lun_attributes_group,
> @@ -7145,7 +7110,6 @@ static struct scsi_host_template ufshcd_driver_template = {
>         .eh_abort_handler       = ufshcd_abort,
>         .eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>         .eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
> -       .eh_timed_out           = ufshcd_eh_timed_out,
>         .this_id                = -1,
>         .sg_tablesize           = SG_ALL,
>         .cmd_per_lun            = UFSHCD_CMD_PER_LUN,



-- 
Regards,
Alim

Re: [PATCH 4/6] ufs: Fix a race condition in the tracing code

[Can Guo]

On 2019-12-27 01:35, Bart Van Assche wrote:
> On 12/25/19 2:59 AM, Can Guo wrote:
>> On 2019-12-25 06:02, Bart Van Assche wrote:
>>> Starting execution of a command before tracing a command may cause 
>>> the
>>> completion handler to free data while it is being traced. Fix this 
>>> race
>>> by tracing a command before it is submitted.
>>> 
>>> Cc: Bean Huo <beanhuo@micron.com>
>>> Cc: Can Guo <cang@codeaurora.org>
>>> Cc: Avri Altman <avri.altman@wdc.com>
>>> Cc: Stanley Chu <stanley.chu@mediatek.com>
>>> Cc: Tomas Winkler <tomas.winkler@intel.com>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 80b028ba39e9..4d9bb1932b39 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -1875,12 +1875,12 @@ void ufshcd_send_command(struct ufs_hba *hba,
>>> unsigned int task_tag)
>>>  {
>>>      hba->lrb[task_tag].issue_time_stamp = ktime_get();
>>>      hba->lrb[task_tag].compl_time_stamp = ktime_set(0, 0);
>>> +    ufshcd_add_command_trace(hba, task_tag, "send");
>> 
>> How about moving it after __set_bit(task_tag, 
>> &hba->outstanding_reqs);?
>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>>>      ufshcd_clk_scaling_start_busy(hba);
>>>      __set_bit(task_tag, &hba->outstanding_reqs);
>>>      ufshcd_writel(hba, 1 << task_tag, 
>>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>>      /* Make sure that doorbell is committed immediately */
>>>      wmb();
>>> -    ufshcd_add_command_trace(hba, task_tag, "send");
>>>  }
> 
> Hi Can,
> 
> As far as I can see ufshcd_add_command_trace() does not read
> hba->outstanding_reqs so calling ufshcd_add_command_trace() before
> changing outstanding_reqs should be fine.
> 
> The order chosen in the posted patch should minimize energy usage of
> the UFS controller, namely by calling the tracing function before
> starting clock scaling.
> 
> Thanks,
> 
> Bart.

True.

One concern regards this change is that since we move the 
ufshcd_add_command_trace() before ringing doorbell, in tracing log,
we will lose the info/status of this specific tag in doorbell register.

Usually, with the old code, after we collect the tracing log, we can
tell whether the doorbell bit was set/rung correctly in doorbell
register or not for an specific tag/task, because it is an important
sign of whether HW had actually accepted the task on that tag. It
is really helpful sometime if things go wrong.

Any good ways to keep the doorbell info as before?

Thanks,

Can Guo.

Re: [PATCH 0/6] Six UFS patches

[Martin K. Petersen]

Bart,

> This patch series includes patches that clean up the UFS driver code
> somewhat.  Please consider these patches for kernel v5.6.

Applied 1-4,6 to 5.6/scsi-queue. Thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler

[Can Guo]

Hi Bart,

On 2019-12-25 06:02, Bart Van Assche wrote:
> The UFS SCSI timeout handler was needed to compensate that
> ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
> time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by 
> eliminating
> tag conflicts") fixed this so the timeout handler is no longer 
> necessary.
> 
> See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout 
> handler").
> 

Sorry for bugging you on this old change. I am afraid we may need to add
this timeout handler back. Because there is till chances that a request
gets stuck somewhere in ufshcd_queuecommand() path before
ufshcd_send_command() gets called. e.g.

ufshcd_queuecommand()
->ufshcd_map_sg()
-->scsi_dma_map()
--->dma_map_sg()
---->dev->ops->map_sg()

map_sg() ops may get stuck. map_sg() method can vary on different 
platforms
based on actual IOMMU engines. We cannot gaurantee map_sg() ops must 
return
immediately as we don't know what is actually inside map_sg() ops.

And if it gets stuck there for a long time till the request times out, 
without
the UFS timeout handler, scsi layer will try to abort this request from 
UFS
driver by calling ufshcd_abort() eventually. ufshcd_abort() will think 
this
request has been completed due to its tag is not in 
hba->outstanding_reqs
or UFS host's door bell reg. However, actually, this request is still in
ufshcd_queuecommand() path. I don't need to continue on the subsequent 
impact
to UFS driver if ufshcd_abort() happens in this case. This is a corner 
case,
but it is still possible (I did see map_sg() ops hangs on real devices).

Having the UFS timeout handler back will prevent this situation as UFS 
timeout
handler checks if the tag is in hba->outstanding_reqs (for our case, it 
is not
in there), if no, it returns BLK_EH_RESET_TIMER so that scsi/block layer 
will
keep waiting.

What do you think? Please let me know your ideas on this, thanks!

<--code snippet-->
static int ufshcd_abort(struct scsi_cmnd *cmd)
{
...
         reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
         /* If command is already aborted/completed, return SUCCESS */
         if (!(test_bit(tag, &hba->outstanding_reqs))) {
                 dev_err(hba->dev,
                         "%s: cmd at tag %d already completed, 
outstanding=0x%lx, doorbell=0x%x\n",
                         __func__, tag, hba->outstanding_reqs, reg);
                 goto out;
         }
...
}
<--code snippet-->

Thanks,
Can Guo.

> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 36 ------------------------------------
>  1 file changed, 36 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index edcc137c436b..763e41286a4b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7092,41 +7092,6 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>  	ufshcd_probe_hba(hba);
>  }
> 
> -static enum blk_eh_timer_return ufshcd_eh_timed_out(struct scsi_cmnd 
> *scmd)
> -{
> -	unsigned long flags;
> -	struct Scsi_Host *host;
> -	struct ufs_hba *hba;
> -	int index;
> -	bool found = false;
> -
> -	if (!scmd || !scmd->device || !scmd->device->host)
> -		return BLK_EH_DONE;
> -
> -	host = scmd->device->host;
> -	hba = shost_priv(host);
> -	if (!hba)
> -		return BLK_EH_DONE;
> -
> -	spin_lock_irqsave(host->host_lock, flags);
> -
> -	for_each_set_bit(index, &hba->outstanding_reqs, hba->nutrs) {
> -		if (hba->lrb[index].cmd == scmd) {
> -			found = true;
> -			break;
> -		}
> -	}
> -
> -	spin_unlock_irqrestore(host->host_lock, flags);
> -
> -	/*
> -	 * Bypass SCSI error handling and reset the block layer timer if this
> -	 * SCSI command was not actually dispatched to UFS driver, otherwise
> -	 * let SCSI layer handle the error as usual.
> -	 */
> -	return found ? BLK_EH_DONE : BLK_EH_RESET_TIMER;
> -}
> -
>  static const struct attribute_group *ufshcd_driver_groups[] = {
>  	&ufs_sysfs_unit_descriptor_group,
>  	&ufs_sysfs_lun_attributes_group,
> @@ -7145,7 +7110,6 @@ static struct scsi_host_template
> ufshcd_driver_template = {
>  	.eh_abort_handler	= ufshcd_abort,
>  	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>  	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
> -	.eh_timed_out		= ufshcd_eh_timed_out,
>  	.this_id		= -1,
>  	.sg_tablesize		= SG_ALL,
>  	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,

Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler

[Bart Van Assche]

On 2020-05-28 02:47, Can Guo wrote:
> Hi Bart,
> 
> On 2019-12-25 06:02, Bart Van Assche wrote:
>> The UFS SCSI timeout handler was needed to compensate that
>> ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
>> time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by eliminating
>> tag conflicts") fixed this so the timeout handler is no longer necessary.
>>
>> See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout
>> handler").
>>
> 
> Sorry for bugging you on this old change. I am afraid we may need to add
> this timeout handler back. Because there is till chances that a request
> gets stuck somewhere in ufshcd_queuecommand() path before
> ufshcd_send_command() gets called. e.g.
> 
> ufshcd_queuecommand()
> ->ufshcd_map_sg()
> -->scsi_dma_map()
> --->dma_map_sg()
> ---->dev->ops->map_sg()
> 
> map_sg() ops may get stuck. map_sg() method can vary on different platforms
> based on actual IOMMU engines. We cannot gaurantee map_sg() ops must return
> immediately as we don't know what is actually inside map_sg() ops.
> 
> And if it gets stuck there for a long time till the request times out,
> without
> the UFS timeout handler, scsi layer will try to abort this request from UFS
> driver by calling ufshcd_abort() eventually. ufshcd_abort() will think this
> request has been completed due to its tag is not in hba->outstanding_reqs
> or UFS host's door bell reg. However, actually, this request is still in
> ufshcd_queuecommand() path. I don't need to continue on the subsequent
> impact
> to UFS driver if ufshcd_abort() happens in this case. This is a corner
> case,
> but it is still possible (I did see map_sg() ops hangs on real devices).
> 
> Having the UFS timeout handler back will prevent this situation as UFS
> timeout
> handler checks if the tag is in hba->outstanding_reqs (for our case, it
> is not
> in there), if no, it returns BLK_EH_RESET_TIMER so that scsi/block layer
> will
> keep waiting.
> 
> What do you think? Please let me know your ideas on this, thanks!

Hi Can,

I see the following issues with the above proposal:
- Although I haven't been able to find explicit documentation of this, I
  think that dma_map_sg() must not sleep. If it would sleep that would
  break most block and SCSI drivers because many of these drivers call
  dma_map_sg() from their .queue_rq() or .queuecommand() implementation
  and if BLK_MQ_F_BLOCKING has not been set these functions must not
  sleep.
- A timeout handler must not be invoked while .queuecommand() is still
  in progress. The SCSI core calls blk_mq_start_request() before it
  calls ufshcd_queuecommand(). The blk_mq_start_request() activates the
  block layer timeout mechanism. ufshcd_queuecommand() must have
  finished before the block layer timeout handler is activated.

Please fix the root cause, namely the map_sg implementation that may get
stuck.

Thanks,

Bart.

Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler

[Can Guo]

On 2020-05-29 00:12, Bart Van Assche wrote:
> On 2020-05-28 02:47, Can Guo wrote:
>> Hi Bart,
>> 
>> On 2019-12-25 06:02, Bart Van Assche wrote:
>>> The UFS SCSI timeout handler was needed to compensate that
>>> ufshcd_queuecommand() could return SCSI_MLQUEUE_HOST_BUSY for a long
>>> time. Commit a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by 
>>> eliminating
>>> tag conflicts") fixed this so the timeout handler is no longer 
>>> necessary.
>>> 
>>> See also commit f550c65b543b ("scsi: ufs: implement scsi host timeout
>>> handler").
>>> 
>> 
>> Sorry for bugging you on this old change. I am afraid we may need to 
>> add
>> this timeout handler back. Because there is till chances that a 
>> request
>> gets stuck somewhere in ufshcd_queuecommand() path before
>> ufshcd_send_command() gets called. e.g.
>> 
>> ufshcd_queuecommand()
>> ->ufshcd_map_sg()
>> -->scsi_dma_map()
>> --->dma_map_sg()
>> ---->dev->ops->map_sg()
>> 
>> map_sg() ops may get stuck. map_sg() method can vary on different 
>> platforms
>> based on actual IOMMU engines. We cannot gaurantee map_sg() ops must 
>> return
>> immediately as we don't know what is actually inside map_sg() ops.
>> 
>> And if it gets stuck there for a long time till the request times out,
>> without
>> the UFS timeout handler, scsi layer will try to abort this request 
>> from UFS
>> driver by calling ufshcd_abort() eventually. ufshcd_abort() will think 
>> this
>> request has been completed due to its tag is not in 
>> hba->outstanding_reqs
>> or UFS host's door bell reg. However, actually, this request is still 
>> in
>> ufshcd_queuecommand() path. I don't need to continue on the subsequent
>> impact
>> to UFS driver if ufshcd_abort() happens in this case. This is a corner
>> case,
>> but it is still possible (I did see map_sg() ops hangs on real 
>> devices).
>> 
>> Having the UFS timeout handler back will prevent this situation as UFS
>> timeout
>> handler checks if the tag is in hba->outstanding_reqs (for our case, 
>> it
>> is not
>> in there), if no, it returns BLK_EH_RESET_TIMER so that scsi/block 
>> layer
>> will
>> keep waiting.
>> 
>> What do you think? Please let me know your ideas on this, thanks!

Hi Bart,

> 
> Hi Can,
> 
> I see the following issues with the above proposal:
> - Although I haven't been able to find explicit documentation of this, 
> I
>   think that dma_map_sg() must not sleep. If it would sleep that would
>   break most block and SCSI drivers because many of these drivers call
>   dma_map_sg() from their .queue_rq() or .queuecommand() implementation
>   and if BLK_MQ_F_BLOCKING has not been set these functions must not
>   sleep.
> - A timeout handler must not be invoked while .queuecommand() is still
>   in progress. The SCSI core calls blk_mq_start_request() before it
>   calls ufshcd_queuecommand(). The blk_mq_start_request() activates the
>   block layer timeout mechanism. ufshcd_queuecommand() must have
>   finished before the block layer timeout handler is activated.
> 
> Please fix the root cause, namely the map_sg implementation that may 
> get
> stuck.
> 
> Thanks,
> 
> Bart.

queuecommand path should not sleep - that is right, due to queuecommand
can be invoked from contexts where preempt is disabled, e.g. softirq.

I don't know why map_sg() ops can take that long, but apparently it does
not sleep, otherwise we should have seen schedule while atomic error 
long
time ago.

> ufshcd_queuecommand() must have
> finished before the block layer timeout handler is activated.
This is the ideal/expected situation, but we are seeing the corner case.

Fixing the root cause of that is one thing, but having the timeout 
handler
back can prevent UFS driver from messing up the subsequent requests 
further
in such case, causing possible data corruption. Is there any drawbacks 
if
we have it back?

Thanks,
Can Guo.

Re: [PATCH 6/6] ufs: Remove the SCSI timeout handler

[Bart Van Assche]

On 2020-05-28 18:39, Can Guo wrote:
> On 2020-05-29 00:12, Bart Van Assche wrote:
>> ufshcd_queuecommand() must have
>> finished before the block layer timeout handler is activated.
>
> This is the ideal/expected situation, but we are seeing the corner case.
> 
> Fixing the root cause of that is one thing, but having the timeout handler
> back can prevent UFS driver from messing up the subsequent requests further
> in such case, causing possible data corruption. Is there any drawbacks if
> we have it back?

Hi Can,

My conclusion from your emails is that ufshcd_queuecommand() can spend
more time than the SCSI timeout (30 seconds) in dma_map_sg(). A
dma_map_sg() call that keeps the CPU busy during more than 30 seconds is
not only weird but it is also a disaster from the point of view of
energy consumption. Please fix the root cause.

Thanks,

Bart.