Reviewed-by: Yaniv Gardi <ygardi@xxxxxxxxxxxxxx> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation = > -----Original Message----- = > From: linux-scsi-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi- = > owner@xxxxxxxxxxxxxxx] On Behalf Of Dolev Raviv = > Sent: Monday, August 12, 2013 4:01 PM = > To: Sujit Reddy Thumma = > Cc: Vinayak Holikatti; Santosh Y; James E.J. Bottomley; linux- = > scsi@xxxxxxxxxxxxxxx; Sujit Reddy Thumma; linux-arm- = > msm@xxxxxxxxxxxxxxx = > Subject: Re: [PATCH V5 1/4] scsi: ufs: Fix broken task management = > command implementation = > = > Tested-by: Dolev Raviv <draviv@xxxxxxxxxxxxxx> = > = > > 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@xxxxxxxxxxxxxx> = > > --- = > > drivers/scsi/ufs/ufshcd.c | 173 = > > ++++++++++++++++++++++++++++++--------------- = > > drivers/scsi/ufs/ufshcd.h | 8 ++- = > > 2 files changed, 122 insertions(+), 59 deletions(-) = > > = > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c = > > index b36ca9a..d7f3746 100644 = > > --- a/drivers/scsi/ufs/ufshcd.c = > > +++ b/drivers/scsi/ufs/ufshcd.c = > > @@ -53,6 +53,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 = > > = > > @@ -183,13 +186,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); = > > } = > > = > > /** = > > @@ -1700,10 +1725,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; @@ -1724,19 +1750,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; = > > } = > > = > > /** = > > @@ -2237,7 +2259,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); = > > } = > > = > > /** = > > @@ -2287,38 +2309,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_reqs)) = > > + 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; = > > = > > @@ -2330,18 +2372,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 = 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(lun_id); = > > + task_req_upiup->input_param2 = cpu_to_be32(task_id); = > > = > > /* send command to the controller */ = > > __set_bit(free_slot, &hba->outstanding_tasks); @@ -2350,20 = > +2389,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; = > > } = > > = > > @@ -2380,14 +2423,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) && @@ -2444,6 = > +2494,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); = > > @@ -2459,9 +2511,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); = > > = > > @@ -2682,7 +2740,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 = > > 59c9c48..fe7c947 100644 = > > --- a/drivers/scsi/ufs/ufshcd.h = > > +++ b/drivers/scsi/ufs/ufshcd.h = > > @@ -174,8 +174,10 @@ 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_condition: condition variable for task management = > > + * @tm_slots_in_use: bit map of task management request slots in use = > > * @ufshcd_state: UFSHCD states = > > * @intr_mask: Interrupt Mask Bits = > > * @ee_ctrl_mask: Exception event control mask @@ -216,8 +218,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; = > > = > > u32 ufshcd_state; = > > u32 intr_mask; = > > -- = > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a = > > member of Code Aurora Forum, hosted by The Linux Foundation. = > > = > > -- = > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" = > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More = > majordomo = > > info at http://vger.kernel.org/majordomo-info.html = > > = > = > = > -- = > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a = > member of Code Aurora Forum, hosted by The Linux Foundation = > = > -- = > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the = > body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info = > at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
