| Message ID | 20220929220021.247097-9-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix a deadlock in the UFS driver | expand |
On Thu, 2022-09-29 at 15:00 -0700, Bart Van Assche wrote: > + ufshcd_link_recovery(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; Bart, you have reset the device and host, in the case, there are pending TMs, Should be cleared locally, just like ufshcd_err_handler() does? Kind regards, Bean
On 9/30/22 08:03, Bean Huo wrote: > On Thu, 2022-09-29 at 15:00 -0700, Bart Van Assche wrote: >> + ufshcd_link_recovery(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; > > Bart, > > you have reset the device and host, in the case, there are pending > TMs, Should be cleared locally, just like ufshcd_err_handler() does? Hi Bean, Do you agree with the following? * SCSI task management functions are only submitted by the SCSI error handler. * The ufshcd_link_recovery() call added by this patch can only be invoked during system suspend. * System suspend only happens after processes and kernel threads have been frozen and after sync() finished. Hence, no I/O should be in progress when ufshcd_wl_suspend() is called and the SCSI error handler should not be running either. * Hence, no SCSI commands other than START STOP UNIT should be in progress when the ufshcd_link_recovery() call added by this patch is invoked. No TMFs should be in progress either. Thanks, Bart.
On Fri, 2022-09-30 at 10:15 -0700, Bart Van Assche wrote: > > > + ufshcd_link_recovery(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; > > > > Bart, > > > > you have reset the device and host, in the case, there are pending > > TMs, Should be cleared locally, just like ufshcd_err_handler() > > does? > > Hi Bean, > > Do you agree with the following? > * SCSI task management functions are only submitted by the SCSI error > handler. > * The ufshcd_link_recovery() call added by this patch can only be > invoked during system suspend. > * System suspend only happens after processes and kernel threads have > been frozen and after sync() finished. Hence, no I/O should be in > progress when ufshcd_wl_suspend() is called and the SCSI error > handler > should not be running either. > * Hence, no SCSI commands other than START STOP UNIT should be in > progress when the ufshcd_link_recovery() call added by this > patch is invoked. No TMFs should be in progress either. > > Thanks, > > Bart. Hi Bart, Yes, install the whole series of changes, ufshcd_link_recovery() will be called during the system is in suspending in the case of SSU timeouts. in this case, outstanding_tasks should be 0? Kind regards, Bean
On 30/09/22 01:00, 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> Comment below. Nevertheless, Reviewed-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/ufs/core/ufshcd.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 5507d93a4bba..0294b51e776a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8295,6 +8295,26 @@ 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); > + > + 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(). > + */ > + ufshcd_link_recovery(hba); This is OK for now, but ufshcd_link_recovery() doesn't check for errors *after* reset, and retry error handling - in which case it might need also the error handling that ufshcd_err_handler() does. It might be better to have only 1 error handling function. > + 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; > +} > + > static const struct attribute_group *ufshcd_driver_groups[] = { > &ufs_sysfs_unit_descriptor_group, > &ufs_sysfs_lun_attributes_group, > @@ -8329,6 +8349,7 @@ static struct scsi_host_template ufshcd_driver_template = { > .eh_abort_handler = ufshcd_abort, > .eh_device_reset_handler = ufshcd_eh_device_reset_handler, > .eh_host_reset_handler = ufshcd_eh_host_reset_handler, > + .eh_timed_out = ufshcd_eh_timed_out, > .this_id = -1, > .sg_tablesize = SG_ALL, > .cmd_per_lun = UFSHCD_CMD_PER_LUN, > @@ -8783,7 +8804,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, > */ > for (retries = 3; retries > 0; --retries) { > ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, > - START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); > + 1 * HZ, 0, REQ_FAILFAST_DEV, RQF_PM, NULL); > if (ret < 0) > break; > if (host_byte(ret) == 0 && scsi_status_is_good(ret))
On 10/2/22 23:18, Adrian Hunter wrote: > This is OK for now, but ufshcd_link_recovery() doesn't check for errors > *after* reset, and retry error handling - in which case it might need also > the error handling that ufshcd_err_handler() does. > > It might be better to have only 1 error handling function. Hi Adrian, I'm fine with sharing code between error handling contexts but I'm not sure a single error handling function is the best solution. Having a single error handling function and calling it from the timeout function would require to add so many if-tests into ufshcd_err_handler() that it would make that function unreadable. Thanks, Bart.
On 10/2/22 14:21, Bean Huo wrote: > Yes, install the whole series of changes, ufshcd_link_recovery() will > be called during the system is in suspending in the case of SSU > timeouts. in this case, outstanding_tasks should be 0? Hi Bean, 'outstanding_tasks' needs to be changed into 'outstanding_reqs' in patch 8/8. My understanding is that ufshcd_link_recovery() calls ufshcd_complete_requests() indirectly. That causes the bits corresponding to completed requests to be cleared from outstanding_reqs but does not guarantee that outstanding_reqs will be zero. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 5507d93a4bba..0294b51e776a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8295,6 +8295,26 @@ 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); + + 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(). + */ + ufshcd_link_recovery(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; +} + static const struct attribute_group *ufshcd_driver_groups[] = { &ufs_sysfs_unit_descriptor_group, &ufs_sysfs_lun_attributes_group, @@ -8329,6 +8349,7 @@ static struct scsi_host_template ufshcd_driver_template = { .eh_abort_handler = ufshcd_abort, .eh_device_reset_handler = ufshcd_eh_device_reset_handler, .eh_host_reset_handler = ufshcd_eh_host_reset_handler, + .eh_timed_out = ufshcd_eh_timed_out, .this_id = -1, .sg_tablesize = SG_ALL, .cmd_per_lun = UFSHCD_CMD_PER_LUN, @@ -8783,7 +8804,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, */ for (retries = 3; retries > 0; --retries) { ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr, - START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL); + 1 * HZ, 0, REQ_FAILFAST_DEV, RQF_PM, NULL); if (ret < 0) break; if (host_byte(ret) == 0 && scsi_status_is_good(ret))
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 | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)