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@xxxxxxx>
Comment below. Nevertheless,
Reviewed-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
> 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))
