lkml.org 
[lkml]   [2022]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v1 1/1] scsi: ufs: core: fix device management cmd timeout flow
On 12/2/22 02:58, Mason Zhang wrote:
> From: Mason Zhang <Mason.Zhang@mediatek.com>
>
> In ufs error handler flow, host will send device management cmd(NOP OUT)
> to device for recovery link. If cmd response timeout, and clear doorbell
> fail, ufshcd_wait_for_dev_cmd will do nothing and return,
> hba->dev_cmd.complete struct not set to null.
>
> In this time, if cmd has been responsed by device, then it will
> call complete() in __ufshcd_transfer_req_compl, because of complete
> struct is alloced in stack, then the KE will occur.
>
> Fix the following crash:
> ipanic_die+0x24/0x38 [mrdump]
> die+0x344/0x748
> arm64_notify_die+0x44/0x104
> do_debug_exception+0x104/0x1e0
> el1_dbg+0x38/0x54
> el1_sync_handler+0x40/0x88
> el1_sync+0x8c/0x140
> queued_spin_lock_slowpath+0x2e4/0x3c0
> __ufshcd_transfer_req_compl+0x3b0/0x1164
> ufshcd_trc_handler+0x15c/0x308
> ufshcd_host_reset_and_restore+0x54/0x260
> ufshcd_reset_and_restore+0x28c/0x57c
> ufshcd_err_handler+0xeb8/0x1b6c
> process_one_work+0x288/0x964
> worker_thread+0x4bc/0xc7c
> kthread+0x15c/0x264
> ret_from_fork+0x10/0x30
>
> Change-Id: Id17da259894294b61bef41cf7dfb94506e7e0310

Please verify patches with checkpatch before posting these upstream. Checkpatch
will tell you that Change-Id tags must be removed before posting a patch upstream.

> Signed-off-by: Mason Zhang <Mason.Zhang@mediatek.com>
> ---
> drivers/ufs/core/ufshcd.c | 46 ++++++++++++++++++---------------------
> 1 file changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index b1f59a5fe632..2b4934a562a6 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2979,35 +2979,31 @@ 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, 1U << lrbp->task_tag) == 0)
> /* successfully cleared the command, retry if needed */
> err = -EAGAIN;
> + /*
> + * Since clearing the command succeeded we also need to
> + * clear the task tag bit from the outstanding_reqs
> + * variable.
> + */
> + spin_lock_irqsave(&hba->outstanding_lock, flags);
> + pending = test_bit(lrbp->task_tag,
> + &hba->outstanding_reqs);
> + if (pending) {
> + hba->dev_cmd.complete = NULL;
> + __clear_bit(lrbp->task_tag,
> + &hba->outstanding_reqs);
> + }
> + spin_unlock_irqrestore(&hba->outstanding_lock, flags);

I don't think it is safe to clear the corresponding bit from outstanding_reqs
if ufshcd_clear_cmds() returns a value != 0.

Instead of making all these changes, would the following patch be sufficient?

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bb4cbfe7fd57..d5deec621d2a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3008,6 +3008,9 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
} else {
dev_err(hba->dev, "%s: failed to clear tag %d\n",
__func__, lrbp->task_tag);
+ spin_lock_irqsave(&hba->outstanding_lock, flags);
+ hba->dev_cmd.complete = NULL;
+ spin_unlock_irqrestore(&hba->outstanding_lock, flags);
}
}

Thanks,

Bart.

\
 
 \ /
  Last update: 2022-12-02 19:50    [W:0.053 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site