| Message ID | 1535970796-25582-5-git-send-email-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs bsg endpoint | expand |
On Mon, Sep 03, 2018 at 01:33:13PM +0300, Avri Altman wrote: > Do that in order to re-use its code if the task request and response > UPIUs are given externally. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshcd.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index d18832a..15be103 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -5599,6 +5599,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag) > } > > static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, > + struct utp_upiu_task_req *raw_task_req_upiup, > int lun_id, int task_id, u8 tm_function, > int task_tag) > { > @@ -5609,6 +5610,13 @@ static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, > task_req_descp->header.dword_2 = > cpu_to_le32(OCS_INVALID_COMMAND_STATUS); > > + if (raw_task_req_upiup) { > + raw_task_req_upiup->header.dword_0 |= cpu_to_be32(task_tag); > + memcpy(task_req_descp->task_req_upiu, raw_task_req_upiup, > + GENERAL_UPIU_REQUEST_SIZE); > + return; > + } The only thing actually shared here is initializing two fields. Why can't we always pass in the a raw > +static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, > + struct utp_upiu_task_req *raw_task_req_upiup, > + struct utp_upiu_task_req *raw_task_rsp_upiup, > + int lun_id, int task_id, u8 tm_function, > + u8 *tm_response) > { And the architecture here seems similar odd. I think someone needs to untangle how the data structures are used in this part of the code. E.g. aways prepare a raw structure in the caller (possibly on stack) and pass it into ufshcd_issue_tm_cmd, which copies it into the DMAable region.
> > --- > > drivers/scsi/ufs/ufshcd.c | 35 ++++++++++++++++++++++++++--------- > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index d18832a..15be103 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -5599,6 +5599,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba > *hba, int tag) > > } > > > > static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, > > + struct utp_upiu_task_req *raw_task_req_upiup, > > int lun_id, int task_id, u8 tm_function, > > int task_tag) > > { > > @@ -5609,6 +5610,13 @@ static void ufshcd_fill_tm_req(struct > utp_task_req_desc *task_req_descp, > > task_req_descp->header.dword_2 = > > cpu_to_le32(OCS_INVALID_COMMAND_STATUS); > > > > + if (raw_task_req_upiup) { > > + raw_task_req_upiup->header.dword_0 |= > cpu_to_be32(task_tag); > > + memcpy(task_req_descp->task_req_upiu, > raw_task_req_upiup, > > + GENERAL_UPIU_REQUEST_SIZE); > > + return; > > + } > > The only thing actually shared here is initializing two fields. Why > can't we always pass in the a raw See below > > > +static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, > > + struct utp_upiu_task_req *raw_task_req_upiup, > > + struct utp_upiu_task_req *raw_task_rsp_upiup, > > + int lun_id, int task_id, u8 tm_function, > > + u8 *tm_response) > > { > > And the architecture here seems similar odd. I think someone needs > to untangle how the data structures are used in this part of the code. > > E.g. aways prepare a raw structure in the caller (possibly on stack) > and pass it into ufshcd_issue_tm_cmd, which copies it into the > DMAable region. Task management is sent on device reset and task abort. This is actually a rare event, and on some platforms, I find it quite difficult to trigger. So preparing the request upiu as a separate step seems a little bit excessive. Another reason why it is a good idea to do it together, Is that both preparing the task request upiu and ringing its doorbell are done under the same host lock. We can however, as you proposed, prepare a "raw" upiu if none is given, But I think we'll end up with the same amount of code, Maybe even less obvious than filling the upiu in one place.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index d18832a..15be103 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5599,6 +5599,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag) } static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, + struct utp_upiu_task_req *raw_task_req_upiup, int lun_id, int task_id, u8 tm_function, int task_tag) { @@ -5609,6 +5610,13 @@ static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, task_req_descp->header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS); + if (raw_task_req_upiup) { + raw_task_req_upiup->header.dword_0 |= cpu_to_be32(task_tag); + memcpy(task_req_descp->task_req_upiu, raw_task_req_upiup, + GENERAL_UPIU_REQUEST_SIZE); + return; + } + task_req_upiup = (struct utp_upiu_task_req *)task_req_descp->task_req_upiu; task_req_upiup->header.dword_0 = @@ -5634,8 +5642,11 @@ static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp, * * 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_upiu_task_req *raw_task_req_upiup, + struct utp_upiu_task_req *raw_task_rsp_upiup, + int lun_id, int task_id, u8 tm_function, + u8 *tm_response) { struct utp_task_req_desc *task_req_descp; struct Scsi_Host *host; @@ -5659,8 +5670,8 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id, task_req_descp += free_slot; task_tag = hba->nutrs + free_slot; - ufshcd_fill_tm_req(task_req_descp, lun_id, task_id, tm_function, - task_tag); + ufshcd_fill_tm_req(task_req_descp, raw_task_req_upiup, lun_id, task_id, + tm_function, task_tag); ufshcd_vops_setup_task_mgmt(hba, free_slot, tm_function); @@ -5692,6 +5703,10 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id, err = -ETIMEDOUT; } else { err = ufshcd_task_req_compl(hba, free_slot, tm_response); + if (raw_task_rsp_upiup) + memcpy(raw_task_rsp_upiup, + task_req_descp->task_rsp_upiu, + GENERAL_UPIU_REQUEST_SIZE); ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete"); } @@ -5726,7 +5741,8 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd) tag = cmd->request->tag; lrbp = &hba->lrb[tag]; - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp); + err = ufshcd_issue_tm_cmd(hba, NULL, NULL, lrbp->lun, 0, + UFS_LOGICAL_RESET, &resp); if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { if (!err) err = resp; @@ -5856,8 +5872,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) } for (poll_cnt = 100; poll_cnt; poll_cnt--) { - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, - UFS_QUERY_TASK, &resp); + err = ufshcd_issue_tm_cmd(hba, NULL, NULL, lrbp->lun, + lrbp->task_tag, UFS_QUERY_TASK, + &resp); if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { /* cmd pending in the device */ dev_err(hba->dev, "%s: cmd pending in the device. tag = %d\n", @@ -5895,8 +5912,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) goto out; } - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, - UFS_ABORT_TASK, &resp); + err = ufshcd_issue_tm_cmd(hba, NULL, NULL, lrbp->lun, lrbp->task_tag, + UFS_ABORT_TASK, &resp); if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { if (!err) { err = resp; /* service response error */
Do that in order to re-use its code if the task request and response UPIUs are given externally. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshcd.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)