[PATCH v3 00/15] Big fixes, retries, handle a race condition

[PATCH v3 00/15] Big fixes, retries, handle a race condition

[Yaniv Gardi]

Important:
This serie of 15 small patches should be pushed after the series of 8 patches
"Fix error message and present UFS variant probe"

V3:
removed specific calls to wmb() since they are redundant.

V2:
a few minor changes

V1:
This serie of 15 small patches should be pushed after the series of 8 patches
I have uploaded to the upstream a week ago:
"Fix error message and present UFS variant probe"

Yaniv Gardi (15):
  scsi: ufs: clear UTRD, UPIU req and rsp before new transfers
  scsi: ufs: clear fields UTRD, UPIU req and rsp before new transfers
  scsi: ufs: verify command tag validity
  scsi: ufs: clear outstanding_request bit in case query timeout
  scsi: ufs: increase fDeviceInit query response timeout
  scsi: ufs: avoid exception event handler racing with PM callbacks
  scsi: ufs: set REQUEST_SENSE command size to 18 bytes
  scsi: ufs: add retries to dme_peer get and set attribute
  scsi: ufs: add retries for hibern8 enter
  scsi: ufs: fix error recovery after the hibern8 exit failure
  scsi: ufs: retry failed query flag requests
  scsi: ufs: reduce the interrupts for power mode change requests
  scsi: ufs: add missing memory barriers
  scsi: ufs: commit descriptors before setting the doorbell
  scsi: ufs: add wrapper for retrying sending query attribute

 drivers/scsi/ufs/ufshcd.c | 409 ++++++++++++++++++++++++++++++++++++----------
 drivers/scsi/ufs/ufshcd.h |   4 +
 2 files changed, 328 insertions(+), 85 deletions(-)

-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 01/15] scsi: ufs: clear UTRD, UPIU req and rsp before new transfers

[Yaniv Gardi]

Clear the UFS data structures before sending new request.

The SCSI command is sent to the device within the UFS UPIU request.
As part of the transfer UPIU preparation, the SCSI command is copied
to the UPIU structure according to the SCSI command size.
As different SCSI commands differ in size from each other, we need
to clear the whole SCSI command field to prevent sending uninitialized
data to the device.

The UPIU response doesn't always include the sense data and can differ
in size.
Hence, the UPIU response should also be cleared before the transfer.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Maya Erez <merez@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 131c720..3428f72 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3,7 +3,7 @@
  *
  * This code is based on drivers/scsi/ufs/ufshcd.c
  * Copyright (C) 2011-2013 Samsung India Software Operations
- * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
  *
  * Authors:
  *	Santosh Yaraganavi <santosh.sy@samsung.com>
@@ -1035,6 +1035,7 @@ static int ufshcd_map_sg(struct ufshcd_lrb *lrbp)
 				cpu_to_le32(lower_32_bits(sg->dma_address));
 			prd_table[i].upper_addr =
 				cpu_to_le32(upper_32_bits(sg->dma_address));
+			prd_table[i].reserved = 0;
 		}
 	} else {
 		lrbp->utr_descriptor_ptr->prd_table_length = 0;
@@ -1117,7 +1118,8 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
 
 	/* Transfer request descriptor header fields */
 	req_desc->header.dword_0 = cpu_to_le32(dword_0);
-
+	/* dword_1 is reserved, hence it is set to 0 */
+	req_desc->header.dword_1 = 0;
 	/*
 	 * assigning invalid value for command status. Controller
 	 * updates OCS on command completion, with the command
@@ -1125,6 +1127,8 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
 	 */
 	req_desc->header.dword_2 =
 		cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+	/* dword_3 is reserved, hence it is set to 0 */
+	req_desc->header.dword_3 = 0;
 }
 
 /**
@@ -1137,6 +1141,7 @@ static
 void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u32 upiu_flags)
 {
 	struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
+	unsigned short cdb_len;
 
 	/* command descriptor fields */
 	ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD(
@@ -1151,8 +1156,12 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u32 upiu_flags)
 	ucd_req_ptr->sc.exp_data_transfer_len =
 		cpu_to_be32(lrbp->cmd->sdb.length);
 
-	memcpy(ucd_req_ptr->sc.cdb, lrbp->cmd->cmnd,
-		(min_t(unsigned short, lrbp->cmd->cmd_len, MAX_CDB_SIZE)));
+	cdb_len = min_t(unsigned short, lrbp->cmd->cmd_len, MAX_CDB_SIZE);
+	memcpy(ucd_req_ptr->sc.cdb, lrbp->cmd->cmnd, cdb_len);
+	if (cdb_len < MAX_CDB_SIZE)
+		memset(ucd_req_ptr->sc.cdb + cdb_len, 0,
+		       (MAX_CDB_SIZE - cdb_len));
+	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 }
 
 /**
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 02/15] scsi: ufs: clear fields UTRD, UPIU req and rsp before new transfers

[Yaniv Gardi]

Some of the data structures (like response UPIU) and/or its elements
(unused fields) should be cleared before sending out the respective
command to UFS device.

This change clears the UPIU response data structure for query commands
and NOP command before sending out the command. We also initialize the
PRDT table length to zero which should take care of commands which doesn't
have any data associated with it. We are also clearing the unused fields in
request UPIU for NOP command.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3428f72..2d3ebca 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1129,6 +1129,8 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
 		cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
 	/* dword_3 is reserved, hence it is set to 0 */
 	req_desc->header.dword_3 = 0;
