On 8/28/21 02:47, Adrian Hunter wrote:

There is a deadlock that seems to be related to this patch, because now
requests are blocked while the error handler waits on the host_sem.

Hi Adrian,

Some but not all of the issues mentioned below have been introduced by 
patch 16/18. Anyway, thank you for having shared your concerns.

Example:

ufshcd_err_handler() races with ufshcd_wl_suspend() for host_sem.
ufshcd_wl_suspend() wins the race but now PM requests deadlock:

because:
  scsi_queue_rq() -> scsi_host_queue_ready() -> scsi_host_in_recovery() is FALSE

because:
  scsi_schedule_eh() has done:
	    scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0)


Some questions for thought:

Won't any holder of host_sem deadlock if it tries to do SCSI requests
and the error handler is waiting on host_sem?

Won't runtime resume deadlock if it is initiated by the error handler?

My understanding is that host_sem is used for the following purposes:
- To prevent that sysfs attributes are read or written after shutdown
   has started (hba->shutting_down).
- To serialize sysfs attribute access, clock scaling, error handling,
   the ufshcd_probe_hba() call from ufshcd_async_scan() and hibernation.

I propose to make the following changes:
- Instead of checking the value of hba->shutting_down from inside sysfs
   attribute callbacks, remove sysfs attributes before starting shutdown.
   That will remove the need to check hba->shutting_down from inside
   sysfs attribute callbacks.
- Leave out the host_sem down() and up() calls from ufshcd_wl_suspend()
   and ufshcd_wl_resume(). Serializing hibernation against e.g. sysfs
   attribute access is not the responsibility of a SCSI LLD - this is the
   responsibility of the power management core.
- Split host_sem. I don't see how else to address the potential deadlock
   between the error handler and runtime resume.

Do you agree with the above?

Thanks,

Bart.