| Message ID | 5e662692bc0ad5108ce91ae3d1ec2b575c34d4ae.1681764704.git.quic_nguyenb@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ufs: core: mcq: Add ufshcd_abort() and error handler support in MCQ mode | expand |
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? > @@ -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. > + 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". > @@ -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? > - 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. 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.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 808387c..ffccb91 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -3007,10 +3007,28 @@ static int ufshcd_compose_dev_cmd(struct ufs_hba *hba, * @mask and wait until the controller confirms that these requests have been * cleared. */ -static int ufshcd_clear_cmds(struct ufs_hba *hba, u32 mask) +static int ufshcd_clear_cmds(struct ufs_hba *hba, unsigned long mask) { unsigned long flags; + int err, tag, result = FAILED; + if (is_mcq_enabled(hba)) { + /* + * MCQ mode. Clean up the MCQ resources similar to + * what the ufshcd_utrl_clear() does for SDB mode. + */ + for_each_set_bit(tag, &mask, hba->nutrs) { + err = ufshcd_mcq_sq_cleanup(hba, tag, &result); + if (err || result) { + dev_err(hba->dev, "%s: failed tag=%d. err=%d, result=%d\n", + __func__, tag, err, result); + return FAILED; + } + } + return 0; + } + + /* Single Doorbell Mode */ /* clear outstanding transaction before retry */ spin_lock_irqsave(hba->host->host_lock, flags); ufshcd_utrl_clear(hba, mask); @@ -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; /* @@ -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 */ + usleep_range(100, 200); + continue; + } + /* command completed already */ + dev_err(hba->dev, "%s: cmd at tag=%d is cleared.\n", + __func__, tag); + goto out; + } + + /* Single Doorbell Mode */ reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); if (reg & (1 << tag)) { /* sleep for max. 200us to stabilize */ @@ -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); @@ -7445,8 +7477,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); @@ -7475,7 +7507,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) } hba->req_abort_count++; - if (!(reg & (1 << tag))) { + if (!is_mcq_enabled(hba) && !(reg & (1 << tag))) { + /* only execute this code in single doorbell mode */ dev_err(hba->dev, "%s: cmd was completed, but without a notifying intr, tag = %d", __func__, tag);
Update ufshcd_clear_cmds() to clean up the mcq resources similar to the function ufshcd_utrl_clear() does for sdb mode. Update ufshcd_try_to_abort_task() to support mcq mode so that this function can be invoked in either mcq or sdb mode. Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com> --- drivers/ufs/core/ufshcd.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)