On Wed, Sep 05, 2018 at 07:08:50PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 05, 2018 at 03:53:41PM +0000, Avri Altman wrote:
> > But on the other hand, task management request and response UPIUs
> > are honorable members of the ufs spec (JEDEC 220C paragraphs 10.7.6 & 10.7.7).
> > and indeed they lives in ufs.h, where they should.
>
> There is no other use anywhere. Remember that there is no need to
> have C data structure match what is in the spec, in fact doing so
> often leads to worse code, which is why we often avoid it.
>
> I'll send out a little series (untested) to show how I'd like to see
> this task menagement code look like.
Ok, the wifi at the meeting I'm at right now doesn't allow sending
mail, so here are the patches as an attachment.
>From 895b2ef4ada52a9c233853c63777faab32a47221 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@xxxxxx>
Date: Wed, 5 Sep 2018 06:31:16 -0700
Subject: scsi: ufs: cleanup struct utp_task_req_desc
Remove the pointless task_req_upiu and task_rsp_upiu indirections,
which are __le32 arrays always cast to given structures and just add
the members directly. Also clean up variables names in use in the
callers a bit to make the code more readable.
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
drivers/scsi/ufs/ufs.h | 30 -----------------
drivers/scsi/ufs/ufshcd.c | 68 ++++++++++++---------------------------
drivers/scsi/ufs/ufshci.h | 25 +++++++-------
3 files changed, 34 insertions(+), 89 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14e5bf7af0bb..16230df242f9 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -519,36 +519,6 @@ struct utp_upiu_rsp {
};
};
-/**
- * struct utp_upiu_task_req - Task request UPIU structure
- * @header - UPIU header structure DW0 to DW-2
- * @input_param1: Input parameter 1 DW-3
- * @input_param2: Input parameter 2 DW-4
- * @input_param3: Input parameter 3 DW-5
- * @reserved: Reserved double words DW-6 to DW-7
- */
-struct utp_upiu_task_req {
- struct utp_upiu_header header;
- __be32 input_param1;
- __be32 input_param2;
- __be32 input_param3;
- __be32 reserved[2];
-};
-
-/**
- * struct utp_upiu_task_rsp - Task Management Response UPIU structure
- * @header: UPIU header structure DW0-DW-2
- * @output_param1: Ouput parameter 1 DW3
- * @output_param2: Output parameter 2 DW4
- * @reserved: Reserved double words DW-5 to DW-7
- */
-struct utp_upiu_task_rsp {
- struct utp_upiu_header header;
- __be32 output_param1;
- __be32 output_param2;
- __be32 reserved[3];
-};
-
/**
* struct ufs_query_req - parameters for building a query request
* @query_func: UPIU header query function
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9d5d2ca7fc4f..178bbf7ed19e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -326,14 +326,11 @@ static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
const char *str)
{
- struct utp_task_req_desc *descp;
- struct utp_upiu_task_req *task_req;
int off = (int)tag - hba->nutrs;
+ struct utp_task_req_desc *descp = &hba->utmrdl_base_addr[off];
- descp = &hba->utmrdl_base_addr[off];
- task_req = (struct utp_upiu_task_req *)descp->task_req_upiu;
- trace_ufshcd_upiu(dev_name(hba->dev), str, &task_req->header,
- &task_req->input_param1);
+ trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
+ &descp->input_param1);
}
static void ufshcd_add_command_trace(struct ufs_hba *hba,
@@ -475,22 +472,13 @@ void ufshcd_print_trs(struct ufs_hba *hba, unsigned long bitmap, bool pr_prdt)
static void ufshcd_print_tmrs(struct ufs_hba *hba, unsigned long bitmap)
{
- struct utp_task_req_desc *tmrdp;
int tag;
for_each_set_bit(tag, &bitmap, hba->nutmrs) {
- tmrdp = &hba->utmrdl_base_addr[tag];
+ struct utp_task_req_desc *tmrdp = &hba->utmrdl_base_addr[tag];
+
dev_err(hba->dev, "TM[%d] - Task Management Header\n", tag);
- ufshcd_hex_dump("TM TRD: ", &tmrdp->header,
- sizeof(struct request_desc_header));
- dev_err(hba->dev, "TM[%d] - Task Management Request UPIU\n",
- tag);
- ufshcd_hex_dump("TM REQ: ", tmrdp->task_req_upiu,
- sizeof(struct utp_upiu_req));
- dev_err(hba->dev, "TM[%d] - Task Management Response UPIU\n",
- tag);
- ufshcd_hex_dump("TM RSP: ", tmrdp->task_rsp_upiu,
- sizeof(struct utp_task_req_desc));
+ ufshcd_hex_dump("", tmrdp, sizeof(*tmrdp));
}
}
@@ -4610,31 +4598,22 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
*/
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;
+ struct utp_task_req_desc *treq = hba->utmrdl_base_addr + index;
unsigned long flags;
int ocs_value;
- int task_result;
spin_lock_irqsave(hba->host->host_lock, flags);
/* Clear completed tasks from outstanding_tasks */
__clear_bit(index, &hba->outstanding_tasks);
- task_req_descp = hba->utmrdl_base_addr;
- ocs_value = ufshcd_get_tmr_ocs(&task_req_descp[index]);
+ ocs_value = ufshcd_get_tmr_ocs(treq);
- if (ocs_value == OCS_SUCCESS) {
- task_rsp_upiup = (struct utp_upiu_task_rsp *)
- task_req_descp[index].task_rsp_upiu;
- task_result = be32_to_cpu(task_rsp_upiup->output_param1);
- task_result = task_result & MASK_TM_SERVICE_RESP;
- if (resp)
- *resp = (u8)task_result;
- } else {
+ if (ocs_value != OCS_SUCCESS)
dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
__func__, ocs_value);
- }
+ else if (resp)
+ *resp = be32_to_cpu(treq->output_param1) & MASK_TM_SERVICE_RESP;
spin_unlock_irqrestore(hba->host->host_lock, flags);
return ocs_value;
@@ -5610,8 +5589,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
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 utp_task_req_desc *treq;
struct Scsi_Host *host;
unsigned long flags;
int free_slot;
@@ -5629,29 +5607,23 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
ufshcd_hold(hba, false);
spin_lock_irqsave(host->host_lock, flags);
- task_req_descp = hba->utmrdl_base_addr;
- task_req_descp += free_slot;
+ treq = hba->utmrdl_base_addr + free_slot;
/* Configure task request descriptor */
- task_req_descp->header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
- task_req_descp->header.dword_2 =
- cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+ treq->header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
+ treq->header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
/* 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,
- lun_id, task_tag);
- task_req_upiup->header.dword_1 =
- UPIU_HEADER_DWORD(0, tm_function, 0, 0);
+ treq->req_header.dword_0 = UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ,
+ 0, lun_id, task_tag);
+ treq->req_header.dword_1 = UPIU_HEADER_DWORD(0, tm_function, 0, 0);
/*
* The host shall provide the same value for LUN field in the basic
* header and for Input Parameter.
*/
- task_req_upiup->input_param1 = cpu_to_be32(lun_id);
- task_req_upiup->input_param2 = cpu_to_be32(task_id);
+ treq->input_param1 = cpu_to_be32(lun_id);
+ treq->input_param2 = cpu_to_be32(task_id);
ufshcd_vops_setup_task_mgmt(hba, free_slot, tm_function);
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index bb5d9c7f3353..6fa889de5ee5 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -433,22 +433,25 @@ struct utp_transfer_req_desc {
__le16 prd_table_offset;
};
-/**
- * struct utp_task_req_desc - UTMRD structure
- * @header: UTMRD header DW-0 to DW-3
- * @task_req_upiu: Pointer to task request UPIU DW-4 to DW-11
- * @task_rsp_upiu: Pointer to task response UPIU DW12 to DW-19
+/*
+ * UTMRD structure.
*/
struct utp_task_req_desc {
-
/* DW 0-3 */
struct request_desc_header header;
- /* DW 4-11 */
- __le32 task_req_upiu[TASK_REQ_UPIU_SIZE_DWORDS];
-
- /* DW 12-19 */
- __le32 task_rsp_upiu[TASK_RSP_UPIU_SIZE_DWORDS];
+ /* DW 4-11 - Task request UPIU structure */
+ struct utp_upiu_header req_header;
+ __be32 input_param1;
+ __be32 input_param2;
+ __be32 input_param3;
+ __be32 __reserved1[2];
+
+ /* DW 12-19 - Task Management Response UPIU structure */
+ struct utp_upiu_header rsp_header;
+ __be32 output_param1;
+ __be32 output_param2;
+ __be32 __reserved2[3];
};
#endif /* End of Header */
--
2.18.0
>From 56db3fe6e2500f4138713ec5d9de97249bd84a8f Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@xxxxxx>
Date: Wed, 5 Sep 2018 10:01:51 -0700
Subject: scsi: ufs: add a low-level __ufshcd_issue_tm_cmd helper
Add a helper that takes a utp_task_req_desc and issues it, which will
be useful for UFS bsg support. Rewrite ufshcd_issue_tm_cmd0x to use
this new helper.
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
drivers/scsi/ufs/ufshcd.c | 137 +++++++++++++++++---------------------
1 file changed, 60 insertions(+), 77 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 178bbf7ed19e..120f7620d01d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -633,19 +633,6 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
}
-/**
- * ufshcd_get_tmr_ocs - Get the UTMRD Overall Command Status
- * @task_req_descp: pointer to utp_task_req_desc structure
- *
- * This function is used to get the OCS field from UTMRD
- * Returns the OCS field in the UTMRD
- */
-static inline int
-ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp)
-{
- return le32_to_cpu(task_req_descp->header.dword_2) & MASK_OCS;
-}
-
/**
* ufshcd_get_tm_free_slot - get a free slot for task management request
* @hba: per adapter instance
@@ -4588,37 +4575,6 @@ 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 non-zero value on error, zero on success
- */
-static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp)
-{
- struct utp_task_req_desc *treq = hba->utmrdl_base_addr + index;
- unsigned long flags;
- int ocs_value;
-
- spin_lock_irqsave(hba->host->host_lock, flags);
-
- /* Clear completed tasks from outstanding_tasks */
- __clear_bit(index, &hba->outstanding_tasks);
-
- ocs_value = ufshcd_get_tmr_ocs(treq);
-
- if (ocs_value != OCS_SUCCESS)
- dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
- __func__, ocs_value);
- else if (resp)
- *resp = be32_to_cpu(treq->output_param1) & MASK_TM_SERVICE_RESP;
- spin_unlock_irqrestore(hba->host->host_lock, flags);
-
- return ocs_value;
-}
-
/**
* ufshcd_scsi_cmd_status - Update SCSI command result based on SCSI status
* @lrbp: pointer to local reference block of completed command
@@ -5576,27 +5532,12 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
return err;
}
-/**
- * ufshcd_issue_tm_cmd - issues task management commands to controller
- * @hba: per adapter instance
- * @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 non-zero value on error, zero on success.
- */
-static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
- u8 tm_function, u8 *tm_response)
+static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
+ struct utp_task_req_desc *treq, u8 tm_function)
{
- struct utp_task_req_desc *treq;
- struct Scsi_Host *host;
+ struct Scsi_Host *host = hba->host;
unsigned long flags;
- int free_slot;
- int err;
- int task_tag;
-
- host = hba->host;
+ int free_slot, task_tag, err;
/*
* Get free slot, sleep if slots are unavailable.
@@ -5607,24 +5548,14 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
ufshcd_hold(hba, false);
spin_lock_irqsave(host->host_lock, flags);
- treq = hba->utmrdl_base_addr + free_slot;
+ task_tag = hba->nutrs + free_slot;
/* Configure task request descriptor */
treq->header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
treq->header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+ treq->req_header.dword_0 |= cpu_to_be32(task_tag);
- /* Configure task request UPIU */
- task_tag = hba->nutrs + free_slot;
- treq->req_header.dword_0 = UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ,
- 0, lun_id, task_tag);
- treq->req_header.dword_1 = UPIU_HEADER_DWORD(0, tm_function, 0, 0);
- /*
- * The host shall provide the same value for LUN field in the basic
- * header and for Input Parameter.
- */
- treq->input_param1 = cpu_to_be32(lun_id);
- treq->input_param2 = cpu_to_be32(task_id);
-
+ memcpy(hba->utmrdl_base_addr + free_slot, treq, sizeof(*treq));
ufshcd_vops_setup_task_mgmt(hba, free_slot, tm_function);
/* send command to the controller */
@@ -5654,8 +5585,14 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
__func__, free_slot);
err = -ETIMEDOUT;
} else {
- err = ufshcd_task_req_compl(hba, free_slot, tm_response);
+ memcpy(treq, hba->utmrdl_base_addr + free_slot, sizeof(*treq));
+
ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete");
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ __clear_bit(free_slot, &hba->outstanding_tasks);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
}
clear_bit(free_slot, &hba->tm_condition);
@@ -5666,6 +5603,52 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
return err;
}
+/**
+ * uufshcd_fill_tm_reqfshcd_issue_tm_cmd - issues task management commands to controller
+ * @hba: per adapter instance
+ * @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 non-zero value on error, zero on success.
+ */
+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 treq = { { 0 }, };
+ int ocs_value, err;
+
+ /* Configure task request descriptor */
+ treq.header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
+ treq.header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+
+ /* Configure task request UPIU */
+ treq.req_header.dword_0 = cpu_to_be32(lun_id << 8) |
+ cpu_to_be32(UPIU_TRANSACTION_TASK_REQ << 24);
+ treq.req_header.dword_1 = cpu_to_be32(tm_function << 16);
+
+ /*
+ * The host shall provide the same value for LUN field in the basic
+ * header and for Input Parameter.
+ */
+ treq.input_param1 = cpu_to_be32(lun_id);
+ treq.input_param2 = cpu_to_be32(task_id);
+
+ err = __ufshcd_issue_tm_cmd(hba, &treq, tm_function);
+ if (err == -ETIMEDOUT)
+ return err;
+
+ ocs_value = le32_to_cpu(treq.header.dword_2) & MASK_OCS;
+ if (ocs_value != OCS_SUCCESS)
+ dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
+ __func__, ocs_value);
+ else if (tm_response)
+ *tm_response = be32_to_cpu(treq.output_param1) &
+ MASK_TM_SERVICE_RESP;
+ return err;
+}
+
/**
* ufshcd_eh_device_reset_handler - device reset handler registered to
* scsi layer.
--
2.18.0
