* Re: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-02 8:20 ` [RFC PATCH v1] ufs: introduce async ufs interface initialization Kiwoong Kim
@ 2020-07-02 15:43 ` kernel test robot
2020-07-07 6:05 ` Avri Altman
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2020-07-02 15:43 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]
Hi Kiwoong,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-introduce-async-ufs-interface-initialization/20200702-163018
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 003a086ffc0d1affbb8300b36225fb8150a2d40a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/scsi/ufs/ufshcd.c:8329:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (!ufshcd_is_ufs_dev_active(hba)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:8360:9: note: uninitialized use occurs here
return ret;
^~~
drivers/scsi/ufs/ufshcd.c:8329:2: note: remove the 'if' if its condition is always true
if (!ufshcd_is_ufs_dev_active(hba)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:8327:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
1 warning generated.
vim +8329 drivers/scsi/ufs/ufshcd.c
8324
8325 static int ufshcd_post_resume(struct ufs_hba *hba)
8326 {
8327 int ret;
8328
> 8329 if (!ufshcd_is_ufs_dev_active(hba)) {
8330 ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
8331 if (ret)
8332 goto out;
8333 }
8334
8335 if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
8336 ufshcd_enable_auto_bkops(hba);
8337 else
8338 /*
8339 * If BKOPs operations are urgently needed at this moment then
8340 * keep auto-bkops enabled or else disable it.
8341 */
8342 ufshcd_urgent_bkops(hba);
8343
8344 hba->clk_gating.is_suspended = false;
8345
8346 if (hba->clk_scaling.is_allowed)
8347 ufshcd_resume_clkscaling(hba);
8348
8349 /* Enable Auto-Hibernate if configured */
8350 ufshcd_auto_hibern8_enable(hba);
8351
8352 if (hba->dev_info.b_rpm_dev_flush_capable) {
8353 hba->dev_info.b_rpm_dev_flush_capable = false;
8354 cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
8355 }
8356
8357 /* Schedule clock gating in case of no access to UFS device yet */
8358 ufshcd_release(hba);
8359 out:
8360 return ret;
8361 }
8362
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75316 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-02 8:20 ` [RFC PATCH v1] ufs: introduce async ufs interface initialization Kiwoong Kim
2020-07-02 15:43 ` kernel test robot
@ 2020-07-07 6:05 ` Avri Altman
2020-07-07 6:09 ` Kiwoong Kim
2020-07-15 8:35 ` Kiwoong Kim
2020-07-18 20:38 ` Bart Van Assche
` (2 subsequent siblings)
4 siblings, 2 replies; 9+ messages in thread
From: Avri Altman @ 2020-07-07 6:05 UTC (permalink / raw)
To: Kiwoong Kim, linux-scsi@vger.kernel.org, alim.akhtar@samsung.com,
jejb@linux.ibm.com, martin.petersen@oracle.com,
beanhuo@micron.com, asutoshd@codeaurora.org, cang@codeaurora.org,
bvanassche@acm.org, grant.jung@samsung.com, sc.suh@samsung.com,
hy50.seo@samsung.com, sh425.lee@samsung.com
Hi,
>
> When you set uic_link_state during sleep statae to
> UIC_LINK_OFF_STATE, UFS driver does interface initialization
> that is a series of some steps including fDeviceInit and thus,
> You might feel that its latency is a little bit longer.
>
> This patch is run it asynchronously to reduce system wake-up time.
Can you share your initial testing findings?
How much time does it save?
>
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
> drivers/scsi/ufs/Kconfig | 10 ++++
> drivers/scsi/ufs/ufshcd.c | 120 ++++++++++++++++++++++++++++++++++---------
> ---
> 2 files changed, 100 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index 8cd9026..723e7cb 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -172,3 +172,13 @@ config SCSI_UFS_EXYNOS
>
> Select this if you have UFS host controller on EXYNOS chipset.
> If unsure, say N.
> +
> +config SCSI_UFSHCD_ASYNC_INIT
> + bool "Asynchronous UFS interface initialization support"
> + depends on SCSI_UFSHCD
> + default n
> + ---help---
> + This selects the support of doing UFS interface initialization
> + asynchronously when you set link state to link off,
> + i.e. UIC_LINK_OFF_STATE, to reduce system wake-up time.
> + Select this if you have UFS Host Controller.
Maybe replace this config switch with platform capability?
So each platform vendor can choose if he is using a sync vs async init?
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 52abe82..b65d38c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8319,6 +8319,80 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> return ret;
> }
>
> +static int ufshcd_post_resume(struct ufs_hba *hba)
Why do you need to move this code around?
If its async - then there is no shared code -
you go through the full init flow, and just goto out?
> +{
> + int ret;
> +
> + if (!ufshcd_is_ufs_dev_active(hba)) {
> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> + if (ret)
> + goto out;
> + }
> +
> + if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> + ufshcd_enable_auto_bkops(hba);
> + else
> + /*
> + * If BKOPs operations are urgently needed at this moment then
> + * keep auto-bkops enabled or else disable it.
> + */
> + ufshcd_urgent_bkops(hba);
> +
> + hba->clk_gating.is_suspended = false;
> +
> + if (hba->clk_scaling.is_allowed)
> + ufshcd_resume_clkscaling(hba);
> +
> + /* Enable Auto-Hibernate if configured */
> + ufshcd_auto_hibern8_enable(hba);
> +
> + if (hba->dev_info.b_rpm_dev_flush_capable) {
> + hba->dev_info.b_rpm_dev_flush_capable = false;
> + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> + }
> +
> + /* Schedule clock gating in case of no access to UFS device yet */
> + ufshcd_release(hba);
> +out:
> + return ret;
> +}
> +
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> +static void ufshcd_async_resume(void *data, async_cookie_t cookie)
> +{
> + struct ufs_hba *hba = (struct ufs_hba *)data;
> + unsigned long flags;
> + int ret = 0;
> + int retries = 2;
> +
> + /* transition to block requests */
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->ufshcd_state = UFSHCD_STATE_RESET;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + /* initialize, instead of set_old_link_state ?? */
> + do {
> + ret = ufshcd_reset_and_restore(hba);
> + if (ret) {
> + dev_err(hba->dev, "%s: reset and restore failed\n",
> + __func__);
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + hba->pm_op_in_progress = 0;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + return;
> + }
> + ret = ufshcd_post_resume(hba);
> + } while (ret && --retries);
> + if (ret)
> + goto reset;
> +
> + hba->pm_op_in_progress = 0;
> + if (ret)
> + ufshcd_update_reg_hist(&hba->ufs_stats.resume_err, (u32)ret);
> +}
> +#endif
> +
> /**
> * ufshcd_resume - helper function for resume operations
> * @hba: per adapter instance
> @@ -8370,6 +8444,14 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> * A full initialization of the host and the device is
> * required since the link was put to off during suspend.
> */
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> + /*
> + * Assuems error free since ufshcd_probe_hba failure is
> + * uncorrectable.
> + */
> + ufshcd_async_schedule(ufshcd_async_resume, hba);
> + goto out_new;
> +#else
> ret = ufshcd_reset_and_restore(hba);
> /*
> * ufshcd_reset_and_restore() should have already
> @@ -8377,38 +8459,12 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> */
> if (ret || !ufshcd_is_link_active(hba))
> goto vendor_suspend;
> +#endif
> }
>
> - if (!ufshcd_is_ufs_dev_active(hba)) {
> - ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> - if (ret)
> - goto set_old_link_state;
> - }
> -
> - if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> - ufshcd_enable_auto_bkops(hba);
> - else
> - /*
> - * If BKOPs operations are urgently needed at this moment then
> - * keep auto-bkops enabled or else disable it.
> - */
> - ufshcd_urgent_bkops(hba);
> -
> - hba->clk_gating.is_suspended = false;
> -
> - if (hba->clk_scaling.is_allowed)
> - ufshcd_resume_clkscaling(hba);
> -
> - /* Enable Auto-Hibernate if configured */
> - ufshcd_auto_hibern8_enable(hba);
> -
> - if (hba->dev_info.b_rpm_dev_flush_capable) {
> - hba->dev_info.b_rpm_dev_flush_capable = false;
> - cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> - }
> -
> - /* Schedule clock gating in case of no access to UFS device yet */
> - ufshcd_release(hba);
> + ret = ufshcd_post_resume(hba);
> + if (ret)
> + goto set_old_link_state;
>
> goto out;
>
> @@ -8427,6 +8483,10 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> hba->pm_op_in_progress = 0;
> if (ret)
> ufshcd_update_reg_hist(&hba->ufs_stats.resume_err, (u32)ret);
> + /* For async init, pm_op_in_progress still needs to be one */
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> +out_new:
> +#endif
> return ret;
> }
Thanks,
Avri
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-07 6:05 ` Avri Altman
@ 2020-07-07 6:09 ` Kiwoong Kim
2020-07-07 6:50 ` Grant Jung
2020-07-15 8:35 ` Kiwoong Kim
1 sibling, 1 reply; 9+ messages in thread
From: Kiwoong Kim @ 2020-07-07 6:09 UTC (permalink / raw)
To: 'Avri Altman', linux-scsi, alim.akhtar, jejb,
martin.petersen, beanhuo, asutoshd, cang, bvanassche, grant.jung,
sc.suh, hy50.seo, sh425.lee
> > When you set uic_link_state during sleep statae to UIC_LINK_OFF_STATE,
> > UFS driver does interface initialization that is a series of some
> > steps including fDeviceInit and thus, You might feel that its latency
> > is a little bit longer.
> >
> > This patch is run it asynchronously to reduce system wake-up time.
> Can you share your initial testing findings?
> How much time does it save?
Will share when I'm done. And I think you might already know and
the time is variant per device and its situation, particularly for fDeviceInit.
Thanks.
Kiwoong Kim
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-07 6:09 ` Kiwoong Kim
@ 2020-07-07 6:50 ` Grant Jung
0 siblings, 0 replies; 9+ messages in thread
From: Grant Jung @ 2020-07-07 6:50 UTC (permalink / raw)
To: 'Kiwoong Kim', 'Avri Altman', linux-scsi,
alim.akhtar, jejb, martin.petersen, beanhuo, asutoshd, cang,
bvanassche, sc.suh, hy50.seo, sh425.lee
> > > When you set uic_link_state during sleep statae to
> > > UIC_LINK_OFF_STATE, UFS driver does interface initialization that is
> > > a series of some steps including fDeviceInit and thus, You might
> > > feel that its latency is a little bit longer.
> > >
> > > This patch is run it asynchronously to reduce system wake-up time.
> > Can you share your initial testing findings?
> > How much time does it save?
>
> Will share when I'm done. And I think you might already know and the time
> is variant per device and its situation, particularly for fDeviceInit.
>
> Thanks.
> Kiwoong Kim
I think it depends on each platform.
I has used this since years ago to reduce system wake-up time and could save about 60ms at that time.
BR
Grant.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-07 6:05 ` Avri Altman
2020-07-07 6:09 ` Kiwoong Kim
@ 2020-07-15 8:35 ` Kiwoong Kim
1 sibling, 0 replies; 9+ messages in thread
From: Kiwoong Kim @ 2020-07-15 8:35 UTC (permalink / raw)
To: 'Avri Altman', linux-scsi, alim.akhtar, jejb,
martin.petersen, beanhuo, asutoshd, cang, bvanassche, grant.jung,
sc.suh, hy50.seo, sh425.lee
> Hi,
>
> >
> > When you set uic_link_state during sleep statae to UIC_LINK_OFF_STATE,
> > UFS driver does interface initialization that is a series of some
> > steps including fDeviceInit and thus, You might feel that its latency
> > is a little bit longer.
> >
> > This patch is run it asynchronously to reduce system wake-up time.
> Can you share your initial testing findings?
> How much time does it save?
For this, you can refer to the Grant's comment and
As you might know, the time reduction relies on devices,
situations - after spo or not or whatever.
The thing is that system wake-up time is very important for product makers
and the period that I'm seeing on this is not an amount that you can ignore.
>
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > ---
> > drivers/scsi/ufs/Kconfig | 10 ++++
> > drivers/scsi/ufs/ufshcd.c | 120
> > ++++++++++++++++++++++++++++++++++---------
> > ---
> > 2 files changed, 100 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig index
> > 8cd9026..723e7cb 100644
> > --- a/drivers/scsi/ufs/Kconfig
> > +++ b/drivers/scsi/ufs/Kconfig
> > @@ -172,3 +172,13 @@ config SCSI_UFS_EXYNOS
> >
> > Select this if you have UFS host controller on EXYNOS chipset.
> > If unsure, say N.
> > +
> > +config SCSI_UFSHCD_ASYNC_INIT
> > + bool "Asynchronous UFS interface initialization support"
> > + depends on SCSI_UFSHCD
> > + default n
> > + ---help---
> > + This selects the support of doing UFS interface initialization
> > + asynchronously when you set link state to link off,
> > + i.e. UIC_LINK_OFF_STATE, to reduce system wake-up time.
> > + Select this if you have UFS Host Controller.
> Maybe replace this config switch with platform capability?
> So each platform vendor can choose if he is using a sync vs async init?
Got it.
>
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 52abe82..b65d38c 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8319,6 +8319,80 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> > return ret;
> > }
> >
> > +static int ufshcd_post_resume(struct ufs_hba *hba)
> Why do you need to move this code around?
> If its async - then there is no shared code - you go through the full init
> flow, and just goto out?
With SCSI_UFSHCD_ASYNC_INIT, ufshcd_reset_and_restore would be run by
worker asynchronously and in this case, some stuffs that are supposed to run
after completion of ufshcd_reset_and_restore without SCSI_UFSHCD_ASYNC_INIT.
So the stuffs should be run somewhere in kworker context.
That's why I teared it.
>
> > +{
> > + int ret;
> > +
> > + if (!ufshcd_is_ufs_dev_active(hba)) {
> > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> > + if (ret)
> > + goto out;
> > + }
> > +
> > + if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> > + ufshcd_enable_auto_bkops(hba);
> > + else
> > + /*
> > + * If BKOPs operations are urgently needed at this moment
> then
> > + * keep auto-bkops enabled or else disable it.
> > + */
> > + ufshcd_urgent_bkops(hba);
> > +
> > + hba->clk_gating.is_suspended = false;
> > +
> > + if (hba->clk_scaling.is_allowed)
> > + ufshcd_resume_clkscaling(hba);
> > +
> > + /* Enable Auto-Hibernate if configured */
> > + ufshcd_auto_hibern8_enable(hba);
> > +
> > + if (hba->dev_info.b_rpm_dev_flush_capable) {
> > + hba->dev_info.b_rpm_dev_flush_capable = false;
> > + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> > + }
> > +
> > + /* Schedule clock gating in case of no access to UFS device yet
> */
> > + ufshcd_release(hba);
> > +out:
> > + return ret;
> > +}
> > +
> > +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> > +static void ufshcd_async_resume(void *data, async_cookie_t cookie) {
> > + struct ufs_hba *hba = (struct ufs_hba *)data;
> > + unsigned long flags;
> > + int ret = 0;
> > + int retries = 2;
> > +
> > + /* transition to block requests */
> > + spin_lock_irqsave(hba->host->host_lock, flags);
> > + hba->ufshcd_state = UFSHCD_STATE_RESET;
> > + spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +
> > + /* initialize, instead of set_old_link_state ?? */
> > + do {
> > + ret = ufshcd_reset_and_restore(hba);
> > + if (ret) {
> > + dev_err(hba->dev, "%s: reset and restore failed\n",
> > + __func__);
> > + spin_lock_irqsave(hba->host->host_lock, flags);
> > + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> > + hba->pm_op_in_progress = 0;
> > + spin_unlock_irqrestore(hba->host->host_lock, flags);
> > + return;
> > + }
> > + ret = ufshcd_post_resume(hba);
> > + } while (ret && --retries);
> > + if (ret)
> > + goto reset;
> > +
> > + hba->pm_op_in_progress = 0;
> > + if (ret)
> > + ufshcd_update_reg_hist(&hba->ufs_stats.resume_err,
> > +(u32)ret); } #endif
> > +
> > /**
> > * ufshcd_resume - helper function for resume operations
> > * @hba: per adapter instance
> > @@ -8370,6 +8444,14 @@ static int ufshcd_resume(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> > * A full initialization of the host and the device is
> > * required since the link was put to off during suspend.
> > */
> > +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> > + /*
> > + * Assuems error free since ufshcd_probe_hba failure is
> > + * uncorrectable.
> > + */
> > + ufshcd_async_schedule(ufshcd_async_resume, hba);
> > + goto out_new;
> > +#else
> > ret = ufshcd_reset_and_restore(hba);
> > /*
> > * ufshcd_reset_and_restore() should have already @@
> > -8377,38 +8459,12 @@ static int ufshcd_resume(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> > */
> > if (ret || !ufshcd_is_link_active(hba))
> > goto vendor_suspend;
> > +#endif
> > }
> >
> > - if (!ufshcd_is_ufs_dev_active(hba)) {
> > - ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> > - if (ret)
> > - goto set_old_link_state;
> > - }
> > -
> > - if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> > - ufshcd_enable_auto_bkops(hba);
> > - else
> > - /*
> > - * If BKOPs operations are urgently needed at this moment
> then
> > - * keep auto-bkops enabled or else disable it.
> > - */
> > - ufshcd_urgent_bkops(hba);
> > -
> > - hba->clk_gating.is_suspended = false;
> > -
> > - if (hba->clk_scaling.is_allowed)
> > - ufshcd_resume_clkscaling(hba);
> > -
> > - /* Enable Auto-Hibernate if configured */
> > - ufshcd_auto_hibern8_enable(hba);
> > -
> > - if (hba->dev_info.b_rpm_dev_flush_capable) {
> > - hba->dev_info.b_rpm_dev_flush_capable = false;
> > - cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> > - }
> > -
> > - /* Schedule clock gating in case of no access to UFS device yet
> */
> > - ufshcd_release(hba);
> > + ret = ufshcd_post_resume(hba);
> > + if (ret)
> > + goto set_old_link_state;
> >
> > goto out;
> >
> > @@ -8427,6 +8483,10 @@ static int ufshcd_resume(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> > hba->pm_op_in_progress = 0;
> > if (ret)
> > ufshcd_update_reg_hist(&hba->ufs_stats.resume_err,
> > (u32)ret);
> > + /* For async init, pm_op_in_progress still needs to be one */
> > +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> > +out_new:
> > +#endif
> > return ret;
> > }
>
> Thanks,
> Avri
>
> >
> > --
> > 2.7.4
Thanks.
Kiwoong Kim
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-02 8:20 ` [RFC PATCH v1] ufs: introduce async ufs interface initialization Kiwoong Kim
2020-07-02 15:43 ` kernel test robot
2020-07-07 6:05 ` Avri Altman
@ 2020-07-18 20:38 ` Bart Van Assche
2020-07-19 0:27 ` Can Guo
2020-07-19 5:16 ` Can Guo
4 siblings, 0 replies; 9+ messages in thread
From: Bart Van Assche @ 2020-07-18 20:38 UTC (permalink / raw)
To: Kiwoong Kim, linux-scsi, alim.akhtar, avri.altman, jejb,
martin.petersen, beanhuo, asutoshd, cang, grant.jung, sc.suh,
hy50.seo, sh425.lee
On 2020-07-02 01:20, Kiwoong Kim wrote:
> When you set uic_link_state during sleep statae to
> UIC_LINK_OFF_STATE, UFS driver does interface initialization
> that is a series of some steps including fDeviceInit and thus,
> You might feel that its latency is a little bit longer.
>
> This patch is run it asynchronously to reduce system wake-up time.
Device drivers like UFS should only perform tasks that are specific to
the supported device(s). Asynchronous resume from a sleep state is a
mechanism that may also benefit other device drivers. Please work with
the maintainers of the power management subsystem (Rafael J. Wysocki and
Pavel Machek) to integrate support for this feature in the kernel power
management subsystem. The kernel power management subsystem exists in
the directory kernel/power.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-02 8:20 ` [RFC PATCH v1] ufs: introduce async ufs interface initialization Kiwoong Kim
` (2 preceding siblings ...)
2020-07-18 20:38 ` Bart Van Assche
@ 2020-07-19 0:27 ` Can Guo
2020-07-19 5:16 ` Can Guo
4 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-07-19 0:27 UTC (permalink / raw)
To: Kiwoong Kim
Cc: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen,
beanhuo, asutoshd, bvanassche, grant.jung, sc.suh, hy50.seo,
sh425.lee
Hi Kiwoong,
On 2020-07-02 16:20, Kiwoong Kim wrote:
> When you set uic_link_state during sleep statae to
> UIC_LINK_OFF_STATE, UFS driver does interface initialization
> that is a series of some steps including fDeviceInit and thus,
> You might feel that its latency is a little bit longer.
>
> This patch is run it asynchronously to reduce system wake-up time.
Have you considered the existing async suspend/resume support from
kernel power management framework? Can device_enable_async_suspend()
serve your purpose?
I don't see how this change works in two ways
#1 During system resume, how do you make sure resume of children
devices come after the resume of their parent? To be more specific,
in UFS's case, how do you make sure scsi devices (sda, sdb...) start
to resume only AFTER hba resume is finished (I mean all steps in hba
probe finished, not just schedule the async resume work). Your test
passed only because that in current scsi device's resume ops, no actual
commands need to be sent during scsi devices resume. If some commands
need to be sent during scsi device resume (say SSU commands), you would
run into error since scsi commands are sent before host is fully
resumed.
If you use kernel power management framework, during resume, dpm_wait()
is called for each device to make its parent has finished resuming.
#2 ufshcd_resume() is called in both hba system and runtime resume path.
Your change even makes hba runtime resume ops "async". How can you make
sure that hba is resumed after pm_runtime_get_sync() returns? Besides,
I don't think it can work well along with block layer PM.
Thanks,
Can Guo
>
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
> drivers/scsi/ufs/Kconfig | 10 ++++
> drivers/scsi/ufs/ufshcd.c | 120
> ++++++++++++++++++++++++++++++++++------------
> 2 files changed, 100 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index 8cd9026..723e7cb 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -172,3 +172,13 @@ config SCSI_UFS_EXYNOS
>
> Select this if you have UFS host controller on EXYNOS chipset.
> If unsure, say N.
> +
> +config SCSI_UFSHCD_ASYNC_INIT
> + bool "Asynchronous UFS interface initialization support"
> + depends on SCSI_UFSHCD
> + default n
> + ---help---
> + This selects the support of doing UFS interface initialization
> + asynchronously when you set link state to link off,
> + i.e. UIC_LINK_OFF_STATE, to reduce system wake-up time.
> + Select this if you have UFS Host Controller.
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 52abe82..b65d38c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8319,6 +8319,80 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> return ret;
> }
>
> +static int ufshcd_post_resume(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + if (!ufshcd_is_ufs_dev_active(hba)) {
> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> + if (ret)
> + goto out;
> + }
> +
> + if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> + ufshcd_enable_auto_bkops(hba);
> + else
> + /*
> + * If BKOPs operations are urgently needed at this moment then
> + * keep auto-bkops enabled or else disable it.
> + */
> + ufshcd_urgent_bkops(hba);
> +
> + hba->clk_gating.is_suspended = false;
> +
> + if (hba->clk_scaling.is_allowed)
> + ufshcd_resume_clkscaling(hba);
> +
> + /* Enable Auto-Hibernate if configured */
> + ufshcd_auto_hibern8_enable(hba);
> +
> + if (hba->dev_info.b_rpm_dev_flush_capable) {
> + hba->dev_info.b_rpm_dev_flush_capable = false;
> + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> + }
> +
> + /* Schedule clock gating in case of no access to UFS device yet */
> + ufshcd_release(hba);
> +out:
> + return ret;
> +}
> +
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> +static void ufshcd_async_resume(void *data, async_cookie_t cookie)
> +{
> + struct ufs_hba *hba = (struct ufs_hba *)data;
> + unsigned long flags;
> + int ret = 0;
> + int retries = 2;
> +
> + /* transition to block requests */
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->ufshcd_state = UFSHCD_STATE_RESET;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + /* initialize, instead of set_old_link_state ?? */
> + do {
> + ret = ufshcd_reset_and_restore(hba);
> + if (ret) {
> + dev_err(hba->dev, "%s: reset and restore failed\n",
> + __func__);
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + hba->pm_op_in_progress = 0;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + return;
> + }
> + ret = ufshcd_post_resume(hba);
> + } while (ret && --retries);
> + if (ret)
> + goto reset;
> +
> + hba->pm_op_in_progress = 0;
> + if (ret)
> + ufshcd_update_reg_hist(&hba->ufs_stats.resume_err, (u32)ret);
> +}
> +#endif
> +
> /**
> * ufshcd_resume - helper function for resume operations
> * @hba: per adapter instance
> @@ -8370,6 +8444,14 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> * A full initialization of the host and the device is
> * required since the link was put to off during suspend.
> */
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> + /*
> + * Assuems error free since ufshcd_probe_hba failure is
> + * uncorrectable.
> + */
> + ufshcd_async_schedule(ufshcd_async_resume, hba);
> + goto out_new;
> +#else
> ret = ufshcd_reset_and_restore(hba);
> /*
> * ufshcd_reset_and_restore() should have already
> @@ -8377,38 +8459,12 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> */
> if (ret || !ufshcd_is_link_active(hba))
> goto vendor_suspend;
> +#endif
> }
>
> - if (!ufshcd_is_ufs_dev_active(hba)) {
> - ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> - if (ret)
> - goto set_old_link_state;
> - }
> -
> - if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> - ufshcd_enable_auto_bkops(hba);
> - else
> - /*
> - * If BKOPs operations are urgently needed at this moment then
> - * keep auto-bkops enabled or else disable it.
> - */
> - ufshcd_urgent_bkops(hba);
> -
> - hba->clk_gating.is_suspended = false;
> -
> - if (hba->clk_scaling.is_allowed)
> - ufshcd_resume_clkscaling(hba);
> -
> - /* Enable Auto-Hibernate if configured */
> - ufshcd_auto_hibern8_enable(hba);
> -
> - if (hba->dev_info.b_rpm_dev_flush_capable) {
> - hba->dev_info.b_rpm_dev_flush_capable = false;
> - cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> - }
> -
> - /* Schedule clock gating in case of no access to UFS device yet */
> - ufshcd_release(hba);
> + ret = ufshcd_post_resume(hba);
> + if (ret)
> + goto set_old_link_state;
>
> goto out;
>
> @@ -8427,6 +8483,10 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> hba->pm_op_in_progress = 0;
> if (ret)
> ufshcd_update_reg_hist(&hba->ufs_stats.resume_err, (u32)ret);
> + /* For async init, pm_op_in_progress still needs to be one */
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> +out_new:
> +#endif
> return ret;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH v1] ufs: introduce async ufs interface initialization
2020-07-02 8:20 ` [RFC PATCH v1] ufs: introduce async ufs interface initialization Kiwoong Kim
` (3 preceding siblings ...)
2020-07-19 0:27 ` Can Guo
@ 2020-07-19 5:16 ` Can Guo
4 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-07-19 5:16 UTC (permalink / raw)
To: Kiwoong Kim
Cc: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen,
beanhuo, asutoshd, bvanassche, grant.jung, sc.suh, hy50.seo,
sh425.lee
Resent, typo fixed
Hi Kiwoong,
On 2020-07-02 16:20, Kiwoong Kim wrote:
> When you set uic_link_state during sleep statae to
> UIC_LINK_OFF_STATE, UFS driver does interface initialization
> that is a series of some steps including fDeviceInit and thus,
> You might feel that its latency is a little bit longer.
>
> This patch is run it asynchronously to reduce system wake-up time.
>
Have you considered the existing async suspend/resume support from
kernel power management framework? Can device_enable_async_suspend()
serve your purpose?
I don't see how this change works in two ways. With this change,
hba resume ops just schedules an async resume work and returns.
#1 During system resume, how do you make sure resume of children
devices come after the resume of their parent? To be more specific,
in UFS's case, how do you make sure scsi devices (sda, sdb...) start
to resume only AFTER hba resume is finished (I mean all steps in hba
probe finished, not just schedule the async resume work)? Your test
passed only because that in current UFS scsi device/target/host's resume
ops, no actual commands need to be sent. If some commands need to be
sent from scsi dev resume ops (say SSU commands sent by sd_resume if
sdp->manage_start_stop == 1), you would run into error since scsi
commands
may be sent before host is fully resumed. If you use kernel power
management framework, during system resume, dpm_wait() is called for
each device to make sure its parent has finished resuming.
#2 ufshcd_resume() is called in both hba system and runtime resume path.
How can you make sure that hba is fully resumed after
pm_runtime_get_sync()
returns? Besides, similar doubt as above in #1, how do you make sure
that,
during runtime resume, scsi device runtime resume ops happen after hba
has
been fully resumed? I don't think it can even work along with block
layer PM.
Thanks,
Can Guo
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
> drivers/scsi/ufs/Kconfig | 10 ++++
> drivers/scsi/ufs/ufshcd.c | 120
> ++++++++++++++++++++++++++++++++++------------
> 2 files changed, 100 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index 8cd9026..723e7cb 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -172,3 +172,13 @@ config SCSI_UFS_EXYNOS
>
> Select this if you have UFS host controller on EXYNOS chipset.
> If unsure, say N.
> +
> +config SCSI_UFSHCD_ASYNC_INIT
> + bool "Asynchronous UFS interface initialization support"
> + depends on SCSI_UFSHCD
> + default n
> + ---help---
> + This selects the support of doing UFS interface initialization
> + asynchronously when you set link state to link off,
> + i.e. UIC_LINK_OFF_STATE, to reduce system wake-up time.
> + Select this if you have UFS Host Controller.
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 52abe82..b65d38c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8319,6 +8319,80 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> return ret;
> }
>
> +static int ufshcd_post_resume(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + if (!ufshcd_is_ufs_dev_active(hba)) {
> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> + if (ret)
> + goto out;
> + }
> +
> + if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> + ufshcd_enable_auto_bkops(hba);
> + else
> + /*
> + * If BKOPs operations are urgently needed at this moment then
> + * keep auto-bkops enabled or else disable it.
> + */
> + ufshcd_urgent_bkops(hba);
> +
> + hba->clk_gating.is_suspended = false;
> +
> + if (hba->clk_scaling.is_allowed)
> + ufshcd_resume_clkscaling(hba);
> +
> + /* Enable Auto-Hibernate if configured */
> + ufshcd_auto_hibern8_enable(hba);
> +
> + if (hba->dev_info.b_rpm_dev_flush_capable) {
> + hba->dev_info.b_rpm_dev_flush_capable = false;
> + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> + }
> +
> + /* Schedule clock gating in case of no access to UFS device yet */
> + ufshcd_release(hba);
> +out:
> + return ret;
> +}
> +
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> +static void ufshcd_async_resume(void *data, async_cookie_t cookie)
> +{
> + struct ufs_hba *hba = (struct ufs_hba *)data;
> + unsigned long flags;
> + int ret = 0;
> + int retries = 2;
> +
> + /* transition to block requests */
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->ufshcd_state = UFSHCD_STATE_RESET;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> + /* initialize, instead of set_old_link_state ?? */
> + do {
> + ret = ufshcd_reset_and_restore(hba);
> + if (ret) {
> + dev_err(hba->dev, "%s: reset and restore failed\n",
> + __func__);
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + hba->pm_op_in_progress = 0;
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + return;
> + }
> + ret = ufshcd_post_resume(hba);
> + } while (ret && --retries);
> + if (ret)
> + goto reset;
> +
> + hba->pm_op_in_progress = 0;
> + if (ret)
> + ufshcd_update_reg_hist(&hba->ufs_stats.resume_err, (u32)ret);
> +}
> +#endif
> +
> /**
> * ufshcd_resume - helper function for resume operations
> * @hba: per adapter instance
> @@ -8370,6 +8444,14 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> * A full initialization of the host and the device is
> * required since the link was put to off during suspend.
> */
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> + /*
> + * Assuems error free since ufshcd_probe_hba failure is
> + * uncorrectable.
> + */
> + ufshcd_async_schedule(ufshcd_async_resume, hba);
> + goto out_new;
> +#else
> ret = ufshcd_reset_and_restore(hba);
> /*
> * ufshcd_reset_and_restore() should have already
> @@ -8377,38 +8459,12 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> */
> if (ret || !ufshcd_is_link_active(hba))
> goto vendor_suspend;
> +#endif
> }
>
> - if (!ufshcd_is_ufs_dev_active(hba)) {
> - ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> - if (ret)
> - goto set_old_link_state;
> - }
> -
> - if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> - ufshcd_enable_auto_bkops(hba);
> - else
> - /*
> - * If BKOPs operations are urgently needed at this moment then
> - * keep auto-bkops enabled or else disable it.
> - */
> - ufshcd_urgent_bkops(hba);
> -
> - hba->clk_gating.is_suspended = false;
> -
> - if (hba->clk_scaling.is_allowed)
> - ufshcd_resume_clkscaling(hba);
> -
> - /* Enable Auto-Hibernate if configured */
> - ufshcd_auto_hibern8_enable(hba);
> -
> - if (hba->dev_info.b_rpm_dev_flush_capable) {
> - hba->dev_info.b_rpm_dev_flush_capable = false;
> - cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> - }
> -
> - /* Schedule clock gating in case of no access to UFS device yet */
> - ufshcd_release(hba);
> + ret = ufshcd_post_resume(hba);
> + if (ret)
> + goto set_old_link_state;
>
> goto out;
>
> @@ -8427,6 +8483,10 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> hba->pm_op_in_progress = 0;
> if (ret)
> ufshcd_update_reg_hist(&hba->ufs_stats.resume_err, (u32)ret);
> + /* For async init, pm_op_in_progress still needs to be one */
> +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> +out_new:
> +#endif
> return ret;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread