Groups > linux.kernel > #1211346 > unrolled thread

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

Started by	Yaniv Gardi <ygardi@codeaurora.org>
First post	2015-08-22 00:20 +0200
Last post	2015-08-22 00:20 +0200
Articles	20 on this page of 23 — 3 participants

Back to article view | Back to linux.kernel

  [PATCH v1 00/15] Big fixes, retries, handle a race condition Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 02/15] scsi: ufs: clear fields UTRD, UPIU req and rsp before new transfers Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 03/15] scsi: ufs: verify command tag validity Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
      Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending  query attribute Rob Herring <robherring2@gmail.com> - 2015-08-23 23:10 +0200
        Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending  query attribute ygardi@codeaurora.org - 2015-08-25 14:50 +0200
          Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending  query attribute ygardi@codeaurora.org - 2015-08-25 15:30 +0200
    [PATCH v1 04/15] scsi: ufs: clear outstanding_request bit in case query timeout Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 11/15] scsi: ufs: retry failed query flag requests Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
      Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell Rob Herring <robherring2@gmail.com> - 2015-08-23 23:20 +0200
        Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting  the doorbell ygardi@codeaurora.org - 2015-08-25 14:40 +0200
          Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell Rob Herring <robherring2@gmail.com> - 2015-08-25 20:30 +0200
            Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting  the doorbell ygardi@codeaurora.org - 2015-08-27 14:20 +0200
              Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell Rob Herring <robherring2@gmail.com> - 2015-08-27 19:30 +0200
                Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting  the doorbell ygardi@codeaurora.org - 2015-08-30 12:00 +0200
          Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting  the doorbell ygardi@codeaurora.org - 2015-08-27 14:30 +0200
            Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell Rob Herring <robherring2@gmail.com> - 2015-08-27 19:10 +0200
    [PATCH v1 10/15] scsi: ufs: fix error recovery after the hibern8 exit failure Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 05/15] scsi: ufs: increase fDeviceInit query response timeout Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 13/15] scsi: ufs: add missing memory barriers Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 12/15] scsi: ufs: reduce the interrupts for power mode change requests Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200
    [PATCH v1 08/15] scsi: ufs: add retries to dme_peer get and set attribute Yaniv Gardi <ygardi@codeaurora.org> - 2015-08-22 00:20 +0200

Page 1 of 2 [1] 2 Next page →

#1211346 — [PATCH v1 00/15] Big fixes, retries, handle a race condition

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 00/15] Big fixes, retries, handle a race condition
Message-ID	<q02Pf-8pP-5@gated-at.bofh.it>

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 | 411 ++++++++++++++++++++++++++++++++++++----------
 drivers/scsi/ufs/ufshcd.h |   4 +
 2 files changed, 331 insertions(+), 84 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
--
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/

[toc] | [next] | [standalone]

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

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 02/15] scsi: ufs: clear fields UTRD, UPIU req and rsp before new transfers
Message-ID	<q02Pg-8pP-15@gated-at.bofh.it>
In reply to	#1211346

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: 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-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/

[toc] | [prev] | [next] | [standalone]

