Re: [PATCH v3 0/2] scsi: ufs: core: Avoid redundant link recovery during system resume

[Bart Van Assche <bvanassche@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

On 11/3/26 1:33 AM, HOYOUNG SEO wrote:
> Any review or comment would be appreciated, especially on 2/2 and the
> system PM vs. runtime PM difference.

Please take a look at the attached review report from an LLM. According
to that review this series isn't ready yet for integration in the
upstream kernel.

Thanks,

Bart.

[-- Attachment #2: review.md --]
[-- Type: text/markdown, Size: 13853 bytes --]

### Summary

**Series:** `[PATCH v3 0/2] scsi: ufs: core: Avoid redundant link recovery during system resume` — Hoyoung Seo (`<hy50.seo@samsung.com>`)
**`checkpatch.pl --strict`:** Clean on both patches (`0 errors, 0 warnings, 0 checks`).

* **Patch 1/2 (`Clear saved errors in ufshcd_link_recovery`):** Sound and worth taking (with a `Fixes:` tag added).
* **Patch 2/2 (`add is_sys_suspended state before recovery`):** Fixes the narrow system-resume (`UFS_SYSTEM_PM`) symptom, **but leaves the exact same unserialized concurrent and redundant [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) bugs unaddressed in runtime resume, system suspend, and shutdown.** In his Nov 3 ping, Hoyoung specifically asked about Patch 2/2 and the system PM vs. runtime PM difference; the breakdown below directly addresses that question.

---

### Patch 1/2: `scsi: ufs: core: Clear saved errors in ufshcd_link_recovery`

#### Assessment
Good fix. In [ufshcd.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722), [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) performs a full [`ufshcd_device_reset()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4707) + [`ufshcd_host_reset_and_restore()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L8278-L8311) without clearing `hba->saved_err`, `hba->saved_uic_err`, or `hba->force_reset`. Caching and clearing them under `hba->host->host_lock` beforehand—and OR-ing `saved_err` / `saved_uic_err` back in alongside `UFSHCD_STATE_ERROR` on failure—matches [`ufshcd_reset_and_restore()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L8336-L8368) and prevents [`ufshcd_err_handling_should_stop()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L6918-L6925) from acting on stale errors (or escalating a later minor error into a full reset).

#### Nit
* Missing `Fixes:` tag (e.g., `Fixes: 7029e2151a7c ("scsi: ufs: Fix a deadlock between PM and the SCSI error handler")` or `Fixes: 35dabf4503b9 ("scsi: ufs: core: Use link recovery when h8 exit fails during runtime resume")`).

---

### Patch 2/2: `scsi: ufs: core: add is_sys_suspended state before recovery`

Patch 2 changes [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7075-L7079) from:
```c
if (hba->pm_op_in_progress) {
```
to:
```c
if (hba->pm_op_in_progress && !hba->is_sys_suspended) {
```

#### Issues (Especially System PM vs. Runtime PM)

1. **The identical unserialized concurrent [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) race still occurs during Runtime Resume (`UFS_RUNTIME_PM`)**
   * During [`ufshcd_wl_runtime_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10672-L10690) → [`__ufshcd_wl_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10552-L10650), `hba->pm_op_in_progress` is `true` and `hba->is_sys_suspended` is `false`.
   * If a `LINERESET` (`UIC_ERROR` → `UFSHCD_UIC_PA_GENERIC_ERROR`) interrupt fires and [`ufshcd_uic_hibern8_exit()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10572) fails:
     * [`ufshcd_check_errors()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7390-L7412) schedules `hba->eh_work`.
     * [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7075-L7079) sees `hba->pm_op_in_progress && !hba->is_sys_suspended == true` and calls [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) immediately—either resetting the device and calling [`ufshcd_hba_stop()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L8286) *underneath* [`ufshcd_uic_pwr_ctrl()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4537-L4610) while it holds `hba->uic_cmd_mutex`, or executing [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) concurrently with [`__ufshcd_wl_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10584) (and similarly [`ufs_mtk_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-mediatek.c#L1908) in [ufs-mediatek.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-mediatek.c#L1908)).

2. **Redundant [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) still happens in Runtime Resume (and `shutting_down` is bypassed in Shutdown) because the early check precedes [`ufshcd_err_handling_should_stop()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L6918-L6925)**
   * Commit `e23ef4f22db3` ("scsi: ufs: core: Fix error handler host_sem issue") moved the `if (hba->pm_op_in_progress)` check *above* both `down(&hba->host_sem)` and [`ufshcd_err_handling_should_stop()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7085).
   * Consequently, whenever `!hba->is_sys_suspended`, [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7075-L7079) never checks `saved_err`, `saved_uic_err`, `force_reset`, `UFSHCD_STATE_ERROR`, `!hba->is_powered`, or `hba->shutting_down`:
     * Even if [`__ufshcd_wl_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10584) completes [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) first (and Patch 1 clears `saved_err` / `saved_uic_err`), `hba->pm_op_in_progress` remains `true` until line 10648. If `eh_work` starts while [`__ufshcd_wl_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10604-L10648) is running [`ufshcd_set_dev_pwr_mode()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10605) or [`ufshcd_bkops_ctrl()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10621), [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7076) still unconditionally performs a second reset on an operational link.
     * Similarly, during [`ufshcd_wl_shutdown()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10941-L10972), `hba->shutting_down` is `true` and `hba->is_sys_suspended` is `false`, so an error during [`__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10962) runs [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) instead of stopping.

3. **System Suspend ([`ufshcd_wl_suspend()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10694-L10721)) holds `host_sem`, yet `hba->is_sys_suspended` is `false` throughout [`__ufshcd_wl_suspend()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10410-L10549)**
   * In [`ufshcd_wl_suspend()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10702-L10715), `down(&hba->host_sem)` is taken at line 10702, whereas `hba->is_sys_suspended = true` is only set at line 10715 *after* [`__ufshcd_wl_suspend(hba, UFS_SYSTEM_PM)`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10707) succeeds.
   * On suspend failure paths (lines 10430, 10471, 10491), [`__ufshcd_wl_suspend()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10430-L10548) calls [`ufshcd_force_error_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L6832-L6838) *before* executing its `set_dev_active:` unwind (lines 10523–10530) and *before* clearing `hba->pm_op_in_progress = false` (line 10547)—and on an SSU timeout, [`ufshcd_eh_timed_out()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L9637-L9674) is already running [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L9650). Because `is_sys_suspended` is `false`, `eh_work` still runs [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7076) concurrently with both.

4. **Commit subject & tags**
   * Subject `scsi: ufs: core: add is_sys_suspended state before recovery` uses lowercase `add` (inconsistent with Patch 1) and misleadingly implies adding a new state variable rather than skipping early link recovery when `is_sys_suspended` is set.
   * Missing `Fixes:` tag (e.g., `Fixes: e23ef4f22db3 ("scsi: ufs: core: Fix error handler host_sem issue")` / `Fixes: 62c015373e1c ("scsi: ufs: core: Move link recovery for hibern8 exit failure to wl_resume")`).

---

### Suggested Direction for v4

1. **Serialize [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) and honor [`ufshcd_err_handling_should_stop()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L6918-L6925):**
   * All five callers of [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) ([`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7076), [`ufshcd_eh_host_reset_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L8395), [`ufshcd_eh_timed_out()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L9650), [`__ufshcd_wl_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10584), and [`ufs_mtk_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-mediatek.c#L1908)) run while `hba->pm_op_in_progress` is `true`, yet [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) has no mutual exclusion of its own. Protecting [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) with a dedicated mutex in [`struct ufs_hba`](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/ufs/ufshcd.h#L1100)—and checking [`ufshcd_err_handling_should_stop()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L6918-L6925) under `host_lock` before [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7076) invokes early link recovery—prevents overlapping resets and ensures Patch 1's cleared error bits (as well as `shutting_down` and `UFSHCD_STATE_ERROR`) actually stop redundant recoveries.
2. **Don't run early asynchronous [`ufshcd_link_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4696-L4722) from [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7075-L7079) during *any* resume (or system suspend):**
   * During both system resume and runtime resume, `hibern8_exit` failures ([`__ufshcd_wl_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10584)), `vops->resume` failures ([`ufs_mtk_resume()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-mediatek.c#L1908)), and PM SCSI command timeouts ([`ufshcd_eh_timed_out()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L9650), since commit `01d5e237b339`) are already handled synchronously by the resume/timeout context without deadlocking on `RPM_RESUMING`. Skipping the early [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7076) recovery during resume (e.g., restricting it to runtime suspend if still needed at all after `01d5e237b339`, and deferring [`ufshcd_force_error_recovery()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L6832-L6838) in [`__ufshcd_wl_suspend()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10410-L10549) until `pm_op_in_progress` is cleared) stops [`ufshcd_err_handler()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L7048-L7093) from resetting the HBA underneath an active [`ufshcd_uic_pwr_ctrl()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L4537-L4610) or [`__ufshcd_wl_suspend()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L10410-L10549) unwind.