+
+	req_desc->prd_table_length = 0;
 }
 
 /**
@@ -1198,6 +1200,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
 	if (query->request.upiu_req.opcode == UPIU_QUERY_OPCODE_WRITE_DESC)
 		memcpy(descp, query->descriptor, len);
 
+	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 }
 
 static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
@@ -1210,6 +1213,11 @@ static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
 	ucd_req_ptr->header.dword_0 =
 		UPIU_HEADER_DWORD(
 			UPIU_TRANSACTION_NOP_OUT, 0, 0, lrbp->task_tag);
+	/* clear rest of the fields of basic header */
+	ucd_req_ptr->header.dword_1 = 0;
+	ucd_req_ptr->header.dword_2 = 0;
+
+	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 }
 
 /**
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 02/15] scsi: ufs: clear fields UTRD, UPIU req and rsp before new transfers

[subhashj]

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>



> Some of the data structures (like response UPIU) and/or its elements
> (unused fields) should be cleared before sending out the respective
> command to UFS device.
>
> This change clears the UPIU response data structure for query commands
> and NOP command before sending out the command. We also initialize the
> PRDT table length to zero which should take care of commands which doesn't
> have any data associated with it. We are also clearing the unused fields
> in
> request UPIU for NOP command.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3428f72..2d3ebca 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1129,6 +1129,8 @@ static void ufshcd_prepare_req_desc_hdr(struct
> ufshcd_lrb *lrbp,
>  		cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>  	/* dword_3 is reserved, hence it is set to 0 */
>  	req_desc->header.dword_3 = 0;
> +
> +	req_desc->prd_table_length = 0;
>  }
>
>  /**
> @@ -1198,6 +1200,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct
> ufs_hba *hba,
>  	if (query->request.upiu_req.opcode == UPIU_QUERY_OPCODE_WRITE_DESC)
>  		memcpy(descp, query->descriptor, len);
>
> +	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
>  }
>
>  static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
> @@ -1210,6 +1213,11 @@ static inline void
> ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
>  	ucd_req_ptr->header.dword_0 =
>  		UPIU_HEADER_DWORD(
>  			UPIU_TRANSACTION_NOP_OUT, 0, 0, lrbp->task_tag);
> +	/* clear rest of the fields of basic header */
> +	ucd_req_ptr->header.dword_1 = 0;
> +	ucd_req_ptr->header.dword_2 = 0;
> +
> +	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
>  }
>
>  /**
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 02/15] scsi: ufs: clear fields UTRD, UPIU req and rsp before new transfers

[subhashj]

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>



> Some of the data structures (like response UPIU) and/or its elements
> (unused fields) should be cleared before sending out the respective
> command to UFS device.
>
> This change clears the UPIU response data structure for query commands
> and NOP command before sending out the command. We also initialize the
> PRDT table length to zero which should take care of commands which doesn't
> have any data associated with it. We are also clearing the unused fields
> in
> request UPIU for NOP command.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3428f72..2d3ebca 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1129,6 +1129,8 @@ static void ufshcd_prepare_req_desc_hdr(struct
> ufshcd_lrb *lrbp,
>  		cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>  	/* dword_3 is reserved, hence it is set to 0 */
>  	req_desc->header.dword_3 = 0;
> +
> +	req_desc->prd_table_length = 0;
>  }
>
>  /**
> @@ -1198,6 +1200,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct
> ufs_hba *hba,
>  	if (query->request.upiu_req.opcode == UPIU_QUERY_OPCODE_WRITE_DESC)
>  		memcpy(descp, query->descriptor, len);
>
> +	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
>  }
>
>  static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
> @@ -1210,6 +1213,11 @@ static inline void
> ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
>  	ucd_req_ptr->header.dword_0 =
>  		UPIU_HEADER_DWORD(
>  			UPIU_TRANSACTION_NOP_OUT, 0, 0, lrbp->task_tag);
> +	/* clear rest of the fields of basic header */
> +	ucd_req_ptr->header.dword_1 = 0;
> +	ucd_req_ptr->header.dword_2 = 0;
> +
> +	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
>  }
>
>  /**
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v3 03/15] scsi: ufs: verify command tag validity

[Yaniv Gardi]

A race condition appear to exist between request completion when
scsi_done() is called to end the request and set the tag back to
-1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error
handling which aborts the command and reuses it to request sense
data. Sending the request sense is done with tag which was set to -1
and so it is invalid.
Assert command tag passed from scsi layer is valid.

Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2d3ebca..8860a57 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
 		struct ufs_pa_layer_attr *desired_pwr_mode);
 static int ufshcd_change_power_mode(struct ufs_hba *hba,
 			     struct ufs_pa_layer_attr *pwr_mode);
+static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
+{
+	return tag >= 0 && tag < hba->nutrs;
+}
 
 static inline int ufshcd_enable_irq(struct ufs_hba *hba)
 {
@@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	hba = shost_priv(host);
 
 	tag = cmd->request->tag;
+	if (!ufshcd_valid_tag(hba, tag)) {
+		dev_err(hba->dev,
+			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
+			__func__, tag, cmd, cmd->request);
+		BUG();
+	}
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	switch (hba->ufshcd_state) {
@@ -3862,13 +3872,23 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	host = cmd->device->host;
 	hba = shost_priv(host);
 	tag = cmd->request->tag;
+	if (!ufshcd_valid_tag(hba, tag)) {
+		dev_err(hba->dev,
+			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
+			__func__, tag, cmd, cmd->request);
+		BUG();
+	}
 
 	ufshcd_hold(hba, false);
+	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)))
+	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;
+	}
 
-	reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	if (!(reg & (1 << tag))) {
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity

[Akinobu Mita]

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> A race condition appear to exist between request completion when
> scsi_done() is called to end the request and set the tag back to
> -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error
> handling which aborts the command and reuses it to request sense
> data. Sending the request sense is done with tag which was set to -1
> and so it is invalid.
> Assert command tag passed from scsi layer is valid.
>
> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2d3ebca..8860a57 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
>                 struct ufs_pa_layer_attr *desired_pwr_mode);
>  static int ufshcd_change_power_mode(struct ufs_hba *hba,
>                              struct ufs_pa_layer_attr *pwr_mode);
> +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
> +{
> +       return tag >= 0 && tag < hba->nutrs;
> +}
>
>  static inline int ufshcd_enable_irq(struct ufs_hba *hba)
>  {
> @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>         hba = shost_priv(host);
>
>         tag = cmd->request->tag;
> +       if (!ufshcd_valid_tag(hba, tag)) {
> +               dev_err(hba->dev,
> +                       "%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
> +                       __func__, tag, cmd, cmd->request);
> +               BUG();
> +       }

Is it better to avoid BUG() by WARN_ON() and return if possible?

Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity

[ygardi]

> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> A race condition appear to exist between request completion when
>> scsi_done() is called to end the request and set the tag back to
>> -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error
>> handling which aborts the command and reuses it to request sense
>> data. Sending the request sense is done with tag which was set to -1
>> and so it is invalid.
>> Assert command tag passed from scsi layer is valid.
>>
>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 2d3ebca..8860a57 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba
>> *hba,
>>                 struct ufs_pa_layer_attr *desired_pwr_mode);
>>  static int ufshcd_change_power_mode(struct ufs_hba *hba,
>>                              struct ufs_pa_layer_attr *pwr_mode);
>> +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
>> +{
>> +       return tag >= 0 && tag < hba->nutrs;
>> +}
>>
>>  static inline int ufshcd_enable_irq(struct ufs_hba *hba)
>>  {
>> @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>>         hba = shost_priv(host);
>>
>>         tag = cmd->request->tag;
>> +       if (!ufshcd_valid_tag(hba, tag)) {
>> +               dev_err(hba->dev,
>> +                       "%s: invalid command tag %d: cmd=0x%p,
>> cmd->request=0x%p",
>> +                       __func__, tag, cmd, cmd->request);
>> +               BUG();
>> +       }
>
> Is it better to avoid BUG() by WARN_ON() and return if possible?

in this specific case, i think BUG() is the way to handle this scenario.
It is very rare, and if invalid_tag is sent, the SW can not proceed, and
it indicates something very wrong happened. either in the block layer that
allocated the tag, or in the UFS that reported nutrs.
So, if we actually hit this scenario, then recovering is not an option and
i believe we need to BUG. hope it makes sense.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity

[subhashj]

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>


> A race condition appear to exist between request completion when
> scsi_done() is called to end the request and set the tag back to
> -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error
> handling which aborts the command and reuses it to request sense
> data. Sending the request sense is done with tag which was set to -1
> and so it is invalid.
> Assert command tag passed from scsi layer is valid.
>
> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2d3ebca..8860a57 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba
> *hba,
>  		struct ufs_pa_layer_attr *desired_pwr_mode);
>  static int ufshcd_change_power_mode(struct ufs_hba *hba,
>  			     struct ufs_pa_layer_attr *pwr_mode);
> +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
> +{
> +	return tag >= 0 && tag < hba->nutrs;
> +}
>
>  static inline int ufshcd_enable_irq(struct ufs_hba *hba)
>  {
> @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>  	hba = shost_priv(host);
>
>  	tag = cmd->request->tag;
> +	if (!ufshcd_valid_tag(hba, tag)) {
> +		dev_err(hba->dev,
> +			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
> +			__func__, tag, cmd, cmd->request);
> +		BUG();
> +	}
>
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	switch (hba->ufshcd_state) {
> @@ -3862,13 +3872,23 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	host = cmd->device->host;
>  	hba = shost_priv(host);
>  	tag = cmd->request->tag;
> +	if (!ufshcd_valid_tag(hba, tag)) {
> +		dev_err(hba->dev,
> +			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
> +			__func__, tag, cmd, cmd->request);
> +		BUG();
> +	}
>
>  	ufshcd_hold(hba, false);
> +	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)))
> +	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;
> +	}
>
> -	reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  	if (!(reg & (1 << tag))) {
>  		dev_err(hba->dev,
>  		"%s: cmd was completed, but without a notifying intr, tag = %d",
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity

[subhashj]

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>


> A race condition appear to exist between request completion when
> scsi_done() is called to end the request and set the tag back to
> -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error
> handling which aborts the command and reuses it to request sense
> data. Sending the request sense is done with tag which was set to -1
> and so it is invalid.
> Assert command tag passed from scsi layer is valid.
>
> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2d3ebca..8860a57 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba
> *hba,
>  		struct ufs_pa_layer_attr *desired_pwr_mode);
>  static int ufshcd_change_power_mode(struct ufs_hba *hba,
>  			     struct ufs_pa_layer_attr *pwr_mode);
> +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
> +{
> +	return tag >= 0 && tag < hba->nutrs;
> +}
>
>  static inline int ufshcd_enable_irq(struct ufs_hba *hba)
>  {
> @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>  	hba = shost_priv(host);
>
>  	tag = cmd->request->tag;
> +	if (!ufshcd_valid_tag(hba, tag)) {
> +		dev_err(hba->dev,
> +			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
> +			__func__, tag, cmd, cmd->request);
> +		BUG();
> +	}
>
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	switch (hba->ufshcd_state) {
> @@ -3862,13 +3872,23 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	host = cmd->device->host;
>  	hba = shost_priv(host);
>  	tag = cmd->request->tag;
> +	if (!ufshcd_valid_tag(hba, tag)) {
> +		dev_err(hba->dev,
> +			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
> +			__func__, tag, cmd, cmd->request);
> +		BUG();
> +	}
>
>  	ufshcd_hold(hba, false);
> +	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)))
> +	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;
> +	}
>
> -	reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  	if (!(reg & (1 << tag))) {
>  		dev_err(hba->dev,
>  		"%s: cmd was completed, but without a notifying intr, tag = %d",
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v3 04/15] scsi: ufs: clear outstanding_request bit in case query timeout

[Yaniv Gardi]

When sending a query to the device returns with a timeout error,
we clear the corresponding bit in the DOORBELL register but
we don't clear the outstanding_request field as we should.
This patch fixes this bug.

Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8860a57..e0b8755 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -364,6 +364,16 @@ static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
 }
 
 /**
+ * ufshcd_outstanding_req_clear - Clear a bit in outstanding request field
+ * @hba: per adapter instance
+ * @tag: position of the bit to be cleared
+ */
+static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
+{
+	__clear_bit(tag, &hba->outstanding_reqs);
+}
+
+/**
  * ufshcd_get_lists_status - Check UCRDY, UTRLRDY and UTMRLRDY
  * @reg: Register value of host controller status
  *
@@ -1502,9 +1512,17 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 
 	if (!time_left) {
 		err = -ETIMEDOUT;
+		dev_dbg(hba->dev, "%s: dev_cmd request timedout, tag %d\n",
+			__func__, lrbp->task_tag);
 		if (!ufshcd_clear_cmd(hba, lrbp->task_tag))
-			/* sucessfully cleared the command, retry if needed */
+			/* successfully cleared the command, retry if needed */
 			err = -EAGAIN;
+		/*
+		 * in case of an error, after clearing the doorbell,
+		 * we also need to clear the outstanding_request
+		 * field in hba
+		 */
+		ufshcd_outstanding_req_clear(hba, lrbp->task_tag);
 	}
 
 	return err;
@@ -3942,7 +3960,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	scsi_dma_unmap(cmd);
 
 	spin_lock_irqsave(host->host_lock, flags);
-	__clear_bit(tag, &hba->outstanding_reqs);
+	ufshcd_outstanding_req_clear(hba, tag);
 	hba->lrb[tag].cmd = NULL;
 	spin_unlock_irqrestore(host->host_lock, flags);
 
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 04/15] scsi: ufs: clear outstanding_request bit in case query timeout

[subhashj]

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>



> When sending a query to the device returns with a timeout error,
> we clear the corresponding bit in the DOORBELL register but
> we don't clear the outstanding_request field as we should.
> This patch fixes this bug.
>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8860a57..e0b8755 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -364,6 +364,16 @@ static inline void ufshcd_utrl_clear(struct ufs_hba
> *hba, u32 pos)
>  }
>
>  /**
> + * ufshcd_outstanding_req_clear - Clear a bit in outstanding request
> field
> + * @hba: per adapter instance
> + * @tag: position of the bit to be cleared
> + */
> +static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int
> tag)
> +{
> +	__clear_bit(tag, &hba->outstanding_reqs);
> +}
> +
> +/**
>   * ufshcd_get_lists_status - Check UCRDY, UTRLRDY and UTMRLRDY
>   * @reg: Register value of host controller status
>   *
> @@ -1502,9 +1512,17 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>
>  	if (!time_left) {
>  		err = -ETIMEDOUT;
> +		dev_dbg(hba->dev, "%s: dev_cmd request timedout, tag %d\n",
> +			__func__, lrbp->task_tag);
>  		if (!ufshcd_clear_cmd(hba, lrbp->task_tag))
> -			/* sucessfully cleared the command, retry if needed */
> +			/* successfully cleared the command, retry if needed */
>  			err = -EAGAIN;
> +		/*
> +		 * in case of an error, after clearing the doorbell,
> +		 * we also need to clear the outstanding_request
> +		 * field in hba
> +		 */
> +		ufshcd_outstanding_req_clear(hba, lrbp->task_tag);
>  	}
>
>  	return err;
> @@ -3942,7 +3960,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	scsi_dma_unmap(cmd);
>
>  	spin_lock_irqsave(host->host_lock, flags);
> -	__clear_bit(tag, &hba->outstanding_reqs);
> +	ufshcd_outstanding_req_clear(hba, tag);
>  	hba->lrb[tag].cmd = NULL;
>  	spin_unlock_irqrestore(host->host_lock, flags);
>
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 04/15] scsi: ufs: clear outstanding_request bit in case query timeout

[subhashj]

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>



> When sending a query to the device returns with a timeout error,
> we clear the corresponding bit in the DOORBELL register but
> we don't clear the outstanding_request field as we should.
> This patch fixes this bug.
>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8860a57..e0b8755 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -364,6 +364,16 @@ static inline void ufshcd_utrl_clear(struct ufs_hba
> *hba, u32 pos)
>  }
>
>  /**
> + * ufshcd_outstanding_req_clear - Clear a bit in outstanding request
> field
> + * @hba: per adapter instance
> + * @tag: position of the bit to be cleared
> + */
> +static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int
> tag)
> +{
> +	__clear_bit(tag, &hba->outstanding_reqs);
> +}
> +
> +/**
>   * ufshcd_get_lists_status - Check UCRDY, UTRLRDY and UTMRLRDY
>   * @reg: Register value of host controller status
>   *
> @@ -1502,9 +1512,17 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>
>  	if (!time_left) {
>  		err = -ETIMEDOUT;
> +		dev_dbg(hba->dev, "%s: dev_cmd request timedout, tag %d\n",
> +			__func__, lrbp->task_tag);
>  		if (!ufshcd_clear_cmd(hba, lrbp->task_tag))
> -			/* sucessfully cleared the command, retry if needed */
> +			/* successfully cleared the command, retry if needed */
>  			err = -EAGAIN;
> +		/*
> +		 * in case of an error, after clearing the doorbell,
> +		 * we also need to clear the outstanding_request
> +		 * field in hba
> +		 */
> +		ufshcd_outstanding_req_clear(hba, lrbp->task_tag);
>  	}
>
>  	return err;
> @@ -3942,7 +3960,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	scsi_dma_unmap(cmd);
>
>  	spin_lock_irqsave(host->host_lock, flags);
> -	__clear_bit(tag, &hba->outstanding_reqs);
> +	ufshcd_outstanding_req_clear(hba, tag);
>  	hba->lrb[tag].cmd = NULL;
>  	spin_unlock_irqrestore(host->host_lock, flags);
>
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v3 05/15] scsi: ufs: increase fDeviceInit query response timeout