#1211348 — [PATCH v1 03/15] scsi: ufs: verify command tag validity

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 03/15] scsi: ufs: verify command tag validity
Message-ID	<q02Pf-8pP-9@gated-at.bofh.it>
In reply to	#1211346

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: Yaniv Gardi <ygardi@codeaurora.org>

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2d3ebca..61e98a9 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,11 +3872,21 @@ 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);
 	/* 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))) {
-- 
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/

[toc] | [prev] | [next] | [standalone]

#1211349 — [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute
Message-ID	<q02Pg-8pP-17@gated-at.bofh.it>
In reply to	#1211346

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 876148b..bfef67d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1827,6 +1827,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, retries);
+	return ret;
+}
+
+/**
  * ufshcd_query_descriptor - API function for sending descriptor requests
  * hba: per-adapter instance
  * opcode: attribute opcode
@@ -3407,7 +3444,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;
@@ -3435,7 +3472,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;
@@ -3541,7 +3578,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);
 }
 
@@ -3604,7 +3641,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);
 }
 
@@ -4360,9 +4397,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
--
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/

[toc] | [prev] | [next] | [standalone]

#1211668 — Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

From	Rob Herring <robherring2@gmail.com>
Date	2015-08-23 23:10 +0200
Subject	Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute
Message-ID	<q0KGC-3W2-25@gated-at.bofh.it>
In reply to	#1211349

On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>
> 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.

Why not just always retry? Are there cases where retrying would be a problem?


>
> 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 876148b..bfef67d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1827,6 +1827,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, retries);

The retry count will be wrong here.

> +       return ret;
> +}
> +
> +/**
>   * ufshcd_query_descriptor - API function for sending descriptor requests
>   * hba: per-adapter instance
>   * opcode: attribute opcode
> @@ -3407,7 +3444,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;
> @@ -3435,7 +3472,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;
> @@ -3541,7 +3578,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);
>  }
>
> @@ -3604,7 +3641,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);
>  }
>
> @@ -4360,9 +4397,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
--
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/

[toc] | [prev] | [next] | [standalone]

#1213023 — Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

From	ygardi@codeaurora.org
Date	2015-08-25 14:50 +0200
Subject	Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute
Message-ID	<q1lPQ-6JQ-17@gated-at.bofh.it>
In reply to	#1211668

> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>
>> 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.
>
> Why not just always retry? Are there cases where retrying would be a
> problem?

There is no problem to retry whenever we encounter a query that returns
with and error.
In the code, it's recommended to replace any call to


>
>
>>
>> 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 876148b..bfef67d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1827,6 +1827,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, retries);
>
> The retry count will be wrong here.

you are correct. will be fixed in V2
>
>> +       return ret;
>> +}
>> +
>> +/**
>>   * ufshcd_query_descriptor - API function for sending descriptor
>> requests
>>   * hba: per-adapter instance
>>   * opcode: attribute opcode
>> @@ -3407,7 +3444,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;
>> @@ -3435,7 +3472,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;
>> @@ -3541,7 +3578,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);
>>  }
>>
>> @@ -3604,7 +3641,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);
>>  }
>>
>> @@ -4360,9 +4397,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
>


--
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/

[toc] | [prev] | [next] | [standalone]

#1213037 — Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

From	ygardi@codeaurora.org
Date	2015-08-25 15:30 +0200
Subject	Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute
Message-ID	<q1msz-7Ih-19@gated-at.bofh.it>
In reply to	#1213023

> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>
>> 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.
>
> Why not just always retry? Are there cases where retrying would be a
> problem?

There is no problem to retry whenever we encounter a query that returns
with and error.

>
>
>>
>> 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 876148b..bfef67d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1827,6 +1827,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, retries);
>
> The retry count will be wrong here.

you are correct. will be fixed in V2
>
>> +       return ret;
>> +}
>> +
>> +/**
>>   * ufshcd_query_descriptor - API function for sending descriptor
>> requests
>>   * hba: per-adapter instance
>>   * opcode: attribute opcode
>> @@ -3407,7 +3444,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;
>> @@ -3435,7 +3472,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;
>> @@ -3541,7 +3578,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);
>>  }
>>
>> @@ -3604,7 +3641,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);
>>  }
>>
>> @@ -4360,9 +4397,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
>


--
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

Delete & Prev | Delete & Next
Move to:


--
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/

[toc] | [prev] | [next] | [standalone]

#1211351 — [PATCH v1 04/15] scsi: ufs: clear outstanding_request bit in case query timeout

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 04/15] scsi: ufs: clear outstanding_request bit in case query timeout
Message-ID	<q02Pg-8pP-23@gated-at.bofh.it>
In reply to	#1211346

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 61e98a9..c346a300 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-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/

[toc] | [prev] | [next] | [standalone]

#1211352 — [PATCH v1 11/15] scsi: ufs: retry failed query flag requests

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 11/15] scsi: ufs: retry failed query flag requests
Message-ID	<q02Pg-8pP-29@gated-at.bofh.it>
In reply to	#1211346

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: 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 30aec4d..0a8aa88 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 4bb2697..2e8f64b 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>
@@ -680,6 +681,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
--
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/

[toc] | [prev] | [next] | [standalone]

#1211353 — [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q02Pg-8pP-33@gated-at.bofh.it>
In reply to	#1211346

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: Yaniv Gardi <ygardi@codeaurora.org>

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fef0660..876148b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 	ufshcd_clk_scaling_start_busy(hba);
 	__set_bit(task_tag, &hba->outstanding_reqs);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+	/* Make sure that doorbell is committed immediately */
