Re: [PATCH] scsi: ufs: core: Keep clock-gating hold on failed W-LUN runtime resume

[Brian Norris <briannorris@chromium.org>]

Hi Stanley,

I'm not extremely familiar with the UFS or block layers here, so I'm
just coming from a general driver + runtime PM perspective. That said, I
have some thoughts:

On Thu, Oct 29, 2026 at 03:08:30PM +0800, Stanley Jhu wrote:
> 1. __ufshcd_wl_suspend() takes one ufshcd_hold() reference and sets
>    hba->clk_gating.is_suspended while the UFS device W-LUN enters
>    RPM_SUSPENDED.
> 2. Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on
>    suspend callback failures"), a failed runtime resume (for example
>    START STOP UNIT completing with an error) leaves the W-LUN in
>    RPM_SUSPENDED with power.runtime_error == 0, while
>    __ufshcd_wl_resume() already clears hba->clk_gating.is_suspended and
>    calls ufshcd_release().
> 3. The next W-LUN runtime resume runs __ufshcd_wl_resume() again without
>    an intervening suspend and calls ufshcd_release() a second time,
>    dropping a reference it does not own and underflowing
>    hba->clk_gating.active_reqs on UFSHCD_CAP_CLK_GATING hosts.

I _think_ this reasoning and patch all looks good, with one possible
exception: ufshcd has explicit recovery in ufshcd_recover_pm_error(),
which relies on checking power.runtime_error, and then explicitly
overrides the status to active to clear the error.

It seems to me like commit eef1d74bf88f might have subverted some of
that logic too, because resume failures will no longer latch a
runtime_error value, so that recovery function may not be doing
much/anything. Is that a problem too?

> Keep hba->clk_gating.is_suspended and the suspend hold across a failed
> W-LUN runtime resume, so the hold stays paired with the W-LUN
> RPM_SUSPENDED state and the next successful resume releases it.
> 
> Fixes: eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures")
> Cc: stable@vger.kernel.org # v7.3+
> Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
> ---
>  drivers/ufs/core/ufshcd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cc75384c6502..9be370cea52a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10626,8 +10626,14 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  out:
>  	if (ret)
>  		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
> -	hba->clk_gating.is_suspended = false;
> -	ufshcd_release(hba);
> +	/*
> +	 * A failed runtime resume leaves the W-LUN RPM_SUSPENDED, so keep
> +	 * is_suspended and the suspend hold for the next resume.
> +	 */
> +	if (!ret || pm_op != UFS_RUNTIME_PM) {
> +		hba->clk_gating.is_suspended = false;
> +		ufshcd_release(hba);
> +	}

This looks right to me, as a non-expert in this code, and it may even
have been wrong before commit eef1d74bf88f.

Reviewed-by: Brian Norris <briannorris@chromium.org>

>  	hba->pm_op_in_progress = false;
>  	return ret;
>  }
> -- 
> 2.56.0.313.g2479322e68-goog
>