[Yaniv Gardi]

fDeviceInit query response time for some devices is too long that default
query request timeout of 100ms may not be enough. Experiments show that
fDeviceInit response sometimes takes 500ms so to be on safer side this
change sets the timeout to 600ms. Without this change, we might
unnecessarily have to retry fDeviceInit query requests multiple times and
each query request timeout prints one error message.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e0b8755..573a8cb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -58,6 +58,12 @@
 #define QUERY_REQ_RETRIES 10
 /* Query request timeout */
 #define QUERY_REQ_TIMEOUT 30 /* msec */
+/*
+ * Query request timeout for fDeviceInit flag
+ * fDeviceInit query response time for some devices is too large that default
+ * QUERY_REQ_TIMEOUT may not be enough for such devices.
+ */
+#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
 
 /* Task management command timeout */
 #define TM_CMD_TIMEOUT	100 /* msecs */
@@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 	struct ufs_query_req *request = NULL;
 	struct ufs_query_res *response = NULL;
 	int err, index = 0, selector = 0;
+	int timeout = QUERY_REQ_TIMEOUT;
 
 	BUG_ON(!hba);
 
@@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 		goto out_unlock;
 	}
 
-	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, QUERY_REQ_TIMEOUT);
+	if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
+		timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
+
+	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
 
 	if (err) {
 		dev_err(hba->dev,
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 05/15] scsi: ufs: increase fDeviceInit query response timeout

[Akinobu Mita]

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> fDeviceInit query response time for some devices is too long that default
> query request timeout of 100ms may not be enough. Experiments show that
> fDeviceInit response sometimes takes 500ms so to be on safer side this
> change sets the timeout to 600ms. Without this change, we might
> unnecessarily have to retry fDeviceInit query requests multiple times and
> each query request timeout prints one error message.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e0b8755..573a8cb 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -58,6 +58,12 @@
>  #define QUERY_REQ_RETRIES 10
>  /* Query request timeout */
>  #define QUERY_REQ_TIMEOUT 30 /* msec */
> +/*
> + * Query request timeout for fDeviceInit flag
> + * fDeviceInit query response time for some devices is too large that default
> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
> + */
> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */

How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
instead of conditionally setting timeout depending on ufs flag?

>
>  /* Task management command timeout */
>  #define TM_CMD_TIMEOUT 100 /* msecs */
> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>         struct ufs_query_req *request = NULL;
>         struct ufs_query_res *response = NULL;
>         int err, index = 0, selector = 0;
> +       int timeout = QUERY_REQ_TIMEOUT;
>
>         BUG_ON(!hba);
>
> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>                 goto out_unlock;
>         }
>
> -       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, QUERY_REQ_TIMEOUT);
> +       if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
> +               timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
> +
> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>
>         if (err) {
>                 dev_err(hba->dev,
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 05/15] scsi: ufs: increase fDeviceInit query response timeout

[ygardi]

> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> fDeviceInit query response time for some devices is too long that
>> default
>> query request timeout of 100ms may not be enough. Experiments show that
>> fDeviceInit response sometimes takes 500ms so to be on safer side this
>> change sets the timeout to 600ms. Without this change, we might
>> unnecessarily have to retry fDeviceInit query requests multiple times
>> and
>> each query request timeout prints one error message.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e0b8755..573a8cb 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -58,6 +58,12 @@
>>  #define QUERY_REQ_RETRIES 10
>>  /* Query request timeout */
>>  #define QUERY_REQ_TIMEOUT 30 /* msec */
>> +/*
>> + * Query request timeout for fDeviceInit flag
>> + * fDeviceInit query response time for some devices is too large that
>> default
>> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
>> + */
>> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
>
> How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
> instead of conditionally setting timeout depending on ufs flag?

Your suggestion obviously could work (increasing the QUERY_REQ_TIMEOUT to
600ms), but in that case we bring extra delay of 570ms to error handling
of query timeout, and in such a case, instead of handling the error after
30ms we handle it after 600ms, which make the SW hang.
does it make sense ?


>
>>
>>  /* Task management command timeout */
>>  #define TM_CMD_TIMEOUT 100 /* msecs */
>> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>> enum query_opcode opcode,
>>         struct ufs_query_req *request = NULL;
>>         struct ufs_query_res *response = NULL;
>>         int err, index = 0, selector = 0;
>> +       int timeout = QUERY_REQ_TIMEOUT;
>>
>>         BUG_ON(!hba);
>>
>> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>> enum query_opcode opcode,
>>                 goto out_unlock;
>>         }
>>
>> -       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY,
>> QUERY_REQ_TIMEOUT);
>> +       if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
>> +               timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
>> +
>> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>>
>>         if (err) {
>>                 dev_err(hba->dev,
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 05/15] scsi: ufs: increase fDeviceInit query response timeout

[Akinobu Mita]

2015-10-25 22:38 GMT+09:00  <ygardi@codeaurora.org>:
>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>> fDeviceInit query response time for some devices is too long that
>>> default
>>> query request timeout of 100ms may not be enough. Experiments show that
>>> fDeviceInit response sometimes takes 500ms so to be on safer side this
>>> change sets the timeout to 600ms. Without this change, we might
>>> unnecessarily have to retry fDeviceInit query requests multiple times
>>> and
>>> each query request timeout prints one error message.
>>>
>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index e0b8755..573a8cb 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -58,6 +58,12 @@
>>>  #define QUERY_REQ_RETRIES 10
>>>  /* Query request timeout */
>>>  #define QUERY_REQ_TIMEOUT 30 /* msec */
>>> +/*
>>> + * Query request timeout for fDeviceInit flag
>>> + * fDeviceInit query response time for some devices is too large that
>>> default
>>> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
>>> + */
>>> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
>>
>> How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
>> instead of conditionally setting timeout depending on ufs flag?
>
> Your suggestion obviously could work (increasing the QUERY_REQ_TIMEOUT to
> 600ms), but in that case we bring extra delay of 570ms to error handling
> of query timeout, and in such a case, instead of handling the error after
> 30ms we handle it after 600ms, which make the SW hang.
> does it make sense ?

Compared to default scsi disk timeout (30s), 600ms does not look very
long.  I was just worried that the code gets complicated if we need to
add yet another QUERY_XYZ_REQ_TIMEOUT macros when it turns out that
30ms timeout is not enough for other query requests under specific
conditions.  But I don't too much care about it for now.

>>
>>>
>>>  /* Task management command timeout */
>>>  #define TM_CMD_TIMEOUT 100 /* msecs */
>>> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>>> enum query_opcode opcode,
>>>         struct ufs_query_req *request = NULL;
>>>         struct ufs_query_res *response = NULL;
>>>         int err, index = 0, selector = 0;
>>> +       int timeout = QUERY_REQ_TIMEOUT;
>>>
>>>         BUG_ON(!hba);
>>>
>>> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>>> enum query_opcode opcode,
>>>                 goto out_unlock;
>>>         }
>>>
>>> -       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY,
>>> QUERY_REQ_TIMEOUT);
>>> +       if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
>>> +               timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
>>> +
>>> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>>>
>>>         if (err) {
>>>                 dev_err(hba->dev,
>>> --
>>> 1.8.5.2
>>>
>>> --
>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>>> member of Code Aurora Forum, hosted by The Linux Foundation
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>

[PATCH v3 06/15] scsi: ufs: avoid exception event handler racing with PM callbacks

[Yaniv Gardi]

If device raises the exception event in the response to the commands
sent during the runtime/system PM callbacks, exception event handler
might run in parallel with PM callbacks and may see unclocked register
accesses. This change fixes this issue by not scheduling the exception
event handler while PM callbacks are running.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 573a8cb..0e54183 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3145,7 +3145,20 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 			scsi_status = result & MASK_SCSI_STATUS;
 			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
 
-			if (ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
+			/*
+			 * Currently we are only supporting BKOPs exception
+			 * events hence we can ignore BKOPs exception event
+			 * during power management callbacks. BKOPs exception
+			 * event is not expected to be raised in runtime suspend
+			 * callback as it allows the urgent bkops.
+			 * During system suspend, we are anyway forcefully
+			 * disabling the bkops and if urgent bkops is needed
+			 * it will be enabled on system resume. Long term
+			 * solution could be to abort the system suspend if
+			 * UFS device needs urgent BKOPs.
+			 */
+			if (!hba->pm_op_in_progress &&
+			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
 				schedule_work(&hba->eeh_work);
 			break;
 		case UPIU_TRANSACTION_REJECT_UPIU:
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 07/15] scsi: ufs: set REQUEST_SENSE command size to 18 bytes

[Yaniv Gardi]

According to UFS device specification REQUEST_SENSE command can
only report back up to 18 bytes of data.

Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0e54183..217b5cf 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -42,6 +42,7 @@
 
 #include "ufshcd.h"
 #include "unipro.h"
+#define UFSHCD_REQ_SENSE_SIZE	18
 
 #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
 				 UTP_TASK_REQ_COMPL |\
@@ -836,7 +837,7 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
 		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sr.sense_data_len);
 		memcpy(lrbp->sense_buffer,
 			lrbp->ucd_rsp_ptr->sr.sense_data,
-			min_t(int, len, SCSI_SENSE_BUFFERSIZE));
+			min_t(int, len, UFSHCD_REQ_SENSE_SIZE));
 	}
 }
 
@@ -1381,7 +1382,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	WARN_ON(lrbp->cmd);
 	lrbp->cmd = cmd;
-	lrbp->sense_bufflen = SCSI_SENSE_BUFFERSIZE;
+	lrbp->sense_bufflen = UFSHCD_REQ_SENSE_SIZE;
 	lrbp->sense_buffer = cmd->sense_buffer;
 	lrbp->task_tag = tag;
 	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
@@ -4817,19 +4818,19 @@ ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp)
 				0,
 				0,
 				0,
-				SCSI_SENSE_BUFFERSIZE,
+				UFSHCD_REQ_SENSE_SIZE,
 				0};
 	char *buffer;
 	int ret;
 
-	buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
+	buffer = kzalloc(UFSHCD_REQ_SENSE_SIZE, GFP_KERNEL);
 	if (!buffer) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
 	ret = scsi_execute_req_flags(sdp, cmd, DMA_FROM_DEVICE, buffer,
-				SCSI_SENSE_BUFFERSIZE, NULL,
+				UFSHCD_REQ_SENSE_SIZE, NULL,
 				msecs_to_jiffies(1000), 3, NULL, REQ_PM);
 	if (ret)
 		pr_err("%s: failed with err %d\n", __func__, ret);
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 07/15] scsi: ufs: set REQUEST_SENSE command size to 18 bytes

[Akinobu Mita]

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> According to UFS device specification REQUEST_SENSE command can
> only report back up to 18 bytes of data.
>
> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0e54183..217b5cf 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -42,6 +42,7 @@
>
>  #include "ufshcd.h"
>  #include "unipro.h"
> +#define UFSHCD_REQ_SENSE_SIZE  18

We usually put a blank line between #include and #define.

[PATCH v3 08/15] scsi: ufs: add retries to dme_peer get and set attribute

