Re: [RFC PATCH v1] ufs: introduce async ufs interface initialization

[Can Guo <cang@codeaurora.org>]

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;
>  }