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@xxxxxxx>
---
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;