[Yaniv Gardi]

The dme_peer get/set attribute commands are prone to errors, therefore
we add three retries for the UIC command sending.
Error code returned from ufshcd_send_uic_cmd() is checked, and unless
it was successful or the retries have finished, another command will be
sent.

Signed-off-by: Lee Susman <lsusman@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 217b5cf..8d5bdf0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -69,6 +69,9 @@
 /* Task management command timeout */
 #define TM_CMD_TIMEOUT	100 /* msecs */
 
+/* maximum number of retries for a general UIC command  */
+#define UFS_UIC_COMMAND_RETRIES 3
+
 /* maximum number of link-startup retries */
 #define DME_LINKSTARTUP_RETRIES 3
 
@@ -2184,6 +2187,7 @@ int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
 	};
 	const char *set = action[!!peer];
 	int ret;
+	int retries = UFS_UIC_COMMAND_RETRIES;
 
 	uic_cmd.command = peer ?
 		UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET;
@@ -2191,10 +2195,18 @@ int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
 	uic_cmd.argument2 = UIC_ARG_ATTR_TYPE(attr_set);
 	uic_cmd.argument3 = mib_val;
 
-	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
-	if (ret)
-		dev_err(hba->dev, "%s: attr-id 0x%x val 0x%x error code %d\n",
-			set, UIC_GET_ATTR_ID(attr_sel), mib_val, ret);
+	do {
+		/* for peer attributes we retry upon failure */
+		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
+		if (ret)
+			dev_dbg(hba->dev, "%s: attr-id 0x%x val 0x%x error code %d\n",
+				set, UIC_GET_ATTR_ID(attr_sel), mib_val, ret);
+	} while (ret && peer && --retries);
+
+	if (!retries)
+		dev_err(hba->dev, "%s: attr-id 0x%x val 0x%x failed %d retries\n",
+				set, UIC_GET_ATTR_ID(attr_sel), mib_val,
+				retries);
 
 	return ret;
 }
@@ -2219,6 +2231,7 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 	};
 	const char *get = action[!!peer];
 	int ret;
+	int retries = UFS_UIC_COMMAND_RETRIES;
 	struct ufs_pa_layer_attr orig_pwr_info;
 	struct ufs_pa_layer_attr temp_pwr_info;
 	bool pwr_mode_change = false;
@@ -2249,14 +2262,19 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 		UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET;
 	uic_cmd.argument1 = attr_sel;
 
-	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
-	if (ret) {
-		dev_err(hba->dev, "%s: attr-id 0x%x error code %d\n",
-			get, UIC_GET_ATTR_ID(attr_sel), ret);
-		goto out;
-	}
+	do {
+		/* for peer attributes we retry upon failure */
+		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
+		if (ret)
+			dev_dbg(hba->dev, "%s: attr-id 0x%x error code %d\n",
+				get, UIC_GET_ATTR_ID(attr_sel), ret);
+	} while (ret && peer && --retries);
+
+	if (!retries)
+		dev_err(hba->dev, "%s: attr-id 0x%x failed %d retries\n",
+				get, UIC_GET_ATTR_ID(attr_sel), retries);
 
-	if (mib_val)
+	if (mib_val && !ret)
 		*mib_val = uic_cmd.argument3;
 
 	if (peer && (hba->quirks & UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE)
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 09/15] scsi: ufs: add retries for hibern8 enter

[Yaniv Gardi]

If hibern8 enter command fails then UFS link state may be unknown which
may result into timeout of all the commands issued after failure.

This change does 2 things (for pre-defined number of retry counts) after
hibern8 enter failure:
1. Recovers the UFS link to active state
2. If link is recovered to active state, tries to put the UFS link in
   hibern8 enter again until retry count expires.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8d5bdf0..5800b08 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -75,6 +75,9 @@
 /* maximum number of link-startup retries */
 #define DME_LINKSTARTUP_RETRIES 3
 
+/* Maximum retries for Hibern8 enter */
+#define UIC_HIBERN8_ENTER_RETRIES 3
+
 /* maximum number of reset retries before giving up */
 #define MAX_HOST_RESET_RETRIES 5
 
@@ -2389,13 +2392,32 @@ out:
 	return ret;
 }
 
-static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
+static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 {
+	int ret;
 	struct uic_command uic_cmd = {0};
 
 	uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
+	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
+
+	if (ret)
+		dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d",
+			__func__, ret);
+
+	return ret;
+}
+
+static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
+{
+	int ret = 0, retries;
 
-	return ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
+	for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) {
+		ret = __ufshcd_uic_hibern8_enter(hba);
+		if (!ret || ret == -ENOLINK)
+			goto out;
+	}
+out:
+	return ret;
 }
 
 static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 09/15] scsi: ufs: add retries for hibern8 enter

[Akinobu Mita]

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> If hibern8 enter command fails then UFS link state may be unknown which
> may result into timeout of all the commands issued after failure.
>
> This change does 2 things (for pre-defined number of retry counts) after
> hibern8 enter failure:
> 1. Recovers the UFS link to active state
> 2. If link is recovered to active state, tries to put the UFS link in
>    hibern8 enter again until retry count expires.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8d5bdf0..5800b08 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -75,6 +75,9 @@
>  /* maximum number of link-startup retries */
>  #define DME_LINKSTARTUP_RETRIES 3
>
> +/* Maximum retries for Hibern8 enter */
> +#define UIC_HIBERN8_ENTER_RETRIES 3
> +
>  /* maximum number of reset retries before giving up */
>  #define MAX_HOST_RESET_RETRIES 5
>
> @@ -2389,13 +2392,32 @@ out:
>         return ret;
>  }
>
> -static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
> +static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>  {
> +       int ret;
>         struct uic_command uic_cmd = {0};
>
>         uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
> +       ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
> +
> +       if (ret)
> +               dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d",
> +                       __func__, ret);

This format string is not terminated with '\n'.
(I found several same issues in ufshcd.c)

> +
> +       return ret;
> +}
> +
> +static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
> +{
> +       int ret = 0, retries;
>

'ret' will soon be initialized in the loop anyway, so it is unnecessary
to initialized here.

> -       return ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
> +       for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) {
> +               ret = __ufshcd_uic_hibern8_enter(hba);
> +               if (!ret || ret == -ENOLINK)
> +                       goto out;
> +       }
> +out:
> +       return ret;
>  }
>
>  static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 09/15] scsi: ufs: add retries for hibern8 enter

[ygardi]

> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> If hibern8 enter command fails then UFS link state may be unknown which
>> may result into timeout of all the commands issued after failure.
>>
>> This change does 2 things (for pre-defined number of retry counts) after
>> hibern8 enter failure:
>> 1. Recovers the UFS link to active state
>> 2. If link is recovered to active state, tries to put the UFS link in
>>    hibern8 enter again until retry count expires.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
>>  1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 8d5bdf0..5800b08 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -75,6 +75,9 @@
>>  /* maximum number of link-startup retries */
>>  #define DME_LINKSTARTUP_RETRIES 3
>>
>> +/* Maximum retries for Hibern8 enter */
>> +#define UIC_HIBERN8_ENTER_RETRIES 3
>> +
>>  /* maximum number of reset retries before giving up */
>>  #define MAX_HOST_RESET_RETRIES 5
>>
>> @@ -2389,13 +2392,32 @@ out:
>>         return ret;
>>  }
>>
>> -static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>> +static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>>  {
>> +       int ret;
>>         struct uic_command uic_cmd = {0};
>>
>>         uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
>> +       ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>> +
>> +       if (ret)
>> +               dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d",
>> +                       __func__, ret);
>
> This format string is not terminated with '\n'.
> (I found several same issues in ufshcd.c)

correct. will be modified in the next v4.
>
>> +
>> +       return ret;
>> +}
>> +
>> +static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>> +{
>> +       int ret = 0, retries;
>>
>
> 'ret' will soon be initialized in the loop anyway, so it is unnecessary
> to initialized here.

correct but, in order to be on the safe side, better to leave this
initialization here, as if UIC_HIBERN8_ENTER_RETRIES is modified to 0, we
will never get into the for-loop, and thus, ret will be un initialized.


>
>> -       return ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>> +       for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0;
>> retries--) {
>> +               ret = __ufshcd_uic_hibern8_enter(hba);
>> +               if (!ret || ret == -ENOLINK)
>> +                       goto out;
>> +       }
>> +out:
>> +       return ret;
>>  }
>>
>>  static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v3 10/15] scsi: ufs: fix error recovery after the hibern8 exit failure

[Yaniv Gardi]

Hibern8 exit can be called from 3 different context:
    - ufshcd_hibern8_exit_work
    - ufshcd_ungate_work
    - runtime/system resume

If hibern8 exit fails for some reason then we try to bring the link to
active state by link startup but this recovery mechanism results into
deadlock or errors from first 2 context listed above. This change fixes
the recovery by adding proper error handling mechanism.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 58 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5800b08..1dbef7d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -609,6 +609,11 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->clk_gating.active_reqs++;
 
+	if (ufshcd_eh_in_progress(hba)) {
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
+		return 0;
+	}
+
 start:
 	switch (hba->clk_gating.state) {
 	case CLKS_ON:
@@ -724,7 +729,8 @@ static void __ufshcd_release(struct ufs_hba *hba)
 	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended
 		|| hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
 		|| hba->lrb_in_use || hba->outstanding_tasks
-		|| hba->active_uic_cmd || hba->uic_async_done)
+		|| hba->active_uic_cmd || hba->uic_async_done
+		|| ufshcd_eh_in_progress(hba))
 		return;
 
 	hba->clk_gating.state = REQ_CLKS_OFF;
@@ -1362,6 +1368,13 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		cmd->scsi_done(cmd);
 		goto out_unlock;
 	}
+
+	/* if error handling is in progress, don't issue commands */
+	if (ufshcd_eh_in_progress(hba)) {
+		set_host_byte(cmd, DID_ERROR);
+		cmd->scsi_done(cmd);
+		goto out_unlock;
+	}
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* acquire the tag to make sure device cmds don't use it */
@@ -2392,6 +2405,31 @@ out:
 	return ret;
 }
 
+static int ufshcd_link_recovery(struct ufs_hba *hba)
+{
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	hba->ufshcd_state = UFSHCD_STATE_RESET;
+	ufshcd_set_eh_in_progress(hba);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	ret = ufshcd_host_reset_and_restore(hba);
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	if (ret)
+		hba->ufshcd_state = UFSHCD_STATE_ERROR;
+	ufshcd_clear_eh_in_progress(hba);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	if (ret)
+		dev_err(hba->dev, "%s: link recovery failed, err %d",
+			__func__, ret);
+
+	return ret;
+}
+
 static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 {
 	int ret;
@@ -2400,10 +2438,18 @@ static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 	uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 
-	if (ret)
+	if (ret) {
 		dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d",
 			__func__, ret);
 
+		/*
+		 * If link recovery fails then return error so that caller
+		 * don't retry the hibern8 enter again.
+		 */
+		if (ufshcd_link_recovery(hba))
+			ret = -ENOLINK;
+	}
+
 	return ret;
 }
 
@@ -2428,8 +2474,9 @@ static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
 	uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	if (ret) {
-		ufshcd_set_link_off(hba);
-		ret = ufshcd_host_reset_and_restore(hba);
+		dev_err(hba->dev, "%s: hibern8 exit failed. ret = %d",
+			__func__, ret);
+		ret = ufshcd_link_recovery(hba);
 	}
 
 	return ret;
