From: Peter Wang peter.wang@mediatek.com
MCQ aborts a command using two methods below: 1. Nullified UTRD, Host controller skips this transfer request, reply Completion Queue entry to Host SW with OCS=ABORTED 2. SQ cleanup, The host controller will post to the Completion Queue to update the OCS field with ABORTED.
SDB aborts a command using UTRLCLR. Task Management response which means a Transfer Request was aborted.
For these three cases, set a flag to notify SCSI to requeue the command after receiving response with OCS_ABORTED.
Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode") Cc: stable@vger.kernel.org Signed-off-by: Peter Wang peter.wang@mediatek.com --- drivers/ufs/core/ufs-mcq.c | 1 + drivers/ufs/core/ufshcd.c | 29 +++++++++++++++-------------- include/ufs/ufshcd.h | 2 ++ 3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 3903947dbed1..1f57ebf24a39 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -642,6 +642,7 @@ static bool ufshcd_mcq_sqe_search(struct ufs_hba *hba, match = le64_to_cpu(utrd->command_desc_base_addr) & CQE_UCD_BA; if (addr == match) { ufshcd_mcq_nullify_sqe(utrd); + lrbp->abort_initiated_by_err = true; ret = true; goto out; } diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index a6f818cdef0e..83a06b0730ea 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -3006,6 +3006,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) ufshcd_prepare_lrbp_crypto(scsi_cmd_to_rq(cmd), lrbp);
lrbp->req_abort_skip = false; + lrbp->abort_initiated_by_err = false;
ufshcd_comp_scsi_upiu(hba, lrbp);
@@ -5404,7 +5405,10 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp, } break; case OCS_ABORTED: - result |= DID_ABORT << 16; + if (lrbp->abort_initiated_by_err) + result |= DID_REQUEUE << 16; + else + result |= DID_ABORT << 16; break; case OCS_INVALID_COMMAND_STATUS: result |= DID_REQUEUE << 16; @@ -6472,25 +6476,12 @@ static bool ufshcd_abort_one(struct request *rq, void *priv) struct Scsi_Host *shost = sdev->host; struct ufs_hba *hba = shost_priv(shost); struct ufshcd_lrb *lrbp = &hba->lrb[tag]; - struct ufs_hw_queue *hwq; - unsigned long flags;
*ret = ufshcd_try_to_abort_task(hba, tag); dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag, hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1, *ret ? "failed" : "succeeded");
- /* Release cmd in MCQ mode if abort succeeds */ - if (hba->mcq_enabled && (*ret == 0)) { - hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(lrbp->cmd)); - if (!hwq) - return 0; - spin_lock_irqsave(&hwq->cq_lock, flags); - if (ufshcd_cmd_inflight(lrbp->cmd)) - ufshcd_release_scsi_cmd(hba, lrbp); - spin_unlock_irqrestore(&hwq->cq_lock, flags); - } - return *ret == 0; }
@@ -7561,6 +7552,16 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) goto out; }
+ /* + * When the host software receives a "FUNCTION COMPLETE", set flag + * to requeue command after receive response with OCS_ABORTED + * SDB mode: UTRLCLR Task Management response which means a Transfer + * Request was aborted. + * MCQ mode: Host will post to CQ with OCS_ABORTED after SQ cleanup + */ + if (!err) + lrbp->abort_initiated_by_err = true; + err = ufshcd_clear_cmd(hba, tag); if (err) dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n", diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 0fd2aebac728..4edeabcd3e88 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -173,6 +173,7 @@ struct ufs_pm_lvl_states { * @crypto_key_slot: the key slot to use for inline crypto (-1 if none) * @data_unit_num: the data unit number for the first block for inline crypto * @req_abort_skip: skip request abort task flag + * @abort_initiated_by_err: abort initiated by error */ struct ufshcd_lrb { struct utp_transfer_req_desc *utr_descriptor_ptr; @@ -202,6 +203,7 @@ struct ufshcd_lrb { #endif
bool req_abort_skip; + bool abort_initiated_by_err; };
/**
On 9/9/24 1:21 AM, peter.wang@mediatek.com wrote:
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 3903947dbed1..1f57ebf24a39 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -642,6 +642,7 @@ static bool ufshcd_mcq_sqe_search(struct ufs_hba *hba, match = le64_to_cpu(utrd->command_desc_base_addr) & CQE_UCD_BA; if (addr == match) { ufshcd_mcq_nullify_sqe(utrd);
}lrbp->abort_initiated_by_err = true; ret = true; goto out;
As mentioned before, I think that this change is wrong. Setting lrbp->abort_initiated_by_err affects the value of scsi_cmnd::result. This member variable is ignored for aborted commands. Although the above change does not affect the SCSI error handler, I think it should be left out because it will confuse anyone who reads the UFS driver code and who has not followed this discussion.
@@ -7561,6 +7552,16 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) goto out; }
- /*
* When the host software receives a "FUNCTION COMPLETE", set flag
* to requeue command after receive response with OCS_ABORTED
* SDB mode: UTRLCLR Task Management response which means a Transfer
* Request was aborted.
* MCQ mode: Host will post to CQ with OCS_ABORTED after SQ cleanup
*/
- if (!err)
lrbp->abort_initiated_by_err = true;
Please add a comment that explains that the purpose of this code is to requeue commands aborted by ufshcd_abort_all().
- @abort_initiated_by_err: abort initiated by error
The member variable name and also the explanation of this member variable are incomprehensible to me.
Thanks,
Bart.
On Mon, 2024-09-09 at 09:34 -0700, Bart Van Assche wrote:
External email : Please do not click links or open attachments until you have verified the sender or the content. On 9/9/24 1:21 AM, peter.wang@mediatek.com wrote:
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-
mcq.c
index 3903947dbed1..1f57ebf24a39 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -642,6 +642,7 @@ static bool ufshcd_mcq_sqe_search(struct
ufs_hba *hba,
match = le64_to_cpu(utrd->command_desc_base_addr) & CQE_UCD_BA; if (addr == match) { ufshcd_mcq_nullify_sqe(utrd); +lrbp->abort_initiated_by_err = true; ret = true; goto out; }
As mentioned before, I think that this change is wrong. Setting lrbp->abort_initiated_by_err affects the value of scsi_cmnd::result. This member variable is ignored for aborted commands. Although the above change does not affect the SCSI error handler, I think it should be left out because it will confuse anyone who reads the UFS driver code and who has not followed this discussion.
Hi Bart,
Since this is unlikely to happen and the code is not expected to reach here, I think it's better to wait and see if it occurs. And open a discussion on how to deal with this case if occurs. I will remove it in the next version.
@@ -7561,6 +7552,16 @@ int ufshcd_try_to_abort_task(struct ufs_hba
*hba, int tag)
goto out; } +/*
- When the host software receives a "FUNCTION COMPLETE", set flag
- to requeue command after receive response with OCS_ABORTED
- SDB mode: UTRLCLR Task Management response which means a
Transfer
Request was aborted.
- MCQ mode: Host will post to CQ with OCS_ABORTED after SQ
cleanup
- */
+if (!err) +lrbp->abort_initiated_by_err = true;
Please add a comment that explains that the purpose of this code is to requeue commands aborted by ufshcd_abort_all().
Okay, I will add more comment.
- @abort_initiated_by_err: abort initiated by error
The member variable name and also the explanation of this member variable are incomprehensible to me.
I will add more flag description too.
Thanks. Peter
Thanks,
Bart.
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jejb-scsi/for-next] [also build test WARNING on mkp-scsi/for-next linus/master v6.11-rc7 next-20240909] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/peter-wang-mediatek-com/ufs-c... base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next patch link: https://lore.kernel.org/r/20240909082100.24019-3-peter.wang%40mediatek.com patch subject: [PATCH v3 2/2] ufs: core: requeue aborted request config: i386-buildonly-randconfig-002-20240910 (https://download.01.org/0day-ci/archive/20240910/202409100826.Iql38Ss9-lkp@i...) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240910/202409100826.Iql38Ss9-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202409100826.Iql38Ss9-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/ufs/core/ufshcd.c: In function 'ufshcd_abort_one':
drivers/ufs/core/ufshcd.c:6491:28: warning: unused variable 'lrbp' [-Wunused-variable]
6491 | struct ufshcd_lrb *lrbp = &hba->lrb[tag]; | ^~~~
vim +/lrbp +6491 drivers/ufs/core/ufshcd.c
2355b66ed20ce4 drivers/scsi/ufs/ufshcd.c Can Guo 2020-08-24 6482 f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6483 static bool ufshcd_abort_one(struct request *rq, void *priv) b817e6ffbad7a1 drivers/ufs/core/ufshcd.c Bart Van Assche 2022-10-31 6484 { f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6485 int *ret = priv; f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6486 u32 tag = rq->tag; f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6487 struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6488 struct scsi_device *sdev = cmd->device; f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6489 struct Scsi_Host *shost = sdev->host; f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6490 struct ufs_hba *hba = shost_priv(shost); 93e6c0e19d5bb1 drivers/ufs/core/ufshcd.c Peter Wang 2023-11-15 @6491 struct ufshcd_lrb *lrbp = &hba->lrb[tag]; ab248643d3d68b drivers/ufs/core/ufshcd.c Bao D. Nguyen 2023-05-29 6492 f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6493 *ret = ufshcd_try_to_abort_task(hba, tag); ab248643d3d68b drivers/ufs/core/ufshcd.c Bao D. Nguyen 2023-05-29 6494 dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag, ab248643d3d68b drivers/ufs/core/ufshcd.c Bao D. Nguyen 2023-05-29 6495 hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1, f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6496 *ret ? "failed" : "succeeded"); 93e6c0e19d5bb1 drivers/ufs/core/ufshcd.c Peter Wang 2023-11-15 6497 f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6498 return *ret == 0; ab248643d3d68b drivers/ufs/core/ufshcd.c Bao D. Nguyen 2023-05-29 6499 } f9c028e7415a5b drivers/ufs/core/ufshcd.c Bart Van Assche 2023-07-27 6500
linux-stable-mirror@lists.linaro.org