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

[Peter Wang]

On Fri, 2026-11-06 at 16:58 +0900, hoyoung seo wrote:
> Hi Peter,
> 
> Thank you for the review, and for the Reviewed-by on patch 1. I will
> add
> it. Note that I have already posted v7, which reorders the series
> following Bart's comments:
> https://lore.kernel.org/linux-scsi/cover.1793943246.git.hy50.seo@xxxxxxxxxxx/
>  
> 
> On your question about making __ufshcd_wl_resume() trigger the error
> handler and wait for it instead of calling ufshcd_link_recovery()
> directly: I think that is a different design from the one in this
> series, and I see a few problems with it. I may be missing something.
> 
>  - While the system PM callbacks run, host_sem is held from
>    ufshcd_wl_suspend() until ufshcd_wl_resume() returns. If
>    __ufshcd_wl_resume() waited for the error handler, the error
> handler
>    could not wait for host_sem (that would be a deadlock), so it
> would
>    have to keep taking the early link recovery path during system
>    resume. This series does the opposite, as Bart suggested: for
> system
>    PM the error handler waits for host_sem, and the early path is
> only
>    used for runtime PM.
> 

Hi hoyoung,

I think both ways could solve this issue. But excluding system PM fast 
recovery seems more complicated. Perhaps we should wait for Bart’s
thoughts, but for me:
1. When wl resume fails, having different error handling would confuse
me.
2. Does the runtime resume error path still have a chance of concurrent
recovery?


>  - The link recovery was put into the resume path on purpose. Commit
>    35dabf4503b9 ("scsi: ufs: core: Use link recovery when h8 exit
> fails
>    during runtime resume") says that the error handler got stuck when
> the
>    h8 exit failed during runtime resume, and commit 62c015373e1c
> moved it
>    to __ufshcd_wl_resume(). As far as I can see this is because
>    ufshcd_err_handling_prepare() waits for the runtime PM operation
> that
>    is in progress. Waiting for the error handler from the resume path
>    would bring that dependency back.
> 

Yes, but now error handler recovery will be handled first, so it won’t
reach 
ufshcd_err_handling_prepare. So, there should be no deadlock concern,
right?


>  - __ufshcd_wl_resume() needs to know whether the recovery succeeded
> to
>    decide how to continue, while the error handler returns no result,
> so
>    it would have to look at the HBA state afterwards. It would also
> need
>    to avoid queueing the error handler again if it is already
> running,
>    similar to what __ufshcd_wl_suspend() does with
>    ufshcd_eh_in_progress(), to prevent a second recovery.
> 

It only needs to queue_work and wait for flush_work, and believe that 
the error handler calling ufshcd_link_recovery will make everything
good. 
Otherwise, __ufshcd_wl_resume has no way to handle it either, right?
Even if it is queued twice, the second time it will check 
ufshcd_err_handling_should_stop and do nothing.


>  - Other callers of ufshcd_link_recovery() (ufshcd_eh_timed_out(),
>    ufshcd_eh_host_reset_handler(), ufs_mtk_resume()) would still call
> it
>    directly.
> 

My earlier thinking was that if an error occurs during resume, 
doing recovery directly is faster, because it isn’t necessarily
accompanied by an err handler worker.
But if the err handler might also be triggered, it would be 
better to simply trigger the error handler, let it perform the 
recovery, and wait for it to finish. Would that make more sense?

Thanks
Peter