@@ -4381,7 +4428,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 	/* UFS device is also active now */
 	ufshcd_set_ufs_dev_active(hba);
 	ufshcd_force_reset_auto_bkops(hba);
-	hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 	hba->wlun_dev_clr_ua = true;
 
 	if (ufshcd_get_max_pwr_mode(hba)) {
@@ -4395,6 +4441,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 					__func__, ret);
 	}
 
+	/* set the state as operational after switching to desired gear */
+	hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 	/*
 	 * If we are in error handling context or in power management callbacks
 	 * context, no need to scan the host
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 11/15] scsi: ufs: retry failed query flag requests

[Yaniv Gardi]

UFS flag query requests may fail sometimes due to timeouts etc.
Add a wrapper function to retry up to 10 times in case of such
failure, similar to retries being made for attribute queries.

Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 63 ++++++++++++++++++++++++++++-------------------
 drivers/scsi/ufs/ufshcd.h |  4 +++
 2 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1dbef7d..f2d4301 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1662,6 +1662,29 @@ static inline void ufshcd_init_query(struct ufs_hba *hba,
 	(*request)->upiu_req.selector = selector;
 }
 
+static int ufshcd_query_flag_retry(struct ufs_hba *hba,
+	enum query_opcode opcode, enum flag_idn idn, bool *flag_res)
+{
+	int ret;
+	int retries;
+
+	for (retries = 0; retries < QUERY_REQ_RETRIES; retries++) {
+		ret = ufshcd_query_flag(hba, opcode, idn, flag_res);
+		if (ret)
+			dev_dbg(hba->dev,
+				"%s: failed with error %d, retries %d\n",
+				__func__, ret, retries);
+		else
+			break;
+	}
+
+	if (ret)
+		dev_err(hba->dev,
+			"%s: query attribute, opcode %d, idn %d, failed with error %d after %d retires\n",
+			__func__, opcode, idn, ret, retries);
+	return ret;
+}
+
 /**
  * ufshcd_query_flag() - API function for sending flag query requests
  * hba: per-adapter instance
@@ -1671,7 +1694,7 @@ static inline void ufshcd_init_query(struct ufs_hba *hba,
  *
  * Returns 0 for success, non-zero in case of failure
  */
