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

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

On 11/6/26 1:44 AM, Peter Wang wrote:
 > 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?

Hi Peter,

I don't think that's guaranteed. ufshcd_err_handler() tests
hba->pm_op_in_progress only once, when it starts, and without holding
any lock. An error handler that started before that flag was set skips
the early path. For runtime PM it then blocks in ufshcd_rpm_get_sync()
in ufshcd_err_handling_prepare(), waiting for the runtime resume that
is in progress. If __ufshcd_wl_resume() waits for that error handler
with flush_work(), neither can proceed. This is the deadlock that
commit 35dabf4503b9 addressed and that the comment in
ufshcd_eh_timed_out() describes.

System PM has a similar problem. __ufshcd_wl_suspend() clears
hba->pm_op_in_progress before it returns, but host_sem stays held
until ufshcd_wl_resume() returns. An error handler that runs between
system suspend and system resume therefore takes the regular path and
blocks in down(&hba->host_sem). A flush_work() call from
__ufshcd_wl_resume() would then never return.

Additionally, ufshcd_schedule_eh_work() does not queue the error
handler if the HBA state is UFSHCD_STATE_ERROR. In that case
queue_work() + flush_work() would not perform any recovery, and
__ufshcd_wl_resume() would have no way to find out whether recovery
succeeded.

 > 1. When wl resume fails, having different error handling would confuse
 > me.

The resume path is not modified by this series: __ufshcd_wl_resume()
calls ufshcd_link_recovery() for both runtime and system PM. What
changes is that the error handler no longer takes the early path, which
does not hold host_sem, during system PM. That path is a workaround for
a runtime PM deadlock. During system PM the error handler can simply
wait for host_sem, which serializes it against the PM callbacks. Doing
that is easier to reason about than letting two concurrent link
recoveries race.

 > 2. Does the runtime resume error path still have a chance of concurrent
 > recovery?

Yes. The cover letter mentions this. That issue predates this series,
and I think it should be addressed separately instead of holding up
these fixes.

Hence, I prefer the approach of v7 of this series.

Thanks,

Bart.