Re: [PATCH v2 8/8] scsi: ufs: Fix a deadlock between PM and the SCSI error handler

[Asutosh Das <quic_asutoshd@quicinc.com>]

On Tue, Sep 27 2022 at 11:45 -0700, Bart Van Assche wrote:
>The following deadlock has been observed on multiple test setups:
>* ufshcd_wl_suspend() is waiting for blk_execute_rq() to complete while it
>  holds host_sem.
>* ufshcd_eh_host_reset_handler() invokes ufshcd_err_handler() and the
>  latter function tries to obtain host_sem.
>This is a deadlock because blk_execute_rq() can't execute SCSI commands
>while the host is in the SHOST_RECOVERY state and because the error
>handler cannot make progress either.
>
>Fix this deadlock as follows:
>* Fail attempts to suspend the system while the SCSI error handler is in
>  progress.
>* If the system is suspending and a START STOP UNIT command times out,
>  handle the SCSI command timeout from inside the context of the SCSI
>  timeout handler instead of activating the SCSI error handler.
>* Reduce the START STOP UNIT command timeout to one second since on
>  Android devices a kernel panic is triggered if an attempt to suspend
>  the system takes more than 20 seconds. One second should be enough for
>  the START STOP UNIT command since this command completes in less than
>  a millisecond for the UFS devices I have access to.
>
>The runtime power management code is not affected by this deadlock since
>hba->host_sem is not touched by the runtime power management functions
>in the UFS driver.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>---
> drivers/ufs/core/ufshcd.c | 51 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>index 5507d93a4bba..010a5d1b984b 100644
>--- a/drivers/ufs/core/ufshcd.c
>+++ b/drivers/ufs/core/ufshcd.c
>@@ -8295,6 +8295,54 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
> 	}
> }
>
>+static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>+{
>+	struct ufs_hba *hba = shost_priv(scmd->device->host);
>+	bool reset_controller = false;
>+	int tag, ret;
>+
>+	if (!hba->system_suspending) {
>+		/* Activate the error handler in the SCSI core. */
>+		return SCSI_EH_NOT_HANDLED;
>+	}
>+
>+	/*
>+	 * Handle errors directly to prevent a deadlock between
>+	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
>+	 */
>+	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
>+		ret = ufshcd_try_to_abort_task(hba, tag);
>+		dev_info(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
>+			 hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
>+			 ret == 0 ? "succeeded" : "failed");
>+		if (ret != 0) {
>+			reset_controller = true;
>+			break;
>+		}
>+	}
>+	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
>+		ret = ufshcd_clear_tm_cmd(hba, tag);

If reset_controller is true, then the HC would be reset and it would
anyway clear up all resources. Would this be needed if reset_controller is true?

>
>+		dev_info(hba->dev, "Aborting TMF %d %s\n", tag,
>+			 ret == 0 ? "succeeded" : "failed");
>+		if (ret != 0) {
>+			reset_controller = true;
>+			break;
>+		}
>+	}
>+	if (reset_controller) {
>+		dev_info(hba->dev, "Resetting controller\n");
>+		ufshcd_reset_and_restore(hba);
>+		if (ufshcd_clear_cmds(hba, 0xffffffff))
ufshcd_reset_and_restore() would reset the host and the device.
So is the ufshcd_clear_cmds() needed after that?

>+			dev_err(hba->dev,
>+				"Clearing outstanding commands failed\n");
>+	}
>+	ufshcd_complete_requests(hba);
>+	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
>+		 __func__, hba->outstanding_tasks);
>+
>+	return hba->outstanding_tasks ? SCSI_EH_RESET_TIMER : SCSI_EH_DONE;