| Message ID | 1533110672-12785-5-git-send-email-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: scsi transport for ufs devices | expand |
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote: > + wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot)); The above is the weirdest API I have seen so far for tag allocation. Why does ufshcd_get_tm_free_slot() does a linear search through a bitmap instead of using the sbitmap data structure? > + ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); > + /* Make sure that doorbell is committed immediately */ > + wmb(); This is the first time that I see someone claiming that issuing a write memory barrier causes the write to be executed faster than without memory barrier. Are you sure that comment is correct and that that wmb() is necessary? > + spin_unlock_irqrestore(host->host_lock, flags); > + > + /* wait until the task management command is completed */ > + err = wait_event_timeout(hba->tm_wq, > + test_bit(free_slot, &hba->tm_condition), > + msecs_to_jiffies(TM_CMD_TIMEOUT)); Did you perhaps start implementing the ufshcd_issue_tm_upiu_cmd() function by copy/pasting ufshcd_issue_tm_cmd()? Please don't do that and instead avoid code duplication by moving shared code in a new function. > + /* ignore the returning value here - ufshcd_check_query_response is > + * bound to fail since dev_cmd.query and dev_cmd.type were left empty. > + * read the response directly ignoring all errors. > + */ Have you verified this patch with checkpatch? This is not the proper kernel comment style. Thanks, Bart.
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote: > +/** > + * ufshcd_exec_raw_upiu_cmd - API function for sending raw upiu commands > + * @hba: per-adapter instance > + * @req_upiu: upiu request - 8 dwards > + * @rsp_upiu: upiu reply - 8 dwards > + * @msgcode: message code, one of UPIU Transaction Codes Initiator to Target > + * @desc_buff: pointer to descriptor buffer, NULL if NA > + * @buff_len: descriptor size, 0 if NA > + * @rw: either READ or WRITE > + * > + * Supports UTP Transfer requests (nop and query), and UTP Task > + * Management requests. > + * It is up to the caller to fill the upiu conent properly, as it will > + * be copied without any further input validations. > + */ > +int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, > + __be32 *req_upiu, __be32 *rsp_upiu, > + int msgcode, > + u8 *desc_buff, int *buff_len, int rw) > +{ Again, what are "dwards"? Documenting the size of a data structure in a function header is very weird. Please change the data type of req_upiu and rsp_upio into a pointer to a structure of the proper size. Thanks, Bart.
Bart, thanks. > -----Original Message----- > From: Bart Van Assche > Sent: Wednesday, August 01, 2018 6:28 PM > To: hch@lst.de; Avri Altman ; linux-scsi@vger.kernel.org; > jthumshirn@suse.de; hare@suse.com; martin.petersen@oracle.com; > jejb@linux.vnet.ibm.com > Cc: Vinayak Holikatti ; Avi Shchislowski ; Alex Lemberg ; Stanislav Nijnikov ; > subhashj@codeaurora.org > Subject: Re: [PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands > > On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote: > > + wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, > &free_slot)); > > The above is the weirdest API I have seen so far for tag allocation. > Why does ufshcd_get_tm_free_slot() does a linear search through a bitmap > instead of using the sbitmap data structure? See my answer below. > > > + ufshcd_writel(hba, 1 << free_slot, > REG_UTP_TASK_REQ_DOOR_BELL); > > + /* Make sure that doorbell is committed immediately */ > > + wmb(); > > This is the first time that I see someone claiming that issuing a write memory > barrier causes the write to be executed faster than without memory barrier. > Are you sure that comment is correct and that that wmb() is necessary? See my answer below > > > + spin_unlock_irqrestore(host->host_lock, flags); > > + > > + /* wait until the task management command is completed */ > > + err = wait_event_timeout(hba->tm_wq, > > + test_bit(free_slot, &hba->tm_condition), > > + msecs_to_jiffies(TM_CMD_TIMEOUT)); > > Did you perhaps start implementing the ufshcd_issue_tm_upiu_cmd() > function by > copy/pasting ufshcd_issue_tm_cmd()? Please don't do that and instead avoid > code > duplication by moving shared code in a new function. Yes I did. I wanted to avoid changing any of the driver's core functionality, just adding the new one. And yes, this weird tagging mechanism and strange comments are just part of ufshcd_issue_tm_cmd(). I will try to do what you said. Thanks. > > > + /* ignore the returning value here - ufshcd_check_query_response is > > + * bound to fail since dev_cmd.query and dev_cmd.type were left > empty. > > + * read the response directly ignoring all errors. > > + */ > > Have you verified this patch with checkpatch? This is not the proper kernel > comment style. Yes I did. Anyway, as this patch is changing significantly, will run it again. > > Thanks, > > Bart.
Thanks, Avri > -----Original Message----- > From: Bart Van Assche > Sent: Wednesday, August 01, 2018 6:36 PM > To: hch@lst.de; Avri Altman ; linux-scsi@vger.kernel.org; > jthumshirn@suse.de; hare@suse.com; martin.petersen@oracle.com; > jejb@linux.vnet.ibm.com > Cc: Vinayak Holikatti ; Avi Shchislowski ; Alex Lemberg ; Stanislav Nijnikov ; > subhashj@codeaurora.org > Subject: Re: [PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands > > On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote: > > +/** > > + * ufshcd_exec_raw_upiu_cmd - API function for sending raw upiu > commands > > + * @hba: per-adapter instance > > + * @req_upiu: upiu request - 8 dwards > > + * @rsp_upiu: upiu reply - 8 dwards > > + * @msgcode: message code, one of UPIU Transaction Codes > Initiator to Target > > + * @desc_buff: pointer to descriptor buffer, NULL if NA > > + * @buff_len: descriptor size, 0 if NA > > + * @rw: either READ or WRITE > > + * > > + * Supports UTP Transfer requests (nop and query), and UTP Task > > + * Management requests. > > + * It is up to the caller to fill the upiu conent properly, as it will > > + * be copied without any further input validations. > > + */ > > +int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, > > + __be32 *req_upiu, __be32 *rsp_upiu, > > + int msgcode, > > + u8 *desc_buff, int *buff_len, int rw) > > +{ > > Again, what are "dwards"? > > Documenting the size of a data structure in a function header is very weird. > Please change the data type of req_upiu and rsp_upio into a pointer to a > structure of the proper size. Ok. > > Thanks, > > Bart. >
On Thu, 2018-08-02 at 11:05 +0000, Avri Altman wrote: > -----Original Message----- > > From: Bart Van Assche > > Sent: Wednesday, August 01, 2018 6:28 PM > > [ ... ] > > > + spin_unlock_irqrestore(host->host_lock, flags); > > > + > > > + /* wait until the task management command is completed */ > > > + err = wait_event_timeout(hba->tm_wq, > > > + test_bit(free_slot, &hba->tm_condition), > > > + msecs_to_jiffies(TM_CMD_TIMEOUT)); > > > > Did you perhaps start implementing the ufshcd_issue_tm_upiu_cmd() > > function by > > copy/pasting ufshcd_issue_tm_cmd()? Please don't do that and instead avoid > > code > > duplication by moving shared code in a new function. > > Yes I did. > I wanted to avoid changing any of the driver's core functionality, just adding the new one. That's not how it should be done. It is considered important in the Linux kernel to avoid code duplication so please have another look at how to avoid code duplication. Thanks, Bart.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index c2ae406..5abf697 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -257,6 +257,8 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, static irqreturn_t ufshcd_intr(int irq, void *__hba); static int ufshcd_change_power_mode(struct ufs_hba *hba, struct ufs_pa_layer_attr *pwr_mode); +static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp); + static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag) { return tag >= 0 && tag < hba->nutrs; @@ -3106,6 +3108,229 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, EXPORT_SYMBOL(ufshcd_map_desc_id_to_length); /** + * ufshcd_issue_tm_upiu_cmd - API for sending "utmrd" requests + * @hba - UFS hba + * @req_upiu - bsg request + * @rsp_upiu - bsg reply + * + * Those requests uses UTP Task Management Request Descriptor - utmrd. + * Therefore, it "rides" the task management infrastructure: uses its tag and + * tasks work queues. + */ +static int ufshcd_issue_tm_upiu_cmd(struct ufs_hba *hba, + __be32 *req_upiu, + __be32 *rsp_upiu) +{ + struct utp_task_req_desc *task_req_descp; + struct Scsi_Host *host = hba->host; + unsigned long flags; + int free_slot; + u8 tm_response = 0xF; + int err; + int task_tag; + + wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot)); + ufshcd_hold(hba, false); + + spin_lock_irqsave(host->host_lock, flags); + + task_req_descp = hba->utmrdl_base_addr; + task_req_descp += free_slot; + + 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_tag = hba->nutrs + free_slot; + + req_upiu[0] |= cpu_to_be32(task_tag); + + /* just copy the upiu request as it is */ + memcpy(task_req_descp->task_req_upiu, req_upiu, + GENERAL_UPIU_REQUEST_SIZE); + + /* send command to the controller */ + __set_bit(free_slot, &hba->outstanding_tasks); + + /* Make sure descriptors are ready before ringing the task doorbell */ + wmb(); + + ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); + /* Make sure that doorbell is committed immediately */ + wmb(); + + spin_unlock_irqrestore(host->host_lock, flags); + + /* wait until the task management command is completed */ + 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, "%s: bsg task management cmd timed-out\n", + __func__); + if (ufshcd_clear_tm_cmd(hba, free_slot)) + dev_WARN(hba->dev, + "%s: unable clear tm cmd (slot %d) timeout\n", + __func__, free_slot); + err = -ETIMEDOUT; + } else { + err = ufshcd_task_req_compl(hba, free_slot, &tm_response); + } + + /* just copy the upiu response as it is */ + memcpy(rsp_upiu, task_req_descp->task_rsp_upiu, + GENERAL_UPIU_REQUEST_SIZE); + + clear_bit(free_slot, &hba->tm_condition); + ufshcd_put_tm_slot(hba, free_slot); + wake_up(&hba->tm_tag_wq); + + ufshcd_release(hba); + return err; +} + +/** + * ufshcd_issue_devman_upiu_cmd - API for sending "utrd" type requests + * @hba: per-adapter instance + * @req_upiu: upiu request - 8 dwards + * @rsp_upiu: upiu reply - 8 dwards + * @msgcode: message code, one of UPIU Transaction Codes Initiator to Target + * @desc_buff: pointer to descriptor buffer, NULL if NA + * @buff_len: descriptor size, 0 if NA + * @rw: either READ or WRITE + * + * Those type of requests uses UTP Transfer Request Descriptor - utrd. + * Therefore, it "rides" the device management infrastructure: uses its tag and + * tasks work queues. + */ +static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, + __be32 *req_upiu, __be32 *rsp_upiu, + u8 *desc_buff, int *buff_len, + int cmd_type, int rw) +{ + struct ufshcd_lrb *lrbp; + int err = 0; + int tag; + struct completion wait; + unsigned long flags; + u32 upiu_flags; + u8 *descp; + + down_read(&hba->clk_scaling_lock); + + wait_event(hba->dev_cmd.tag_wq, ufshcd_get_dev_cmd_tag(hba, &tag)); + + init_completion(&wait); + lrbp = &hba->lrb[tag]; + WARN_ON(lrbp->cmd); + + lrbp->cmd = NULL; + lrbp->sense_bufflen = 0; + lrbp->sense_buffer = NULL; + lrbp->task_tag = tag; + lrbp->lun = 0; + lrbp->command_type = (hba->ufs_version == UFSHCI_VERSION_20) ? + UTP_CMD_TYPE_UFS_STORAGE : + UTP_CMD_TYPE_DEV_MANAGE; + lrbp->intr_cmd = true; + hba->dev_cmd.type = cmd_type; + + /* update the task tag in the request upiu */ + req_upiu[0] |= cpu_to_be32(tag); + + ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE); + + /* just copy the upiu request as it is */ + memcpy(lrbp->ucd_req_ptr, req_upiu, GENERAL_UPIU_REQUEST_SIZE); + + if (desc_buff && rw == WRITE) { + descp = (u8 *)lrbp->ucd_req_ptr + GENERAL_UPIU_REQUEST_SIZE; + memcpy(descp, desc_buff, *buff_len); + *buff_len = 0; + } + + memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp)); + + hba->dev_cmd.complete = &wait; + + /* Make sure descriptors are ready before ringing the doorbell */ + wmb(); + spin_lock_irqsave(hba->host->host_lock, flags); + ufshcd_send_command(hba, tag); + spin_unlock_irqrestore(hba->host->host_lock, flags); + + /* ignore the returning value here - ufshcd_check_query_response is + * bound to fail since dev_cmd.query and dev_cmd.type were left empty. + * read the response directly ignoring all errors. + */ + ufshcd_wait_for_dev_cmd(hba, lrbp, QUERY_REQ_TIMEOUT); + + /* just copy the upiu request as it is */ + memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, GENERAL_UPIU_REQUEST_SIZE); + + ufshcd_put_dev_cmd_tag(hba, tag); + wake_up(&hba->dev_cmd.tag_wq); + up_read(&hba->clk_scaling_lock); + return err; +} + +/** + * ufshcd_exec_raw_upiu_cmd - API function for sending raw upiu commands + * @hba: per-adapter instance + * @req_upiu: upiu request - 8 dwards + * @rsp_upiu: upiu reply - 8 dwards + * @msgcode: message code, one of UPIU Transaction Codes Initiator to Target + * @desc_buff: pointer to descriptor buffer, NULL if NA + * @buff_len: descriptor size, 0 if NA + * @rw: either READ or WRITE + * + * Supports UTP Transfer requests (nop and query), and UTP Task + * Management requests. + * It is up to the caller to fill the upiu conent properly, as it will + * be copied without any further input validations. + */ +int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, + __be32 *req_upiu, __be32 *rsp_upiu, + int msgcode, + u8 *desc_buff, int *buff_len, int rw) +{ + int err; + int cmd_type = DEV_CMD_TYPE_QUERY; + + if (desc_buff && rw == READ) { + err = -ENOTSUPP; + goto out; + } + + switch (msgcode) { + case UPIU_TRANSACTION_NOP_OUT: + cmd_type = DEV_CMD_TYPE_NOP; + /* fall through */ + case UPIU_TRANSACTION_QUERY_REQ: + ufshcd_hold(hba, false); + mutex_lock(&hba->dev_cmd.lock); + err = ufshcd_issue_devman_upiu_cmd(hba, req_upiu, rsp_upiu, + desc_buff, buff_len, + cmd_type, rw); + mutex_unlock(&hba->dev_cmd.lock); + ufshcd_release(hba); + + break; + case UPIU_TRANSACTION_TASK_REQ: + err = ufshcd_issue_tm_upiu_cmd(hba, req_upiu, rsp_upiu); + + break; + default: + err = -EINVAL; + + break; + } + +out: + return err; +} + +/** * ufshcd_read_desc_param - read the specified descriptor parameter * @hba: Pointer to adapter instance * @desc_id: descriptor idn value diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 33fdd3f..1895834 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -892,6 +892,10 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id, u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba); +int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, + __be32 *req_upiu, __be32 *rsp_upiu, int msgcode, + u8 *desc_buff, int *buff_len, int rw); + /* Wrapper functions for safely calling variant operations */ static inline const char *ufshcd_get_var_name(struct ufs_hba *hba) {
The UFS host software uses a combination of a host register set, and Transfer Request Descriptors in system memory to communicate with host controller hardware. In its mmio space, a separate places are assigned to UTP Transfer Request Descriptor ("utrd") list, and to UTP Task Management Request Descriptor ("utmrd") list. The provided API supports utrd-typed requests: nop out and device management commands. It also supports utmrd-type requests: task management requests. Other UPIU types are not supported for now. We utilize the already existing code for tag and task work queues. That is, all utrd-typed UPIUs are "disguised" as device management commands. Similarly, the utmrd-typed UPUIs uses the task management infrastructure. It is up to the caller to fill the upiu request properly, as it will be copied without any further input validations. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshcd.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/scsi/ufs/ufshcd.h | 4 + 2 files changed, 229 insertions(+)