Re: [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] scsi: ufs: core: unsynchronized link recovery in
  ufshcd_err_handler()
--

commit bf85ee52f6ee7c2e7ab3e9f87aa7985d6566f949
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

This commit replaces the hba->pm_op_in_progress check in
ufshcd_err_handler() with ufshcd_rpm_transition_in_progress() so that early
link recovery without hba->host_sem only runs when the WLUN runtime PM
status is RPM_SUSPENDING or RPM_RESUMING.

> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index 99f654b79035f..c576f648f242d 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -418,6 +418,19 @@ static inline void ufshcd_rpm_put(struct ufs_hba *hba)
>  	pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
>  }
>  
> +/* Whether a runtime suspend or resume of the WLUN is in progress. */
> +static inline bool ufshcd_rpm_transition_in_progress(struct ufs_hba *hba)
> +{
> +#ifdef CONFIG_PM
> +	enum rpm_status status =
> +		READ_ONCE(hba->ufs_device_wlun->sdev_gendev.power.runtime_status);
> +
> +	return status == RPM_SUSPENDING || status == RPM_RESUMING;
> +#else
> +	return false;
> +#endif
> +}
> +
>  /**
>   * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
>   * @dev_info: pointer of instance of struct ufs_dev_info
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index c2c34b044c3ec..1608a856cac71 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7094,9 +7094,14 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 * runtime resume. This avoids potential deadlocks that could
>  		 * happen if we tried to resume the device while a PM operation
>  		 * is already in progress.
> +		 *
> +		 * Do this only while a runtime PM operation is in progress. The
> +		 * system PM and poweroff callbacks hold host_sem, and
> +		 * shutting_down makes the regular path below stop, so it does
> +		 * not reset the link concurrently with them.
>  		 */
>  		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> +		if (ufshcd_rpm_transition_in_progress(hba)) {
>  			ufshcd_link_recovery(hba);
>  			ufshcd_rpm_put(hba);
>  			return;

[Severity: High]
Does checking runtime_status here widen the window for unsynchronized
ufshcd_link_recovery() outside the ufshcd_hold() region of
__ufshcd_wl_suspend() and __ufshcd_wl_resume()?

The runtime PM core sets runtime_status to RPM_SUSPENDING before
sdev_runtime_suspend() runs blk_pre_runtime_suspend(), and keeps
RPM_SUSPENDING or RPM_RESUMING until after sdev_runtime_suspend() or
sdev_runtime_resume() returns.  That window spans before
__ufshcd_wl_suspend() calls ufshcd_hold() and after __ufshcd_wl_suspend() or
__ufshcd_wl_resume() calls ufshcd_release() and clears
hba->pm_op_in_progress:

CPU0                                  CPU1 (eh_wq)
rpm_suspend()
  runtime_status = RPM_SUSPENDING
  ...
  __ufshcd_wl_suspend()
    ufshcd_hold(hba)
    ufshcd_link_state_transition()
      /* fails */
    ufshcd_force_error_recovery(hba)
      queue_work(..., &hba->eh_work)
    ...
    ufshcd_release(hba)
    hba->pm_op_in_progress = false
                                      ufshcd_err_handler()
                                        ufshcd_rpm_transition_in_progress()
                                          /* still RPM_SUSPENDING */
                                        ufshcd_link_recovery(hba)
                                          /* runs without ufshcd_hold() */
  blk_post_runtime_suspend()
  runtime_status = RPM_ACTIVE

Because ufshcd_link_recovery() runs in this window without ufshcd_hold(),
hba->host_sem, or ufshcd_err_handling_prepare(), it can access controller
MMIO registers while clocks are gated off (for example, during
blk_pre_runtime_suspend() or after ufshcd_release()), or reset and activate
the host and link after a successful __ufshcd_wl_suspend() right before
rpm_suspend() marks the WLUN RPM_SUSPENDED.

Also, during runtime PM itself, if a UIC error schedules eh_work while
__ufshcd_wl_resume() fails ufshcd_uic_hibern8_exit(), or while
__ufshcd_wl_suspend() rolls back at set_dev_active, ufshcd_err_handler() and
the runtime PM callback still run ufshcd_link_recovery() concurrently.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/728258269.41794200283102.JavaMail.epsvc@epcpadp2new?part=3