+	wmb();
 }
 
 /**
@@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		goto out;
 	}
 
+	/* Make sure descriptors are ready before ringing the doorbell */
+	wmb();
 	/* issue command to the controller */
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_send_command(hba, tag);
@@ -1627,6 +1631,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
--
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/

[toc] | [prev] | [next] | [standalone]

#1211670 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	Rob Herring <robherring2@gmail.com>
Date	2015-08-23 23:20 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q0KQh-47k-1@gated-at.bofh.it>
In reply to	#1211353

On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>
> 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: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fef0660..876148b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
>         ufshcd_clk_scaling_start_busy(hba);
>         __set_bit(task_tag, &hba->outstanding_reqs);
>         ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> +       /* Make sure that doorbell is committed immediately */
> +       wmb();

Is this really necessary? Is there a measurable difference?

>  }
>
>  /**
> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>                 goto out;
>         }
>
> +       /* Make sure descriptors are ready before ringing the doorbell */
> +       wmb();

The writel for the doorbell will do a barrier first. (I didn't check
what exactly ufshcd_writel does, but that is why I don't like these
private access wrappers.)

>         /* issue command to the controller */
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         ufshcd_send_command(hba, tag);
> @@ -1627,6 +1631,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
--
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/

[toc] | [prev] | [next] | [standalone]

#1213020 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	ygardi@codeaurora.org
Date	2015-08-25 14:40 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q1lGa-6y6-13@gated-at.bofh.it>
In reply to	#1211670

> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>
>> 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: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index fef0660..876148b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba,
>> unsigned int task_tag)
>>         ufshcd_clk_scaling_start_busy(hba);
>>         __set_bit(task_tag, &hba->outstanding_reqs);
>>         ufshcd_writel(hba, 1 << task_tag,
>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> +       /* Make sure that doorbell is committed immediately */
>> +       wmb();
>
> Is this really necessary? Is there a measurable difference?

I'm not sure if there is a measurable difference, but as the Door-Bell
register is the one that actually responsible for the HW execution of the
requests, anyhow, it's recommended to its value will be written
instantly to the memory.

Also, as the Interrupt context reads this register, and compare it to the
SW mirroring value (hba->outstanding_reqs) in order to realize what
requests are already completed, it's important to get the correct value
by reading this register, otherwise we might realize a request completion
while it was never even submitted.

>
>>  }
>>
>>  /**
>> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>>                 goto out;
>>         }
>>
>> +       /* Make sure descriptors are ready before ringing the doorbell
>> */
>> +       wmb();
>
> The writel for the doorbell will do a barrier first. (I didn't check
> what exactly ufshcd_writel does, but that is why I don't like these
> private access wrappers.)
>
>>         /* issue command to the controller */
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>         ufshcd_send_command(hba, tag);
>> @@ -1627,6 +1631,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
> --
> 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
>


--
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/

[toc] | [prev] | [next] | [standalone]

#1213226 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	Rob Herring <robherring2@gmail.com>
Date	2015-08-25 20:30 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q1r8S-68b-39@gated-at.bofh.it>
In reply to	#1213020

