* Re: [PATCH v1] scsi: ufs: core: add is_sys_suspended state before recovery
2026-10-23 8:50 ` [PATCH v1] scsi: ufs: core: add is_sys_suspended state before recovery HOYOUNG SEO
@ 2026-10-23 9:24 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-10-23 9:24 UTC (permalink / raw)
To: HOYOUNG SEO, linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: ufs: ufshcd_err_handler() still runs recovery after ufshcd_wl_resume()
--
commit cfd94f0f9cc528872dada8ade87c19c3c23304de
Author: HOYOUNG SEO <hy50.seo@samsung.com>
scsi: ufs: core: add is_sys_suspended state before recovery
This commit checks !hba->is_sys_suspended before calling
ufshcd_link_recovery() in ufshcd_err_handler() to avoid redundant link
recovery when a linereset and hibern8_exit failure occur during
ufshcd_wl_resume().
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cc75384c65021..988cb127ef0e8 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7041,7 +7041,7 @@ static void ufshcd_err_handler(struct work_struct *work)
> * is already in progress.
> */
> ufshcd_rpm_get_noresume(hba);
> - if (hba->pm_op_in_progress) {
> + if (hba->pm_op_in_progress && !hba->is_sys_suspended) {
[Severity: Medium]
Does skipping the early return here actually prevent ufshcd_err_handler()
from running recovery after ufshcd_wl_resume() finishes?
When a linereset or UIC error and hibern8_exit failure occur during
ufshcd_wl_resume() -> __ufshcd_wl_resume(), the interrupt handler latches the
error bits into hba->saved_err and hba->saved_uic_err and schedules
hba->eh_work.
In drivers/ufs/core/ufshcd.c, __ufshcd_wl_resume() then calls
ufshcd_link_recovery():
if (ufshcd_is_link_hibern8(hba)) {
ret = ufshcd_uic_hibern8_exit(hba);
if (!ret) {
ufshcd_set_link_active(hba);
} else {
dev_err(hba->dev, "%s: hibern8 exit failed %d\n",
__func__, ret);
ret = ufshcd_link_recovery(hba);
if (ret)
goto vendor_suspend;
}
}
Unlike ufshcd_reset_and_restore(), neither ufshcd_link_recovery() nor
ufshcd_host_reset_and_restore() clears hba->saved_err, hba->saved_uic_err, or
hba->force_reset.
With this change, ufshcd_err_handler() sees hba->is_sys_suspended is true,
skips the early return block, and waits on down(&hba->host_sem):
ufshcd_rpm_get_noresume(hba);
if (hba->pm_op_in_progress && !hba->is_sys_suspended) {
ufshcd_link_recovery(hba);
ufshcd_rpm_put(hba);
return;
}
ufshcd_rpm_put(hba);
}
down(&hba->host_sem);
spin_lock_irqsave(&hba->host->host_lock, flags);
if (ufshcd_err_handling_should_stop(hba)) {
Once ufshcd_wl_resume() releases hba->host_sem, ufshcd_err_handler() wakes up
and checks ufshcd_err_handling_should_stop():
static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
{
return (!hba->is_powered || hba->shutting_down ||
!hba->ufs_device_wlun ||
hba->ufshcd_state == UFSHCD_STATE_ERROR ||
(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
ufshcd_is_link_broken(hba))));
}
Because hba->saved_err and hba->saved_uic_err are still non-zero after
ufshcd_link_recovery(), ufshcd_err_handling_should_stop() returns false.
Wouldn't ufshcd_err_handler() then proceed into ufshcd_err_handling_prepare()
to quiesce all SCSI queues and run redundant error recovery (such as
ufshcd_reset_and_restore() when UFSHCD_UIC_HIBERN8_MASK or a fatal/DL UIC
error was latched, or ufshcd_abort_all()) on the link that __ufshcd_wl_resume()
already recovered?
--
Sashiko AI review · https://sashiko.dev/#/patchset/1582071873.61792745883325.JavaMail.epsvc@epcpadp2new?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread