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 <[email protected]>
---
drivers/ufs/core/ufshcd.c | 45 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 39 insertions(+), 6 deletions(-)
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);
--
2.7.4
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.