On Tue, Aug 25, 2015 at 7:36 AM,  <ygardi@codeaurora.org> wrote:
>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>>
>>> 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: Yaniv Gardi <ygardi@codeaurora.org>
>>>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index fef0660..876148b 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba,
>>> unsigned int task_tag)
>>>         ufshcd_clk_scaling_start_busy(hba);
>>>         __set_bit(task_tag, &hba->outstanding_reqs);
>>>         ufshcd_writel(hba, 1 << task_tag,
>>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>> +       /* Make sure that doorbell is committed immediately */
>>> +       wmb();
>>
>> Is this really necessary? Is there a measurable difference?
>
> I'm not sure if there is a measurable difference, but as the Door-Bell
> register is the one that actually responsible for the HW execution of the
> requests, anyhow, it's recommended to its value will be written
> instantly to the memory.

A barrier doesn't guarantee speed, only ordering. Unless you can
measure the difference, you should not have it.

> Also, as the Interrupt context reads this register, and compare it to the
> SW mirroring value (hba->outstanding_reqs) in order to realize what
> requests are already completed, it's important to get the correct value
> by reading this register, otherwise we might realize a request completion
> while it was never even submitted.

If a register read can pass a register write out of order, then your
h/w is broken. Plus what if the interrupt occurs before the barrier.

Rob
--
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/

[toc] | [prev] | [next] | [standalone]

#1214500 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	ygardi@codeaurora.org
Date	2015-08-27 14:20 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q24jT-4aB-1@gated-at.bofh.it>
In reply to	#1213226

> On Tue, Aug 25, 2015 at 7:36 AM,  <ygardi@codeaurora.org> wrote:
>>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>>>
>>>> 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: Yaniv Gardi <ygardi@codeaurora.org>
>>>>
>>>> ---
>>>>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index fef0660..876148b 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba,
>>>> unsigned int task_tag)
>>>>         ufshcd_clk_scaling_start_busy(hba);
>>>>         __set_bit(task_tag, &hba->outstanding_reqs);
>>>>         ufshcd_writel(hba, 1 << task_tag,
>>>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>>> +       /* Make sure that doorbell is committed immediately */
>>>> +       wmb();
>>>
>>> Is this really necessary? Is there a measurable difference?
>>
>> I'm not sure if there is a measurable difference, but as the Door-Bell
>> register is the one that actually responsible for the HW execution of
>> the
>> requests, anyhow, it's recommended to its value will be written
>> instantly to the memory.
>
> A barrier doesn't guarantee speed, only ordering. Unless you can
> measure the difference, you should not have it.

Rob,
let me have an example:
context#1 updates outstanding_reqs variable and write(DOOR_BELL)
context#2 upon interrupt of a request completion the following happens:
  report completion on each one of the bits in:
  outstanding_reqs ^ read(DOOR_BELL);

0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in slot 0)
1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in slot
0 and 1)
2. the new value 0x3 is still not written to the DR so DORR_BELL is still
0x1, but outstanding_reqs is already updated = 0x3
3. the request in slot 0 just completed, and interrupt happens, so
DORR_BELL is now 0 (request in slot 0 completed)
4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 =>
wrong conclusion since the request in slot 1 never completed, and actually
never started.


>
>> Also, as the Interrupt context reads this register, and compare it to
>> the
>> SW mirroring value (hba->outstanding_reqs) in order to realize what
>> requests are already completed, it's important to get the correct value
>> by reading this register, otherwise we might realize a request
>> completion
>> while it was never even submitted.
>
> If a register read can pass a register write out of order, then your
> h/w is broken. Plus what if the interrupt occurs before the barrier.
>
> Rob
> --
> 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
>


--
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/

[toc] | [prev] | [next] | [standalone]

#1214731 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	Rob Herring <robherring2@gmail.com>
Date	2015-08-27 19:30 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q299U-2Go-11@gated-at.bofh.it>
In reply to	#1214500

On Thu, Aug 27, 2015 at 7:11 AM,  <ygardi@codeaurora.org> wrote:
>> On Tue, Aug 25, 2015 at 7:36 AM,  <ygardi@codeaurora.org> wrote:
>>>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>>>>
>>>>> 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: Yaniv Gardi <ygardi@codeaurora.org>
>>>>>
>>>>> ---
>>>>>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>> index fef0660..876148b 100644
>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba,
>>>>> unsigned int task_tag)
>>>>>         ufshcd_clk_scaling_start_busy(hba);
>>>>>         __set_bit(task_tag, &hba->outstanding_reqs);
>>>>>         ufshcd_writel(hba, 1 << task_tag,
>>>>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>>>> +       /* Make sure that doorbell is committed immediately */
>>>>> +       wmb();
>>>>
>>>> Is this really necessary? Is there a measurable difference?
>>>
>>> I'm not sure if there is a measurable difference, but as the Door-Bell
>>> register is the one that actually responsible for the HW execution of
>>> the
>>> requests, anyhow, it's recommended to its value will be written
>>> instantly to the memory.
>>
>> A barrier doesn't guarantee speed, only ordering. Unless you can
>> measure the difference, you should not have it.
>
> Rob,
> let me have an example:
> context#1 updates outstanding_reqs variable and write(DOOR_BELL)
> context#2 upon interrupt of a request completion the following happens:
>   report completion on each one of the bits in:
>   outstanding_reqs ^ read(DOOR_BELL);
>
> 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in slot 0)
> 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in slot
> 0 and 1)
> 2. the new value 0x3 is still not written to the DR so DORR_BELL is still
> 0x1, but outstanding_reqs is already updated = 0x3
> 3. the request in slot 0 just completed, and interrupt happens, so
> DORR_BELL is now 0 (request in slot 0 completed)
> 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 =>
> wrong conclusion since the request in slot 1 never completed, and actually
> never started.

