* [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings
2013-09-19 11:14 [PATCH V7 0/6] scsi: ufs: TM, fatal-error handling and other fixes Sujit Reddy Thumma
@ 2013-09-19 11:14 ` Sujit Reddy Thumma
2013-09-23 12:14 ` vinayak holikatti
2013-09-19 11:14 ` [PATCH V7 2/6] scsi: ufs: make undeclared functions static Sujit Reddy Thumma
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Sujit Reddy Thumma @ 2013-09-19 11:14 UTC (permalink / raw)
To: Vinayak Holikatti, Santosh Y
Cc: James E.J. Bottomley, linux-scsi, Sujit Reddy Thumma,
linux-arm-msm
Fix many warnings with incorrect endian assumptions
which makes the code unportable to new architectures.
The UFS specification defines the byte order as big-endian
for UPIU structure and little-endian for the host controller
transfer/task management descriptors.
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
---
drivers/scsi/ufs/ufs.h | 36 ++++++++++++++++++------------------
drivers/scsi/ufs/ufshcd.c | 42 ++++++++----------------------------------
drivers/scsi/ufs/ufshci.h | 32 ++++++++++++++++----------------
3 files changed, 42 insertions(+), 68 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 7210500..f42d1ce 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -196,9 +196,9 @@ enum {
* @dword_2: UPIU header DW-2
*/
struct utp_upiu_header {
- u32 dword_0;
- u32 dword_1;
- u32 dword_2;
+ __be32 dword_0;
+ __be32 dword_1;
+ __be32 dword_2;
};
/**
@@ -207,7 +207,7 @@ struct utp_upiu_header {
* @cdb: Command Descriptor Block CDB DW-4 to DW-7
*/
struct utp_upiu_cmd {
- u32 exp_data_transfer_len;
+ __be32 exp_data_transfer_len;
u8 cdb[MAX_CDB_SIZE];
};
@@ -228,10 +228,10 @@ struct utp_upiu_query {
u8 idn;
u8 index;
u8 selector;
- u16 reserved_osf;
- u16 length;
- u32 value;
- u32 reserved[2];
+ __be16 reserved_osf;
+ __be16 length;
+ __be32 value;
+ __be32 reserved[2];
};
/**
@@ -256,9 +256,9 @@ struct utp_upiu_req {
* @sense_data: Sense data field DW-8 to DW-12
*/
struct utp_cmd_rsp {
- u32 residual_transfer_count;
- u32 reserved[4];
- u16 sense_data_len;
+ __be32 residual_transfer_count;
+ __be32 reserved[4];
+ __be16 sense_data_len;
u8 sense_data[18];
};
@@ -286,10 +286,10 @@ struct utp_upiu_rsp {
*/
struct utp_upiu_task_req {
struct utp_upiu_header header;
- u32 input_param1;
- u32 input_param2;
- u32 input_param3;
- u32 reserved[2];
+ __be32 input_param1;
+ __be32 input_param2;
+ __be32 input_param3;
+ __be32 reserved[2];
};
/**
@@ -301,9 +301,9 @@ struct utp_upiu_task_req {
*/
struct utp_upiu_task_rsp {
struct utp_upiu_header header;
- u32 output_param1;
- u32 output_param2;
- u32 reserved[3];
+ __be32 output_param1;
+ __be32 output_param2;
+ __be32 reserved[3];
};
/**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 04884d6..064c9d9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -163,7 +163,7 @@ static inline int ufshcd_is_device_present(u32 reg_hcs)
*/
static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
{
- return lrbp->utr_descriptor_ptr->header.dword_2 & MASK_OCS;
+ return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
}
/**
@@ -176,7 +176,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
static inline int
ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
{
- return task_req_descp->header.dword_2 & MASK_OCS;
+ return le32_to_cpu(task_req_descp->header.dword_2) & MASK_OCS;
}
/**
@@ -390,26 +390,6 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
}
/**
- * ufshcd_query_to_cpu() - formats the buffer to native cpu endian
- * @response: upiu query response to convert
- */
-static inline void ufshcd_query_to_cpu(struct utp_upiu_query *response)
-{
- response->length = be16_to_cpu(response->length);
- response->value = be32_to_cpu(response->value);
-}
-
-/**
- * ufshcd_query_to_be() - formats the buffer to big endian
- * @request: upiu query request to convert
- */
-static inline void ufshcd_query_to_be(struct utp_upiu_query *request)
-{
- request->length = cpu_to_be16(request->length);
- request->value = cpu_to_be32(request->value);
-}
-
-/**
* ufshcd_copy_query_response() - Copy the Query Response and the data
* descriptor
* @hba: per adapter instance
@@ -425,7 +405,6 @@ void ufshcd_copy_query_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
UPIU_RSP_CODE_OFFSET;
memcpy(&query_res->upiu_res, &lrbp->ucd_rsp_ptr->qr, QUERY_OSF_SIZE);
- ufshcd_query_to_cpu(&query_res->upiu_res);
/* Get the descriptor */
@@ -749,7 +728,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
{
struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
struct ufs_query *query = &hba->dev_cmd.query;
- u16 len = query->request.upiu_req.length;
+ u16 len = be16_to_cpu(query->request.upiu_req.length);
u8 *descp = (u8 *)lrbp->ucd_req_ptr + GENERAL_UPIU_REQUEST_SIZE;
/* Query request header */
@@ -766,7 +745,6 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
/* Copy the Query Request buffer as is */
memcpy(&ucd_req_ptr->qr, &query->request.upiu_req,
QUERY_OSF_SIZE);
- ufshcd_query_to_be(&ucd_req_ptr->qr);
/* Copy the Descriptor */
if ((len > 0) && (query->request.upiu_req.opcode ==
@@ -1151,7 +1129,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
}
if (flag_res)
- *flag_res = (response->upiu_res.value &
+ *flag_res = (be32_to_cpu(response->upiu_res.value) &
MASK_QUERY_UPIU_FLAG_LOC) & 0x1;
out_unlock:
@@ -1195,7 +1173,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
switch (opcode) {
case UPIU_QUERY_OPCODE_WRITE_ATTR:
request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
- request->upiu_req.value = *attr_val;
+ request->upiu_req.value = cpu_to_be32(*attr_val);
break;
case UPIU_QUERY_OPCODE_READ_ATTR:
request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
@@ -1222,7 +1200,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
goto out_unlock;
}
- *attr_val = response->upiu_res.value;
+ *attr_val = be32_to_cpu(response->upiu_res.value);
out_unlock:
mutex_unlock(&hba->dev_cmd.lock);
@@ -2568,12 +2546,8 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
task_req_upiup->header.dword_1 =
UPIU_HEADER_DWORD(0, tm_function, 0, 0);
- task_req_upiup->input_param1 = lrbp->lun;
- task_req_upiup->input_param1 =
- cpu_to_be32(task_req_upiup->input_param1);
- task_req_upiup->input_param2 = lrbp->task_tag;
- task_req_upiup->input_param2 =
- cpu_to_be32(task_req_upiup->input_param2);
+ task_req_upiup->input_param1 = cpu_to_be32(lrbp->lun);
+ task_req_upiup->input_param2 = cpu_to_be32(lrbp->task_tag);
/* send command to the controller */
__set_bit(free_slot, &hba->outstanding_tasks);
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 0475c66..9abc7e3 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -304,10 +304,10 @@ enum {
* @size: size of physical segment DW-3
*/
struct ufshcd_sg_entry {
- u32 base_addr;
- u32 upper_addr;
- u32 reserved;
- u32 size;
+ __le32 base_addr;
+ __le32 upper_addr;
+ __le32 reserved;
+ __le32 size;
};
/**
@@ -330,10 +330,10 @@ struct utp_transfer_cmd_desc {
* @dword3: Descriptor Header DW3
*/
struct request_desc_header {
- u32 dword_0;
- u32 dword_1;
- u32 dword_2;
- u32 dword_3;
+ __le32 dword_0;
+ __le32 dword_1;
+ __le32 dword_2;
+ __le32 dword_3;
};
/**
@@ -352,16 +352,16 @@ struct utp_transfer_req_desc {
struct request_desc_header header;
/* DW 4-5*/
- u32 command_desc_base_addr_lo;
- u32 command_desc_base_addr_hi;
+ __le32 command_desc_base_addr_lo;
+ __le32 command_desc_base_addr_hi;
/* DW 6 */
- u16 response_upiu_length;
- u16 response_upiu_offset;
+ __le16 response_upiu_length;
+ __le16 response_upiu_offset;
/* DW 7 */
- u16 prd_table_length;
- u16 prd_table_offset;
+ __le16 prd_table_length;
+ __le16 prd_table_offset;
};
/**
@@ -376,10 +376,10 @@ struct utp_task_req_desc {
struct request_desc_header header;
/* DW 4-11 */
- u32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
+ __le32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
/* DW 12-19 */
- u32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
+ __le32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
};
#endif /* End of Header */
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings
2013-09-19 11:14 ` [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings Sujit Reddy Thumma
@ 2013-09-23 12:14 ` vinayak holikatti
2013-11-18 9:04 ` vinayak holikatti
0 siblings, 1 reply; 15+ messages in thread
From: vinayak holikatti @ 2013-09-23 12:14 UTC (permalink / raw)
To: Sujit Reddy Thumma
Cc: Santosh Y, James E.J. Bottomley, linux-scsi, linux-arm-msm
On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
>
> Fix many warnings with incorrect endian assumptions
> which makes the code unportable to new architectures.
>
> The UFS specification defines the byte order as big-endian
> for UPIU structure and little-endian for the host controller
> transfer/task management descriptors.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> ---
> drivers/scsi/ufs/ufs.h | 36 ++++++++++++++++++------------------
> drivers/scsi/ufs/ufshcd.c | 42 ++++++++----------------------------------
> drivers/scsi/ufs/ufshci.h | 32 ++++++++++++++++----------------
> 3 files changed, 42 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 7210500..f42d1ce 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -196,9 +196,9 @@ enum {
> * @dword_2: UPIU header DW-2
> */
> struct utp_upiu_header {
> - u32 dword_0;
> - u32 dword_1;
> - u32 dword_2;
> + __be32 dword_0;
> + __be32 dword_1;
> + __be32 dword_2;
> };
>
> /**
> @@ -207,7 +207,7 @@ struct utp_upiu_header {
> * @cdb: Command Descriptor Block CDB DW-4 to DW-7
> */
> struct utp_upiu_cmd {
> - u32 exp_data_transfer_len;
> + __be32 exp_data_transfer_len;
> u8 cdb[MAX_CDB_SIZE];
> };
>
> @@ -228,10 +228,10 @@ struct utp_upiu_query {
> u8 idn;
> u8 index;
> u8 selector;
> - u16 reserved_osf;
> - u16 length;
> - u32 value;
> - u32 reserved[2];
> + __be16 reserved_osf;
> + __be16 length;
> + __be32 value;
> + __be32 reserved[2];
> };
>
> /**
> @@ -256,9 +256,9 @@ struct utp_upiu_req {
> * @sense_data: Sense data field DW-8 to DW-12
> */
> struct utp_cmd_rsp {
> - u32 residual_transfer_count;
> - u32 reserved[4];
> - u16 sense_data_len;
> + __be32 residual_transfer_count;
> + __be32 reserved[4];
> + __be16 sense_data_len;
> u8 sense_data[18];
> };
>
> @@ -286,10 +286,10 @@ struct utp_upiu_rsp {
> */
> struct utp_upiu_task_req {
> struct utp_upiu_header header;
> - u32 input_param1;
> - u32 input_param2;
> - u32 input_param3;
> - u32 reserved[2];
> + __be32 input_param1;
> + __be32 input_param2;
> + __be32 input_param3;
> + __be32 reserved[2];
> };
>
> /**
> @@ -301,9 +301,9 @@ struct utp_upiu_task_req {
> */
> struct utp_upiu_task_rsp {
> struct utp_upiu_header header;
> - u32 output_param1;
> - u32 output_param2;
> - u32 reserved[3];
> + __be32 output_param1;
> + __be32 output_param2;
> + __be32 reserved[3];
> };
>
> /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 04884d6..064c9d9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -163,7 +163,7 @@ static inline int ufshcd_is_device_present(u32 reg_hcs)
> */
> static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
> {
> - return lrbp->utr_descriptor_ptr->header.dword_2 & MASK_OCS;
> + return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
> }
>
> /**
> @@ -176,7 +176,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
> static inline int
> ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
> {
> - return task_req_descp->header.dword_2 & MASK_OCS;
> + return le32_to_cpu(task_req_descp->header.dword_2) & MASK_OCS;
> }
>
> /**
> @@ -390,26 +390,6 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
> }
>
> /**
> - * ufshcd_query_to_cpu() - formats the buffer to native cpu endian
> - * @response: upiu query response to convert
> - */
> -static inline void ufshcd_query_to_cpu(struct utp_upiu_query *response)
> -{
> - response->length = be16_to_cpu(response->length);
> - response->value = be32_to_cpu(response->value);
> -}
> -
> -/**
> - * ufshcd_query_to_be() - formats the buffer to big endian
> - * @request: upiu query request to convert
> - */
> -static inline void ufshcd_query_to_be(struct utp_upiu_query *request)
> -{
> - request->length = cpu_to_be16(request->length);
> - request->value = cpu_to_be32(request->value);
> -}
> -
> -/**
> * ufshcd_copy_query_response() - Copy the Query Response and the data
> * descriptor
> * @hba: per adapter instance
> @@ -425,7 +405,6 @@ void ufshcd_copy_query_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> UPIU_RSP_CODE_OFFSET;
>
> memcpy(&query_res->upiu_res, &lrbp->ucd_rsp_ptr->qr, QUERY_OSF_SIZE);
> - ufshcd_query_to_cpu(&query_res->upiu_res);
>
>
> /* Get the descriptor */
> @@ -749,7 +728,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
> {
> struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
> struct ufs_query *query = &hba->dev_cmd.query;
> - u16 len = query->request.upiu_req.length;
> + u16 len = be16_to_cpu(query->request.upiu_req.length);
> u8 *descp = (u8 *)lrbp->ucd_req_ptr + GENERAL_UPIU_REQUEST_SIZE;
>
> /* Query request header */
> @@ -766,7 +745,6 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
> /* Copy the Query Request buffer as is */
> memcpy(&ucd_req_ptr->qr, &query->request.upiu_req,
> QUERY_OSF_SIZE);
> - ufshcd_query_to_be(&ucd_req_ptr->qr);
>
> /* Copy the Descriptor */
> if ((len > 0) && (query->request.upiu_req.opcode ==
> @@ -1151,7 +1129,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
> }
>
> if (flag_res)
> - *flag_res = (response->upiu_res.value &
> + *flag_res = (be32_to_cpu(response->upiu_res.value) &
> MASK_QUERY_UPIU_FLAG_LOC) & 0x1;
>
> out_unlock:
> @@ -1195,7 +1173,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> switch (opcode) {
> case UPIU_QUERY_OPCODE_WRITE_ATTR:
> request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
> - request->upiu_req.value = *attr_val;
> + request->upiu_req.value = cpu_to_be32(*attr_val);
> break;
> case UPIU_QUERY_OPCODE_READ_ATTR:
> request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
> @@ -1222,7 +1200,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> goto out_unlock;
> }
>
> - *attr_val = response->upiu_res.value;
> + *attr_val = be32_to_cpu(response->upiu_res.value);
>
> out_unlock:
> mutex_unlock(&hba->dev_cmd.lock);
> @@ -2568,12 +2546,8 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
> task_req_upiup->header.dword_1 =
> UPIU_HEADER_DWORD(0, tm_function, 0, 0);
>
> - task_req_upiup->input_param1 = lrbp->lun;
> - task_req_upiup->input_param1 =
> - cpu_to_be32(task_req_upiup->input_param1);
> - task_req_upiup->input_param2 = lrbp->task_tag;
> - task_req_upiup->input_param2 =
> - cpu_to_be32(task_req_upiup->input_param2);
> + task_req_upiup->input_param1 = cpu_to_be32(lrbp->lun);
> + task_req_upiup->input_param2 = cpu_to_be32(lrbp->task_tag);
>
> /* send command to the controller */
> __set_bit(free_slot, &hba->outstanding_tasks);
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index 0475c66..9abc7e3 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -304,10 +304,10 @@ enum {
> * @size: size of physical segment DW-3
> */
> struct ufshcd_sg_entry {
> - u32 base_addr;
> - u32 upper_addr;
> - u32 reserved;
> - u32 size;
> + __le32 base_addr;
> + __le32 upper_addr;
> + __le32 reserved;
> + __le32 size;
> };
>
> /**
> @@ -330,10 +330,10 @@ struct utp_transfer_cmd_desc {
> * @dword3: Descriptor Header DW3
> */
> struct request_desc_header {
> - u32 dword_0;
> - u32 dword_1;
> - u32 dword_2;
> - u32 dword_3;
> + __le32 dword_0;
> + __le32 dword_1;
> + __le32 dword_2;
> + __le32 dword_3;
> };
>
> /**
> @@ -352,16 +352,16 @@ struct utp_transfer_req_desc {
> struct request_desc_header header;
>
> /* DW 4-5*/
> - u32 command_desc_base_addr_lo;
> - u32 command_desc_base_addr_hi;
> + __le32 command_desc_base_addr_lo;
> + __le32 command_desc_base_addr_hi;
>
> /* DW 6 */
> - u16 response_upiu_length;
> - u16 response_upiu_offset;
> + __le16 response_upiu_length;
> + __le16 response_upiu_offset;
>
> /* DW 7 */
> - u16 prd_table_length;
> - u16 prd_table_offset;
> + __le16 prd_table_length;
> + __le16 prd_table_offset;
> };
>
> /**
> @@ -376,10 +376,10 @@ struct utp_task_req_desc {
> struct request_desc_header header;
>
> /* DW 4-11 */
> - u32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
> + __le32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
>
> /* DW 12-19 */
> - u32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
> + __le32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
> };
>
> #endif /* End of Header */
>
Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings
2013-09-23 12:14 ` vinayak holikatti
@ 2013-11-18 9:04 ` vinayak holikatti
2014-05-26 5:34 ` Sujit Reddy Thumma
0 siblings, 1 reply; 15+ messages in thread
From: vinayak holikatti @ 2013-11-18 9:04 UTC (permalink / raw)
To: James E.J. Bottomley
Cc: Santosh Y, linux-scsi, linux-arm-msm, Sujit Reddy Thumma
On Mon, Sep 23, 2013 at 5:44 PM, vinayak holikatti
<vinholikatti@gmail.com> wrote:
> On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
> <sthumma@codeaurora.org> wrote:
>>
>> Fix many warnings with incorrect endian assumptions
>> which makes the code unportable to new architectures.
>>
>> The UFS specification defines the byte order as big-endian
>> for UPIU structure and little-endian for the host controller
>> transfer/task management descriptors.
>>
>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> ---
>> drivers/scsi/ufs/ufs.h | 36 ++++++++++++++++++------------------
>> drivers/scsi/ufs/ufshcd.c | 42 ++++++++----------------------------------
>> drivers/scsi/ufs/ufshci.h | 32 ++++++++++++++++----------------
>> 3 files changed, 42 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index 7210500..f42d1ce 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -196,9 +196,9 @@ enum {
>> * @dword_2: UPIU header DW-2
>> */
>> struct utp_upiu_header {
>> - u32 dword_0;
>> - u32 dword_1;
>> - u32 dword_2;
>> + __be32 dword_0;
>> + __be32 dword_1;
>> + __be32 dword_2;
>> };
>>
>> /**
>> @@ -207,7 +207,7 @@ struct utp_upiu_header {
>> * @cdb: Command Descriptor Block CDB DW-4 to DW-7
>> */
>> struct utp_upiu_cmd {
>> - u32 exp_data_transfer_len;
>> + __be32 exp_data_transfer_len;
>> u8 cdb[MAX_CDB_SIZE];
>> };
>>
>> @@ -228,10 +228,10 @@ struct utp_upiu_query {
>> u8 idn;
>> u8 index;
>> u8 selector;
>> - u16 reserved_osf;
>> - u16 length;
>> - u32 value;
>> - u32 reserved[2];
>> + __be16 reserved_osf;
>> + __be16 length;
>> + __be32 value;
>> + __be32 reserved[2];
>> };
>>
>> /**
>> @@ -256,9 +256,9 @@ struct utp_upiu_req {
>> * @sense_data: Sense data field DW-8 to DW-12
>> */
>> struct utp_cmd_rsp {
>> - u32 residual_transfer_count;
>> - u32 reserved[4];
>> - u16 sense_data_len;
>> + __be32 residual_transfer_count;
>> + __be32 reserved[4];
>> + __be16 sense_data_len;
>> u8 sense_data[18];
>> };
>>
>> @@ -286,10 +286,10 @@ struct utp_upiu_rsp {
>> */
>> struct utp_upiu_task_req {
>> struct utp_upiu_header header;
>> - u32 input_param1;
>> - u32 input_param2;
>> - u32 input_param3;
>> - u32 reserved[2];
>> + __be32 input_param1;
>> + __be32 input_param2;
>> + __be32 input_param3;
>> + __be32 reserved[2];
>> };
>>
>> /**
>> @@ -301,9 +301,9 @@ struct utp_upiu_task_req {
>> */
>> struct utp_upiu_task_rsp {
>> struct utp_upiu_header header;
>> - u32 output_param1;
>> - u32 output_param2;
>> - u32 reserved[3];
>> + __be32 output_param1;
>> + __be32 output_param2;
>> + __be32 reserved[3];
>> };
>>
>> /**
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 04884d6..064c9d9 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -163,7 +163,7 @@ static inline int ufshcd_is_device_present(u32 reg_hcs)
>> */
>> static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
>> {
>> - return lrbp->utr_descriptor_ptr->header.dword_2 & MASK_OCS;
>> + return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
>> }
>>
>> /**
>> @@ -176,7 +176,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
>> static inline int
>> ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
>> {
>> - return task_req_descp->header.dword_2 & MASK_OCS;
>> + return le32_to_cpu(task_req_descp->header.dword_2) & MASK_OCS;
>> }
>>
>> /**
>> @@ -390,26 +390,6 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
>> }
>>
>> /**
>> - * ufshcd_query_to_cpu() - formats the buffer to native cpu endian
>> - * @response: upiu query response to convert
>> - */
>> -static inline void ufshcd_query_to_cpu(struct utp_upiu_query *response)
>> -{
>> - response->length = be16_to_cpu(response->length);
>> - response->value = be32_to_cpu(response->value);
>> -}
>> -
>> -/**
>> - * ufshcd_query_to_be() - formats the buffer to big endian
>> - * @request: upiu query request to convert
>> - */
>> -static inline void ufshcd_query_to_be(struct utp_upiu_query *request)
>> -{
>> - request->length = cpu_to_be16(request->length);
>> - request->value = cpu_to_be32(request->value);
>> -}
>> -
>> -/**
>> * ufshcd_copy_query_response() - Copy the Query Response and the data
>> * descriptor
>> * @hba: per adapter instance
>> @@ -425,7 +405,6 @@ void ufshcd_copy_query_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>> UPIU_RSP_CODE_OFFSET;
>>
>> memcpy(&query_res->upiu_res, &lrbp->ucd_rsp_ptr->qr, QUERY_OSF_SIZE);
>> - ufshcd_query_to_cpu(&query_res->upiu_res);
>>
>>
>> /* Get the descriptor */
>> @@ -749,7 +728,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
>> {
>> struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
>> struct ufs_query *query = &hba->dev_cmd.query;
>> - u16 len = query->request.upiu_req.length;
>> + u16 len = be16_to_cpu(query->request.upiu_req.length);
>> u8 *descp = (u8 *)lrbp->ucd_req_ptr + GENERAL_UPIU_REQUEST_SIZE;
>>
>> /* Query request header */
>> @@ -766,7 +745,6 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
>> /* Copy the Query Request buffer as is */
>> memcpy(&ucd_req_ptr->qr, &query->request.upiu_req,
>> QUERY_OSF_SIZE);
>> - ufshcd_query_to_be(&ucd_req_ptr->qr);
>>
>> /* Copy the Descriptor */
>> if ((len > 0) && (query->request.upiu_req.opcode ==
>> @@ -1151,7 +1129,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>> }
>>
>> if (flag_res)
>> - *flag_res = (response->upiu_res.value &
>> + *flag_res = (be32_to_cpu(response->upiu_res.value) &
>> MASK_QUERY_UPIU_FLAG_LOC) & 0x1;
>>
>> out_unlock:
>> @@ -1195,7 +1173,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
>> switch (opcode) {
>> case UPIU_QUERY_OPCODE_WRITE_ATTR:
>> request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
>> - request->upiu_req.value = *attr_val;
>> + request->upiu_req.value = cpu_to_be32(*attr_val);
>> break;
>> case UPIU_QUERY_OPCODE_READ_ATTR:
>> request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
>> @@ -1222,7 +1200,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
>> goto out_unlock;
>> }
>>
>> - *attr_val = response->upiu_res.value;
>> + *attr_val = be32_to_cpu(response->upiu_res.value);
>>
>> out_unlock:
>> mutex_unlock(&hba->dev_cmd.lock);
>> @@ -2568,12 +2546,8 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>> task_req_upiup->header.dword_1 =
>> UPIU_HEADER_DWORD(0, tm_function, 0, 0);
>>
>> - task_req_upiup->input_param1 = lrbp->lun;
>> - task_req_upiup->input_param1 =
>> - cpu_to_be32(task_req_upiup->input_param1);
>> - task_req_upiup->input_param2 = lrbp->task_tag;
>> - task_req_upiup->input_param2 =
>> - cpu_to_be32(task_req_upiup->input_param2);
>> + task_req_upiup->input_param1 = cpu_to_be32(lrbp->lun);
>> + task_req_upiup->input_param2 = cpu_to_be32(lrbp->task_tag);
>>
>> /* send command to the controller */
>> __set_bit(free_slot, &hba->outstanding_tasks);
>> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
>> index 0475c66..9abc7e3 100644
>> --- a/drivers/scsi/ufs/ufshci.h
>> +++ b/drivers/scsi/ufs/ufshci.h
>> @@ -304,10 +304,10 @@ enum {
>> * @size: size of physical segment DW-3
>> */
>> struct ufshcd_sg_entry {
>> - u32 base_addr;
>> - u32 upper_addr;
>> - u32 reserved;
>> - u32 size;
>> + __le32 base_addr;
>> + __le32 upper_addr;
>> + __le32 reserved;
>> + __le32 size;
>> };
>>
>> /**
>> @@ -330,10 +330,10 @@ struct utp_transfer_cmd_desc {
>> * @dword3: Descriptor Header DW3
>> */
>> struct request_desc_header {
>> - u32 dword_0;
>> - u32 dword_1;
>> - u32 dword_2;
>> - u32 dword_3;
>> + __le32 dword_0;
>> + __le32 dword_1;
>> + __le32 dword_2;
>> + __le32 dword_3;
>> };
>>
>> /**
>> @@ -352,16 +352,16 @@ struct utp_transfer_req_desc {
>> struct request_desc_header header;
>>
>> /* DW 4-5*/
>> - u32 command_desc_base_addr_lo;
>> - u32 command_desc_base_addr_hi;
>> + __le32 command_desc_base_addr_lo;
>> + __le32 command_desc_base_addr_hi;
>>
>> /* DW 6 */
>> - u16 response_upiu_length;
>> - u16 response_upiu_offset;
>> + __le16 response_upiu_length;
>> + __le16 response_upiu_offset;
>>
>> /* DW 7 */
>> - u16 prd_table_length;
>> - u16 prd_table_offset;
>> + __le16 prd_table_length;
>> + __le16 prd_table_offset;
>> };
>>
>> /**
>> @@ -376,10 +376,10 @@ struct utp_task_req_desc {
>> struct request_desc_header header;
>>
>> /* DW 4-11 */
>> - u32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
>> + __le32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
>>
>> /* DW 12-19 */
>> - u32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
>> + __le32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
>> };
>>
>> #endif /* End of Header */
>>
> Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
Hi James,
Please merge this series of patches to scsi 'misc' branch. Let us know
if you are expecting any changes.
--
Regards,
Vinayak Holikatti
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings
2013-11-18 9:04 ` vinayak holikatti
@ 2014-05-26 5:34 ` Sujit Reddy Thumma
0 siblings, 0 replies; 15+ messages in thread
From: Sujit Reddy Thumma @ 2014-05-26 5:34 UTC (permalink / raw)
To: vinayak holikatti
Cc: James E.J. Bottomley, Santosh Y, linux-scsi, linux-arm-msm
On 11/18/2013 2:34 PM, vinayak holikatti wrote:
> On Mon, Sep 23, 2013 at 5:44 PM, vinayak holikatti
> <vinholikatti@gmail.com> wrote:
>> On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
>> <sthumma@codeaurora.org> wrote:
>>>
>>> Fix many warnings with incorrect endian assumptions
>>> which makes the code unportable to new architectures.
>>>
>>> The UFS specification defines the byte order as big-endian
>>> for UPIU structure and little-endian for the host controller
>>> transfer/task management descriptors.
>>>
>>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>>> ---
>>> drivers/scsi/ufs/ufs.h | 36 ++++++++++++++++++------------------
>>> drivers/scsi/ufs/ufshcd.c | 42 ++++++++----------------------------------
>>> drivers/scsi/ufs/ufshci.h | 32 ++++++++++++++++----------------
>>> 3 files changed, 42 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>>> index 7210500..f42d1ce 100644
>>> --- a/drivers/scsi/ufs/ufs.h
>>> +++ b/drivers/scsi/ufs/ufs.h
>>> @@ -196,9 +196,9 @@ enum {
>>> * @dword_2: UPIU header DW-2
>>> */
>>> struct utp_upiu_header {
>>> - u32 dword_0;
>>> - u32 dword_1;
>>> - u32 dword_2;
>>> + __be32 dword_0;
>>> + __be32 dword_1;
>>> + __be32 dword_2;
>>> };
>>>
>>> /**
>>> @@ -207,7 +207,7 @@ struct utp_upiu_header {
>>> * @cdb: Command Descriptor Block CDB DW-4 to DW-7
>>> */
>>> struct utp_upiu_cmd {
>>> - u32 exp_data_transfer_len;
>>> + __be32 exp_data_transfer_len;
>>> u8 cdb[MAX_CDB_SIZE];
>>> };
>>>
>>> @@ -228,10 +228,10 @@ struct utp_upiu_query {
>>> u8 idn;
>>> u8 index;
>>> u8 selector;
>>> - u16 reserved_osf;
>>> - u16 length;
>>> - u32 value;
>>> - u32 reserved[2];
>>> + __be16 reserved_osf;
>>> + __be16 length;
>>> + __be32 value;
>>> + __be32 reserved[2];
>>> };
>>>
>>> /**
>>> @@ -256,9 +256,9 @@ struct utp_upiu_req {
>>> * @sense_data: Sense data field DW-8 to DW-12
>>> */
>>> struct utp_cmd_rsp {
>>> - u32 residual_transfer_count;
>>> - u32 reserved[4];
>>> - u16 sense_data_len;
>>> + __be32 residual_transfer_count;
>>> + __be32 reserved[4];
>>> + __be16 sense_data_len;
>>> u8 sense_data[18];
>>> };
>>>
>>> @@ -286,10 +286,10 @@ struct utp_upiu_rsp {
>>> */
>>> struct utp_upiu_task_req {
>>> struct utp_upiu_header header;
>>> - u32 input_param1;
>>> - u32 input_param2;
>>> - u32 input_param3;
>>> - u32 reserved[2];
>>> + __be32 input_param1;
>>> + __be32 input_param2;
>>> + __be32 input_param3;
>>> + __be32 reserved[2];
>>> };
>>>
>>> /**
>>> @@ -301,9 +301,9 @@ struct utp_upiu_task_req {
>>> */
>>> struct utp_upiu_task_rsp {
>>> struct utp_upiu_header header;
>>> - u32 output_param1;
>>> - u32 output_param2;
>>> - u32 reserved[3];
>>> + __be32 output_param1;
>>> + __be32 output_param2;
>>> + __be32 reserved[3];
>>> };
>>>
>>> /**
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 04884d6..064c9d9 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -163,7 +163,7 @@ static inline int ufshcd_is_device_present(u32 reg_hcs)
>>> */
>>> static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
>>> {
>>> - return lrbp->utr_descriptor_ptr->header.dword_2 & MASK_OCS;
>>> + return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
>>> }
>>>
>>> /**
>>> @@ -176,7 +176,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
>>> static inline int
>>> ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
>>> {
>>> - return task_req_descp->header.dword_2 & MASK_OCS;
>>> + return le32_to_cpu(task_req_descp->header.dword_2) & MASK_OCS;
>>> }
>>>
>>> /**
>>> @@ -390,26 +390,6 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
>>> }
>>>
>>> /**
>>> - * ufshcd_query_to_cpu() - formats the buffer to native cpu endian
>>> - * @response: upiu query response to convert
>>> - */
>>> -static inline void ufshcd_query_to_cpu(struct utp_upiu_query *response)
>>> -{
>>> - response->length = be16_to_cpu(response->length);
>>> - response->value = be32_to_cpu(response->value);
>>> -}
>>> -
>>> -/**
>>> - * ufshcd_query_to_be() - formats the buffer to big endian
>>> - * @request: upiu query request to convert
>>> - */
>>> -static inline void ufshcd_query_to_be(struct utp_upiu_query *request)
>>> -{
>>> - request->length = cpu_to_be16(request->length);
>>> - request->value = cpu_to_be32(request->value);
>>> -}
>>> -
>>> -/**
>>> * ufshcd_copy_query_response() - Copy the Query Response and the data
>>> * descriptor
>>> * @hba: per adapter instance
>>> @@ -425,7 +405,6 @@ void ufshcd_copy_query_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>>> UPIU_RSP_CODE_OFFSET;
>>>
>>> memcpy(&query_res->upiu_res, &lrbp->ucd_rsp_ptr->qr, QUERY_OSF_SIZE);
>>> - ufshcd_query_to_cpu(&query_res->upiu_res);
>>>
>>>
>>> /* Get the descriptor */
>>> @@ -749,7 +728,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
>>> {
>>> struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
>>> struct ufs_query *query = &hba->dev_cmd.query;
>>> - u16 len = query->request.upiu_req.length;
>>> + u16 len = be16_to_cpu(query->request.upiu_req.length);
>>> u8 *descp = (u8 *)lrbp->ucd_req_ptr + GENERAL_UPIU_REQUEST_SIZE;
>>>
>>> /* Query request header */
>>> @@ -766,7 +745,6 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
>>> /* Copy the Query Request buffer as is */
>>> memcpy(&ucd_req_ptr->qr, &query->request.upiu_req,
>>> QUERY_OSF_SIZE);
>>> - ufshcd_query_to_be(&ucd_req_ptr->qr);
>>>
>>> /* Copy the Descriptor */
>>> if ((len > 0) && (query->request.upiu_req.opcode ==
>>> @@ -1151,7 +1129,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>>> }
>>>
>>> if (flag_res)
>>> - *flag_res = (response->upiu_res.value &
>>> + *flag_res = (be32_to_cpu(response->upiu_res.value) &
>>> MASK_QUERY_UPIU_FLAG_LOC) & 0x1;
>>>
>>> out_unlock:
>>> @@ -1195,7 +1173,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
>>> switch (opcode) {
>>> case UPIU_QUERY_OPCODE_WRITE_ATTR:
>>> request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
>>> - request->upiu_req.value = *attr_val;
>>> + request->upiu_req.value = cpu_to_be32(*attr_val);
>>> break;
>>> case UPIU_QUERY_OPCODE_READ_ATTR:
>>> request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
>>> @@ -1222,7 +1200,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
>>> goto out_unlock;
>>> }
>>>
>>> - *attr_val = response->upiu_res.value;
>>> + *attr_val = be32_to_cpu(response->upiu_res.value);
>>>
>>> out_unlock:
>>> mutex_unlock(&hba->dev_cmd.lock);
>>> @@ -2568,12 +2546,8 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>>> task_req_upiup->header.dword_1 =
>>> UPIU_HEADER_DWORD(0, tm_function, 0, 0);
>>>
>>> - task_req_upiup->input_param1 = lrbp->lun;
>>> - task_req_upiup->input_param1 =
>>> - cpu_to_be32(task_req_upiup->input_param1);
>>> - task_req_upiup->input_param2 = lrbp->task_tag;
>>> - task_req_upiup->input_param2 =
>>> - cpu_to_be32(task_req_upiup->input_param2);
>>> + task_req_upiup->input_param1 = cpu_to_be32(lrbp->lun);
>>> + task_req_upiup->input_param2 = cpu_to_be32(lrbp->task_tag);
>>>
>>> /* send command to the controller */
>>> __set_bit(free_slot, &hba->outstanding_tasks);
>>> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
>>> index 0475c66..9abc7e3 100644
>>> --- a/drivers/scsi/ufs/ufshci.h
>>> +++ b/drivers/scsi/ufs/ufshci.h
>>> @@ -304,10 +304,10 @@ enum {
>>> * @size: size of physical segment DW-3
>>> */
>>> struct ufshcd_sg_entry {
>>> - u32 base_addr;
>>> - u32 upper_addr;
>>> - u32 reserved;
>>> - u32 size;
>>> + __le32 base_addr;
>>> + __le32 upper_addr;
>>> + __le32 reserved;
>>> + __le32 size;
>>> };
>>>
>>> /**
>>> @@ -330,10 +330,10 @@ struct utp_transfer_cmd_desc {
>>> * @dword3: Descriptor Header DW3
>>> */
>>> struct request_desc_header {
>>> - u32 dword_0;
>>> - u32 dword_1;
>>> - u32 dword_2;
>>> - u32 dword_3;
>>> + __le32 dword_0;
>>> + __le32 dword_1;
>>> + __le32 dword_2;
>>> + __le32 dword_3;
>>> };
>>>
>>> /**
>>> @@ -352,16 +352,16 @@ struct utp_transfer_req_desc {
>>> struct request_desc_header header;
>>>
>>> /* DW 4-5*/
>>> - u32 command_desc_base_addr_lo;
>>> - u32 command_desc_base_addr_hi;
>>> + __le32 command_desc_base_addr_lo;
>>> + __le32 command_desc_base_addr_hi;
>>>
>>> /* DW 6 */
>>> - u16 response_upiu_length;
>>> - u16 response_upiu_offset;
>>> + __le16 response_upiu_length;
>>> + __le16 response_upiu_offset;
>>>
>>> /* DW 7 */
>>> - u16 prd_table_length;
>>> - u16 prd_table_offset;
>>> + __le16 prd_table_length;
>>> + __le16 prd_table_offset;
>>> };
>>>
>>> /**
>>> @@ -376,10 +376,10 @@ struct utp_task_req_desc {
>>> struct request_desc_header header;
>>>
>>> /* DW 4-11 */
>>> - u32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
>>> + __le32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
>>>
>>> /* DW 12-19 */
>>> - u32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
>>> + __le32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
>>> };
>>>
>>> #endif /* End of Header */
>>>
>> Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
>
> Hi James,
>
> Please merge this series of patches to scsi 'misc' branch. Let us know
> if you are expecting any changes.
>
Hi James,
I have sent the patches again rebased on scsi-misc. Could you please
merge them?
--
Regards,
Sujit
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 2/6] scsi: ufs: make undeclared functions static
2013-09-19 11:14 [PATCH V7 0/6] scsi: ufs: TM, fatal-error handling and other fixes Sujit Reddy Thumma
2013-09-19 11:14 ` [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings Sujit Reddy Thumma
@ 2013-09-19 11:14 ` Sujit Reddy Thumma
2013-09-23 12:14 ` vinayak holikatti
2013-09-19 11:14 ` [PATCH V7 3/6] scsi: ufs: Fix broken task management command implementation Sujit Reddy Thumma
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Sujit Reddy Thumma @ 2013-09-19 11:14 UTC (permalink / raw)
To: Vinayak Holikatti, Santosh Y
Cc: James E.J. Bottomley, linux-scsi, Sujit Reddy Thumma,
linux-arm-msm
Make undeclared functions static and declare exported symbols
to suppress warnings from sparse tool.
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
---
drivers/scsi/ufs/ufshcd.c | 4 ++--
drivers/scsi/ufs/ufshcd.h | 2 ++
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 064c9d9..d476cc3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1148,7 +1148,7 @@ out_unlock:
*
* Returns 0 for success, non-zero in case of failure
*/
-int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
+static int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
enum attr_idn idn, u8 index, u8 selector, u32 *attr_val)
{
struct ufs_query_req *request;
@@ -1459,7 +1459,7 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
*
* Returns 0 on success, non-zero value on failure
*/
-int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
+static int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
{
struct uic_command uic_cmd = {0};
struct completion pwr_done;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 577679a..767ee9e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -263,6 +263,8 @@ static inline void check_upiu_size(void)
GENERAL_UPIU_REQUEST_SIZE + QUERY_DESC_MAX_SIZE);
}
+extern int ufshcd_suspend(struct ufs_hba *hba, pm_message_t state);
+extern int ufshcd_resume(struct ufs_hba *hba);
extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
extern int ufshcd_runtime_resume(struct ufs_hba *hba);
extern int ufshcd_runtime_idle(struct ufs_hba *hba);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V7 2/6] scsi: ufs: make undeclared functions static
2013-09-19 11:14 ` [PATCH V7 2/6] scsi: ufs: make undeclared functions static Sujit Reddy Thumma
@ 2013-09-23 12:14 ` vinayak holikatti
0 siblings, 0 replies; 15+ messages in thread
From: vinayak holikatti @ 2013-09-23 12:14 UTC (permalink / raw)
To: Sujit Reddy Thumma
Cc: Santosh Y, James E.J. Bottomley, linux-scsi, linux-arm-msm
On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> Make undeclared functions static and declare exported symbols
> to suppress warnings from sparse tool.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 4 ++--
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 064c9d9..d476cc3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1148,7 +1148,7 @@ out_unlock:
> *
> * Returns 0 for success, non-zero in case of failure
> */
> -int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> +static int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> enum attr_idn idn, u8 index, u8 selector, u32 *attr_val)
> {
> struct ufs_query_req *request;
> @@ -1459,7 +1459,7 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
> *
> * Returns 0 on success, non-zero value on failure
> */
> -int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
> +static int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
> {
> struct uic_command uic_cmd = {0};
> struct completion pwr_done;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 577679a..767ee9e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -263,6 +263,8 @@ static inline void check_upiu_size(void)
> GENERAL_UPIU_REQUEST_SIZE + QUERY_DESC_MAX_SIZE);
> }
>
> +extern int ufshcd_suspend(struct ufs_hba *hba, pm_message_t state);
> +extern int ufshcd_resume(struct ufs_hba *hba);
> extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
> extern int ufshcd_runtime_resume(struct ufs_hba *hba);
> extern int ufshcd_runtime_idle(struct ufs_hba *hba);
>
Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 3/6] scsi: ufs: Fix broken task management command implementation
2013-09-19 11:14 [PATCH V7 0/6] scsi: ufs: TM, fatal-error handling and other fixes Sujit Reddy Thumma
2013-09-19 11:14 ` [PATCH V7 1/6] scsi: ufs: fix endianness sparse warnings Sujit Reddy Thumma
2013-09-19 11:14 ` [PATCH V7 2/6] scsi: ufs: make undeclared functions static Sujit Reddy Thumma
@ 2013-09-19 11:14 ` Sujit Reddy Thumma
2013-09-23 12:15 ` vinayak holikatti
2013-09-19 11:14 ` [PATCH V7 4/6] scsi: ufs: Fix hardware race conditions while aborting a command Sujit Reddy Thumma
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Sujit Reddy Thumma @ 2013-09-19 11:14 UTC (permalink / raw)
To: Vinayak Holikatti, Santosh Y
Cc: James E.J. Bottomley, linux-scsi, Sujit Reddy Thumma,
linux-arm-msm
Currently, sending Task Management (TM) command to the card might
be broken in some scenarios as listed below:
Problem: If there are more than 8 TM commands the implementation
returns error to the caller.
Fix: Wait for one of the slots to be emptied and send the command.
Problem: Sometimes it is necessary for the caller to know the TM service
response code to determine the task status.
Fix: Propogate the service response to the caller.
Problem: If the TM command times out no proper error recovery is
implemented.
Fix: Clear the command in the controller door-bell register, so that
further commands for the same slot don't fail.
Problem: While preparing the TM command descriptor, the task tag used
should be unique across SCSI/NOP/QUERY/TM commands and not the
task tag of the command which the TM command is trying to manage.
Fix: Use a unique task tag instead of task tag of SCSI command.
Problem: Since the TM command involves H/W communication, abruptly ending
the request on kill interrupt signal might cause h/w malfunction.
Fix: Wait for hardware completion interrupt with TASK_UNINTERRUPTIBLE
set.
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
Tested-by: Dolev Raviv <draviv@codeaurora.org>
---
drivers/scsi/ufs/ufshcd.c | 169 +++++++++++++++++++++++++++++++---------------
drivers/scsi/ufs/ufshcd.h | 8 ++-
2 files changed, 122 insertions(+), 55 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d476cc3..c3acadc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -55,6 +55,9 @@
/* Query request timeout */
#define QUERY_REQ_TIMEOUT 30 /* msec */
+/* Task management command timeout */
+#define TM_CMD_TIMEOUT 100 /* msecs */
+
/* Expose the flag value from utp_upiu_query.value */
#define MASK_QUERY_UPIU_FLAG_LOC 0xFF
@@ -182,13 +185,35 @@ ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
/**
* ufshcd_get_tm_free_slot - get a free slot for task management request
* @hba: per adapter instance
+ * @free_slot: pointer to variable with available slot value
*
- * Returns maximum number of task management request slots in case of
- * task management queue full or returns the free slot number
+ * Get a free tag and lock it until ufshcd_put_tm_slot() is called.
+ * Returns 0 if free slot is not available, else return 1 with tag value
+ * in @free_slot.
*/
-static inline int ufshcd_get_tm_free_slot(struct ufs_hba *hba)
+static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot)
{
- return find_first_zero_bit(&hba->outstanding_tasks, hba->nutmrs);
+ int tag;
+ bool ret = false;
+
+ if (!free_slot)
+ goto out;
+
+ do {
+ tag = find_first_zero_bit(&hba->tm_slots_in_use, hba->nutmrs);
+ if (tag >= hba->nutmrs)
+ goto out;
+ } while (test_and_set_bit_lock(tag, &hba->tm_slots_in_use));
+
+ *free_slot = tag;
+ ret = true;
+out:
+ return ret;
+}
+
+static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
+{
+ clear_bit_unlock(slot, &hba->tm_slots_in_use);
}
/**
@@ -1912,10 +1937,11 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
* ufshcd_task_req_compl - handle task management request completion
* @hba: per adapter instance
* @index: index of the completed request
+ * @resp: task management service response
*
- * Returns SUCCESS/FAILED
+ * Returns non-zero value on error, zero on success
*/
-static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index)
+static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp)
{
struct utp_task_req_desc *task_req_descp;
struct utp_upiu_task_rsp *task_rsp_upiup;
@@ -1936,19 +1962,15 @@ static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index)
task_req_descp[index].task_rsp_upiu;
task_result = be32_to_cpu(task_rsp_upiup->header.dword_1);
task_result = ((task_result & MASK_TASK_RESPONSE) >> 8);
-
- if (task_result != UPIU_TASK_MANAGEMENT_FUNC_COMPL &&
- task_result != UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED)
- task_result = FAILED;
- else
- task_result = SUCCESS;
+ if (resp)
+ *resp = (u8)task_result;
} else {
- task_result = FAILED;
- dev_err(hba->dev,
- "trc: Invalid ocs = %x\n", ocs_value);
+ dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
+ __func__, ocs_value);
}
spin_unlock_irqrestore(hba->host->host_lock, flags);
- return task_result;
+
+ return ocs_value;
}
/**
@@ -2447,7 +2469,7 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba)
tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
- wake_up_interruptible(&hba->ufshcd_tm_wait_queue);
+ wake_up(&hba->tm_wq);
}
/**
@@ -2497,38 +2519,58 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
return retval;
}
+static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
+{
+ int err = 0;
+ u32 mask = 1 << tag;
+ unsigned long flags;
+
+ if (!test_bit(tag, &hba->outstanding_tasks))
+ goto out;
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ ufshcd_writel(hba, ~(1 << tag), REG_UTP_TASK_REQ_LIST_CLEAR);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ /* poll for max. 1 sec to clear door bell register by h/w */
+ err = ufshcd_wait_for_register(hba,
+ REG_UTP_TASK_REQ_DOOR_BELL,
+ mask, 0, 1000, 1000);
+out:
+ return err;
+}
+
/**
* ufshcd_issue_tm_cmd - issues task management commands to controller
* @hba: per adapter instance
- * @lrbp: pointer to local reference block
+ * @lun_id: LUN ID to which TM command is sent
+ * @task_id: task ID to which the TM command is applicable
+ * @tm_function: task management function opcode
+ * @tm_response: task management service response return value
*
- * Returns SUCCESS/FAILED
+ * Returns non-zero value on error, zero on success.
*/
-static int
-ufshcd_issue_tm_cmd(struct ufs_hba *hba,
- struct ufshcd_lrb *lrbp,
- u8 tm_function)
+static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
+ u8 tm_function, u8 *tm_response)
{
struct utp_task_req_desc *task_req_descp;
struct utp_upiu_task_req *task_req_upiup;
struct Scsi_Host *host;
unsigned long flags;
- int free_slot = 0;
+ int free_slot;
int err;
+ int task_tag;
host = hba->host;
- spin_lock_irqsave(host->host_lock, flags);
-
- /* If task management queue is full */
- free_slot = ufshcd_get_tm_free_slot(hba);
- if (free_slot >= hba->nutmrs) {
- spin_unlock_irqrestore(host->host_lock, flags);
- dev_err(hba->dev, "Task management queue full\n");
- err = FAILED;
- goto out;
- }
+ /*
+ * Get free slot, sleep if slots are unavailable.
+ * Even though we use wait_event() which sleeps indefinitely,
+ * the maximum wait time is bounded by %TM_CMD_TIMEOUT.
+ */
+ wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot));
+ spin_lock_irqsave(host->host_lock, flags);
task_req_descp = hba->utmrdl_base_addr;
task_req_descp += free_slot;
@@ -2540,14 +2582,15 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
/* Configure task request UPIU */
task_req_upiup =
(struct utp_upiu_task_req *) task_req_descp->task_req_upiu;
+ task_tag = hba->nutrs + free_slot;
task_req_upiup->header.dword_0 =
UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
- lrbp->lun, lrbp->task_tag);
+ lun_id, task_tag);
task_req_upiup->header.dword_1 =
UPIU_HEADER_DWORD(0, tm_function, 0, 0);
- task_req_upiup->input_param1 = cpu_to_be32(lrbp->lun);
- task_req_upiup->input_param2 = cpu_to_be32(lrbp->task_tag);
+ task_req_upiup->input_param1 = cpu_to_be32(lun_id);
+ task_req_upiup->input_param2 = cpu_to_be32(task_id);
/* send command to the controller */
__set_bit(free_slot, &hba->outstanding_tasks);
@@ -2556,20 +2599,24 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
spin_unlock_irqrestore(host->host_lock, flags);
/* wait until the task management command is completed */
- err =
- wait_event_interruptible_timeout(hba->ufshcd_tm_wait_queue,
- (test_bit(free_slot,
- &hba->tm_condition) != 0),
- 60 * HZ);
+ err = wait_event_timeout(hba->tm_wq,
+ test_bit(free_slot, &hba->tm_condition),
+ msecs_to_jiffies(TM_CMD_TIMEOUT));
if (!err) {
- dev_err(hba->dev,
- "Task management command timed-out\n");
- err = FAILED;
- goto out;
+ dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
+ __func__, tm_function);
+ if (ufshcd_clear_tm_cmd(hba, free_slot))
+ dev_WARN(hba->dev, "%s: unable clear tm cmd (slot %d) after timeout\n",
+ __func__, free_slot);
+ err = -ETIMEDOUT;
+ } else {
+ err = ufshcd_task_req_compl(hba, free_slot, tm_response);
}
+
clear_bit(free_slot, &hba->tm_condition);
- err = ufshcd_task_req_compl(hba, free_slot);
-out:
+ ufshcd_put_tm_slot(hba, free_slot);
+ wake_up(&hba->tm_tag_wq);
+
return err;
}
@@ -2586,14 +2633,21 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
unsigned int tag;
u32 pos;
int err;
+ u8 resp = 0xF;
+ struct ufshcd_lrb *lrbp;
host = cmd->device->host;
hba = shost_priv(host);
tag = cmd->request->tag;
- err = ufshcd_issue_tm_cmd(hba, &hba->lrb[tag], UFS_LOGICAL_RESET);
- if (err == FAILED)
+ lrbp = &hba->lrb[tag];
+ err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp);
+ if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
+ err = FAILED;
goto out;
+ } else {
+ err = SUCCESS;
+ }
for (pos = 0; pos < hba->nutrs; pos++) {
if (test_bit(pos, &hba->outstanding_reqs) &&
@@ -2650,6 +2704,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
unsigned long flags;
unsigned int tag;
int err;
+ u8 resp = 0xF;
+ struct ufshcd_lrb *lrbp;
host = cmd->device->host;
hba = shost_priv(host);
@@ -2665,9 +2721,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
}
spin_unlock_irqrestore(host->host_lock, flags);
- err = ufshcd_issue_tm_cmd(hba, &hba->lrb[tag], UFS_ABORT_TASK);
- if (err == FAILED)
+ lrbp = &hba->lrb[tag];
+ err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
+ UFS_ABORT_TASK, &resp);
+ if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
+ err = FAILED;
goto out;
+ } else {
+ err = SUCCESS;
+ }
scsi_dma_unmap(cmd);
@@ -2890,7 +2952,8 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
host->max_cmd_len = MAX_CDB_SIZE;
/* Initailize wait queue for task management */
- init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
+ init_waitqueue_head(&hba->tm_wq);
+ init_waitqueue_head(&hba->tm_tag_wq);
/* Initialize work queues */
INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 767ee9e..84d09d1 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -174,7 +174,9 @@ struct ufs_dev_cmd {
* @irq: Irq number of the controller
* @active_uic_cmd: handle of active UIC command
* @uic_cmd_mutex: mutex for uic command
- * @ufshcd_tm_wait_queue: wait queue for task management
+ * @tm_wq: wait queue for task management
+ * @tm_tag_wq: wait queue for free task management slots
+ * @tm_slots_in_use: bit map of task management request slots in use
* @pwr_done: completion for power mode change
* @tm_condition: condition variable for task management
* @ufshcd_state: UFSHCD states
@@ -217,8 +219,10 @@ struct ufs_hba {
struct uic_command *active_uic_cmd;
struct mutex uic_cmd_mutex;
- wait_queue_head_t ufshcd_tm_wait_queue;
+ wait_queue_head_t tm_wq;
+ wait_queue_head_t tm_tag_wq;
unsigned long tm_condition;
+ unsigned long tm_slots_in_use;
struct completion *pwr_done;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V7 3/6] scsi: ufs: Fix broken task management command implementation
2013-09-19 11:14 ` [PATCH V7 3/6] scsi: ufs: Fix broken task management command implementation Sujit Reddy Thumma
@ 2013-09-23 12:15 ` vinayak holikatti
0 siblings, 0 replies; 15+ messages in thread
From: vinayak holikatti @ 2013-09-23 12:15 UTC (permalink / raw)
To: Sujit Reddy Thumma
Cc: Santosh Y, James E.J. Bottomley, linux-scsi, linux-arm-msm
On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> Currently, sending Task Management (TM) command to the card might
> be broken in some scenarios as listed below:
>
> Problem: If there are more than 8 TM commands the implementation
> returns error to the caller.
> Fix: Wait for one of the slots to be emptied and send the command.
>
> Problem: Sometimes it is necessary for the caller to know the TM service
> response code to determine the task status.
> Fix: Propogate the service response to the caller.
>
> Problem: If the TM command times out no proper error recovery is
> implemented.
> Fix: Clear the command in the controller door-bell register, so that
> further commands for the same slot don't fail.
>
> Problem: While preparing the TM command descriptor, the task tag used
> should be unique across SCSI/NOP/QUERY/TM commands and not the
> task tag of the command which the TM command is trying to manage.
> Fix: Use a unique task tag instead of task tag of SCSI command.
>
> Problem: Since the TM command involves H/W communication, abruptly ending
> the request on kill interrupt signal might cause h/w malfunction.
> Fix: Wait for hardware completion interrupt with TASK_UNINTERRUPTIBLE
> set.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
> Tested-by: Dolev Raviv <draviv@codeaurora.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 169 +++++++++++++++++++++++++++++++---------------
> drivers/scsi/ufs/ufshcd.h | 8 ++-
> 2 files changed, 122 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d476cc3..c3acadc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -55,6 +55,9 @@
> /* Query request timeout */
> #define QUERY_REQ_TIMEOUT 30 /* msec */
>
> +/* Task management command timeout */
> +#define TM_CMD_TIMEOUT 100 /* msecs */
> +
> /* Expose the flag value from utp_upiu_query.value */
> #define MASK_QUERY_UPIU_FLAG_LOC 0xFF
>
> @@ -182,13 +185,35 @@ ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
> /**
> * ufshcd_get_tm_free_slot - get a free slot for task management request
> * @hba: per adapter instance
> + * @free_slot: pointer to variable with available slot value
> *
> - * Returns maximum number of task management request slots in case of
> - * task management queue full or returns the free slot number
> + * Get a free tag and lock it until ufshcd_put_tm_slot() is called.
> + * Returns 0 if free slot is not available, else return 1 with tag value
> + * in @free_slot.
> */
> -static inline int ufshcd_get_tm_free_slot(struct ufs_hba *hba)
> +static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot)
> {
> - return find_first_zero_bit(&hba->outstanding_tasks, hba->nutmrs);
> + int tag;
> + bool ret = false;
> +
> + if (!free_slot)
> + goto out;
> +
> + do {
> + tag = find_first_zero_bit(&hba->tm_slots_in_use, hba->nutmrs);
> + if (tag >= hba->nutmrs)
> + goto out;
> + } while (test_and_set_bit_lock(tag, &hba->tm_slots_in_use));
> +
> + *free_slot = tag;
> + ret = true;
> +out:
> + return ret;
> +}
> +
> +static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
> +{
> + clear_bit_unlock(slot, &hba->tm_slots_in_use);
> }
>
> /**
> @@ -1912,10 +1937,11 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> * ufshcd_task_req_compl - handle task management request completion
> * @hba: per adapter instance
> * @index: index of the completed request
> + * @resp: task management service response
> *
> - * Returns SUCCESS/FAILED
> + * Returns non-zero value on error, zero on success
> */
> -static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index)
> +static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp)
> {
> struct utp_task_req_desc *task_req_descp;
> struct utp_upiu_task_rsp *task_rsp_upiup;
> @@ -1936,19 +1962,15 @@ static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index)
> task_req_descp[index].task_rsp_upiu;
> task_result = be32_to_cpu(task_rsp_upiup->header.dword_1);
> task_result = ((task_result & MASK_TASK_RESPONSE) >> 8);
> -
> - if (task_result != UPIU_TASK_MANAGEMENT_FUNC_COMPL &&
> - task_result != UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED)
> - task_result = FAILED;
> - else
> - task_result = SUCCESS;
> + if (resp)
> + *resp = (u8)task_result;
> } else {
> - task_result = FAILED;
> - dev_err(hba->dev,
> - "trc: Invalid ocs = %x\n", ocs_value);
> + dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
> + __func__, ocs_value);
> }
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> - return task_result;
> +
> + return ocs_value;
> }
>
> /**
> @@ -2447,7 +2469,7 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba)
>
> tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
> - wake_up_interruptible(&hba->ufshcd_tm_wait_queue);
> + wake_up(&hba->tm_wq);
> }
>
> /**
> @@ -2497,38 +2519,58 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
> return retval;
> }
>
> +static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
> +{
> + int err = 0;
> + u32 mask = 1 << tag;
> + unsigned long flags;
> +
> + if (!test_bit(tag, &hba->outstanding_tasks))
> + goto out;
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + ufshcd_writel(hba, ~(1 << tag), REG_UTP_TASK_REQ_LIST_CLEAR);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + /* poll for max. 1 sec to clear door bell register by h/w */
> + err = ufshcd_wait_for_register(hba,
> + REG_UTP_TASK_REQ_DOOR_BELL,
> + mask, 0, 1000, 1000);
> +out:
> + return err;
> +}
> +
> /**
> * ufshcd_issue_tm_cmd - issues task management commands to controller
> * @hba: per adapter instance
> - * @lrbp: pointer to local reference block
> + * @lun_id: LUN ID to which TM command is sent
> + * @task_id: task ID to which the TM command is applicable
> + * @tm_function: task management function opcode
> + * @tm_response: task management service response return value
> *
> - * Returns SUCCESS/FAILED
> + * Returns non-zero value on error, zero on success.
> */
> -static int
> -ufshcd_issue_tm_cmd(struct ufs_hba *hba,
> - struct ufshcd_lrb *lrbp,
> - u8 tm_function)
> +static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
> + u8 tm_function, u8 *tm_response)
> {
> struct utp_task_req_desc *task_req_descp;
> struct utp_upiu_task_req *task_req_upiup;
> struct Scsi_Host *host;
> unsigned long flags;
> - int free_slot = 0;
> + int free_slot;
> int err;
> + int task_tag;
>
> host = hba->host;
>
> - spin_lock_irqsave(host->host_lock, flags);
> -
> - /* If task management queue is full */
> - free_slot = ufshcd_get_tm_free_slot(hba);
> - if (free_slot >= hba->nutmrs) {
> - spin_unlock_irqrestore(host->host_lock, flags);
> - dev_err(hba->dev, "Task management queue full\n");
> - err = FAILED;
> - goto out;
> - }
> + /*
> + * Get free slot, sleep if slots are unavailable.
> + * Even though we use wait_event() which sleeps indefinitely,
> + * the maximum wait time is bounded by %TM_CMD_TIMEOUT.
> + */
> + wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot));
>
> + spin_lock_irqsave(host->host_lock, flags);
> task_req_descp = hba->utmrdl_base_addr;
> task_req_descp += free_slot;
>
> @@ -2540,14 +2582,15 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
> /* Configure task request UPIU */
> task_req_upiup =
> (struct utp_upiu_task_req *) task_req_descp->task_req_upiu;
> + task_tag = hba->nutrs + free_slot;
> task_req_upiup->header.dword_0 =
> UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
> - lrbp->lun, lrbp->task_tag);
> + lun_id, task_tag);
> task_req_upiup->header.dword_1 =
> UPIU_HEADER_DWORD(0, tm_function, 0, 0);
>
> - task_req_upiup->input_param1 = cpu_to_be32(lrbp->lun);
> - task_req_upiup->input_param2 = cpu_to_be32(lrbp->task_tag);
> + task_req_upiup->input_param1 = cpu_to_be32(lun_id);
> + task_req_upiup->input_param2 = cpu_to_be32(task_id);
>
> /* send command to the controller */
> __set_bit(free_slot, &hba->outstanding_tasks);
> @@ -2556,20 +2599,24 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
> spin_unlock_irqrestore(host->host_lock, flags);
>
> /* wait until the task management command is completed */
> - err =
> - wait_event_interruptible_timeout(hba->ufshcd_tm_wait_queue,
> - (test_bit(free_slot,
> - &hba->tm_condition) != 0),
> - 60 * HZ);
> + err = wait_event_timeout(hba->tm_wq,
> + test_bit(free_slot, &hba->tm_condition),
> + msecs_to_jiffies(TM_CMD_TIMEOUT));
> if (!err) {
> - dev_err(hba->dev,
> - "Task management command timed-out\n");
> - err = FAILED;
> - goto out;
> + dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
> + __func__, tm_function);
> + if (ufshcd_clear_tm_cmd(hba, free_slot))
> + dev_WARN(hba->dev, "%s: unable clear tm cmd (slot %d) after timeout\n",
> + __func__, free_slot);
> + err = -ETIMEDOUT;
> + } else {
> + err = ufshcd_task_req_compl(hba, free_slot, tm_response);
> }
> +
> clear_bit(free_slot, &hba->tm_condition);
> - err = ufshcd_task_req_compl(hba, free_slot);
> -out:
> + ufshcd_put_tm_slot(hba, free_slot);
> + wake_up(&hba->tm_tag_wq);
> +
> return err;
> }
>
> @@ -2586,14 +2633,21 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
> unsigned int tag;
> u32 pos;
> int err;
> + u8 resp = 0xF;
> + struct ufshcd_lrb *lrbp;
>
> host = cmd->device->host;
> hba = shost_priv(host);
> tag = cmd->request->tag;
>
> - err = ufshcd_issue_tm_cmd(hba, &hba->lrb[tag], UFS_LOGICAL_RESET);
> - if (err == FAILED)
> + lrbp = &hba->lrb[tag];
> + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp);
> + if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> + err = FAILED;
> goto out;
> + } else {
> + err = SUCCESS;
> + }
>
> for (pos = 0; pos < hba->nutrs; pos++) {
> if (test_bit(pos, &hba->outstanding_reqs) &&
> @@ -2650,6 +2704,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> unsigned long flags;
> unsigned int tag;
> int err;
> + u8 resp = 0xF;
> + struct ufshcd_lrb *lrbp;
>
> host = cmd->device->host;
> hba = shost_priv(host);
> @@ -2665,9 +2721,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> }
> spin_unlock_irqrestore(host->host_lock, flags);
>
> - err = ufshcd_issue_tm_cmd(hba, &hba->lrb[tag], UFS_ABORT_TASK);
> - if (err == FAILED)
> + lrbp = &hba->lrb[tag];
> + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
> + UFS_ABORT_TASK, &resp);
> + if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> + err = FAILED;
> goto out;
> + } else {
> + err = SUCCESS;
> + }
>
> scsi_dma_unmap(cmd);
>
> @@ -2890,7 +2952,8 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> host->max_cmd_len = MAX_CDB_SIZE;
>
> /* Initailize wait queue for task management */
> - init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
> + init_waitqueue_head(&hba->tm_wq);
> + init_waitqueue_head(&hba->tm_tag_wq);
>
> /* Initialize work queues */
> INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 767ee9e..84d09d1 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -174,7 +174,9 @@ struct ufs_dev_cmd {
> * @irq: Irq number of the controller
> * @active_uic_cmd: handle of active UIC command
> * @uic_cmd_mutex: mutex for uic command
> - * @ufshcd_tm_wait_queue: wait queue for task management
> + * @tm_wq: wait queue for task management
> + * @tm_tag_wq: wait queue for free task management slots
> + * @tm_slots_in_use: bit map of task management request slots in use
> * @pwr_done: completion for power mode change
> * @tm_condition: condition variable for task management
> * @ufshcd_state: UFSHCD states
> @@ -217,8 +219,10 @@ struct ufs_hba {
> struct uic_command *active_uic_cmd;
> struct mutex uic_cmd_mutex;
>
> - wait_queue_head_t ufshcd_tm_wait_queue;
> + wait_queue_head_t tm_wq;
> + wait_queue_head_t tm_tag_wq;
> unsigned long tm_condition;
> + unsigned long tm_slots_in_use;
>
> struct completion *pwr_done;
>
Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 4/6] scsi: ufs: Fix hardware race conditions while aborting a command
2013-09-19 11:14 [PATCH V7 0/6] scsi: ufs: TM, fatal-error handling and other fixes Sujit Reddy Thumma
` (2 preceding siblings ...)
2013-09-19 11:14 ` [PATCH V7 3/6] scsi: ufs: Fix broken task management command implementation Sujit Reddy Thumma
@ 2013-09-19 11:14 ` Sujit Reddy Thumma
2013-09-23 12:16 ` vinayak holikatti
2013-09-19 11:14 ` [PATCH V7 5/6] scsi: ufs: Fix device and host reset methods Sujit Reddy Thumma
2013-09-19 11:14 ` [PATCH V7 6/6] scsi: ufs: Improve UFS fatal error handling Sujit Reddy Thumma
5 siblings, 1 reply; 15+ messages in thread
From: Sujit Reddy Thumma @ 2013-09-19 11:14 UTC (permalink / raw)
To: Vinayak Holikatti, Santosh Y
Cc: James E.J. Bottomley, linux-scsi, Sujit Reddy Thumma,
linux-arm-msm
There is a possible race condition in the hardware when the abort
command is issued to terminate the ongoing SCSI command as described
below:
- A bit in the door-bell register is set in the controller for a
new SCSI command.
- In some rare situations, before controller get a chance to issue
the command to the device, the software issued an abort command.
- If the device recieves abort command first then it returns success
because the command itself is not present.
- Now if the controller commits the command to device it will be
processed.
- Software thinks that command is aborted and proceed while still
the device is processing it.
- The software, controller and device may go out of sync because of
this race condition.
To avoid this, query task presence in the device before sending abort
task command so that after the abort operation, the command is guaranteed
to be non-existent in both controller and the device.
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
Tested-by: Dolev Raviv <draviv@codeaurora.org>
---
drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c3acadc..52f66e4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2695,6 +2695,12 @@ static int ufshcd_host_reset(struct scsi_cmnd *cmd)
* ufshcd_abort - abort a specific command
* @cmd: SCSI command pointer
*
+ * Abort the pending command in device by sending UFS_ABORT_TASK task management
+ * command, and in host controller by clearing the door-bell register. There can
+ * be race between controller sending the command to the device while abort is
+ * issued. To avoid that, first issue UFS_QUERY_TASK to check if the command is
+ * really issued and then try to abort it.
+ *
* Returns SUCCESS/FAILED
*/
static int ufshcd_abort(struct scsi_cmnd *cmd)
@@ -2703,7 +2709,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
struct ufs_hba *hba;
unsigned long flags;
unsigned int tag;
- int err;
+ int err = 0;
+ int poll_cnt;
u8 resp = 0xF;
struct ufshcd_lrb *lrbp;
@@ -2711,33 +2718,59 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
hba = shost_priv(host);
tag = cmd->request->tag;
- spin_lock_irqsave(host->host_lock, flags);
+ /* If command is already aborted/completed, return SUCCESS */
+ if (!(test_bit(tag, &hba->outstanding_reqs)))
+ goto out;
- /* check if command is still pending */
- if (!(test_bit(tag, &hba->outstanding_reqs))) {
- err = FAILED;
- spin_unlock_irqrestore(host->host_lock, flags);
+ lrbp = &hba->lrb[tag];
+ for (poll_cnt = 100; poll_cnt; poll_cnt--) {
+ err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
+ UFS_QUERY_TASK, &resp);
+ if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) {
+ /* cmd pending in the device */
+ break;
+ } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
+ u32 reg;
+
+ /*
+ * cmd not pending in the device, check if it is
+ * in transition.
+ */
+ reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+ if (reg & (1 << tag)) {
+ /* sleep for max. 200us to stabilize */
+ usleep_range(100, 200);
+ continue;
+ }
+ /* command completed already */
+ goto out;
+ } else {
+ if (!err)
+ err = resp; /* service response error */
+ goto out;
+ }
+ }
+
+ if (!poll_cnt) {
+ err = -EBUSY;
goto out;
}
- spin_unlock_irqrestore(host->host_lock, flags);
- lrbp = &hba->lrb[tag];
err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
UFS_ABORT_TASK, &resp);
if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
- err = FAILED;
+ if (!err)
+ err = resp; /* service response error */
goto out;
- } else {
- err = SUCCESS;
}
+ err = ufshcd_clear_cmd(hba, tag);
+ if (err)
+ goto out;
+
scsi_dma_unmap(cmd);
spin_lock_irqsave(host->host_lock, flags);
-
- /* clear the respective UTRLCLR register bit */
- ufshcd_utrl_clear(hba, tag);
-
__clear_bit(tag, &hba->outstanding_reqs);
hba->lrb[tag].cmd = NULL;
spin_unlock_irqrestore(host->host_lock, flags);
@@ -2745,6 +2778,13 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
clear_bit_unlock(tag, &hba->lrb_in_use);
wake_up(&hba->dev_cmd.tag_wq);
out:
+ if (!err) {
+ err = SUCCESS;
+ } else {
+ dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
+ err = FAILED;
+ }
+
return err;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V7 4/6] scsi: ufs: Fix hardware race conditions while aborting a command
2013-09-19 11:14 ` [PATCH V7 4/6] scsi: ufs: Fix hardware race conditions while aborting a command Sujit Reddy Thumma
@ 2013-09-23 12:16 ` vinayak holikatti
0 siblings, 0 replies; 15+ messages in thread
From: vinayak holikatti @ 2013-09-23 12:16 UTC (permalink / raw)
To: Sujit Reddy Thumma
Cc: Santosh Y, James E.J. Bottomley, linux-scsi, linux-arm-msm
On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> There is a possible race condition in the hardware when the abort
> command is issued to terminate the ongoing SCSI command as described
> below:
>
> - A bit in the door-bell register is set in the controller for a
> new SCSI command.
> - In some rare situations, before controller get a chance to issue
> the command to the device, the software issued an abort command.
> - If the device recieves abort command first then it returns success
> because the command itself is not present.
> - Now if the controller commits the command to device it will be
> processed.
> - Software thinks that command is aborted and proceed while still
> the device is processing it.
> - The software, controller and device may go out of sync because of
> this race condition.
>
> To avoid this, query task presence in the device before sending abort
> task command so that after the abort operation, the command is guaranteed
> to be non-existent in both controller and the device.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
> Tested-by: Dolev Raviv <draviv@codeaurora.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c3acadc..52f66e4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2695,6 +2695,12 @@ static int ufshcd_host_reset(struct scsi_cmnd *cmd)
> * ufshcd_abort - abort a specific command
> * @cmd: SCSI command pointer
> *
> + * Abort the pending command in device by sending UFS_ABORT_TASK task management
> + * command, and in host controller by clearing the door-bell register. There can
> + * be race between controller sending the command to the device while abort is
> + * issued. To avoid that, first issue UFS_QUERY_TASK to check if the command is
> + * really issued and then try to abort it.
> + *
> * Returns SUCCESS/FAILED
> */
> static int ufshcd_abort(struct scsi_cmnd *cmd)
> @@ -2703,7 +2709,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> struct ufs_hba *hba;
> unsigned long flags;
> unsigned int tag;
> - int err;
> + int err = 0;
> + int poll_cnt;
> u8 resp = 0xF;
> struct ufshcd_lrb *lrbp;
>
> @@ -2711,33 +2718,59 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> hba = shost_priv(host);
> tag = cmd->request->tag;
>
> - spin_lock_irqsave(host->host_lock, flags);
> + /* If command is already aborted/completed, return SUCCESS */
> + if (!(test_bit(tag, &hba->outstanding_reqs)))
> + goto out;
>
> - /* check if command is still pending */
> - if (!(test_bit(tag, &hba->outstanding_reqs))) {
> - err = FAILED;
> - spin_unlock_irqrestore(host->host_lock, flags);
> + lrbp = &hba->lrb[tag];
> + for (poll_cnt = 100; poll_cnt; poll_cnt--) {
> + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
> + UFS_QUERY_TASK, &resp);
> + if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) {
> + /* cmd pending in the device */
> + break;
> + } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> + u32 reg;
> +
> + /*
> + * cmd not pending in the device, check if it is
> + * in transition.
> + */
> + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> + if (reg & (1 << tag)) {
> + /* sleep for max. 200us to stabilize */
> + usleep_range(100, 200);
> + continue;
> + }
> + /* command completed already */
> + goto out;
> + } else {
> + if (!err)
> + err = resp; /* service response error */
> + goto out;
> + }
> + }
> +
> + if (!poll_cnt) {
> + err = -EBUSY;
> goto out;
> }
> - spin_unlock_irqrestore(host->host_lock, flags);
>
> - lrbp = &hba->lrb[tag];
> err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
> UFS_ABORT_TASK, &resp);
> if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> - err = FAILED;
> + if (!err)
> + err = resp; /* service response error */
> goto out;
> - } else {
> - err = SUCCESS;
> }
>
> + err = ufshcd_clear_cmd(hba, tag);
> + if (err)
> + goto out;
> +
> scsi_dma_unmap(cmd);
>
> spin_lock_irqsave(host->host_lock, flags);
> -
> - /* clear the respective UTRLCLR register bit */
> - ufshcd_utrl_clear(hba, tag);
> -
> __clear_bit(tag, &hba->outstanding_reqs);
> hba->lrb[tag].cmd = NULL;
> spin_unlock_irqrestore(host->host_lock, flags);
> @@ -2745,6 +2778,13 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> clear_bit_unlock(tag, &hba->lrb_in_use);
> wake_up(&hba->dev_cmd.tag_wq);
> out:
> + if (!err) {
> + err = SUCCESS;
> + } else {
> + dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
> + err = FAILED;
> + }
> +
> return err;
> }
>
Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 5/6] scsi: ufs: Fix device and host reset methods
2013-09-19 11:14 [PATCH V7 0/6] scsi: ufs: TM, fatal-error handling and other fixes Sujit Reddy Thumma
` (3 preceding siblings ...)
2013-09-19 11:14 ` [PATCH V7 4/6] scsi: ufs: Fix hardware race conditions while aborting a command Sujit Reddy Thumma
@ 2013-09-19 11:14 ` Sujit Reddy Thumma
2013-09-23 12:17 ` vinayak holikatti
2013-09-19 11:14 ` [PATCH V7 6/6] scsi: ufs: Improve UFS fatal error handling Sujit Reddy Thumma
5 siblings, 1 reply; 15+ messages in thread
From: Sujit Reddy Thumma @ 2013-09-19 11:14 UTC (permalink / raw)
To: Vinayak Holikatti, Santosh Y
Cc: James E.J. Bottomley, linux-scsi, Sujit Reddy Thumma,
linux-arm-msm
As of now SCSI initiated error handling is broken because,
the reset APIs don't try to bring back the device initialized and
ready for further transfers.
In case of timeouts, the scsi error handler takes care of handling aborts
and resets. Improve the error handling in such scenario by resetting the
device and host and re-initializing them in proper manner.
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
Tested-by: Dolev Raviv <draviv@codeaurora.org>
---
drivers/scsi/ufs/ufshcd.c | 240 ++++++++++++++++++++++++++++++++++++----------
drivers/scsi/ufs/ufshcd.h | 2 +
2 files changed, 189 insertions(+), 53 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 52f66e4..5462310 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -74,9 +74,14 @@ enum {
/* UFSHCD states */
enum {
- UFSHCD_STATE_OPERATIONAL,
UFSHCD_STATE_RESET,
UFSHCD_STATE_ERROR,
+ UFSHCD_STATE_OPERATIONAL,
+};
+
+/* UFSHCD error handling flags */
+enum {
+ UFSHCD_EH_IN_PROGRESS = (1 << 0),
};
/* Interrupt configuration options */
@@ -86,6 +91,16 @@ enum {
UFSHCD_INT_CLEAR,
};
+#define ufshcd_set_eh_in_progress(h) \
+ (h->eh_flags |= UFSHCD_EH_IN_PROGRESS)
+#define ufshcd_eh_in_progress(h) \
+ (h->eh_flags & UFSHCD_EH_IN_PROGRESS)
+#define ufshcd_clear_eh_in_progress(h) \
+ (h->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
+
+static void ufshcd_tmc_handler(struct ufs_hba *hba);
+static void ufshcd_async_scan(void *data, async_cookie_t cookie);
+
/*
* ufshcd_wait_for_register - wait for register value to change
* @hba - per-adapter interface
@@ -856,10 +871,25 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
tag = cmd->request->tag;
- if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ switch (hba->ufshcd_state) {
+ case UFSHCD_STATE_OPERATIONAL:
+ break;
+ case UFSHCD_STATE_RESET:
err = SCSI_MLQUEUE_HOST_BUSY;
- goto out;
+ goto out_unlock;
+ case UFSHCD_STATE_ERROR:
+ set_host_byte(cmd, DID_ERROR);
+ cmd->scsi_done(cmd);
+ goto out_unlock;
+ default:
+ dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
+ __func__, hba->ufshcd_state);
+ set_host_byte(cmd, DID_BAD_TARGET);
+ 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 */
if (test_and_set_bit_lock(tag, &hba->lrb_in_use)) {
@@ -896,6 +926,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
/* issue command to the controller */
spin_lock_irqsave(hba->host->host_lock, flags);
ufshcd_send_command(hba, tag);
+out_unlock:
spin_unlock_irqrestore(hba->host->host_lock, flags);
out:
return err;
@@ -1707,8 +1738,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
if (hba->ufshcd_state == UFSHCD_STATE_RESET)
scsi_unblock_requests(hba->host);
- hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
-
out:
return err;
}
@@ -2455,8 +2484,12 @@ static void ufshcd_err_handler(struct ufs_hba *hba)
}
return;
fatal_eh:
- hba->ufshcd_state = UFSHCD_STATE_ERROR;
- schedule_work(&hba->feh_workq);
+ /* handle fatal errors only when link is functional */
+ if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
+ /* block commands at driver layer until error is handled */
+ hba->ufshcd_state = UFSHCD_STATE_ERROR;
+ schedule_work(&hba->feh_workq);
+ }
}
/**
@@ -2621,12 +2654,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
}
/**
- * ufshcd_device_reset - reset device and abort all the pending commands
+ * ufshcd_eh_device_reset_handler - device reset handler registered to
+ * scsi layer.
* @cmd: SCSI command pointer
*
* Returns SUCCESS/FAILED
*/
-static int ufshcd_device_reset(struct scsi_cmnd *cmd)
+static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
{
struct Scsi_Host *host;
struct ufs_hba *hba;
@@ -2635,6 +2669,7 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
int err;
u8 resp = 0xF;
struct ufshcd_lrb *lrbp;
+ unsigned long flags;
host = cmd->device->host;
hba = shost_priv(host);
@@ -2643,55 +2678,33 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
lrbp = &hba->lrb[tag];
err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp);
if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
- err = FAILED;
+ if (!err)
+ err = resp;
goto out;
- } else {
- err = SUCCESS;
}
- for (pos = 0; pos < hba->nutrs; pos++) {
- if (test_bit(pos, &hba->outstanding_reqs) &&
- (hba->lrb[tag].lun == hba->lrb[pos].lun)) {
-
- /* clear the respective UTRLCLR register bit */
- ufshcd_utrl_clear(hba, pos);
-
- clear_bit(pos, &hba->outstanding_reqs);
-
- if (hba->lrb[pos].cmd) {
- scsi_dma_unmap(hba->lrb[pos].cmd);
- hba->lrb[pos].cmd->result =
- DID_ABORT << 16;
- hba->lrb[pos].cmd->scsi_done(cmd);
- hba->lrb[pos].cmd = NULL;
- clear_bit_unlock(pos, &hba->lrb_in_use);
- wake_up(&hba->dev_cmd.tag_wq);
- }
+ /* clear the commands that were pending for corresponding LUN */
+ for_each_set_bit(pos, &hba->outstanding_reqs, hba->nutrs) {
+ if (hba->lrb[pos].lun == lrbp->lun) {
+ err = ufshcd_clear_cmd(hba, pos);
+ if (err)
+ break;
}
- } /* end of for */
+ }
+ spin_lock_irqsave(host->host_lock, flags);
+ ufshcd_transfer_req_compl(hba);
+ spin_unlock_irqrestore(host->host_lock, flags);
out:
+ if (!err) {
+ err = SUCCESS;
+ } else {
+ dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
+ err = FAILED;
+ }
return err;
}
/**
- * ufshcd_host_reset - Main reset function registered with scsi layer
- * @cmd: SCSI command pointer
- *
- * Returns SUCCESS/FAILED
- */
-static int ufshcd_host_reset(struct scsi_cmnd *cmd)
-{
- struct ufs_hba *hba;
-
- hba = shost_priv(cmd->device->host);
-
- if (hba->ufshcd_state == UFSHCD_STATE_RESET)
- return SUCCESS;
-
- return ufshcd_do_reset(hba);
-}
-
-/**
* ufshcd_abort - abort a specific command
* @cmd: SCSI command pointer
*
@@ -2789,6 +2802,122 @@ out:
}
/**
+ * ufshcd_host_reset_and_restore - reset and restore host controller
+ * @hba: per-adapter instance
+ *
+ * Note that host controller reset may issue DME_RESET to
+ * local and remote (device) Uni-Pro stack and the attributes
+ * are reset to default state.
+ *
+ * Returns zero on success, non-zero on failure
+ */
+static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
+{
+ int err;
+ async_cookie_t cookie;
+ unsigned long flags;
+
+ /* Reset the host controller */
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ ufshcd_hba_stop(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ err = ufshcd_hba_enable(hba);
+ if (err)
+ goto out;
+
+ /* Establish the link again and restore the device */
+ cookie = async_schedule(ufshcd_async_scan, hba);
+ /* wait for async scan to be completed */
+ async_synchronize_cookie(++cookie);
+ if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL)
+ err = -EIO;
+out:
+ if (err)
+ dev_err(hba->dev, "%s: Host init failed %d\n", __func__, err);
+
+ return err;
+}
+
+/**
+ * ufshcd_reset_and_restore - reset and re-initialize host/device
+ * @hba: per-adapter instance
+ *
+ * Reset and recover device, host and re-establish link. This
+ * is helpful to recover the communication in fatal error conditions.
+ *
+ * Returns zero on success, non-zero on failure
+ */
+static int ufshcd_reset_and_restore(struct ufs_hba *hba)
+{
+ int err = 0;
+ unsigned long flags;
+
+ err = ufshcd_host_reset_and_restore(hba);
+
+ /*
+ * After reset the door-bell might be cleared, complete
+ * outstanding requests in s/w here.
+ */
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ ufshcd_transfer_req_compl(hba);
+ ufshcd_tmc_handler(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ return err;
+}
+
+/**
+ * ufshcd_eh_host_reset_handler - host reset handler registered to scsi layer
+ * @cmd - SCSI command pointer
+ *
+ * Returns SUCCESS/FAILED
+ */
+static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd)
+{
+ int err;
+ unsigned long flags;
+ struct ufs_hba *hba;
+
+ hba = shost_priv(cmd->device->host);
+
+ /*
+ * Check if there is any race with fatal error handling.
+ * If so, wait for it to complete. Even though fatal error
+ * handling does reset and restore in some cases, don't assume
+ * anything out of it. We are just avoiding race here.
+ */
+ do {
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ if (!(work_pending(&hba->feh_workq) ||
+ hba->ufshcd_state == UFSHCD_STATE_RESET))
+ break;
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ dev_dbg(hba->dev, "%s: reset in progress\n", __func__);
+ flush_work(&hba->feh_workq);
+ } while (1);
+
+ hba->ufshcd_state = UFSHCD_STATE_RESET;
+ ufshcd_set_eh_in_progress(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ err = ufshcd_reset_and_restore(hba);
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ if (!err) {
+ err = SUCCESS;
+ hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+ } else {
+ err = FAILED;
+ hba->ufshcd_state = UFSHCD_STATE_ERROR;
+ }
+ ufshcd_clear_eh_in_progress(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ return err;
+}
+
+/**
* ufshcd_async_scan - asynchronous execution for link startup
* @data: data pointer to pass to this function
* @cookie: cookie data
@@ -2813,8 +2942,13 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
goto out;
ufshcd_force_reset_auto_bkops(hba);
- scsi_scan_host(hba->host);
- pm_runtime_put_sync(hba->dev);
+ hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+
+ /* If we are in error handling context no need to scan the host */
+ if (!ufshcd_eh_in_progress(hba)) {
+ scsi_scan_host(hba->host);
+ pm_runtime_put_sync(hba->dev);
+ }
out:
return;
}
@@ -2827,8 +2961,8 @@ static struct scsi_host_template ufshcd_driver_template = {
.slave_alloc = ufshcd_slave_alloc,
.slave_destroy = ufshcd_slave_destroy,
.eh_abort_handler = ufshcd_abort,
- .eh_device_reset_handler = ufshcd_device_reset,
- .eh_host_reset_handler = ufshcd_host_reset,
+ .eh_device_reset_handler = ufshcd_eh_device_reset_handler,
+ .eh_host_reset_handler = ufshcd_eh_host_reset_handler,
.this_id = -1,
.sg_tablesize = SG_ALL,
.cmd_per_lun = UFSHCD_CMD_PER_LUN,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 84d09d1..48c7d9b 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -180,6 +180,7 @@ struct ufs_dev_cmd {
* @pwr_done: completion for power mode change
* @tm_condition: condition variable for task management
* @ufshcd_state: UFSHCD states
+ * @eh_flags: Error handling flags
* @intr_mask: Interrupt Mask Bits
* @ee_ctrl_mask: Exception event control mask
* @feh_workq: Work queue for fatal controller error handling
@@ -227,6 +228,7 @@ struct ufs_hba {
struct completion *pwr_done;
u32 ufshcd_state;
+ u32 eh_flags;
u32 intr_mask;
u16 ee_ctrl_mask;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V7 5/6] scsi: ufs: Fix device and host reset methods
2013-09-19 11:14 ` [PATCH V7 5/6] scsi: ufs: Fix device and host reset methods Sujit Reddy Thumma
@ 2013-09-23 12:17 ` vinayak holikatti
0 siblings, 0 replies; 15+ messages in thread
From: vinayak holikatti @ 2013-09-23 12:17 UTC (permalink / raw)
To: Sujit Reddy Thumma
Cc: Santosh Y, James E.J. Bottomley, linux-scsi, linux-arm-msm
On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> As of now SCSI initiated error handling is broken because,
> the reset APIs don't try to bring back the device initialized and
> ready for further transfers.
>
> In case of timeouts, the scsi error handler takes care of handling aborts
> and resets. Improve the error handling in such scenario by resetting the
> device and host and re-initializing them in proper manner.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
> Tested-by: Dolev Raviv <draviv@codeaurora.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 240 ++++++++++++++++++++++++++++++++++++----------
> drivers/scsi/ufs/ufshcd.h | 2 +
> 2 files changed, 189 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 52f66e4..5462310 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -74,9 +74,14 @@ enum {
>
> /* UFSHCD states */
> enum {
> - UFSHCD_STATE_OPERATIONAL,
> UFSHCD_STATE_RESET,
> UFSHCD_STATE_ERROR,
> + UFSHCD_STATE_OPERATIONAL,
> +};
> +
> +/* UFSHCD error handling flags */
> +enum {
> + UFSHCD_EH_IN_PROGRESS = (1 << 0),
> };
>
> /* Interrupt configuration options */
> @@ -86,6 +91,16 @@ enum {
> UFSHCD_INT_CLEAR,
> };
>
> +#define ufshcd_set_eh_in_progress(h) \
> + (h->eh_flags |= UFSHCD_EH_IN_PROGRESS)
> +#define ufshcd_eh_in_progress(h) \
> + (h->eh_flags & UFSHCD_EH_IN_PROGRESS)
> +#define ufshcd_clear_eh_in_progress(h) \
> + (h->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
> +
> +static void ufshcd_tmc_handler(struct ufs_hba *hba);
> +static void ufshcd_async_scan(void *data, async_cookie_t cookie);
> +
> /*
> * ufshcd_wait_for_register - wait for register value to change
> * @hba - per-adapter interface
> @@ -856,10 +871,25 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>
> tag = cmd->request->tag;
>
> - if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + switch (hba->ufshcd_state) {
> + case UFSHCD_STATE_OPERATIONAL:
> + break;
> + case UFSHCD_STATE_RESET:
> err = SCSI_MLQUEUE_HOST_BUSY;
> - goto out;
> + goto out_unlock;
> + case UFSHCD_STATE_ERROR:
> + set_host_byte(cmd, DID_ERROR);
> + cmd->scsi_done(cmd);
> + goto out_unlock;
> + default:
> + dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
> + __func__, hba->ufshcd_state);
> + set_host_byte(cmd, DID_BAD_TARGET);
> + 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 */
> if (test_and_set_bit_lock(tag, &hba->lrb_in_use)) {
> @@ -896,6 +926,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> /* issue command to the controller */
> spin_lock_irqsave(hba->host->host_lock, flags);
> ufshcd_send_command(hba, tag);
> +out_unlock:
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> out:
> return err;
> @@ -1707,8 +1738,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
> if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> scsi_unblock_requests(hba->host);
>
> - hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> -
> out:
> return err;
> }
> @@ -2455,8 +2484,12 @@ static void ufshcd_err_handler(struct ufs_hba *hba)
> }
> return;
> fatal_eh:
> - hba->ufshcd_state = UFSHCD_STATE_ERROR;
> - schedule_work(&hba->feh_workq);
> + /* handle fatal errors only when link is functional */
> + if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
> + /* block commands at driver layer until error is handled */
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + schedule_work(&hba->feh_workq);
> + }
> }
>
> /**
> @@ -2621,12 +2654,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
> }
>
> /**
> - * ufshcd_device_reset - reset device and abort all the pending commands
> + * ufshcd_eh_device_reset_handler - device reset handler registered to
> + * scsi layer.
> * @cmd: SCSI command pointer
> *
> * Returns SUCCESS/FAILED
> */
> -static int ufshcd_device_reset(struct scsi_cmnd *cmd)
> +static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
> {
> struct Scsi_Host *host;
> struct ufs_hba *hba;
> @@ -2635,6 +2669,7 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
> int err;
> u8 resp = 0xF;
> struct ufshcd_lrb *lrbp;
> + unsigned long flags;
>
> host = cmd->device->host;
> hba = shost_priv(host);
> @@ -2643,55 +2678,33 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
> lrbp = &hba->lrb[tag];
> err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp);
> if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> - err = FAILED;
> + if (!err)
> + err = resp;
> goto out;
> - } else {
> - err = SUCCESS;
> }
>
> - for (pos = 0; pos < hba->nutrs; pos++) {
> - if (test_bit(pos, &hba->outstanding_reqs) &&
> - (hba->lrb[tag].lun == hba->lrb[pos].lun)) {
> -
> - /* clear the respective UTRLCLR register bit */
> - ufshcd_utrl_clear(hba, pos);
> -
> - clear_bit(pos, &hba->outstanding_reqs);
> -
> - if (hba->lrb[pos].cmd) {
> - scsi_dma_unmap(hba->lrb[pos].cmd);
> - hba->lrb[pos].cmd->result =
> - DID_ABORT << 16;
> - hba->lrb[pos].cmd->scsi_done(cmd);
> - hba->lrb[pos].cmd = NULL;
> - clear_bit_unlock(pos, &hba->lrb_in_use);
> - wake_up(&hba->dev_cmd.tag_wq);
> - }
> + /* clear the commands that were pending for corresponding LUN */
> + for_each_set_bit(pos, &hba->outstanding_reqs, hba->nutrs) {
> + if (hba->lrb[pos].lun == lrbp->lun) {
> + err = ufshcd_clear_cmd(hba, pos);
> + if (err)
> + break;
> }
> - } /* end of for */
> + }
> + spin_lock_irqsave(host->host_lock, flags);
> + ufshcd_transfer_req_compl(hba);
> + spin_unlock_irqrestore(host->host_lock, flags);
> out:
> + if (!err) {
> + err = SUCCESS;
> + } else {
> + dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
> + err = FAILED;
> + }
> return err;
> }
>
> /**
> - * ufshcd_host_reset - Main reset function registered with scsi layer
> - * @cmd: SCSI command pointer
> - *
> - * Returns SUCCESS/FAILED
> - */
> -static int ufshcd_host_reset(struct scsi_cmnd *cmd)
> -{
> - struct ufs_hba *hba;
> -
> - hba = shost_priv(cmd->device->host);
> -
> - if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> - return SUCCESS;
> -
> - return ufshcd_do_reset(hba);
> -}
> -
> -/**
> * ufshcd_abort - abort a specific command
> * @cmd: SCSI command pointer
> *
> @@ -2789,6 +2802,122 @@ out:
> }
>
> /**
> + * ufshcd_host_reset_and_restore - reset and restore host controller
> + * @hba: per-adapter instance
> + *
> + * Note that host controller reset may issue DME_RESET to
> + * local and remote (device) Uni-Pro stack and the attributes
> + * are reset to default state.
> + *
> + * Returns zero on success, non-zero on failure
> + */
> +static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
> +{
> + int err;
> + async_cookie_t cookie;
> + unsigned long flags;
> +
> + /* Reset the host controller */
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + ufshcd_hba_stop(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + err = ufshcd_hba_enable(hba);
> + if (err)
> + goto out;
> +
> + /* Establish the link again and restore the device */
> + cookie = async_schedule(ufshcd_async_scan, hba);
> + /* wait for async scan to be completed */
> + async_synchronize_cookie(++cookie);
> + if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL)
> + err = -EIO;
> +out:
> + if (err)
> + dev_err(hba->dev, "%s: Host init failed %d\n", __func__, err);
> +
> + return err;
> +}
> +
> +/**
> + * ufshcd_reset_and_restore - reset and re-initialize host/device
> + * @hba: per-adapter instance
> + *
> + * Reset and recover device, host and re-establish link. This
> + * is helpful to recover the communication in fatal error conditions.
> + *
> + * Returns zero on success, non-zero on failure
> + */
> +static int ufshcd_reset_and_restore(struct ufs_hba *hba)
> +{
> + int err = 0;
> + unsigned long flags;
> +
> + err = ufshcd_host_reset_and_restore(hba);
> +
> + /*
> + * After reset the door-bell might be cleared, complete
> + * outstanding requests in s/w here.
> + */
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + ufshcd_transfer_req_compl(hba);
> + ufshcd_tmc_handler(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + return err;
> +}
> +
> +/**
> + * ufshcd_eh_host_reset_handler - host reset handler registered to scsi layer
> + * @cmd - SCSI command pointer
> + *
> + * Returns SUCCESS/FAILED
> + */
> +static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd)
> +{
> + int err;
> + unsigned long flags;
> + struct ufs_hba *hba;
> +
> + hba = shost_priv(cmd->device->host);
> +
> + /*
> + * Check if there is any race with fatal error handling.
> + * If so, wait for it to complete. Even though fatal error
> + * handling does reset and restore in some cases, don't assume
> + * anything out of it. We are just avoiding race here.
> + */
> + do {
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + if (!(work_pending(&hba->feh_workq) ||
> + hba->ufshcd_state == UFSHCD_STATE_RESET))
> + break;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + dev_dbg(hba->dev, "%s: reset in progress\n", __func__);
> + flush_work(&hba->feh_workq);
> + } while (1);
> +
> + hba->ufshcd_state = UFSHCD_STATE_RESET;
> + ufshcd_set_eh_in_progress(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + err = ufshcd_reset_and_restore(hba);
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + if (!err) {
> + err = SUCCESS;
> + hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> + } else {
> + err = FAILED;
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + }
> + ufshcd_clear_eh_in_progress(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + return err;
> +}
> +
> +/**
> * ufshcd_async_scan - asynchronous execution for link startup
> * @data: data pointer to pass to this function
> * @cookie: cookie data
> @@ -2813,8 +2942,13 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
> goto out;
>
> ufshcd_force_reset_auto_bkops(hba);
> - scsi_scan_host(hba->host);
> - pm_runtime_put_sync(hba->dev);
> + hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +
> + /* If we are in error handling context no need to scan the host */
> + if (!ufshcd_eh_in_progress(hba)) {
> + scsi_scan_host(hba->host);
> + pm_runtime_put_sync(hba->dev);
> + }
> out:
> return;
> }
> @@ -2827,8 +2961,8 @@ static struct scsi_host_template ufshcd_driver_template = {
> .slave_alloc = ufshcd_slave_alloc,
> .slave_destroy = ufshcd_slave_destroy,
> .eh_abort_handler = ufshcd_abort,
> - .eh_device_reset_handler = ufshcd_device_reset,
> - .eh_host_reset_handler = ufshcd_host_reset,
> + .eh_device_reset_handler = ufshcd_eh_device_reset_handler,
> + .eh_host_reset_handler = ufshcd_eh_host_reset_handler,
> .this_id = -1,
> .sg_tablesize = SG_ALL,
> .cmd_per_lun = UFSHCD_CMD_PER_LUN,
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 84d09d1..48c7d9b 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -180,6 +180,7 @@ struct ufs_dev_cmd {
> * @pwr_done: completion for power mode change
> * @tm_condition: condition variable for task management
> * @ufshcd_state: UFSHCD states
> + * @eh_flags: Error handling flags
> * @intr_mask: Interrupt Mask Bits
> * @ee_ctrl_mask: Exception event control mask
> * @feh_workq: Work queue for fatal controller error handling
> @@ -227,6 +228,7 @@ struct ufs_hba {
> struct completion *pwr_done;
>
> u32 ufshcd_state;
> + u32 eh_flags;
> u32 intr_mask;
> u16 ee_ctrl_mask;
>
Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 6/6] scsi: ufs: Improve UFS fatal error handling
2013-09-19 11:14 [PATCH V7 0/6] scsi: ufs: TM, fatal-error handling and other fixes Sujit Reddy Thumma
` (4 preceding siblings ...)
2013-09-19 11:14 ` [PATCH V7 5/6] scsi: ufs: Fix device and host reset methods Sujit Reddy Thumma
@ 2013-09-19 11:14 ` Sujit Reddy Thumma
2013-09-23 12:17 ` vinayak holikatti
5 siblings, 1 reply; 15+ messages in thread
From: Sujit Reddy Thumma @ 2013-09-19 11:14 UTC (permalink / raw)
To: Vinayak Holikatti, Santosh Y
Cc: James E.J. Bottomley, linux-scsi, Sujit Reddy Thumma,
linux-arm-msm
Error handling in UFS driver is broken and resets the host controller
for fatal errors without re-initialization. Correct the fatal error
handling sequence according to UFS Host Controller Interface (HCI)
v1.1 specification.
o Processed requests which are completed w/wo error are reported to
SCSI layer and any pending commands that are not started are aborted
in the controller and re-queued into scsi mid-layer queue.
o Upon determining fatal error condition the host controller may hang
forever until a reset is applied. Block SCSI layer for sending new
requests and apply reset in a separate error handling work.
o SCSI is informed about the expected Unit-Attention exception from the
device for the immediate command after a reset so that the SCSI layer
take necessary steps to establish communication with the device.
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
Tested-by: Dolev Raviv <draviv@codeaurora.org>
---
drivers/scsi/ufs/ufshcd.c | 229 ++++++++++++++++++++++++++++------------------
drivers/scsi/ufs/ufshcd.h | 10 +-
2 files changed, 149 insertions(+), 90 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5462310..0c28772 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -84,6 +84,14 @@ enum {
UFSHCD_EH_IN_PROGRESS = (1 << 0),
};
+/* UFSHCD UIC layer error flags */
+enum {
+ UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
+ UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
+ UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
+ UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
+};
+
/* Interrupt configuration options */
enum {
UFSHCD_INT_DISABLE,
@@ -100,6 +108,8 @@ enum {
static void ufshcd_tmc_handler(struct ufs_hba *hba);
static void ufshcd_async_scan(void *data, async_cookie_t cookie);
+static int ufshcd_reset_and_restore(struct ufs_hba *hba);
+static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
/*
* ufshcd_wait_for_register - wait for register value to change
@@ -1735,9 +1745,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
goto out;
}
- if (hba->ufshcd_state == UFSHCD_STATE_RESET)
- scsi_unblock_requests(hba->host);
-
out:
return err;
}
@@ -1863,66 +1870,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
}
/**
- * ufshcd_do_reset - reset the host controller
- * @hba: per adapter instance
- *
- * Returns SUCCESS/FAILED
- */
-static int ufshcd_do_reset(struct ufs_hba *hba)
-{
- struct ufshcd_lrb *lrbp;
- unsigned long flags;
- int tag;
-
- /* block commands from midlayer */
- scsi_block_requests(hba->host);
-
- spin_lock_irqsave(hba->host->host_lock, flags);
- hba->ufshcd_state = UFSHCD_STATE_RESET;
-
- /* send controller to reset state */
- ufshcd_hba_stop(hba);
- spin_unlock_irqrestore(hba->host->host_lock, flags);
-
- /* abort outstanding commands */
- for (tag = 0; tag < hba->nutrs; tag++) {
- if (test_bit(tag, &hba->outstanding_reqs)) {
- lrbp = &hba->lrb[tag];
- if (lrbp->cmd) {
- scsi_dma_unmap(lrbp->cmd);
- lrbp->cmd->result = DID_RESET << 16;
- lrbp->cmd->scsi_done(lrbp->cmd);
- lrbp->cmd = NULL;
- clear_bit_unlock(tag, &hba->lrb_in_use);
- }
- }
- }
-
- /* complete device management command */
- if (hba->dev_cmd.complete)
- complete(hba->dev_cmd.complete);
-
- /* clear outstanding request/task bit maps */
- hba->outstanding_reqs = 0;
- hba->outstanding_tasks = 0;
-
- /* Host controller enable */
- if (ufshcd_hba_enable(hba)) {
- dev_err(hba->dev,
- "Reset: Controller initialization failed\n");
- return FAILED;
- }
-
- if (ufshcd_link_startup(hba)) {
- dev_err(hba->dev,
- "Reset: Link start-up failed\n");
- return FAILED;
- }
-
- return SUCCESS;
-}
-
-/**
* ufshcd_slave_alloc - handle initial SCSI device configurations
* @sdev: pointer to SCSI device
*
@@ -1939,6 +1886,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
sdev->use_10_for_ms = 1;
scsi_set_tag_type(sdev, MSG_SIMPLE_TAG);
+ /* allow SCSI layer to restart the device in case of errors */
+ sdev->allow_restart = 1;
+
/*
* Inform SCSI Midlayer that the LUN queue depth is same as the
* controller queue depth. If a LUN queue depth is less than the
@@ -2134,6 +2084,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
case OCS_ABORTED:
result |= DID_ABORT << 16;
break;
+ case OCS_INVALID_COMMAND_STATUS:
+ result |= DID_REQUEUE << 16;
+ break;
case OCS_INVALID_CMD_TABLE_ATTR:
case OCS_INVALID_PRDT_ATTR:
case OCS_MISMATCH_DATA_BUF_SIZE:
@@ -2451,45 +2404,145 @@ out:
}
/**
- * ufshcd_fatal_err_handler - handle fatal errors
- * @hba: per adapter instance
+ * ufshcd_err_handler - handle UFS errors that require s/w attention
+ * @work: pointer to work structure
*/
-static void ufshcd_fatal_err_handler(struct work_struct *work)
+static void ufshcd_err_handler(struct work_struct *work)
{
struct ufs_hba *hba;
- hba = container_of(work, struct ufs_hba, feh_workq);
+ unsigned long flags;
+ u32 err_xfer = 0;
+ u32 err_tm = 0;
+ int err = 0;
+ int tag;
+
+ hba = container_of(work, struct ufs_hba, eh_work);
pm_runtime_get_sync(hba->dev);
- /* check if reset is already in progress */
- if (hba->ufshcd_state != UFSHCD_STATE_RESET)
- ufshcd_do_reset(hba);
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ goto out;
+ }
+
+ hba->ufshcd_state = UFSHCD_STATE_RESET;
+ ufshcd_set_eh_in_progress(hba);
+
+ /* Complete requests that have door-bell cleared by h/w */
+ ufshcd_transfer_req_compl(hba);
+ ufshcd_tmc_handler(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ /* Clear pending transfer requests */
+ for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs)
+ if (ufshcd_clear_cmd(hba, tag))
+ err_xfer |= 1 << tag;
+
+ /* Clear pending task management requests */
+ for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs)
+ if (ufshcd_clear_tm_cmd(hba, tag))
+ err_tm |= 1 << tag;
+
+ /* Complete the requests that are cleared by s/w */
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ ufshcd_transfer_req_compl(hba);
+ ufshcd_tmc_handler(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ /* Fatal errors need reset */
+ if (err_xfer || err_tm || (hba->saved_err & INT_FATAL_ERRORS) ||
+ ((hba->saved_err & UIC_ERROR) &&
+ (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR))) {
+ err = ufshcd_reset_and_restore(hba);
+ if (err) {
+ dev_err(hba->dev, "%s: reset and restore failed\n",
+ __func__);
+ hba->ufshcd_state = UFSHCD_STATE_ERROR;
+ }
+ /*
+ * Inform scsi mid-layer that we did reset and allow to handle
+ * Unit Attention properly.
+ */
+ scsi_report_bus_reset(hba->host, 0);
+ hba->saved_err = 0;
+ hba->saved_uic_err = 0;
+ }
+ ufshcd_clear_eh_in_progress(hba);
+
+out:
+ scsi_unblock_requests(hba->host);
pm_runtime_put_sync(hba->dev);
}
/**
- * ufshcd_err_handler - Check for fatal errors
- * @work: pointer to a work queue structure
+ * ufshcd_update_uic_error - check and set fatal UIC error flags.
+ * @hba: per-adapter instance
*/
-static void ufshcd_err_handler(struct ufs_hba *hba)
+static void ufshcd_update_uic_error(struct ufs_hba *hba)
{
u32 reg;
+ /* PA_INIT_ERROR is fatal and needs UIC reset */
+ reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
+ if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
+ hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
+
+ /* UIC NL/TL/DME errors needs software retry */
+ reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
+ if (reg)
+ hba->uic_error |= UFSHCD_UIC_NL_ERROR;
+
+ reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
+ if (reg)
+ hba->uic_error |= UFSHCD_UIC_TL_ERROR;
+
+ reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
+ if (reg)
+ hba->uic_error |= UFSHCD_UIC_DME_ERROR;
+
+ dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
+ __func__, hba->uic_error);
+}
+
+/**
+ * ufshcd_check_errors - Check for errors that need s/w attention
+ * @hba: per-adapter instance
+ */
+static void ufshcd_check_errors(struct ufs_hba *hba)
+{
+ bool queue_eh_work = false;
+
if (hba->errors & INT_FATAL_ERRORS)
- goto fatal_eh;
+ queue_eh_work = true;
if (hba->errors & UIC_ERROR) {
- reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
- if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
- goto fatal_eh;
+ hba->uic_error = 0;
+ ufshcd_update_uic_error(hba);
+ if (hba->uic_error)
+ queue_eh_work = true;
}
- return;
-fatal_eh:
- /* handle fatal errors only when link is functional */
- if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
- /* block commands at driver layer until error is handled */
- hba->ufshcd_state = UFSHCD_STATE_ERROR;
- schedule_work(&hba->feh_workq);
+
+ if (queue_eh_work) {
+ /* handle fatal errors only when link is functional */
+ if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
+ /* block commands from scsi mid-layer */
+ scsi_block_requests(hba->host);
+
+ /* transfer error masks to sticky bits */
+ hba->saved_err |= hba->errors;
+ hba->saved_uic_err |= hba->uic_error;
+
+ hba->ufshcd_state = UFSHCD_STATE_ERROR;
+ schedule_work(&hba->eh_work);
+ }
}
+ /*
+ * if (!queue_eh_work) -
+ * Other errors are either non-fatal where host recovers
+ * itself without s/w intervention or errors that will be
+ * handled by the SCSI core layer.
+ */
}
/**
@@ -2514,7 +2567,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
{
hba->errors = UFSHCD_ERROR_MASK & intr_status;
if (hba->errors)
- ufshcd_err_handler(hba);
+ ufshcd_check_errors(hba);
if (intr_status & UFSHCD_UIC_MASK)
ufshcd_uic_cmd_compl(hba, intr_status);
@@ -2889,12 +2942,12 @@ static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd)
*/
do {
spin_lock_irqsave(hba->host->host_lock, flags);
- if (!(work_pending(&hba->feh_workq) ||
+ if (!(work_pending(&hba->eh_work) ||
hba->ufshcd_state == UFSHCD_STATE_RESET))
break;
spin_unlock_irqrestore(hba->host->host_lock, flags);
dev_dbg(hba->dev, "%s: reset in progress\n", __func__);
- flush_work(&hba->feh_workq);
+ flush_work(&hba->eh_work);
} while (1);
hba->ufshcd_state = UFSHCD_STATE_RESET;
@@ -3130,7 +3183,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
init_waitqueue_head(&hba->tm_tag_wq);
/* Initialize work queues */
- INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
+ INIT_WORK(&hba->eh_work, ufshcd_err_handler);
INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
/* Initialize UIC command mutex */
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 48c7d9b..acf318e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -183,9 +183,12 @@ struct ufs_dev_cmd {
* @eh_flags: Error handling flags
* @intr_mask: Interrupt Mask Bits
* @ee_ctrl_mask: Exception event control mask
- * @feh_workq: Work queue for fatal controller error handling
+ * @eh_work: Worker to handle UFS errors that require s/w attention
* @eeh_work: Worker to handle exception events
* @errors: HBA errors
+ * @uic_error: UFS interconnect layer error status
+ * @saved_err: sticky error mask
+ * @saved_uic_err: sticky UIC error mask
* @dev_cmd: ufs device management command information
* @auto_bkops_enabled: to track whether bkops is enabled in device
*/
@@ -233,11 +236,14 @@ struct ufs_hba {
u16 ee_ctrl_mask;
/* Work Queues */
- struct work_struct feh_workq;
+ struct work_struct eh_work;
struct work_struct eeh_work;
/* HBA Errors */
u32 errors;
+ u32 uic_error;
+ u32 saved_err;
+ u32 saved_uic_err;
/* Device management request data */
struct ufs_dev_cmd dev_cmd;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V7 6/6] scsi: ufs: Improve UFS fatal error handling
2013-09-19 11:14 ` [PATCH V7 6/6] scsi: ufs: Improve UFS fatal error handling Sujit Reddy Thumma
@ 2013-09-23 12:17 ` vinayak holikatti
0 siblings, 0 replies; 15+ messages in thread
From: vinayak holikatti @ 2013-09-23 12:17 UTC (permalink / raw)
To: Sujit Reddy Thumma
Cc: Santosh Y, James E.J. Bottomley, linux-scsi, linux-arm-msm
On Thu, Sep 19, 2013 at 4:44 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> Error handling in UFS driver is broken and resets the host controller
> for fatal errors without re-initialization. Correct the fatal error
> handling sequence according to UFS Host Controller Interface (HCI)
> v1.1 specification.
>
> o Processed requests which are completed w/wo error are reported to
> SCSI layer and any pending commands that are not started are aborted
> in the controller and re-queued into scsi mid-layer queue.
>
> o Upon determining fatal error condition the host controller may hang
> forever until a reset is applied. Block SCSI layer for sending new
> requests and apply reset in a separate error handling work.
>
> o SCSI is informed about the expected Unit-Attention exception from the
> device for the immediate command after a reset so that the SCSI layer
> take necessary steps to establish communication with the device.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
> Tested-by: Dolev Raviv <draviv@codeaurora.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 229 ++++++++++++++++++++++++++++------------------
> drivers/scsi/ufs/ufshcd.h | 10 +-
> 2 files changed, 149 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5462310..0c28772 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -84,6 +84,14 @@ enum {
> UFSHCD_EH_IN_PROGRESS = (1 << 0),
> };
>
> +/* UFSHCD UIC layer error flags */
> +enum {
> + UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
> + UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
> + UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
> + UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
> +};
> +
> /* Interrupt configuration options */
> enum {
> UFSHCD_INT_DISABLE,
> @@ -100,6 +108,8 @@ enum {
>
> static void ufshcd_tmc_handler(struct ufs_hba *hba);
> static void ufshcd_async_scan(void *data, async_cookie_t cookie);
> +static int ufshcd_reset_and_restore(struct ufs_hba *hba);
> +static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
>
> /*
> * ufshcd_wait_for_register - wait for register value to change
> @@ -1735,9 +1745,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
> goto out;
> }
>
> - if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> - scsi_unblock_requests(hba->host);
> -
> out:
> return err;
> }
> @@ -1863,66 +1870,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
> }
>
> /**
> - * ufshcd_do_reset - reset the host controller
> - * @hba: per adapter instance
> - *
> - * Returns SUCCESS/FAILED
> - */
> -static int ufshcd_do_reset(struct ufs_hba *hba)
> -{
> - struct ufshcd_lrb *lrbp;
> - unsigned long flags;
> - int tag;
> -
> - /* block commands from midlayer */
> - scsi_block_requests(hba->host);
> -
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - hba->ufshcd_state = UFSHCD_STATE_RESET;
> -
> - /* send controller to reset state */
> - ufshcd_hba_stop(hba);
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> -
> - /* abort outstanding commands */
> - for (tag = 0; tag < hba->nutrs; tag++) {
> - if (test_bit(tag, &hba->outstanding_reqs)) {
> - lrbp = &hba->lrb[tag];
> - if (lrbp->cmd) {
> - scsi_dma_unmap(lrbp->cmd);
> - lrbp->cmd->result = DID_RESET << 16;
> - lrbp->cmd->scsi_done(lrbp->cmd);
> - lrbp->cmd = NULL;
> - clear_bit_unlock(tag, &hba->lrb_in_use);
> - }
> - }
> - }
> -
> - /* complete device management command */
> - if (hba->dev_cmd.complete)
> - complete(hba->dev_cmd.complete);
> -
> - /* clear outstanding request/task bit maps */
> - hba->outstanding_reqs = 0;
> - hba->outstanding_tasks = 0;
> -
> - /* Host controller enable */
> - if (ufshcd_hba_enable(hba)) {
> - dev_err(hba->dev,
> - "Reset: Controller initialization failed\n");
> - return FAILED;
> - }
> -
> - if (ufshcd_link_startup(hba)) {
> - dev_err(hba->dev,
> - "Reset: Link start-up failed\n");
> - return FAILED;
> - }
> -
> - return SUCCESS;
> -}
> -
> -/**
> * ufshcd_slave_alloc - handle initial SCSI device configurations
> * @sdev: pointer to SCSI device
> *
> @@ -1939,6 +1886,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
> sdev->use_10_for_ms = 1;
> scsi_set_tag_type(sdev, MSG_SIMPLE_TAG);
>
> + /* allow SCSI layer to restart the device in case of errors */
> + sdev->allow_restart = 1;
> +
> /*
> * Inform SCSI Midlayer that the LUN queue depth is same as the
> * controller queue depth. If a LUN queue depth is less than the
> @@ -2134,6 +2084,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> case OCS_ABORTED:
> result |= DID_ABORT << 16;
> break;
> + case OCS_INVALID_COMMAND_STATUS:
> + result |= DID_REQUEUE << 16;
> + break;
> case OCS_INVALID_CMD_TABLE_ATTR:
> case OCS_INVALID_PRDT_ATTR:
> case OCS_MISMATCH_DATA_BUF_SIZE:
> @@ -2451,45 +2404,145 @@ out:
> }
>
> /**
> - * ufshcd_fatal_err_handler - handle fatal errors
> - * @hba: per adapter instance
> + * ufshcd_err_handler - handle UFS errors that require s/w attention
> + * @work: pointer to work structure
> */
> -static void ufshcd_fatal_err_handler(struct work_struct *work)
> +static void ufshcd_err_handler(struct work_struct *work)
> {
> struct ufs_hba *hba;
> - hba = container_of(work, struct ufs_hba, feh_workq);
> + unsigned long flags;
> + u32 err_xfer = 0;
> + u32 err_tm = 0;
> + int err = 0;
> + int tag;
> +
> + hba = container_of(work, struct ufs_hba, eh_work);
>
> pm_runtime_get_sync(hba->dev);
> - /* check if reset is already in progress */
> - if (hba->ufshcd_state != UFSHCD_STATE_RESET)
> - ufshcd_do_reset(hba);
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + goto out;
> + }
> +
> + hba->ufshcd_state = UFSHCD_STATE_RESET;
> + ufshcd_set_eh_in_progress(hba);
> +
> + /* Complete requests that have door-bell cleared by h/w */
> + ufshcd_transfer_req_compl(hba);
> + ufshcd_tmc_handler(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + /* Clear pending transfer requests */
> + for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs)
> + if (ufshcd_clear_cmd(hba, tag))
> + err_xfer |= 1 << tag;
> +
> + /* Clear pending task management requests */
> + for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs)
> + if (ufshcd_clear_tm_cmd(hba, tag))
> + err_tm |= 1 << tag;
> +
> + /* Complete the requests that are cleared by s/w */
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + ufshcd_transfer_req_compl(hba);
> + ufshcd_tmc_handler(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + /* Fatal errors need reset */
> + if (err_xfer || err_tm || (hba->saved_err & INT_FATAL_ERRORS) ||
> + ((hba->saved_err & UIC_ERROR) &&
> + (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR))) {
> + err = ufshcd_reset_and_restore(hba);
> + if (err) {
> + dev_err(hba->dev, "%s: reset and restore failed\n",
> + __func__);
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + }
> + /*
> + * Inform scsi mid-layer that we did reset and allow to handle
> + * Unit Attention properly.
> + */
> + scsi_report_bus_reset(hba->host, 0);
> + hba->saved_err = 0;
> + hba->saved_uic_err = 0;
> + }
> + ufshcd_clear_eh_in_progress(hba);
> +
> +out:
> + scsi_unblock_requests(hba->host);
> pm_runtime_put_sync(hba->dev);
> }
>
> /**
> - * ufshcd_err_handler - Check for fatal errors
> - * @work: pointer to a work queue structure
> + * ufshcd_update_uic_error - check and set fatal UIC error flags.
> + * @hba: per-adapter instance
> */
> -static void ufshcd_err_handler(struct ufs_hba *hba)
> +static void ufshcd_update_uic_error(struct ufs_hba *hba)
> {
> u32 reg;
>
> + /* PA_INIT_ERROR is fatal and needs UIC reset */
> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> + if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> + hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
> +
> + /* UIC NL/TL/DME errors needs software retry */
> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
> + if (reg)
> + hba->uic_error |= UFSHCD_UIC_NL_ERROR;
> +
> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
> + if (reg)
> + hba->uic_error |= UFSHCD_UIC_TL_ERROR;
> +
> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
> + if (reg)
> + hba->uic_error |= UFSHCD_UIC_DME_ERROR;
> +
> + dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
> + __func__, hba->uic_error);
> +}
> +
> +/**
> + * ufshcd_check_errors - Check for errors that need s/w attention
> + * @hba: per-adapter instance
> + */
> +static void ufshcd_check_errors(struct ufs_hba *hba)
> +{
> + bool queue_eh_work = false;
> +
> if (hba->errors & INT_FATAL_ERRORS)
> - goto fatal_eh;
> + queue_eh_work = true;
>
> if (hba->errors & UIC_ERROR) {
> - reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> - if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> - goto fatal_eh;
> + hba->uic_error = 0;
> + ufshcd_update_uic_error(hba);
> + if (hba->uic_error)
> + queue_eh_work = true;
> }
> - return;
> -fatal_eh:
> - /* handle fatal errors only when link is functional */
> - if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
> - /* block commands at driver layer until error is handled */
> - hba->ufshcd_state = UFSHCD_STATE_ERROR;
> - schedule_work(&hba->feh_workq);
> +
> + if (queue_eh_work) {
> + /* handle fatal errors only when link is functional */
> + if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
> + /* block commands from scsi mid-layer */
> + scsi_block_requests(hba->host);
> +
> + /* transfer error masks to sticky bits */
> + hba->saved_err |= hba->errors;
> + hba->saved_uic_err |= hba->uic_error;
> +
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + schedule_work(&hba->eh_work);
> + }
> }
> + /*
> + * if (!queue_eh_work) -
> + * Other errors are either non-fatal where host recovers
> + * itself without s/w intervention or errors that will be
> + * handled by the SCSI core layer.
> + */
> }
>
> /**
> @@ -2514,7 +2567,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> {
> hba->errors = UFSHCD_ERROR_MASK & intr_status;
> if (hba->errors)
> - ufshcd_err_handler(hba);
> + ufshcd_check_errors(hba);
>
> if (intr_status & UFSHCD_UIC_MASK)
> ufshcd_uic_cmd_compl(hba, intr_status);
> @@ -2889,12 +2942,12 @@ static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd)
> */
> do {
> spin_lock_irqsave(hba->host->host_lock, flags);
> - if (!(work_pending(&hba->feh_workq) ||
> + if (!(work_pending(&hba->eh_work) ||
> hba->ufshcd_state == UFSHCD_STATE_RESET))
> break;
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> dev_dbg(hba->dev, "%s: reset in progress\n", __func__);
> - flush_work(&hba->feh_workq);
> + flush_work(&hba->eh_work);
> } while (1);
>
> hba->ufshcd_state = UFSHCD_STATE_RESET;
> @@ -3130,7 +3183,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> init_waitqueue_head(&hba->tm_tag_wq);
>
> /* Initialize work queues */
> - INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
> + INIT_WORK(&hba->eh_work, ufshcd_err_handler);
> INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
>
> /* Initialize UIC command mutex */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 48c7d9b..acf318e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -183,9 +183,12 @@ struct ufs_dev_cmd {
> * @eh_flags: Error handling flags
> * @intr_mask: Interrupt Mask Bits
> * @ee_ctrl_mask: Exception event control mask
> - * @feh_workq: Work queue for fatal controller error handling
> + * @eh_work: Worker to handle UFS errors that require s/w attention
> * @eeh_work: Worker to handle exception events
> * @errors: HBA errors
> + * @uic_error: UFS interconnect layer error status
> + * @saved_err: sticky error mask
> + * @saved_uic_err: sticky UIC error mask
> * @dev_cmd: ufs device management command information
> * @auto_bkops_enabled: to track whether bkops is enabled in device
> */
> @@ -233,11 +236,14 @@ struct ufs_hba {
> u16 ee_ctrl_mask;
>
> /* Work Queues */
> - struct work_struct feh_workq;
> + struct work_struct eh_work;
> struct work_struct eeh_work;
>
> /* HBA Errors */
> u32 errors;
> + u32 uic_error;
> + u32 saved_err;
> + u32 saved_uic_err;
>
> /* Device management request data */
> struct ufs_dev_cmd dev_cmd;
Acked-by: Vinayak Holikatti <vinholikatti@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread