Message ID | cover.1683688692.git.quic_nguyenb@quicinc.com |
---|---|
Headers | show |
Series | ufs: core: mcq: Add ufshcd_abort() and error handler support in MCQ mode | expand |
On 5/9/23 22:24, Bao D. Nguyen wrote: > The utp command descriptor base address is a 57-bit field in the utp > utp transfer request descriptor. Combine the two 32-bit > command_desc_base_addr_lo/hi fields into a 64-bit for better handling > of this field. The "utp utp" in the patch description probably should be changed into "UTP"? Otherwise this patch looks fine to me. Hence: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 5/9/23 22:24, Bao D. Nguyen wrote: > +/** > + * ufshcd_mcq_sq_cleanup - Clean up submission queue resources > + * associated with the pending command. > + * @hba - per adapter instance. > + * @task_tag - The command's task tag. > + * @result - Result of the clean up operation. > + * > + * Returns 0 and result on completion. Returns error code if > + * the operation fails. > + */ > +int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag, int *result) > +{ > + struct ufshcd_lrb *lrbp = &hba->lrb[task_tag]; > + struct scsi_cmnd *cmd = lrbp->cmd; > + struct ufs_hw_queue *hwq; > + void __iomem *reg, *opr_sqd_base; > + u32 nexus, id, val; > + int err; > + > + if (task_tag != hba->nutrs - UFSHCD_NUM_RESERVED) { > + if (!cmd) > + return FAILED; > + hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd)); > + } else { > + hwq = hba->dev_cmd_queue; > + } > + > + id = hwq->id; > + > + mutex_lock(&hwq->sq_mutex); > + > + /* stop the SQ fetching before working on it */ > + err = ufshcd_mcq_sq_stop(hba, hwq); > + if (err) > + goto unlock; > + > + /* SQCTI = EXT_IID, IID, LUN, Task Tag */ > + nexus = lrbp->lun << 8 | task_tag; > + opr_sqd_base = mcq_opr_base(hba, OPR_SQD, id); > + writel(nexus, opr_sqd_base + REG_SQCTI); > + > + /* SQRTCy.ICU = 1 */ > + writel(SQ_ICU, opr_sqd_base + REG_SQRTC); > + > + /* Poll SQRTSy.CUS = 1. Return result from SQRTSy.RTC */ > + reg = opr_sqd_base + REG_SQRTS; > + err = read_poll_timeout(readl, val, val & SQ_CUS, 20, > + MCQ_POLL_US, false, reg); > + if (err) > + dev_err(hba->dev, "%s: failed. hwq=%d, lun=0x%x, tag=%d\n", > + __func__, id, lrbp->lun, task_tag); > + > + *result = FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg)); > + > + if (ufshcd_mcq_sq_start(hba, hwq)) > + err = FAILED; > + > +unlock: > + mutex_unlock(&hwq->sq_mutex); > + return err; > +} Please do not use the FAILED / SUCCESS return values in this function. These values should only be returned by functions related to SCSI error handling. Please do the following: * Return a negative Unix error code in case of failure. * Return zero upon success. * Return the FIELD_GET() result as a positive value. * Remove the 'int *result' argument. > + /* prevent concurrent access to sq hw */ > + struct mutex sq_mutex; Hmm ... a submission queue (SQ) exists in host memory so I'm not sure the "hw" part of the above comment is correct. Thanks, Bart.
On 5/9/23 22:24, Bao D. Nguyen wrote: > +bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd) > +{ > + struct request *rq; > + > + if (!cmd) > + return false; > + > + rq = scsi_cmd_to_rq(cmd); > + if (!rq || !blk_mq_request_started(rq)) > + return false; > + > + return true; The return value of scsi_cmd_to_rq() is never NULL so please remove the !rq test. > @@ -7450,8 +7499,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > > ufshcd_hold(hba, false); > reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); > - /* If command is already aborted/completed, return FAILED. */ > - if (!(test_bit(tag, &hba->outstanding_reqs))) { > + if (!is_mcq_enabled(hba) && !test_bit(tag, &hba->outstanding_reqs)) { > + /* If command is already aborted/completed, return FAILED. */ > dev_err(hba->dev, > "%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n", > __func__, tag, hba->outstanding_reqs, reg); With the above change, the doorbell register is read even in MCQ mode. Shouldn't reading the doorbell register be skipped in MCQ mode? Thanks, Bart.