Barriers alone will never solve this problem. They may narrow the
window possibly, but the problem is still there. What you have to have
is a spinlock around all accesses to both outstanding_reqs and
doorbell register. And guess what, spinlocks have appropriate barriers
to ensure visibility of what they protect. Or perhaps the h/w provides
another way to signal what slots have completed. Using the same
register for doorbell and completion status is not ideal.

Rob
--
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/

[toc] | [prev] | [next] | [standalone]

#1215904 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	ygardi@codeaurora.org
Date	2015-08-30 12:00 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q37z5-5rG-39@gated-at.bofh.it>
In reply to	#1214731

> On Thu, Aug 27, 2015 at 7:11 AM,  <ygardi@codeaurora.org> wrote:
>>> On Tue, Aug 25, 2015 at 7:36 AM,  <ygardi@codeaurora.org> wrote:
>>>>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>>>>>
>>>>>> 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: Yaniv Gardi <ygardi@codeaurora.org>
>>>>>>
>>>>>> ---
>>>>>>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>>> index fef0660..876148b 100644
>>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba,
>>>>>> unsigned int task_tag)
>>>>>>         ufshcd_clk_scaling_start_busy(hba);
>>>>>>         __set_bit(task_tag, &hba->outstanding_reqs);
>>>>>>         ufshcd_writel(hba, 1 << task_tag,
>>>>>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>>>>> +       /* Make sure that doorbell is committed immediately */
>>>>>> +       wmb();
>>>>>
>>>>> Is this really necessary? Is there a measurable difference?
>>>>
>>>> I'm not sure if there is a measurable difference, but as the Door-Bell
>>>> register is the one that actually responsible for the HW execution of
>>>> the
>>>> requests, anyhow, it's recommended to its value will be written
>>>> instantly to the memory.
>>>
>>> A barrier doesn't guarantee speed, only ordering. Unless you can
>>> measure the difference, you should not have it.
>>
>> Rob,
>> let me have an example:
>> context#1 updates outstanding_reqs variable and write(DOOR_BELL)
>> context#2 upon interrupt of a request completion the following happens:
>>   report completion on each one of the bits in:
>>   outstanding_reqs ^ read(DOOR_BELL);
>>
>> 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in
>> slot 0)
>> 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in
>> slot
>> 0 and 1)
>> 2. the new value 0x3 is still not written to the DR so DORR_BELL is
>> still
>> 0x1, but outstanding_reqs is already updated = 0x3
>> 3. the request in slot 0 just completed, and interrupt happens, so
>> DORR_BELL is now 0 (request in slot 0 completed)
>> 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 =>
>> wrong conclusion since the request in slot 1 never completed, and
>> actually
>> never started.
>
> Barriers alone will never solve this problem. They may narrow the
> window possibly, but the problem is still there. What you have to have
> is a spinlock around all accesses to both outstanding_reqs and
> doorbell register. And guess what, spinlocks have appropriate barriers
> to ensure visibility of what they protect. Or perhaps the h/w provides
> another way to signal what slots have completed. Using the same
> register for doorbell and completion status is not ideal.
>

can i assume spin_lock_irqsave() and spin_unlock_irqrestore()
both provide barriers ? i couldn't find the barrier instruction
when following the call chain...


> Rob
> --
> 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
>


--
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/

[toc] | [prev] | [next] | [standalone]

#1214507 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	ygardi@codeaurora.org
Date	2015-08-27 14:30 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q24tA-4lT-11@gated-at.bofh.it>
In reply to	#1213020

>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>>
>>> 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: Yaniv Gardi <ygardi@codeaurora.org>
>>>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index fef0660..876148b 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba,
>>> unsigned int task_tag)
>>>         ufshcd_clk_scaling_start_busy(hba);
>>>         __set_bit(task_tag, &hba->outstanding_reqs);
>>>         ufshcd_writel(hba, 1 << task_tag,
>>> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>> +       /* Make sure that doorbell is committed immediately */
>>> +       wmb();
>>
>> Is this really necessary? Is there a measurable difference?
>
> I'm not sure if there is a measurable difference, but as the Door-Bell
> register is the one that actually responsible for the HW execution of the
> requests, anyhow, it's recommended to its value will be written
> instantly to the memory.
>
> Also, as the Interrupt context reads this register, and compare it to the
> SW mirroring value (hba->outstanding_reqs) in order to realize what
> requests are already completed, it's important to get the correct value
> by reading this register, otherwise we might realize a request completion
> while it was never even submitted.
>
>>
>>>  }
>>>
>>>  /**
>>> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host
>>> *host, struct scsi_cmnd *cmd)
>>>                 goto out;
>>>         }
>>>
>>> +       /* Make sure descriptors are ready before ringing the doorbell
>>> */
>>> +       wmb();
>>
>> The writel for the doorbell will do a barrier first. (I didn't check
>> what exactly ufshcd_writel does, but that is why I don't like these
>> private access wrappers.)

