| Message ID | cover.1681764704.git.quic_nguyenb@quicinc.com |
|---|---|
| Headers | show |
| Series | ufs: core: mcq: Add ufshcd_abort() and error handler support in MCQ mode | expand |
Bao D. Nguyen <quic_nguyenb@quicinc.com> 於 2023年4月18日 週二 上午5:12寫道： > > This patch series enable support for ufshcd_abort() and error handler in MCQ mode. > The first 3 patches are for supporting ufshcd_abort(). > The 4th and 5th patches are for supporting error handler. > > Bao D. Nguyen (5): > ufs: mcq: Add supporting functions for mcq abort > ufs: mcq: Add support for clean up mcq resources > ufs: mcq: Added ufshcd_mcq_abort() > ufs: mcq: Use ufshcd_mcq_poll_cqe_lock() in mcq mode > ufs: core: Add error handling for MCQ mode > --- > v1->v2: > patch #1: Addressed Powen's comment. Replaced read_poll_timeout() > with ufshcd_mcq_poll_register(). The function read_poll_timeout() > may sleep while the caller is holding a spin_lock(). Poll the registers > in a tight loop instead. > --- > > drivers/ufs/core/ufs-mcq.c | 258 ++++++++++++++++++++++++++++++++++++++++- > drivers/ufs/core/ufshcd-priv.h | 15 ++- > drivers/ufs/core/ufshcd.c | 140 ++++++++++++++++++---- > drivers/ufs/host/ufs-qcom.c | 2 +- > include/ufs/ufshcd.h | 2 +- > include/ufs/ufshci.h | 17 +++ > 6 files changed, 404 insertions(+), 30 deletions(-) > > -- > 2.7.4 > This series looks good to me. Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
On 4/17/23 14:05, Bao D. Nguyen wrote: > + if (!lrbp->cmd) { > + dev_err(hba->dev, > + "%s: skip abort. cmd at tag %d already completed.\n", > + __func__, tag); > + goto out; > + } Please do not use lrbp->cmd to check whether or not a command has completed. > + if (ufshcd_mcq_sqe_search(hba, hwq, tag)) { > + /* > + * Failure. The command should not be "stuck" in SQ for > + * a long time which resulted in command being aborted. > + */ > + dev_err(hba->dev, "%s: cmd found in sq. hwq=%d, tag=%d\n", > + __func__, hwq->id, tag); > + /* Set the Command Type to 0xF per the spec */ > + ufshcd_mcq_nullify_cmd(hba, hwq); The above looks wrong to me. How can ufshcd_mcq_nullify_cmd() identify a command if the 'tag' argument is not passed to that function? > + /* > + * The command is not in the Submission Queue, and it is not > + * in the Completion Queue either. Query the device to see if > + * the command is being processed in the device. > + */ Please only use capitals if these are required. > + if (lrbp->cmd) > + ufshcd_release_scsi_cmd(hba, lrbp); Same comment here - do not use lrbp->cmd to check for completion. Thanks, Bart.
On 4/25/2023 5:08 PM, Bart Van Assche wrote: > On 4/17/23 14:05, Bao D. Nguyen wrote: >> @@ -3110,7 +3128,7 @@ static int ufshcd_wait_for_dev_cmd(struct >> ufs_hba *hba, >> err = -ETIMEDOUT; >> dev_dbg(hba->dev, "%s: dev_cmd request timedout, tag %d\n", >> __func__, lrbp->task_tag); >> - if (ufshcd_clear_cmds(hba, 1U << lrbp->task_tag) == 0) { >> + if (ufshcd_clear_cmds(hba, 1UL << lrbp->task_tag) == 0) { >> /* successfully cleared the command, retry if needed */ >> err = -EAGAIN; >> /* > > Is this change necessary? My intention was to support tag greater than 31 and less than 64. The 1U << only works up to 32 bits. > >> @@ -7379,6 +7397,20 @@ static int ufshcd_try_to_abort_task(struct >> ufs_hba *hba, int tag) >> */ >> dev_err(hba->dev, "%s: cmd at tag %d not pending in the >> device.\n", >> __func__, tag); >> + if (is_mcq_enabled(hba)) { >> + /* MCQ mode */ >> + if (lrbp->cmd) { >> + /* sleep for max. 200us to stabilize */ > > What is being stabilized here? Please make this comment more clear. This is to keep the same operation/timing as in SDB mode. > >> + usleep_range(100, 200); >> + continue; >> + } >> + /* command completed already */ >> + dev_err(hba->dev, "%s: cmd at tag=%d is cleared.\n", >> + __func__, tag); >> + goto out; >> + } > > Please do not use lrbp->cmd to check whether or not a command has > completed. See also my patch "scsi: ufs: Fix handling of lrbp->cmd". I have been thinking how to replace lrbp->cmd, but could not find a good solution. Throughout this patch series, I am using lrbp->cmd as a way to find the pending command that is being aborted and clean up the resources associated with it. Any suggestions to achieve it? Thanks. > >> @@ -7415,7 +7447,7 @@ static int ufshcd_try_to_abort_task(struct >> ufs_hba *hba, int tag) >> goto out; >> } >> - err = ufshcd_clear_cmds(hba, 1U << tag); >> + err = ufshcd_clear_cmds(hba, 1UL << tag); >> if (err) >> dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err >> %d\n", >> __func__, tag, err); > > Is this change necessary? Same as above, I intended to support 64 > tag > 31 > >> - if (!(test_bit(tag, &hba->outstanding_reqs))) { >> + if (!is_mcq_enabled(hba) && !(test_bit(tag, >> &hba->outstanding_reqs))) { > > Please leave out one pair of superfluous parentheses from the above > expression. Yes, I will change. > > Thanks, > > Bart.
On 5/3/23 21:01, Bao D. Nguyen wrote: > On 4/25/2023 5:08 PM, Bart Van Assche wrote: >> On 4/17/23 14:05, Bao D. Nguyen wrote: >>> @@ -3110,7 +3128,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba, >>> err = -ETIMEDOUT; >>> dev_dbg(hba->dev, "%s: dev_cmd request timedout, tag %d\n", >>> __func__, lrbp->task_tag); >>> - if (ufshcd_clear_cmds(hba, 1U << lrbp->task_tag) == 0) { >>> + if (ufshcd_clear_cmds(hba, 1UL << lrbp->task_tag) == 0) { >>> /* successfully cleared the command, retry if needed */ >>> err = -EAGAIN; >>> /* >> >> Is this change necessary? > My intention was to support tag greater than 31 and less than 64. > The 1U << only works up to 32 bits. The UFSHCI 4.0 standard and also the Linux kernel UFS driver support more than 64 outstanding commands so the above change is not sufficient. >> Please do not use lrbp->cmd to check whether or not a command has completed. See also my patch "scsi: ufs: Fix handling of lrbp->cmd". > I have been thinking how to replace lrbp->cmd, but could not find a good solution. Throughout this patch series, I am using lrbp->cmd as a way to find the pending command that is being aborted and clean up the resources associated with it. Any suggestions to achieve it? Using lrbp->cmd is fine but checking whether or not it is NULL is not OK. How about using blk_mq_request_started() to check whether or not a request is still pending? Thanks, Bart.