| Message ID | 1535970796-25582-4-git-send-email-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs bsg endpoint | expand |
In general this looks good, but a question below: > index ed37914..d18832a 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -5598,6 +5598,32 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag) > return err; > } > > +static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, > + int lun_id, int task_id, u8 tm_function, > + int task_tag) > +{ > + struct utp_upiu_task_req *task_req_upiup; > + > + /* 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); > + > + task_req_upiup = > + (struct utp_upiu_task_req *)task_req_descp->task_req_upiu; Why is task_req_upiu a __le32 array instead of using the proper structure? Doing that would nicely clean up the code: static void ufshcd_fill_tm_req(struct utp_task_req_desc *treq, int lun_id, int task_id, u8 tm_function, int task_tag) { /* 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->task_req_upiup->header.dword_0 = UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0, lun_id, task_tag); treq->task_req_upiup->header.dword_1 = UPIU_HEADER_DWORD(0, tm_function, 0, 0); treq->task_req_upiup->input_param1 = cpu_to_be32(lun_id); treq->task_req_upiup->input_param2 = cpu_to_be32(task_id); }
> -----Original Message----- > From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> > On Behalf Of Christoph Hellwig > Sent: Tuesday, September 04, 2018 10:12 PM > To: Avri Altman <Avri.Altman@wdc.com> > Cc: Christoph Hellwig <hch@lst.de>; Johannes Thumshirn > <jthumshirn@suse.de>; Hannes Reinecke <hare@suse.com>; Bart Van Assche > <Bart.VanAssche@wdc.com>; James E.J. Bottomley > <jejb@linux.vnet.ibm.com>; Martin K. Petersen > <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; Stanislav Nijnikov > <Stanislav.Nijnikov@wdc.com>; Avi Shchislowski > <Avi.Shchislowski@wdc.com>; Alex Lemberg <Alex.Lemberg@wdc.com>; > Subhash Jadavani <subhashj@codeaurora.org>; Vinayak Holikatti > <Vinayak.Holikatti@wdc.com> > Subject: Re: [PATCH v3 3/7] scsi: ufs: Add fill task management request > > In general this looks good, but a question below: > > > index ed37914..d18832a 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -5598,6 +5598,32 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba > *hba, int tag) > > return err; > > } > > > > +static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, > > + int lun_id, int task_id, u8 tm_function, > > + int task_tag) > > +{ > > + struct utp_upiu_task_req *task_req_upiup; > > + > > + /* 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); > > + > > + task_req_upiup = > > + (struct utp_upiu_task_req *)task_req_descp->task_req_upiu; > > > Why is task_req_upiu a __le32 array instead of using the proper > structure? Looking into the UFSHCI spec (JESD223C March 2016) paragraph 6.2, It doesn't specify any inner structure of the task management request or response, just a bunch of 8 DW each. I guess this is why it is defined as a __le32 array. So the host controller is not aware of the inner structure of this Sequence, and probably shouldn't. Making it aware of that, e.g. by moving utp_upiu_task_req and utp_upiu_task_rsp from ufs.h to ufshci.h will break this abstraction. > > Doing that would nicely clean up the code: > > static void ufshcd_fill_tm_req(struct utp_task_req_desc *treq, > int lun_id, int task_id, u8 tm_function, > int task_tag) > { > /* 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->task_req_upiup->header.dword_0 = > UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0, > lun_id, > task_tag); > treq->task_req_upiup->header.dword_1 = > UPIU_HEADER_DWORD(0, tm_function, 0, 0); > treq->task_req_upiup->input_param1 = cpu_to_be32(lun_id); > treq->task_req_upiup->input_param2 = cpu_to_be32(task_id); > }
On Wed, Sep 05, 2018 at 07:30:03AM +0000, Avri Altman wrote: > Looking into the UFSHCI spec (JESD223C March 2016) paragraph 6.2, > It doesn't specify any inner structure of the task management > request or response, just a bunch of 8 DW each. > I guess this is why it is defined as a __le32 array. > > So the host controller is not aware of the inner structure of this > Sequence, and probably shouldn't. Making it aware of that, > e.g. by moving utp_upiu_task_req and utp_upiu_task_rsp > from ufs.h to ufshci.h will break this abstraction. Well, then just kill struct utp_upiu_task_req entirely and use the dwords form the spec.
> On Wed, Sep 05, 2018 at 07:30:03AM +0000, Avri Altman wrote: > > Looking into the UFSHCI spec (JESD223C March 2016) paragraph 6.2, > > It doesn't specify any inner structure of the task management > > request or response, just a bunch of 8 DW each. > > I guess this is why it is defined as a __le32 array. > > > > So the host controller is not aware of the inner structure of this > > Sequence, and probably shouldn't. Making it aware of that, > > e.g. by moving utp_upiu_task_req and utp_upiu_task_rsp > > from ufs.h to ufshci.h will break this abstraction. > > Well, then just kill struct utp_upiu_task_req entirely and use > the dwords form the spec. 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. So just removing it, replacing it with an opaque array is pointless. Trying to understand why this is happening for task management requests, But not for device management requests, I came to realized that you were originally right Saying: " I think someone needs to untangle how the data structures are used in this part of the code." Unlike utrd-typed requests (scsi & device management), that have their memory space properly configured (utrdl_base_addr in ufshcd_host_memory_configure()), task management requests - utmrd-typed requests (utmrdl_base_addr) do not get similar attention. Will fix that in a separate patch prior to this one. Thanks, Avri
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.
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@lst.de> 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@lst.de> --- 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 */
Ok thanks. Will test and those as the first 2 patches in the series. Thanks a lot, Avri > -----Original Message----- > From: Christoph Hellwig <hch@lst.de> > Sent: Wednesday, September 05, 2018 8:15 PM > To: Avri Altman <Avri.Altman@wdc.com> > Cc: Christoph Hellwig <hch@lst.de>; Johannes Thumshirn > <jthumshirn@suse.de>; Hannes Reinecke <hare@suse.com>; Bart Van Assche > <Bart.VanAssche@wdc.com>; James E.J. Bottomley > <jejb@linux.vnet.ibm.com>; Martin K. Petersen > <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; Stanislav Nijnikov > <Stanislav.Nijnikov@wdc.com>; Avi Shchislowski > <Avi.Shchislowski@wdc.com>; Alex Lemberg <Alex.Lemberg@wdc.com>; > Subhash Jadavani <subhashj@codeaurora.org>; Vinayak Holikatti > <Vinayak.Holikatti@wdc.com> > Subject: Re: [PATCH v3 3/7] scsi: ufs: Add fill task management request > > 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.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index ed37914..d18832a 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5598,6 +5598,32 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag) return err; } +static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, + int lun_id, int task_id, u8 tm_function, + int task_tag) +{ + struct utp_upiu_task_req *task_req_upiup; + + /* 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); + + task_req_upiup = + (struct utp_upiu_task_req *)task_req_descp->task_req_upiu; + 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); + /* + * 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); +} + /** * ufshcd_issue_tm_cmd - issues task management commands to controller * @hba: per adapter instance @@ -5612,7 +5638,6 @@ 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; @@ -5632,27 +5657,10 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id, spin_lock_irqsave(host->host_lock, flags); task_req_descp = hba->utmrdl_base_addr; task_req_descp += 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); - - /* 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); - /* - * 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); + + ufshcd_fill_tm_req(task_req_descp, lun_id, task_id, tm_function, + task_tag); ufshcd_vops_setup_task_mgmt(hba, free_slot, tm_function);
Do that in preparation to re-use ufshcd_issue_tm_cmd code. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshcd.c | 50 +++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 21 deletions(-)