the barrier here is important and a must.
before it we prepare descriptors.
after it we write to DOOR-BELL.
(and after the DOOR-BELL we have another one.)
if we remove it, we might get the DOOR-BELL written, before the
descriptors written.




>>
>>>         /* issue command to the controller */
>>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>>         ufshcd_send_command(hba, tag);
>>> @@ -1627,6 +1631,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
>> --
>> 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
>>
>
>
> --
> 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
>


--
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/

[toc] | [prev] | [next] | [standalone]

#1214715 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

From	Rob Herring <robherring2@gmail.com>
Date	2015-08-27 19:10 +0200
Subject	Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
Message-ID	<q28Qz-2jx-33@gated-at.bofh.it>
In reply to	#1214507

On Thu, Aug 27, 2015 at 7:28 AM,  <ygardi@codeaurora.org> wrote:
>>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> wrote:
>>>>
>>>> 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.

[...]

>>>> +       /* Make sure descriptors are ready before ringing the doorbell
>>>> */
>>>> +       wmb();
>>>
>>> The writel for the doorbell will do a barrier first. (I didn't check
>>> what exactly ufshcd_writel does, but that is why I don't like these
>>> private access wrappers.)
>
> the barrier here is important and a must.
> before it we prepare descriptors.
> after it we write to DOOR-BELL.
> (and after the DOOR-BELL we have another one.)
> if we remove it, we might get the DOOR-BELL written, before the
> descriptors written.

If you dig into what writel does, you will see that what the code here
ends up being is:

descriptor setup
wmb()
__iomwb() -> wmb on arm64
doorbell register write

So explain why you need 2 barriers.

Barriers in a driver are a red flag. Usually they are not needed, but
sometimes they are. You have to be able to explain why if they are.
Pretty much every descriptor based DMA device works as you describe
and has the ordering problem. Because of that, the core code takes
care of this.

Rob
--
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/

[toc] | [prev] | [next] | [standalone]

#1211354 — [PATCH v1 10/15] scsi: ufs: fix error recovery after the hibern8 exit failure

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 10/15] scsi: ufs: fix error recovery after the hibern8 exit failure
Message-ID	<q02Pg-8pP-31@gated-at.bofh.it>
In reply to	#1211346

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: 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 6d47e9e..30aec4d 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
--
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/

[toc] | [prev] | [next] | [standalone]

#1211355 — [PATCH v1 05/15] scsi: ufs: increase fDeviceInit query response timeout

From	Yaniv Gardi <ygardi@codeaurora.org>
Date	2015-08-22 00:20 +0200
Subject	[PATCH v1 05/15] scsi: ufs: increase fDeviceInit query response timeout
Message-ID	<q02Ph-8pP-41@gated-at.bofh.it>
In reply to	#1211346

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: 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 c346a300..ab48220 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
--
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/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2 [1] 2 Next page →

csiph-web

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

Contents

#1211346 — [PATCH v1 00/15] Big fixes, retries, handle a race condition

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

#1211348 — [PATCH v1 03/15] scsi: ufs: verify command tag validity

#1211349 — [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

#1211668 — Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

#1213023 — Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

#1213037 — Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

#1211351 — [PATCH v1 04/15] scsi: ufs: clear outstanding_request bit in case query timeout

#1211352 — [PATCH v1 11/15] scsi: ufs: retry failed query flag requests

#1211353 — [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1211670 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1213020 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1213226 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1214500 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1214731 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1215904 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1214507 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1214715 — Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

#1211354 — [PATCH v1 10/15] scsi: ufs: fix error recovery after the hibern8 exit failure

#1211355 — [PATCH v1 05/15] scsi: ufs: increase fDeviceInit query response timeout