[PATCH v9 0/3] scsi: ufs: core: Avoid redundant link recovery from the error handler during PM

[PATCH v9 0/3] scsi: ufs: core: Avoid redundant link recovery from the error handler during PM

[HOYOUNG SEO]

If a LINERESET is reported and the hibern8 exit fails during system
resume, both ufshcd_err_handler() and __ufshcd_wl_resume() call
ufshcd_link_recovery(), and the two recoveries are not serialized.

Patch 1 makes ufshcd_link_recovery() clear the saved errors like
ufshcd_reset_and_restore() does, so the error handler does not act on
errors that have already been recovered.

Patch 2 makes ufshcd_wl_poweroff() hold host_sem like
ufshcd_wl_suspend() does, so that the error handler waits for it, and
fixes the unbalanced release of host_sem if hibernation is aborted.

Patch 3 restricts the early link recovery in ufshcd_err_handler(), which
does not hold host_sem, to runtime PM. For system PM and poweroff the
error handler now waits for host_sem, and for shutdown it stops.

The early link recovery is kept for runtime suspend and runtime resume.
During runtime resume it can still run concurrently with
__ufshcd_wl_resume(). Removing it there needs a closer look at the
deadlock that it avoids.

Changes in v9:
- Patch 1: add Reviewed-by: Bean Huo.
- Patch 2: if __ufshcd_wl_suspend() fails, release host_sem and return
  the error, as ufshcd_wl_suspend() does, so that the error handler does
  not block forever, as suggested by Peter Wang and Bart Van Assche.
  Describe the behavior change in the commit message. Drop the
  Reviewed-by tag of Bart Van Assche because the code changed.
- Patch 3: also require hba->pm_op_in_progress for the early link
  recovery, checked together with the runtime status in
  ufshcd_rpm_transition_in_progress() as suggested by Bean Huo. The
  runtime status of the WLUN alone is also set during the
  block layer steps before and after the runtime PM callbacks, which
  widened the window in which the error handler recovers the link
  without the PM callbacks holding the controller (Sashiko review of v8,
  Bean Huo and Bart Van Assche). Drop the Reviewed-by tag of Peter Wang
  because the condition changed.

Changes in v8:
- Patch 1: add Reviewed-by: Peter Wang.
- Patch 2: add Reviewed-by: Bart Van Assche. No other change.
- Patch 3: use the runtime PM status of the WLUN (RPM_SUSPENDING or
  RPM_RESUMING) instead of the new member hba->runtime_pm_in_progress, as
  suggested by Bart Van Assche.

Changes in v7:
- Patch 1: add Reviewed-by: Bart Van Assche.
- Patch 2 (was v6 patch 3): hold host_sem in ufshcd_wl_poweroff() until
  ufshcd_wl_resume() releases it, instead of setting hba->shutting_down,
  as suggested by Bart Van Assche. This fixes the failure of
  ufs_mtk_system_suspend() and the state left behind when hibernation is
  aborted, and the unbalanced release of host_sem. Add a Fixes: tag and
  move the patch before the one that depends on it, so that the series
  stays bisectable.
- Patch 3 (was v6 patch 2): use READ_ONCE() and WRITE_ONCE() for
  hba->runtime_pm_in_progress, and describe the behavior change during
  shutdown in the commit message.

Changes in v6:
- Go back to a single flag instead of the system PM flag of v5, and set
  it in the runtime PM callbacks. The error handler no longer reads two
  variables that are not updated atomically (Sashiko review of v5).
- Add a patch that sets hba->shutting_down in ufshcd_wl_poweroff()
  (Sashiko review of v4).

Changes in v5:
- Patch 2: track system PM (hba->system_pm_in_progress) instead of
  runtime PM, so that the early link recovery is only skipped where
  host_sem is held.

Changes in v4 (as suggested by Bart Van Assche):
- Replace the is_sys_suspended check of v3 patch 2 with a flag set in
  __ufshcd_wl_suspend() and __ufshcd_wl_resume(), which also covers a
  system suspend failure.
- Add Fixes: tags.
- Do not serialize ufshcd_link_recovery() with a mutex: it calls
  ufshcd_probe_hba(), and a device command timing out inside it ends up
  in ufshcd_eh_timed_out(), which calls ufshcd_link_recovery() again.

Changes in v3:
- Patch 2: reword the commit message only, no code change. The error
  handler does not always run a full recovery after a failed system
  resume; it stops according to ufshcd_err_handling_should_stop() (e.g.
  in UFSHCD_STATE_ERROR). Describe this behavior and refer to the commit
  that added the early link recovery (Sashiko review).
- Patch 1: no change.

Changes in v2:
- Add patch 1. As pointed out by the Sashiko review, saved_err and
  saved_uic_err are still set after the resume path recovered the link,
  so with v1 alone ufshcd_err_handler() would run a redundant recovery
  once host_sem is released.
- Patch 2: describe the race and the system/runtime PM difference in
  the commit message, and add a comment.

v8: https://lore.kernel.org/linux-scsi/1582071873.61794200283414.JavaMail.epsvc@epcpadp2new/
v7: https://lore.kernel.org/linux-scsi/cover.1793943246.git.hy50.seo@samsung.com/
v6: https://lore.kernel.org/linux-scsi/cover.1793931113.git.hy50.seo@samsung.com/
v5: https://lore.kernel.org/linux-scsi/cover.1793865172.git.hy50.seo@samsung.com/
v4: https://lore.kernel.org/linux-scsi/cover.1793851432.git.hy50.seo@samsung.com/
v3: https://lore.kernel.org/linux-scsi/cover.1792989286.git.hy50.seo@samsung.com/
v2: https://lore.kernel.org/linux-scsi/cover.1792755091.git.hy50.seo@samsung.com/

HOYOUNG SEO (3):
  scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()
  scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()
  scsi: ufs: core: Limit early link recovery in the error handler to
    runtime PM

 drivers/ufs/core/ufshcd-priv.h | 17 +++++++++++++++++
 drivers/ufs/core/ufshcd.c      | 32 ++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

-- 
2.34.1