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

[PATCH v10 0/4] 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.

Patches 2 and 3 make ufshcd_wl_poweroff() behave like
ufshcd_wl_suspend() by sharing its implementation. In particular it now
holds host_sem, so that the error handler waits for it, and the release
of host_sem is balanced if hibernation is aborted.

Patch 4 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 v10:
- Patch 1: no change.
- Patches 2 and 3 (were v9 patch 2): move the body of
  ufshcd_wl_suspend() into a helper that takes the PM operation and call
  it from ufshcd_wl_poweroff() too, as suggested by Bart Van Assche.
  This also handles a runtime suspended WLUN, sets hba->is_sys_suspended
  and converts a positive status into -EBUSY (Sashiko review of v9).
  Drop the Reviewed-by tag of Peter Wang because the code changed.
- Patch 4 (was v9 patch 3): make the check a static function in
  ufshcd.c named ufshcd_in_wl_rpm_callback(), and use READ_ONCE() and
  WRITE_ONCE() for hba->pm_op_in_progress, as suggested by Bart Van
  Assche. The Reviewed-by tag of Peter Wang is kept; the functional
  change is the same as in v9.

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.
- Patch 3: also require hba->pm_op_in_progress for the early link
  recovery, as suggested by Bean Huo and Bart Van Assche. The runtime
  status alone is also set during the block layer steps before and after
  the runtime PM callbacks (Sashiko review of v8).

Changes in v8:
- Patch 1: add Reviewed-by: Peter Wang.
- 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.
- 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. Add a Fixes: tag
  and move the patch before the one that depends on it.
- 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 (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 (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.

v9: https://lore.kernel.org/linux-scsi/728258269.41794277203059.JavaMail.epsvc@epcpadp1new/
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 (4):
  scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()
  scsi: ufs: core: Add a helper for the system suspend callbacks
  scsi: ufs: core: Make ufshcd_wl_poweroff() behave like
    ufshcd_wl_suspend()
  scsi: ufs: core: Limit early link recovery in the error handler to
    runtime PM

 drivers/ufs/core/ufshcd.c | 65 +++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 13 deletions(-)

-- 
2.34.1

Re: [PATCH v10 0/4] scsi: ufs: core: Avoid redundant link recovery from the error handler during PM

[Stanley Jhu]

For the series:

Reviewed-by: Stanley Jhu <stanleyjhu@google.com>