-static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
+int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 			enum flag_idn idn, bool *flag_res)
 {
 	struct ufs_query_req *request = NULL;
@@ -2656,17 +2679,12 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
  */
 static int ufshcd_complete_dev_init(struct ufs_hba *hba)
 {
-	int i, retries, err = 0;
+	int i;
+	int err;
 	bool flag_res = 1;
 
-	for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
-		/* Set the fDeviceInit flag */
-		err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
-					QUERY_FLAG_IDN_FDEVICEINIT, NULL);
-		if (!err || err == -ETIMEDOUT)
-			break;
-		dev_dbg(hba->dev, "%s: error %d retrying\n", __func__, err);
-	}
+	err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
+		QUERY_FLAG_IDN_FDEVICEINIT, NULL);
 	if (err) {
 		dev_err(hba->dev,
 			"%s setting fDeviceInit flag failed with error %d\n",
@@ -2674,18 +2692,11 @@ static int ufshcd_complete_dev_init(struct ufs_hba *hba)
 		goto out;
 	}
 
-	/* poll for max. 100 iterations for fDeviceInit flag to clear */
-	for (i = 0; i < 100 && !err && flag_res; i++) {
-		for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
-			err = ufshcd_query_flag(hba,
-					UPIU_QUERY_OPCODE_READ_FLAG,
-					QUERY_FLAG_IDN_FDEVICEINIT, &flag_res);
-			if (!err || err == -ETIMEDOUT)
-				break;
-			dev_dbg(hba->dev, "%s: error %d retrying\n", __func__,
-					err);
-		}
-	}
+	/* poll for max. 1000 iterations for fDeviceInit flag to clear */
+	for (i = 0; i < 1000 && !err && flag_res; i++)
+		err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
+			QUERY_FLAG_IDN_FDEVICEINIT, &flag_res);
+
 	if (err)
 		dev_err(hba->dev,
 			"%s reading fDeviceInit flag failed with error %d\n",
@@ -3432,7 +3443,7 @@ static int ufshcd_enable_auto_bkops(struct ufs_hba *hba)
 	if (hba->auto_bkops_enabled)
 		goto out;
 
-	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
+	err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
 			QUERY_FLAG_IDN_BKOPS_EN, NULL);
 	if (err) {
 		dev_err(hba->dev, "%s: failed to enable bkops %d\n",
@@ -3481,7 +3492,7 @@ static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
 		goto out;
 	}
 
-	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
+	err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
 			QUERY_FLAG_IDN_BKOPS_EN, NULL);
 	if (err) {
 		dev_err(hba->dev, "%s: failed to disable bkops %d\n",
@@ -4452,8 +4463,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 
 		/* clear any previous UFS device information */
 		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
-		if (!ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
-				       QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
+		if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
+				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
 			hba->dev_info.f_power_on_wp_en = flag;
 
 		if (!hba->is_init_prefetch)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2570d94..f8ce680 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -3,6 +3,7 @@
  *
  * This code is based on drivers/scsi/ufs/ufshcd.h
  * Copyright (C) 2011-2013 Samsung India Software Operations
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
  *
  * Authors:
  *	Santosh Yaraganavi <santosh.sy@samsung.com>
@@ -681,6 +682,9 @@ static inline int ufshcd_dme_peer_get(struct ufs_hba *hba,
 	return ufshcd_dme_get_attr(hba, attr_sel, mib_val, DME_PEER);
 }
 
+/* Expose Query-Request API */
+int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
+	enum flag_idn idn, bool *flag_res);
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
 
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests

[Yaniv Gardi]

DME commands such as Hibern8 enter/exit and gear switch generate 2
completion interrupts, one for confirmation that command is received
by local UniPro and 2nd one is the final confirmation after communication
with remote UniPro. Currently both of these completions are registered
as interrupt events which is not quite necessary and instead we can
just wait for the interrupt of 2nd completion, this should reduce
the number of interrupts and could reduce the unnecessary CPU wakeups
to handle extra interrupts.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f2d4301..fc2a52d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
  * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
  * @hba: per adapter instance
  * @uic_cmd: UIC command
+ * @completion: initialize the completion only if this is set to true
  *
  * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
  * with mutex held and host_lock locked.
  * Returns 0 only if success.
  */
 static int
-__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
+__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
+		      bool completion)
 {
 	if (!ufshcd_ready_for_uic_cmd(hba)) {
 		dev_err(hba->dev,
@@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 		return -EIO;
 	}
 
-	init_completion(&uic_cmd->done);
+	if (completion)
+		init_completion(&uic_cmd->done);
 
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
@@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	ufshcd_add_delay_before_dme_cmd(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
+	ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	if (!ret)
 		ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
@@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	unsigned long flags;
 	u8 status;
 	int ret;
+	bool reenable_intr = false;
 
 	mutex_lock(&hba->uic_cmd_mutex);
 	init_completion(&uic_async_done);
@@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->uic_async_done = &uic_async_done;
-	ret = __ufshcd_send_uic_cmd(hba, cmd);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	if (ret) {
-		dev_err(hba->dev,
-			"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
-			cmd->command, cmd->argument3, ret);
-		goto out;
+	if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
+		ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
+		/*
+		 * Make sure UIC command completion interrupt is disabled before
+		 * issuing UIC command.
+		 */
+		wmb();
+		reenable_intr = true;
 	}
-	ret = ufshcd_wait_for_uic_cmd(hba, cmd);
+	ret = __ufshcd_send_uic_cmd(hba, cmd, false);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	if (ret) {
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
@@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	}
 out:
 	spin_lock_irqsave(hba->host->host_lock, flags);
+	hba->active_uic_cmd = NULL;
 	hba->uic_async_done = NULL;
+	if (reenable_intr)
+		ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	mutex_unlock(&hba->uic_cmd_mutex);
 
@@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  */
 static irqreturn_t ufshcd_intr(int irq, void *__hba)
 {
-	u32 intr_status;
+	u32 intr_status, enabled_intr_status;
 	irqreturn_t retval = IRQ_NONE;
 	struct ufs_hba *hba = __hba;
 
 	spin_lock(hba->host->host_lock);
 	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
+	enabled_intr_status =
+		intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (intr_status) {
+	if (intr_status)
 		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
-		ufshcd_sl_intr(hba, intr_status);
+
+	if (enabled_intr_status) {
+		ufshcd_sl_intr(hba, enabled_intr_status);
 		retval = IRQ_HANDLED;
 	}
 	spin_unlock(hba->host->host_lock);
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests

[Akinobu Mita]

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> DME commands such as Hibern8 enter/exit and gear switch generate 2
> completion interrupts, one for confirmation that command is received
> by local UniPro and 2nd one is the final confirmation after communication
> with remote UniPro. Currently both of these completions are registered
> as interrupt events which is not quite necessary and instead we can
> just wait for the interrupt of 2nd completion, this should reduce
> the number of interrupts and could reduce the unnecessary CPU wakeups
> to handle extra interrupts.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f2d4301..fc2a52d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>   * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
>   * @hba: per adapter instance
>   * @uic_cmd: UIC command
> + * @completion: initialize the completion only if this is set to true
>   *
>   * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
>   * with mutex held and host_lock locked.
>   * Returns 0 only if success.
>   */
>  static int
> -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
> +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
> +                     bool completion)
>  {
>         if (!ufshcd_ready_for_uic_cmd(hba)) {
>                 dev_err(hba->dev,
> @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>                 return -EIO;
>         }
>
> -       init_completion(&uic_cmd->done);
> +       if (completion)
> +               init_completion(&uic_cmd->done);
>
>         ufshcd_dispatch_uic_cmd(hba, uic_cmd);
>
> @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>         ufshcd_add_delay_before_dme_cmd(hba);
>
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
> +       ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>         if (!ret)
>                 ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
> @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>         unsigned long flags;
>         u8 status;
>         int ret;
> +       bool reenable_intr = false;
>
>         mutex_lock(&hba->uic_cmd_mutex);
>         init_completion(&uic_async_done);
> @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         hba->uic_async_done = &uic_async_done;
> -       ret = __ufshcd_send_uic_cmd(hba, cmd);
> -       spin_unlock_irqrestore(hba->host->host_lock, flags);
> -       if (ret) {
> -               dev_err(hba->dev,
> -                       "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
> -                       cmd->command, cmd->argument3, ret);
> -               goto out;
> +       if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
> +               ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
> +               /*
> +                * Make sure UIC command completion interrupt is disabled before
> +                * issuing UIC command.
> +                */
> +               wmb();
> +               reenable_intr = true;
>         }
> -       ret = ufshcd_wait_for_uic_cmd(hba, cmd);
> +       ret = __ufshcd_send_uic_cmd(hba, cmd, false);
> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
>         if (ret) {
>                 dev_err(hba->dev,
>                         "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
> @@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>         }
>  out:
>         spin_lock_irqsave(hba->host->host_lock, flags);
> +       hba->active_uic_cmd = NULL;
>         hba->uic_async_done = NULL;
> +       if (reenable_intr)
> +               ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>         mutex_unlock(&hba->uic_cmd_mutex);
>
> @@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>   */
>  static irqreturn_t ufshcd_intr(int irq, void *__hba)
>  {
> -       u32 intr_status;
> +       u32 intr_status, enabled_intr_status;
>         irqreturn_t retval = IRQ_NONE;
>         struct ufs_hba *hba = __hba;
>
>         spin_lock(hba->host->host_lock);
>         intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> +       enabled_intr_status =
> +               intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);

Is it better to store interrupt mask to new member field in ufs_hba
when ufshcd_{enable,disable}_intr() is called and avoid register
read every interrupt?  Because register read is much slower than
normal memory read and we don't want to slow high IOPS workload.

>
> -       if (intr_status) {
> +       if (intr_status)
>                 ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> -               ufshcd_sl_intr(hba, intr_status);
> +
> +       if (enabled_intr_status) {
> +               ufshcd_sl_intr(hba, enabled_intr_status);
>                 retval = IRQ_HANDLED;
>         }
>         spin_unlock(hba->host->host_lock);
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests

[Akinobu Mita]

2015-10-21 23:57 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> DME commands such as Hibern8 enter/exit and gear switch generate 2
>> completion interrupts, one for confirmation that command is received
>> by local UniPro and 2nd one is the final confirmation after communication
>> with remote UniPro. Currently both of these completions are registered
>> as interrupt events which is not quite necessary and instead we can
>> just wait for the interrupt of 2nd completion, this should reduce
>> the number of interrupts and could reduce the unnecessary CPU wakeups
>> to handle extra interrupts.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 41 +++++++++++++++++++++++++++--------------
>>  1 file changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index f2d4301..fc2a52d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>>   * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
>>   * @hba: per adapter instance
>>   * @uic_cmd: UIC command
>> + * @completion: initialize the completion only if this is set to true
>>   *
>>   * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
>>   * with mutex held and host_lock locked.
>>   * Returns 0 only if success.
>>   */
>>  static int
>> -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>> +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
>> +                     bool completion)
>>  {
>>         if (!ufshcd_ready_for_uic_cmd(hba)) {
>>                 dev_err(hba->dev,
>> @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>>                 return -EIO;
>>         }
>>
>> -       init_completion(&uic_cmd->done);
>> +       if (completion)
>> +               init_completion(&uic_cmd->done);
>>
>>         ufshcd_dispatch_uic_cmd(hba, uic_cmd);
>>
>> @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>>         ufshcd_add_delay_before_dme_cmd(hba);
>>
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>> -       ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
>> +       ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         if (!ret)
>>                 ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
>> @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>>         unsigned long flags;
>>         u8 status;
>>         int ret;
>> +       bool reenable_intr = false;
>>
>>         mutex_lock(&hba->uic_cmd_mutex);
>>         init_completion(&uic_async_done);
>> @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>>
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>         hba->uic_async_done = &uic_async_done;
>> -       ret = __ufshcd_send_uic_cmd(hba, cmd);
>> -       spin_unlock_irqrestore(hba->host->host_lock, flags);
>> -       if (ret) {
>> -               dev_err(hba->dev,
>> -                       "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
>> -                       cmd->command, cmd->argument3, ret);
>> -               goto out;
>> +       if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
>> +               ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
>> +               /*
>> +                * Make sure UIC command completion interrupt is disabled before
>> +                * issuing UIC command.
>> +                */
>> +               wmb();
>> +               reenable_intr = true;

mmiowb() is more suitable here?  Please see
"ACQUIRES VS I/O ACCESSES" subsection in Documentation/memory-barriers.txt

>>         }
>> -       ret = ufshcd_wait_for_uic_cmd(hba, cmd);
>> +       ret = __ufshcd_send_uic_cmd(hba, cmd, false);
>> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         if (ret) {
>>                 dev_err(hba->dev,
>>                         "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
>> @@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>>         }
>>  out:
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>> +       hba->active_uic_cmd = NULL;
>>         hba->uic_async_done = NULL;
>> +       if (reenable_intr)
>> +               ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         mutex_unlock(&hba->uic_cmd_mutex);
>>
>> @@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>>   */
>>  static irqreturn_t ufshcd_intr(int irq, void *__hba)
>>  {
>> -       u32 intr_status;
>> +       u32 intr_status, enabled_intr_status;
>>         irqreturn_t retval = IRQ_NONE;
>>         struct ufs_hba *hba = __hba;
>>
>>         spin_lock(hba->host->host_lock);
>>         intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>> +       enabled_intr_status =
>> +               intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
>
> Is it better to store interrupt mask to new member field in ufs_hba
> when ufshcd_{enable,disable}_intr() is called and avoid register
> read every interrupt?  Because register read is much slower than
> normal memory read and we don't want to slow high IOPS workload.

After thinking more about this, do we really need to change ufshcd_intr()
to skip interrupt disabled events?  When the 2nd completion (the final
comfirmation) interrupt happens, we can just handle the 1st (DME command
completion) event, too.  So I think it is unnecessary to touch
ufshcd_intr() in this patch at all.

>
>>
>> -       if (intr_status) {
>> +       if (intr_status)
>>                 ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
>> -               ufshcd_sl_intr(hba, intr_status);
>> +
>> +       if (enabled_intr_status) {
>> +               ufshcd_sl_intr(hba, enabled_intr_status);
>>                 retval = IRQ_HANDLED;
>>         }
>>         spin_unlock(hba->host->host_lock);
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests

[ygardi]

> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> DME commands such as Hibern8 enter/exit and gear switch generate 2
>> completion interrupts, one for confirmation that command is received
>> by local UniPro and 2nd one is the final confirmation after
>> communication
>> with remote UniPro. Currently both of these completions are registered
>> as interrupt events which is not quite necessary and instead we can
>> just wait for the interrupt of 2nd completion, this should reduce
>> the number of interrupts and could reduce the unnecessary CPU wakeups
>> to handle extra interrupts.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 41
>> +++++++++++++++++++++++++++--------------
>>  1 file changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index f2d4301..fc2a52d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
>> struct uic_command *uic_cmd)
>>   * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
>>   * @hba: per adapter instance
>>   * @uic_cmd: UIC command
>> + * @completion: initialize the completion only if this is set to true
>>   *
>>   * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
>>   * with mutex held and host_lock locked.
>>   * Returns 0 only if success.
>>   */
>>  static int
>> -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>> +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
>> +                     bool completion)
>>  {
>>         if (!ufshcd_ready_for_uic_cmd(hba)) {
>>                 dev_err(hba->dev,
>> @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct
>> uic_command *uic_cmd)
>>                 return -EIO;
>>         }
>>
>> -       init_completion(&uic_cmd->done);
>> +       if (completion)
>> +               init_completion(&uic_cmd->done);
>>
>>         ufshcd_dispatch_uic_cmd(hba, uic_cmd);
>>
>> @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct
>> uic_command *uic_cmd)
>>         ufshcd_add_delay_before_dme_cmd(hba);
>>
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>> -       ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
>> +       ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         if (!ret)
>>                 ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
>> @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
>> *hba, struct uic_command *cmd)
>>         unsigned long flags;
>>         u8 status;
>>         int ret;
>> +       bool reenable_intr = false;
>>
>>         mutex_lock(&hba->uic_cmd_mutex);
>>         init_completion(&uic_async_done);
>> @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
>> *hba, struct uic_command *cmd)
>>
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>         hba->uic_async_done = &uic_async_done;
>> -       ret = __ufshcd_send_uic_cmd(hba, cmd);
>> -       spin_unlock_irqrestore(hba->host->host_lock, flags);
>> -       if (ret) {
>> -               dev_err(hba->dev,
>> -                       "pwr ctrl cmd 0x%x with mode 0x%x uic error
>> %d\n",
>> -                       cmd->command, cmd->argument3, ret);
>> -               goto out;
>> +       if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL)
>> {
>> +               ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
>> +               /*
>> +                * Make sure UIC command completion interrupt is
>> disabled before
>> +                * issuing UIC command.
>> +                */
>> +               wmb();
>> +               reenable_intr = true;
>>         }
>> -       ret = ufshcd_wait_for_uic_cmd(hba, cmd);
>> +       ret = __ufshcd_send_uic_cmd(hba, cmd, false);
>> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         if (ret) {
>>                 dev_err(hba->dev,
>>                         "pwr ctrl cmd 0x%x with mode 0x%x uic error
>> %d\n",
>> @@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
>> *hba, struct uic_command *cmd)
>>         }
>>  out:
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>> +       hba->active_uic_cmd = NULL;
>>         hba->uic_async_done = NULL;
>> +       if (reenable_intr)
>> +               ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         mutex_unlock(&hba->uic_cmd_mutex);
>>
>> @@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba,
>> u32 intr_status)
>>   */
>>  static irqreturn_t ufshcd_intr(int irq, void *__hba)
>>  {
>> -       u32 intr_status;
>> +       u32 intr_status, enabled_intr_status;
>>         irqreturn_t retval = IRQ_NONE;
>>         struct ufs_hba *hba = __hba;
>>
>>         spin_lock(hba->host->host_lock);
>>         intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>> +       enabled_intr_status =
>> +               intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
>
> Is it better to store interrupt mask to new member field in ufs_hba
> when ufshcd_{enable,disable}_intr() is called and avoid register
> read every interrupt?  Because register read is much slower than
> normal memory read and we don't want to slow high IOPS workload.
>

Mita, this is a good idea, but it would require a little bit more
of code changing, which requires more testing to make sure no new issues
appear.
I would add this as a future note, but currently, i think there is no
major hit in performance. if you insist on changing it in this patch, it
might
delay the entire patch-set which i hope doesn't happen. do you agree?


>>
>> -       if (intr_status) {
>> +       if (intr_status)
>>                 ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
>> -               ufshcd_sl_intr(hba, intr_status);
>> +
>> +       if (enabled_intr_status) {
>> +               ufshcd_sl_intr(hba, enabled_intr_status);
>>                 retval = IRQ_HANDLED;
>>         }
>>         spin_unlock(hba->host->host_lock);
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH v3 13/15] scsi: ufs: add missing memory barriers

[Yaniv Gardi]

Performing several writes to UFS host controller registers has
no gurrantee of ordering, so we must make sure register writes
to setup request list base address etc. are performed before the
run/stop register is enabled.
In addition, when setting up a task request, we must make sure
the updating of descriptors takes places before ringing the
doorbell, similarly to setting up a transfer request.

Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fc2a52d..298511a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg)
 	 *  1		UTRLRDY
 	 *  2		UTMRLRDY
 	 *  3		UCRDY
-	 *  4		HEI
-	 *  5		DEI
-	 * 6-7		reserved
+	 * 4-7		reserved
 	 */
-	return (((reg) & (0xFF)) >> 1) ^ (0x07);
+	return ((reg & 0xFF) >> 1) ^ 0x07;
 }
 
 /**
@@ -2726,7 +2724,7 @@ out:
  * To bring UFS host controller to operational state,
  * 1. Enable required interrupts
  * 2. Configure interrupt aggregation
- * 3. Program UTRL and UTMRL base addres
+ * 3. Program UTRL and UTMRL base address
  * 4. Configure run-stop-registers
  *
  * Returns 0 on success, non-zero value on failure
@@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
 			REG_UTP_TASK_REQ_LIST_BASE_H);
 
 	/*
+	 * Make sure base address and interrupt setup are updated before
+	 * enabling the run/stop registers below.
+	 */
+	wmb();
+
+	/*
 	 * UCRDY, UTMRLDY and UTRLRDY bits must be 1
-	 * DEI, HEI bits must be 0
 	 */
 	reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
 	if (!(ufshcd_get_lists_status(reg))) {
@@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
 
 	/* send command to the controller */
 	__set_bit(free_slot, &hba->outstanding_tasks);
+
+	/* Make sure descriptors are ready before ringing the task doorbell */
+	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);
 
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers

[Akinobu Mita]

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> Performing several writes to UFS host controller registers has
> no gurrantee of ordering, so we must make sure register writes
> to setup request list base address etc. are performed before the
> run/stop register is enabled.
> In addition, when setting up a task request, we must make sure
> the updating of descriptors takes places before ringing the
> doorbell, similarly to setting up a transfer request.
>
> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fc2a52d..298511a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg)
>          *  1           UTRLRDY
>          *  2           UTMRLRDY
>          *  3           UCRDY
> -        *  4           HEI
> -        *  5           DEI
> -        * 6-7          reserved
> +        * 4-7          reserved
>          */
> -       return (((reg) & (0xFF)) >> 1) ^ (0x07);
> +       return ((reg & 0xFF) >> 1) ^ 0x07;
>  }
>
>  /**
> @@ -2726,7 +2724,7 @@ out:
>   * To bring UFS host controller to operational state,
>   * 1. Enable required interrupts
>   * 2. Configure interrupt aggregation
> - * 3. Program UTRL and UTMRL base addres
> + * 3. Program UTRL and UTMRL base address
>   * 4. Configure run-stop-registers
>   *
>   * Returns 0 on success, non-zero value on failure
> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
>                         REG_UTP_TASK_REQ_LIST_BASE_H);
>
>         /*
> +        * Make sure base address and interrupt setup are updated before
> +        * enabling the run/stop registers below.
> +        */
> +       wmb();
> +
> +       /*
>          * UCRDY, UTMRLDY and UTRLRDY bits must be 1
> -        * DEI, HEI bits must be 0
>          */
>         reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>         if (!(ufshcd_get_lists_status(reg))) {
> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
>
>         /* send command to the controller */
>         __set_bit(free_slot, &hba->outstanding_tasks);
> +
> +       /* Make sure descriptors are ready before ringing the task doorbell */
> +       wmb();
> +
>         ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
> +       /* Make sure that doorbell is committed immediately */
> +       wmb();

Is this wmb() after ringing tm doorbell is needed?

>
>         spin_unlock_irqrestore(host->host_lock, flags);
>
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers

[ygardi]

> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> Performing several writes to UFS host controller registers has
>> no gurrantee of ordering, so we must make sure register writes
>> to setup request list base address etc. are performed before the
>> run/stop register is enabled.
>> In addition, when setting up a task request, we must make sure
>> the updating of descriptors takes places before ringing the
>> doorbell, similarly to setting up a transfer request.
>>
>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index fc2a52d..298511a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg)
>>          *  1           UTRLRDY
>>          *  2           UTMRLRDY
>>          *  3           UCRDY
>> -        *  4           HEI
>> -        *  5           DEI
>> -        * 6-7          reserved
>> +        * 4-7          reserved
>>          */
>> -       return (((reg) & (0xFF)) >> 1) ^ (0x07);
>> +       return ((reg & 0xFF) >> 1) ^ 0x07;
>>  }
>>
>>  /**
>> @@ -2726,7 +2724,7 @@ out:
>>   * To bring UFS host controller to operational state,
>>   * 1. Enable required interrupts
>>   * 2. Configure interrupt aggregation
>> - * 3. Program UTRL and UTMRL base addres
>> + * 3. Program UTRL and UTMRL base address
>>   * 4. Configure run-stop-registers
>>   *
>>   * Returns 0 on success, non-zero value on failure
>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
>> ufs_hba *hba)
>>                         REG_UTP_TASK_REQ_LIST_BASE_H);
>>
>>         /*
>> +        * Make sure base address and interrupt setup are updated before
>> +        * enabling the run/stop registers below.
>> +        */
>> +       wmb();
>> +
>> +       /*
>>          * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>> -        * DEI, HEI bits must be 0
>>          */
>>         reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>>         if (!(ufshcd_get_lists_status(reg))) {
>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>> *hba, int lun_id, int task_id,
>>
>>         /* send command to the controller */
>>         __set_bit(free_slot, &hba->outstanding_tasks);
>> +
>> +       /* Make sure descriptors are ready before ringing the task
>> doorbell */
>> +       wmb();
>> +
>>         ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
>> +       /* Make sure that doorbell is committed immediately */
>> +       wmb();
>
> Is this wmb() after ringing tm doorbell is needed?

Well, Mita, in the case of DB register (Request DB and TASK DB), every
write operation to the DB should be barrier, as if not, out of order
writing to this register might cause inconsistency in its value, and thus,
un-handled requests/tasks.

>
>>
>>         spin_unlock_irqrestore(host->host_lock, flags);
>>
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers

[Akinobu Mita]

2015-10-25 23:40 GMT+09:00  <ygardi@codeaurora.org>:
>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>> Performing several writes to UFS host controller registers has
>>> no gurrantee of ordering, so we must make sure register writes
>>> to setup request list base address etc. are performed before the
>>> run/stop register is enabled.
>>> In addition, when setting up a task request, we must make sure
>>> the updating of descriptors takes places before ringing the
>>> doorbell, similarly to setting up a transfer request.
>>>
>>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index fc2a52d..298511a 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg)
>>>          *  1           UTRLRDY
>>>          *  2           UTMRLRDY
>>>          *  3           UCRDY
>>> -        *  4           HEI
>>> -        *  5           DEI
>>> -        * 6-7          reserved
>>> +        * 4-7          reserved
>>>          */
>>> -       return (((reg) & (0xFF)) >> 1) ^ (0x07);
>>> +       return ((reg & 0xFF) >> 1) ^ 0x07;
>>>  }
>>>
>>>  /**
>>> @@ -2726,7 +2724,7 @@ out:
>>>   * To bring UFS host controller to operational state,
>>>   * 1. Enable required interrupts
>>>   * 2. Configure interrupt aggregation
>>> - * 3. Program UTRL and UTMRL base addres
>>> + * 3. Program UTRL and UTMRL base address
>>>   * 4. Configure run-stop-registers
>>>   *
>>>   * Returns 0 on success, non-zero value on failure
>>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
>>> ufs_hba *hba)
>>>                         REG_UTP_TASK_REQ_LIST_BASE_H);
>>>
>>>         /*
>>> +        * Make sure base address and interrupt setup are updated before
>>> +        * enabling the run/stop registers below.
>>> +        */
>>> +       wmb();
>>> +
>>> +       /*
>>>          * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>>> -        * DEI, HEI bits must be 0
>>>          */
>>>         reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>>>         if (!(ufshcd_get_lists_status(reg))) {
>>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>>> *hba, int lun_id, int task_id,
>>>
>>>         /* send command to the controller */
>>>         __set_bit(free_slot, &hba->outstanding_tasks);
>>> +
>>> +       /* Make sure descriptors are ready before ringing the task
>>> doorbell */
>>> +       wmb();
>>> +
>>>         ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
>>> +       /* Make sure that doorbell is committed immediately */
>>> +       wmb();
>>
>> Is this wmb() after ringing tm doorbell is needed?
>
> Well, Mita, in the case of DB register (Request DB and TASK DB), every
> write operation to the DB should be barrier, as if not, out of order
> writing to this register might cause inconsistency in its value, and thus,
> un-handled requests/tasks.

I couldn't fully understand why out of order writing to TASK DB
register causes inconsistency.
In the TASK request completion (ufshcd_tmc_handler), TASK DB register
is read before handling finished requests, so it ensures that all
write operations for TASK DB have been performed.

>>
>>>
>>>         spin_unlock_irqrestore(host->host_lock, flags);
>>>
>>> --
>>> 1.8.5.2
>>>
>>> --
>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>>> member of Code Aurora Forum, hosted by The Linux Foundation
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>

Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers

[ygardi]

> 2015-10-25 23:40 GMT+09:00  <ygardi@codeaurora.org>:
>>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>>> Performing several writes to UFS host controller registers has
>>>> no gurrantee of ordering, so we must make sure register writes
>>>> to setup request list base address etc. are performed before the
>>>> run/stop register is enabled.
>>>> In addition, when setting up a task request, we must make sure
>>>> the updating of descriptors takes places before ringing the
>>>> doorbell, similarly to setting up a transfer request.
>>>>
>>>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>>
>>>> ---
>>>>  drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
>>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index fc2a52d..298511a 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32
>>>> reg)
>>>>          *  1           UTRLRDY
>>>>          *  2           UTMRLRDY
>>>>          *  3           UCRDY
>>>> -        *  4           HEI
>>>> -        *  5           DEI
>>>> -        * 6-7          reserved
>>>> +        * 4-7          reserved
>>>>          */
>>>> -       return (((reg) & (0xFF)) >> 1) ^ (0x07);
>>>> +       return ((reg & 0xFF) >> 1) ^ 0x07;
>>>>  }
>>>>
>>>>  /**
>>>> @@ -2726,7 +2724,7 @@ out:
>>>>   * To bring UFS host controller to operational state,
>>>>   * 1. Enable required interrupts
>>>>   * 2. Configure interrupt aggregation
>>>> - * 3. Program UTRL and UTMRL base addres
>>>> + * 3. Program UTRL and UTMRL base address
>>>>   * 4. Configure run-stop-registers
>>>>   *
>>>>   * Returns 0 on success, non-zero value on failure
>>>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
>>>> ufs_hba *hba)
>>>>                         REG_UTP_TASK_REQ_LIST_BASE_H);
>>>>
>>>>         /*
>>>> +        * Make sure base address and interrupt setup are updated
>>>> before
>>>> +        * enabling the run/stop registers below.
>>>> +        */
>>>> +       wmb();
>>>> +
>>>> +       /*
>>>>          * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>>>> -        * DEI, HEI bits must be 0
>>>>          */
>>>>         reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>>>>         if (!(ufshcd_get_lists_status(reg))) {
>>>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>>>> *hba, int lun_id, int task_id,
>>>>
>>>>         /* send command to the controller */
>>>>         __set_bit(free_slot, &hba->outstanding_tasks);
>>>> +
>>>> +       /* Make sure descriptors are ready before ringing the task
>>>> doorbell */
>>>> +       wmb();
>>>> +
>>>>         ufshcd_writel(hba, 1 << free_slot,
>>>> REG_UTP_TASK_REQ_DOOR_BELL);
>>>> +       /* Make sure that doorbell is committed immediately */
>>>> +       wmb();
>>>
>>> Is this wmb() after ringing tm doorbell is needed?
>>
>> Well, Mita, in the case of DB register (Request DB and TASK DB), every
>> write operation to the DB should be barrier, as if not, out of order
>> writing to this register might cause inconsistency in its value, and
>> thus,
>> un-handled requests/tasks.
>
> I couldn't fully understand why out of order writing to TASK DB
> register causes inconsistency.
> In the TASK request completion (ufshcd_tmc_handler), TASK DB register
> is read before handling finished requests, so it ensures that all
> write operations for TASK DB have been performed.
>

thanks, Mita
your explanation makes sense.
It will be fixed in v4.


>>>
>>>>
>>>>         spin_unlock_irqrestore(host->host_lock, flags);
>>>>
>>>> --
>>>> 1.8.5.2
>>>>
>>>> --
>>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>>>> member of Code Aurora Forum, hosted by The Linux Foundation
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
>>>> in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>

Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers

[ygardi]

>> 2015-10-25 23:40 GMT+09:00  <ygardi@codeaurora.org>:
>>>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>>>> Performing several writes to UFS host controller registers has
>>>>> no gurrantee of ordering, so we must make sure register writes
>>>>> to setup request list base address etc. are performed before the
>>>>> run/stop register is enabled.
>>>>> In addition, when setting up a task request, we must make sure
>>>>> the updating of descriptors takes places before ringing the
>>>>> doorbell, similarly to setting up a transfer request.
>>>>>
>>>>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>>>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>>>
>>>>> ---
>>>>>  drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
>>>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>> index fc2a52d..298511a 100644
>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32
>>>>> reg)
>>>>>          *  1           UTRLRDY
>>>>>          *  2           UTMRLRDY
>>>>>          *  3           UCRDY
>>>>> -        *  4           HEI
>>>>> -        *  5           DEI
>>>>> -        * 6-7          reserved
>>>>> +        * 4-7          reserved
>>>>>          */
>>>>> -       return (((reg) & (0xFF)) >> 1) ^ (0x07);
>>>>> +       return ((reg & 0xFF) >> 1) ^ 0x07;
>>>>>  }
>>>>>
>>>>>  /**
>>>>> @@ -2726,7 +2724,7 @@ out:
>>>>>   * To bring UFS host controller to operational state,
>>>>>   * 1. Enable required interrupts
>>>>>   * 2. Configure interrupt aggregation
>>>>> - * 3. Program UTRL and UTMRL base addres
>>>>> + * 3. Program UTRL and UTMRL base address
>>>>>   * 4. Configure run-stop-registers
>>>>>   *
>>>>>   * Returns 0 on success, non-zero value on failure
>>>>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
>>>>> ufs_hba *hba)
>>>>>                         REG_UTP_TASK_REQ_LIST_BASE_H);
>>>>>
>>>>>         /*
>>>>> +        * Make sure base address and interrupt setup are updated
>>>>> before
>>>>> +        * enabling the run/stop registers below.
>>>>> +        */
>>>>> +       wmb();
>>>>> +
>>>>> +       /*
>>>>>          * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>>>>> -        * DEI, HEI bits must be 0
>>>>>          */
>>>>>         reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>>>>>         if (!(ufshcd_get_lists_status(reg))) {
>>>>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>>>>> *hba, int lun_id, int task_id,
>>>>>
>>>>>         /* send command to the controller */
>>>>>         __set_bit(free_slot, &hba->outstanding_tasks);
>>>>> +
>>>>> +       /* Make sure descriptors are ready before ringing the task
>>>>> doorbell */
>>>>> +       wmb();
>>>>> +
>>>>>         ufshcd_writel(hba, 1 << free_slot,
>>>>> REG_UTP_TASK_REQ_DOOR_BELL);
>>>>> +       /* Make sure that doorbell is committed immediately */
>>>>> +       wmb();
>>>>
>>>> Is this wmb() after ringing tm doorbell is needed?
>>>
>>> Well, Mita, in the case of DB register (Request DB and TASK DB), every
>>> write operation to the DB should be barrier, as if not, out of order
>>> writing to this register might cause inconsistency in its value, and
>>> thus,
>>> un-handled requests/tasks.
>>
>> I couldn't fully understand why out of order writing to TASK DB
>> register causes inconsistency.
>> In the TASK request completion (ufshcd_tmc_handler), TASK DB register
>> is read before handling finished requests, so it ensures that all
>> write operations for TASK DB have been performed.
>>
>
> thanks, Mita
> your explanation makes sense.
> It will be fixed in v4.
>

sorry, it will fixed in v5.

Yaniv

>
>>>>
>>>>>
>>>>>         spin_unlock_irqrestore(host->host_lock, flags);
>>>>>
>>>>> --
>>>>> 1.8.5.2
>>>>>
>>>>> --
>>>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>>>>> member of Code Aurora Forum, hosted by The Linux Foundation
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
>>>>> in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>
>
>
>

Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers

[ygardi]

>> 2015-10-25 23:40 GMT+09:00  <ygardi@codeaurora.org>:
>>>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>>>> Performing several writes to UFS host controller registers has
>>>>> no gurrantee of ordering, so we must make sure register writes
>>>>> to setup request list base address etc. are performed before the
>>>>> run/stop register is enabled.
>>>>> In addition, when setting up a task request, we must make sure
>>>>> the updating of descriptors takes places before ringing the
>>>>> doorbell, similarly to setting up a transfer request.
>>>>>
>>>>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>>>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>>>
>>>>> ---
>>>>>  drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------
>>>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>> index fc2a52d..298511a 100644
>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32
>>>>> reg)
>>>>>          *  1           UTRLRDY
>>>>>          *  2           UTMRLRDY
>>>>>          *  3           UCRDY
>>>>> -        *  4           HEI
>>>>> -        *  5           DEI
>>>>> -        * 6-7          reserved
>>>>> +        * 4-7          reserved
>>>>>          */
>>>>> -       return (((reg) & (0xFF)) >> 1) ^ (0x07);
>>>>> +       return ((reg & 0xFF) >> 1) ^ 0x07;
>>>>>  }
>>>>>
>>>>>  /**
>>>>> @@ -2726,7 +2724,7 @@ out:
>>>>>   * To bring UFS host controller to operational state,
>>>>>   * 1. Enable required interrupts
>>>>>   * 2. Configure interrupt aggregation
>>>>> - * 3. Program UTRL and UTMRL base addres
>>>>> + * 3. Program UTRL and UTMRL base address
>>>>>   * 4. Configure run-stop-registers
>>>>>   *
>>>>>   * Returns 0 on success, non-zero value on failure
>>>>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
>>>>> ufs_hba *hba)
>>>>>                         REG_UTP_TASK_REQ_LIST_BASE_H);
>>>>>
>>>>>         /*
>>>>> +        * Make sure base address and interrupt setup are updated
>>>>> before
>>>>> +        * enabling the run/stop registers below.
>>>>> +        */
>>>>> +       wmb();
>>>>> +
>>>>> +       /*
>>>>>          * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>>>>> -        * DEI, HEI bits must be 0
>>>>>          */
>>>>>         reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>>>>>         if (!(ufshcd_get_lists_status(reg))) {
>>>>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>>>>> *hba, int lun_id, int task_id,
>>>>>
>>>>>         /* send command to the controller */
>>>>>         __set_bit(free_slot, &hba->outstanding_tasks);
>>>>> +
>>>>> +       /* Make sure descriptors are ready before ringing the task
>>>>> doorbell */
>>>>> +       wmb();
>>>>> +
>>>>>         ufshcd_writel(hba, 1 << free_slot,
>>>>> REG_UTP_TASK_REQ_DOOR_BELL);
>>>>> +       /* Make sure that doorbell is committed immediately */
>>>>> +       wmb();
>>>>
>>>> Is this wmb() after ringing tm doorbell is needed?
>>>
>>> Well, Mita, in the case of DB register (Request DB and TASK DB), every
>>> write operation to the DB should be barrier, as if not, out of order
>>> writing to this register might cause inconsistency in its value, and
>>> thus,
>>> un-handled requests/tasks.
>>
>> I couldn't fully understand why out of order writing to TASK DB
>> register causes inconsistency.
>> In the TASK request completion (ufshcd_tmc_handler), TASK DB register
>> is read before handling finished requests, so it ensures that all
>> write operations for TASK DB have been performed.
>>
>
> thanks, Mita
> your explanation makes sense.
> It will be fixed in v4.
>

sorry, it will fixed in v5.

Yaniv

>
>>>>
>>>>>
>>>>>         spin_unlock_irqrestore(host->host_lock, flags);
>>>>>
>>>>> --
>>>>> 1.8.5.2
>>>>>
>>>>> --
>>>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>>>>> member of Code Aurora Forum, hosted by The Linux Foundation
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
>>>>> in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>
>
>
>

[PATCH v3 14/15] scsi: ufs: commit descriptors before setting the doorbell

[Yaniv Gardi]

Add a write memory barrier to make sure descriptors prepared are actually
written to memory before ringing the doorbell. We have also added the
write memory barrier after ringing the doorbell register so that
controller sees the new request immediately.

Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 298511a..eaf98bb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1627,6 +1627,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 
 	hba->dev_cmd.complete = &wait;
 
+	/* Make sure descriptors are ready before ringing the doorbell */
+	wmb();
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_send_command(hba, tag);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 15/15] scsi: ufs: add wrapper for retrying sending query attribute

