| Message ID | 20240122083324.11797-1-hy50.seo@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] scsi: ufs: core: Remove the ufshcd_release in ufshcd_err_handling_prepare | expand |
On 1/22/24 00:33, SEO HOYOUNG wrote: > If err_handler is performed in the suspend/resume situation, ufs_release > can be called twice and active_reqs valid can be negative. > This is because ufshcd_errhandling_prepare() and > ufshcd_err_handling_unprepare() repeatedly release calls. > Eventually, active_reqs have a value different from the intention. > To prevent this, release duplication processing was removed. > > Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com> > --- > drivers/ufs/core/ufshcd.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 7c59d7a02243..423e83074a20 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -6351,7 +6351,6 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) > ufshcd_hold(hba); > if (!ufshcd_is_clkgating_allowed(hba)) > ufshcd_setup_clocks(hba, true); > - ufshcd_release(hba); > pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; > ufshcd_vops_resume(hba, pm_op); > } else { I think that the above ufshcd_release() call pairs with the ufshcd_hold() call three lines above it and hence that removing that call would be wrong. Thanks, Bart.
> -----Original Message----- > From: Bart Van Assche <bvanassche@acm.org> > Sent: Tuesday, January 23, 2024 5:37 AM > To: SEO HOYOUNG <hy50.seo@samsung.com>; linux-scsi@vger.kernel.org; linux- > kernel@vger.kernel.org; alim.akhtar@samsung.com; avri.altman@wdc.com; > jejb@linux.ibm.com; martin.petersen@oracle.com; beanhuo@micron.com; > kwangwon.min@samsung.com; kwmad.kim@samsung.com; sh425.lee@samsung.com; > sc.suh@samsung.com; quic_nguyenb@quicinc.com; cpgs@samsung.com; > grant.jung@samsung.com; junwoo80.lee@samsung.com > Subject: Re: [PATCH v1] scsi: ufs: core: Remove the ufshcd_release in > ufshcd_err_handling_prepare > > On 1/22/24 00:33, SEO HOYOUNG wrote: > > If err_handler is performed in the suspend/resume situation, > > ufs_release can be called twice and active_reqs valid can be negative. > > This is because ufshcd_errhandling_prepare() and > > ufshcd_err_handling_unprepare() repeatedly release calls. > > Eventually, active_reqs have a value different from the intention. > > To prevent this, release duplication processing was removed. > > > > Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com> > > --- > > drivers/ufs/core/ufshcd.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index 7c59d7a02243..423e83074a20 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -6351,7 +6351,6 @@ static void ufshcd_err_handling_prepare(struct > ufs_hba *hba) > > ufshcd_hold(hba); > > if (!ufshcd_is_clkgating_allowed(hba)) > > ufshcd_setup_clocks(hba, true); > > - ufshcd_release(hba); > > pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : > UFS_RUNTIME_PM; > > ufshcd_vops_resume(hba, pm_op); > > } else { > > I think that the above ufshcd_release() call pairs with the ufshcd_hold() > call three lines above it and hence that removing that call would be wrong. > > Thanks, > > Bart. Hi, It was a different when I tested it. If __ufshcd_wl_resume() is called active_reqs is 1. Because ufshcd_hold() is called in __ufshcd_wl_suspend(). If occurred hibern8_exit failed in __ufschd_wl_resume(), ufshcd_release() is called in the :out syntax, and active_reqs becomes 0. After that, active_reqs becomes 0 because ufshcd_hold() is called from ufshcd_err_handling_repare()and ufshcd_release() is called again while err_handler is operating. When err_handler is completed, active_reqs becomes negative because ufshcd_release() is called again in ufshcd_err_handling_unprepare(). I tested it while printing the log, and if I misanalyzed it, let me know. Thanks, SEO.
On 1/22/24 18:38, hoyoung seo wrote: > When err_handler is completed, active_reqs becomes negative because > ufshcd_release() is called again in ufshcd_err_handling_unprepare(). > I tested it while printing the log, and if I misanalyzed it, let me know. Please repeat your analysis. I think this patch is wrong. Thanks, Bart.
> -----Original Message----- > From: Bart Van Assche <bvanassche@acm.org> > Sent: Thursday, January 25, 2024 1:17 AM > To: hoyoung seo <hy50.seo@samsung.com>; linux-scsi@vger.kernel.org; linux- > kernel@vger.kernel.org; alim.akhtar@samsung.com; avri.altman@wdc.com; > jejb@linux.ibm.com; martin.petersen@oracle.com; beanhuo@micron.com; > kwangwon.min@samsung.com; kwmad.kim@samsung.com; sh425.lee@samsung.com; > sc.suh@samsung.com; quic_nguyenb@quicinc.com; cpgs@samsung.com; > grant.jung@samsung.com; junwoo80.lee@samsung.com > Subject: Re: [PATCH v1] scsi: ufs: core: Remove the ufshcd_release in > ufshcd_err_handling_prepare > > On 1/22/24 18:38, hoyoung seo wrote: > > When err_handler is completed, active_reqs becomes negative because > > ufshcd_release() is called again in ufshcd_err_handling_unprepare(). > > I tested it while printing the log, and if I misanalyzed it, let me know. > > Please repeat your analysis. I think this patch is wrong. > > Thanks, > > Bart. Hi, I do not understand. why you said my patch is wrong. If ufs entered suspend with hibern8 state then the hba->clk_gating.active_reqs is 1. After that run wl_resume(), ufs drvier send hibern8 exit command. At that time, if the command timeout or error occurs, the err_handler is activated. Then the active_reqs pair may not fit. So to sum up, ufs_release() is performed 3 time. (wl_resume(), ufshcd_err_handling_prepare(), ufshcd_err_handling_unprepare()) And the ufshcd_hold() is performed 2 time(__ufshcd_wl_suspend(), ufshcd_err_handling_prepare()) So the paire of active_reqs is not correct. So I deleted the ufshcd_release() in ufshcd_err_handling_prepare(). The ufshcd_release() was not called again even in the pm_op_in_progress state in 4.xx version of the kernel. But now if is_sys_suspended is 1, then ufshcd_release() is called once more. I don't understand why this is added and the pair doesn't fit. Please check it again. Thanks. Seo.
On 1/31/24 00:23, hoyoung seo wrote:
> I do not understand. why you said my patch is wrong.
My apologies - I misread the patch.
Bart.
On 1/22/24 00:33, SEO HOYOUNG wrote: > This is because ufshcd_errhandling_prepare() and > ufshcd_err_handling_unprepare() repeatedly release calls. It would have been much more clear if it would have been mentioned that ufshcd_err_handling_prepare() should call ufshcd_hold() once and also that ufshcd_err_handling_unprepare() should call ufshcd_release() once. Additionally, a Fixes: tag is missing. Is this patch perhaps a fix for commit c72e79c0ad2b ("scsi: ufs: Recover HBA runtime PM error in error handler")? Can Guo, since you wrote that patch, can you please take a look at this patch? > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 7c59d7a02243..423e83074a20 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -6351,7 +6351,6 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) > ufshcd_hold(hba); > if (!ufshcd_is_clkgating_allowed(hba)) > ufshcd_setup_clocks(hba, true); > - ufshcd_release(hba); > pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; > ufshcd_vops_resume(hba, pm_op); > } else { Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 2/1/2024 1:44 AM, Bart Van Assche wrote: > On 1/22/24 00:33, SEO HOYOUNG wrote: >> This is because ufshcd_errhandling_prepare() and >> ufshcd_err_handling_unprepare() repeatedly release calls. > > It would have been much more clear if it would have been mentioned that > ufshcd_err_handling_prepare() should call ufshcd_hold() once and > also that ufshcd_err_handling_unprepare() should call ufshcd_release() > once. > > Additionally, a Fixes: tag is missing. Is this patch perhaps a fix for > commit > c72e79c0ad2b ("scsi: ufs: Recover HBA runtime PM error in error handler")? > Can Guo, since you wrote that patch, can you please take a look at this > patch? > >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 7c59d7a02243..423e83074a20 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -6351,7 +6351,6 @@ static void ufshcd_err_handling_prepare(struct >> ufs_hba *hba) >> ufshcd_hold(hba); >> if (!ufshcd_is_clkgating_allowed(hba)) >> ufshcd_setup_clocks(hba, true); >> - ufshcd_release(hba); When we are here, it means runtime resume has failed. When I wrote the code (in older kernel), ufshcd_runtime_resume(), if fails, bails without calling ufshcd_release(), eventually ufshcd_err_handling_unprepare() would call ufshcd_release() to balance. But now, I see that if __ufshcd_wl_resume() fails, ufshcd_release() is called anyways, so ufshcd_release() is not required here. Hence, Reviewed-by: Can Guo <quic_cang@quicinc.com> >> pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; >> ufshcd_vops_resume(hba, pm_op); >> } else { > > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Mon, 22 Jan 2024 17:33:24 +0900, SEO HOYOUNG wrote: > If err_handler is performed in the suspend/resume situation, ufs_release > can be called twice and active_reqs valid can be negative. > This is because ufshcd_errhandling_prepare() and > ufshcd_err_handling_unprepare() repeatedly release calls. > Eventually, active_reqs have a value different from the intention. > To prevent this, release duplication processing was removed. > > [...] Applied to 6.8/scsi-fixes, thanks! [1/1] scsi: ufs: core: Remove the ufshcd_release in ufshcd_err_handling_prepare https://git.kernel.org/mkp/scsi/c/17e94b258541
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 7c59d7a02243..423e83074a20 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -6351,7 +6351,6 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) ufshcd_hold(hba); if (!ufshcd_is_clkgating_allowed(hba)) ufshcd_setup_clocks(hba, true); - ufshcd_release(hba); pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; ufshcd_vops_resume(hba, pm_op); } else {
If err_handler is performed in the suspend/resume situation, ufs_release can be called twice and active_reqs valid can be negative. This is because ufshcd_errhandling_prepare() and ufshcd_err_handling_unprepare() repeatedly release calls. Eventually, active_reqs have a value different from the intention. To prevent this, release duplication processing was removed. Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com> --- drivers/ufs/core/ufshcd.c | 1 - 1 file changed, 1 deletion(-)