ufshcd_eh_timed_out() calls ufshcd_link_recovery() when a command times out during a power management operation. Link recovery sends a NOP OUT, which needs hba->dev_cmd.lock and the only reserved tag. 1. Runtime resume sends a query with the reserved tag while it holds hba->dev_cmd.lock. 2. The device does not answer and the query times out. 3. ufshcd_eh_timed_out() calls ufshcd_link_recovery(). 4. In MCQ mode ufshcd_verify_dev_init() waits for hba->dev_cmd.lock while __ufshcd_wl_resume() waits for the query. Resume hangs. 5. In legacy mode the query completes but the timeout handler still holds the reserved tag. The NOP OUT fails with -ENOMEM and the host enters UFSHCD_STATE_ERROR. Stop the host controller in ufshcd_eh_timed_out() and recover the link in ufshcd_set_dev_pwr_mode(), which issues START STOP UNIT without hba->dev_cmd.lock or the reserved tag. For modes other than active, ufshcd_set_dev_pwr_mode() then fails and the caller aborts the power transition. Recovering the link there needs four more changes. First, START STOP UNIT is completed with DID_TRANSPORT_FAILFAST and is not retried. ufshcd_set_dev_pwr_mode() also recovers the link if START STOP UNIT succeeds but the controller is disabled. Hosts with UFSHCI_QUIRK_BROKEN_HCE do not use the HCE bit, so ufshcd_set_dev_pwr_mode() checks only the timeout result on them. Second, in MCQ mode ufshcd_eh_timed_out() reaps the completion queue before it completes the command, so a late completion cannot complete the tag twice. Third, a timed-out reserved request forces ufshcd_err_handler(), because no power management caller recovers after it. Fourth, in MCQ mode ufshcd_err_handler() does not poll the completion queues while the controller is disabled. Fixes: 01d5e237b339 ("scsi: ufs: core: Handle PM commands timeout before SCSI EH") Cc: stable@vger.kernel.org # 7.2.x Suggested-by: Bart Van Assche Signed-off-by: Stanley Jhu --- drivers/ufs/core/ufshcd.c | 43 ++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index ddc953b45ce9..0f5365bd90f0 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -7108,8 +7108,12 @@ static void ufshcd_err_handler(struct work_struct *work) ufshcd_set_eh_in_progress(hba); spin_unlock_irqrestore(&hba->host->host_lock, flags); - /* Complete requests that have door-bell cleared by h/w */ - ufshcd_complete_requests(hba, false); + /* + * Complete requests that have door-bell cleared by h/w. + * CQTPy must not be read while the controller is disabled. + */ + if (!hba->mcq_enabled || ufshcd_is_hba_active(hba)) + ufshcd_complete_requests(hba, false); spin_lock_irqsave(&hba->host->host_lock, flags); again: needs_restore = false; @@ -9660,17 +9664,20 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd) } /* - * Handle the timeout directly to prevent a deadlock between - * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler(). + * Stop the host controller so that @scmd can be completed here. + * ufshcd_set_dev_pwr_mode() recovers the link for START STOP + * UNIT. ufshcd_err_handler() recovers it for other commands. */ - ufshcd_link_recovery(hba); - dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n", - __func__, hba->outstanding_tasks); + ufshcd_hba_stop(hba); + if (!ufshcd_is_scsi_cmd(scmd)) + ufshcd_force_error_recovery(hba); + if (hba->mcq_enabled) { + struct ufs_hw_queue *hwq = + ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(scmd)); + + ufshcd_mcq_compl_all_cqes_lock(hba, hwq); + } - /* - * ufshcd_link_recovery() may already have completed @scmd, e.g. via - * the existing MCQ force-completion path. - */ if (!test_bit(SCMD_STATE_COMPLETE, &scmd->state)) { if (!hba->mcq_enabled) { struct request *rq = scsi_cmd_to_rq(scmd); @@ -9680,7 +9687,7 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd) } if (ufshcd_is_scsi_cmd(scmd)) { - set_host_byte(scmd, DID_REQUEUE); + set_host_byte(scmd, DID_TRANSPORT_FAILFAST); ufshcd_release_scsi_cmd(hba, scmd); } else { set_host_byte(scmd, DID_TIME_OUT); @@ -10137,6 +10144,9 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev, { const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 }; struct scsi_failure failure_defs[] = { + { + .result = DID_TRANSPORT_FAILFAST << 16, + }, { .allowed = 2, .result = SCMD_FAILURE_RESULT_ANY, @@ -10172,6 +10182,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, struct scsi_sense_hdr sshdr; struct scsi_device *sdp; unsigned long flags; + bool fenced; int ret; spin_lock_irqsave(&hba->host->host_lock, flags); @@ -10199,6 +10210,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, * already suspended childs. */ ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr); + fenced = ret > 0 && host_byte(ret) == DID_TRANSPORT_FAILFAST; if (ret) { sdev_printk(KERN_WARNING, sdp, "START_STOP failed for power mode: %d, result %x\n", @@ -10212,6 +10224,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, hba->curr_dev_pwr_mode = pwr_mode; } + if (fenced || (!(hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) && + !ufshcd_is_hba_active(hba))) { + ret = ufshcd_link_recovery(hba); + if (!ret && hba->curr_dev_pwr_mode != pwr_mode) + ret = -EIO; + } + scsi_device_put(sdp); WRITE_ONCE(hba->host->eh_noresume, 0); return ret; -- 2.56.0.611.g1030f0b940-goog From: Brian Kao ufshcd_set_dev_pwr_mode() now recovers the link after a START STOP UNIT timeout. ufshcd_err_handler() must not recover the link while a PM callback runs. Its pm_op_in_progress branch still does so when a fatal UIC error arrives during that recovery. Remove the pm_op_in_progress branch, so ufshcd_err_handler() waits for the callback and only one context enables the host controller. 1. Runtime suspend sends START STOP UNIT and the device does not answer. 2. ufshcd_set_dev_pwr_mode() starts link recovery after the timeout. 3. A fatal UIC error queues ufshcd_err_handler(). 4. ufshcd_err_handler() sees pm_op_in_progress and starts a second link recovery. Both enable the host controller at once. Removing the pm_op_in_progress branch needs one more change. A fatal error during runtime resume then has no recovery: 1. A fatal UIC error queues ufshcd_err_handler(), which waits for the runtime resume. 2. START STOP UNIT fails with DID_BAD_TARGET and resume fails. 3. ufshcd_err_handler() resumes again and fails the same way. The block layer pm_only count underflows and I/O hangs. Recover the link in __ufshcd_wl_resume() when START STOP UNIT fails while error handling is scheduled for a fatal error, as it does after a failed hibern8 exit. Fixes: f966e02ae521 ("scsi: ufs: core: Fix runtime suspend error deadlock") Fixes: e23ef4f22db3 ("scsi: ufs: core: Fix error handler host_sem issue") Cc: stable@vger.kernel.org # 7.2.x Signed-off-by: Brian Kao [stanleyjhu: recover the link in __ufshcd_wl_resume() after a fatal error] Signed-off-by: Stanley Jhu --- drivers/ufs/core/ufshcd.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 0f5365bd90f0..825906b643fa 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -7074,23 +7074,6 @@ static void ufshcd_err_handler(struct work_struct *work) hba->saved_uic_err, hba->force_reset, ufshcd_is_link_broken(hba) ? "; link is broken" : ""); - if (hba->ufs_device_wlun) { - /* - * Use ufshcd_rpm_get_noresume() here to safely perform link - * recovery even if an error occurs during runtime suspend or - * runtime resume. This avoids potential deadlocks that could - * happen if we tried to resume the device while a PM operation - * is already in progress. - */ - ufshcd_rpm_get_noresume(hba); - if (hba->pm_op_in_progress) { - ufshcd_link_recovery(hba); - ufshcd_rpm_put(hba); - return; - } - ufshcd_rpm_put(hba); - } - down(&hba->host_sem); spin_lock_irqsave(&hba->host->host_lock, flags); if (ufshcd_err_handling_should_stop(hba)) { @@ -10638,6 +10621,8 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) if (!ufshcd_is_ufs_dev_active(hba)) { ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE); + if (ret && hba->ufshcd_state == UFSHCD_STATE_EH_SCHEDULED_FATAL) + ret = ufshcd_link_recovery(hba); if (ret) goto set_old_link_state; ufshcd_set_timestamp_attr(hba); -- 2.56.0.611.g1030f0b940-goog