[Yaniv Gardi]

Sometimes queries from the device might return a failure so it is
recommended to retry sending the query, before giving up.
This change adds a wrapper to retry sending a query attribute,
in cases where we need to wait longer, before we continue,
or before reporting a failure.

Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 51 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index eaf98bb..c75e13d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1823,6 +1823,43 @@ out:
 }
 
 /**
+ * ufshcd_query_attr_retry() - API function for sending query
+ * attribute with retries
+ * @hba: per-adapter instance
+ * @opcode: attribute opcode
+ * @idn: attribute idn to access
+ * @index: index field
+ * @selector: selector field
+ * @attr_val: the attribute value after the query request
+ * completes
+ *
+ * Returns 0 for success, non-zero in case of failure
+*/
+static int ufshcd_query_attr_retry(struct ufs_hba *hba,
+	enum query_opcode opcode, enum attr_idn idn, u8 index, u8 selector,
+	u32 *attr_val)
+{
+	int ret = 0;
+	u32 retries;
+
+	 for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
+		ret = ufshcd_query_attr(hba, opcode, idn, index,
+						selector, attr_val);
+		if (ret)
+			dev_dbg(hba->dev, "%s: failed with error %d, retries %d\n",
+				__func__, ret, retries);
+		else
+			break;
+	}
+
+	if (ret)
+		dev_err(hba->dev,
+			"%s: query attribute, idn %d, failed with error %d after %d retires\n",
+			__func__, idn, ret, QUERY_REQ_RETRIES);
+	return ret;
+}
+
+/**
  * ufshcd_query_descriptor - API function for sending descriptor requests
  * hba: per-adapter instance
  * opcode: attribute opcode
@@ -3403,7 +3440,7 @@ static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
 
 	val = hba->ee_ctrl_mask & ~mask;
 	val &= 0xFFFF; /* 2 bytes */
-	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
 			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
 	if (!err)
 		hba->ee_ctrl_mask &= ~mask;
@@ -3431,7 +3468,7 @@ static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
 
 	val = hba->ee_ctrl_mask | mask;
 	val &= 0xFFFF; /* 2 bytes */
-	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
 			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
 	if (!err)
 		hba->ee_ctrl_mask |= mask;
@@ -3537,7 +3574,7 @@ static void  ufshcd_force_reset_auto_bkops(struct ufs_hba *hba)
 
 static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 *status)
 {
-	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+	return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
 			QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
 }
 
@@ -3600,7 +3637,7 @@ static int ufshcd_urgent_bkops(struct ufs_hba *hba)
 
 static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
 {
-	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+	return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
 			QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
 }
 
@@ -4356,9 +4393,9 @@ static void ufshcd_init_icc_levels(struct ufs_hba *hba)
 	dev_dbg(hba->dev, "%s: setting icc_level 0x%x",
 			__func__, hba->init_prefetch_data.icc_level);
 
-	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
-			QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
-			&hba->init_prefetch_data.icc_level);
+	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+		QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
+		&hba->init_prefetch_data.icc_level);
 
 	if (ret)
 		dev_err(hba->dev,
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation