From: Adrian Hunter <adrian.hunter@intel.com>
To: Asutosh Das <asutoshd@codeaurora.org>,
cang@codeaurora.org, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org,
Alim Akhtar <alim.akhtar@samsung.com>,
Avri Altman <avri.altman@wdc.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Stanley Chu <stanley.chu@mediatek.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Lee Jones <lee.jones@linaro.org>, Bean Huo <beanhuo@micron.com>,
Kiwoong Kim <kwmad.kim@samsung.com>,
Wei Yongjun <weiyongjun1@huawei.com>,
Dan Carpenter <dan.carpenter@oracle.com>,
Yue Hu <huyue2@yulong.com>, Dinghao Liu <dinghao.liu@zju.edu.cn>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Satya Tangirala <satyat@google.com>,
open list <linux-kernel@vger.kernel.org>,
"moderated list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES"
<linux-arm-kernel@lists.infradead.org>,
"open list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES"
<linux-samsung-soc@vger.kernel.org>,
"moderated list:UNIVERSAL FLASH STORAGE HOST CONTROLLER
DRIVER..." <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
Date: Sun, 11 Apr 2021 20:52:04 +0300 [thread overview]
Message-ID: <0e967ede-e233-8202-d638-5b1d24144192@intel.com> (raw)
In-Reply-To: <1b3d53dad245a7166f3f67a4c65f3a731e6600b3.1617893198.git.asutoshd@codeaurora.org>
On 8/04/21 5:49 pm, Asutosh Das wrote:
> During runtime-suspend of ufs host, the scsi devices are
> already suspended and so are the queues associated with them.
> But the ufs host sends SSU (START_STOP_UNIT) to wlun
> during its runtime-suspend.
> During the process blk_queue_enter checks if the queue is not in
> suspended state. If so, it waits for the queue to resume, and never
> comes out of it.
> The commit
> (d55d15a33: scsi: block: Do not accept any requests while suspended)
> adds the check if the queue is in suspended state in blk_queue_enter().
>
> Call trace:
> __switch_to+0x174/0x2c4
> __schedule+0x478/0x764
> schedule+0x9c/0xe0
> blk_queue_enter+0x158/0x228
> blk_mq_alloc_request+0x40/0xa4
> blk_get_request+0x2c/0x70
> __scsi_execute+0x60/0x1c4
> ufshcd_set_dev_pwr_mode+0x124/0x1e4
> ufshcd_suspend+0x208/0x83c
> ufshcd_runtime_suspend+0x40/0x154
> ufshcd_pltfrm_runtime_suspend+0x14/0x20
> pm_generic_runtime_suspend+0x28/0x3c
> __rpm_callback+0x80/0x2a4
> rpm_suspend+0x308/0x614
> rpm_idle+0x158/0x228
> pm_runtime_work+0x84/0xac
> process_one_work+0x1f0/0x470
> worker_thread+0x26c/0x4c8
> kthread+0x13c/0x320
> ret_from_fork+0x10/0x18
>
> Fix this by registering ufs device wlun as a scsi driver and
> registering it for block runtime-pm. Also make this as a
> supplier for all other luns. That way, this device wlun
> suspends after all the consumers and resumes after
> hba resumes.
>
> Co-developed-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
<SNIP>
> @@ -5815,14 +5857,14 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba)
>
> hba->is_sys_suspended = false;
> /*
> - * Set RPM status of hba device to RPM_ACTIVE,
> + * Set RPM status of wlun device to RPM_ACTIVE,
> * this also clears its runtime error.
> */
> - ret = pm_runtime_set_active(hba->dev);
> + ret = pm_runtime_set_active(&hba->sdev_ufs_device->sdev_gendev);
It may be that it was hba->dev with a runtime error. So,
also need here:
if (ret)
ret = pm_runtime_set_active(hba->dev);
> @@ -8671,7 +8701,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> enum ufs_dev_pwr_mode req_dev_pwr_mode;
> enum uic_link_state req_link_state;
>
> - hba->pm_op_in_progress = 1;
> + hba->pm_op_in_progress = true;
> if (!ufshcd_is_shutdown_pm(pm_op)) {
> pm_lvl = ufshcd_is_runtime_pm(pm_op) ?
> hba->rpm_lvl : hba->spm_lvl;
> @@ -8694,17 +8724,17 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>
> if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
> req_link_state == UIC_LINK_ACTIVE_STATE) {
> - goto disable_clks;
> + goto enable_scaling;
Previously "goto disable_clks" would go to the vendor callback
ufshcd_vops_suspend(), but now it is skipped.
> }
>
> if ((req_dev_pwr_mode == hba->curr_dev_pwr_mode) &&
> (req_link_state == hba->uic_link_state))
> - goto enable_gating;
> + goto enable_scaling;
>
> /* UFS device & link must be active before we enter in this function */
> if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba)) {
> ret = -EINVAL;
> - goto enable_gating;
> + goto enable_scaling;
> }
<SNIP>
> +/**
> + * ufshcd_suspend - helper function for suspend operations
> + * @hba: per adapter instance
> + *
> + * This function will put disable irqs, turn off clocks
> + * and set vreg and hba-vreg in lpm mode.
> + * Also check the description of __ufshcd_wl_suspend().
> + */
> +static void ufshcd_suspend(struct ufs_hba *hba)
> +{
> + hba->pm_op_in_progress = 1;
Seems like pm_op_in_progress wouldn't be needed in this function,
nor ufshcd_resume().
You could probably simplify the callers slighly by
putting the "if (!hbs->is_powered) return" check here and in
ufshcd_resume().
> +
> + /*
> + * Disable the host irq as host controller as there won't be any
> + * host controller transaction expected till resume.
> + */
> ufshcd_disable_irq(hba);
> ufshcd_setup_clocks(hba, false);
> if (ufshcd_is_clkgating_allowed(hba)) {
> @@ -8948,6 +9054,43 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> trace_ufshcd_clk_gating(dev_name(hba->dev),
> hba->clk_gating.state);
> }
> +
> + ufshcd_vreg_set_lpm(hba);
> + /* Put the host controller in low power mode if possible */
> + ufshcd_hba_vreg_set_lpm(hba);
> + hba->pm_op_in_progress = 0;
> +}
> +
> +/**
> + * ufshcd_resume - helper function for resume operations
> + * @hba: per adapter instance
> + *
> + * This function basically turns on the regulators, clocks and
> + * irqs of the hba.
> + * Also check the description of __ufshcd_wl_resume().
> + *
> + * Returns 0 for success and non-zero for failure
> + */
> +static int ufshcd_resume(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + hba->pm_op_in_progress = 1;
> +
> + ufshcd_hba_vreg_set_hpm(hba);
> + ret = ufshcd_vreg_set_hpm(hba);
> + if (ret)
> + goto out;
> +
> + /* Make sure clocks are enabled before accessing controller */
> + ret = ufshcd_setup_clocks(hba, true);
> + if (ret)
> + goto disable_vreg;
> +
> + /* enable the host irq as host controller would be active soon */
> + ufshcd_enable_irq(hba);
> + goto out;
> +
> disable_vreg:
> ufshcd_vreg_set_lpm(hba);
> out:
> @@ -8962,6 +9105,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> * @hba: per adapter instance
> *
> * Check the description of ufshcd_suspend() function for more details.
> + * Also check the description of __ufshcd_wl_suspend().
> *
> * Returns 0 for success and non-zero for failure
> */
> @@ -8982,21 +9126,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
> hba->curr_dev_pwr_mode) &&
> (ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl) ==
> hba->uic_link_state) &&
> !hba->dev_info.b_rpm_dev_flush_capable)
> goto out;
The logic above can be removed.
>
> - if (pm_runtime_suspended(hba->dev)) {
> - /*
> - * UFS device and/or UFS link low power states during runtime
> - * suspend seems to be different than what is expected during
> - * system suspend. Hence runtime resume the devic & link and
> - * let the system suspend low power states to take effect.
> - * TODO: If resume takes longer time, we might have optimize
> - * it in future by not resuming everything if possible.
> - */
> - ret = ufshcd_runtime_resume(hba);
> - if (ret)
> - goto out;
> - }
System suspend/resume and runtime suspend/resume are identical
for hba->dev, so the logic becomes:
if (pm_runtime_suspended(hba->dev))
goto out;
> -
> - ret = ufshcd_suspend(hba, UFS_SYSTEM_PM);
> + ufshcd_suspend(hba);
> out:
> trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> @@ -9018,25 +9148,20 @@ EXPORT_SYMBOL(ufshcd_system_suspend);
>
> int ufshcd_system_resume(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> - if (!hba->is_powered || pm_runtime_suspended(hba->dev))
> - /*
> - * Let the runtime resume take care of resuming
> - * if runtime suspended.
> - */
> + if (!hba->is_powered)
System suspend/resume and runtime suspend/resume are identical
for hba->dev, so the original logic is fine here.
> goto out;
> else
> - ret = ufshcd_resume(hba, UFS_SYSTEM_PM);
> + ufshcd_resume(hba);
> out:
> - trace_ufshcd_system_resume(dev_name(hba->dev), ret,
> + trace_ufshcd_system_resume(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - if (!ret)
> - hba->is_sys_suspended = false;
> +
> + hba->is_sys_suspended = false;
> up(&hba->host_sem);
It seems likely that hba->host_sem down / up should be in
ufshcd_wl_suspend() / ufshcd_wl_resume() respectively, instead
of here.
Ditto hba->is_sys_suspended
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_system_resume);
>
> @@ -9045,23 +9170,23 @@ EXPORT_SYMBOL(ufshcd_system_resume);
> * @hba: per adapter instance
> *
> * Check the description of ufshcd_suspend() function for more details.
> + * Also check the description of __ufshcd_wl_suspend().
> *
> * Returns 0 for success and non-zero for failure
> */
> int ufshcd_runtime_suspend(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> if (!hba->is_powered)
> goto out;
> else
> - ret = ufshcd_suspend(hba, UFS_RUNTIME_PM);
> + ufshcd_suspend(hba);
> out:
> - trace_ufshcd_runtime_suspend(dev_name(hba->dev), ret,
> + trace_ufshcd_runtime_suspend(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_runtime_suspend);
>
> @@ -9069,37 +9194,25 @@ EXPORT_SYMBOL(ufshcd_runtime_suspend);
> * ufshcd_runtime_resume - runtime resume routine
> * @hba: per adapter instance
> *
> - * This function basically brings the UFS device, UniPro link and controller
> + * This function basically brings controller
> * to active state. Following operations are done in this function:
> *
> * 1. Turn on all the controller related clocks
> - * 2. Bring the UniPro link out of Hibernate state
> - * 3. If UFS device is in sleep state, turn ON VCC rail and bring the UFS device
> - * to active state.
> - * 4. If auto-bkops is enabled on the device, disable it.
> - *
> - * So following would be the possible power state after this function return
> - * successfully:
> - * S1: UFS device in Active state with VCC rail ON
> - * UniPro link in Active state
> - * All the UFS/UniPro controller clocks are ON
> - *
> - * Returns 0 for success and non-zero for failure
> + * 2. Turn ON VCC rail
> */
> int ufshcd_runtime_resume(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> if (!hba->is_powered)
> goto out;
> else
> - ret = ufshcd_resume(hba, UFS_RUNTIME_PM);
> + ufshcd_resume(hba);
> out:
> - trace_ufshcd_runtime_resume(dev_name(hba->dev), ret,
> + trace_ufshcd_runtime_resume(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_runtime_resume);
>
> @@ -9113,18 +9226,13 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
> * ufshcd_shutdown - shutdown routine
> * @hba: per adapter instance
> *
> - * This function would power off both UFS device and UFS link.
> + * This function would turn off both UFS device and UFS hba
> + * regulators. It would also disable clocks.
> *
> * Returns 0 always to allow force shutdown even in case of errors.
> */
> int ufshcd_shutdown(struct ufs_hba *hba)
> {
> - int ret = 0;
> -
> - down(&hba->host_sem);
> - hba->shutting_down = true;
> - up(&hba->host_sem);
> -
> if (!hba->is_powered)
> goto out;
>
> @@ -9133,10 +9241,8 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>
> pm_runtime_get_sync(hba->dev);
>
> - ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
> + ufshcd_suspend(hba);
> out:
> - if (ret)
> - dev_err(hba->dev, "%s failed, err %d\n", __func__, ret);
> hba->is_powered = false;
> /* allow force shutdown even in case of errors */
> return 0;
> @@ -9150,6 +9256,8 @@ EXPORT_SYMBOL(ufshcd_shutdown);
> */
> void ufshcd_remove(struct ufs_hba *hba)
> {
> + if (hba->sdev_ufs_device)
> + scsi_autopm_get_device(hba->sdev_ufs_device);
> ufs_bsg_remove(hba);
> ufs_sysfs_remove_nodes(hba->dev);
> blk_cleanup_queue(hba->tmf_queue);
> @@ -9453,15 +9561,175 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> }
> EXPORT_SYMBOL_GPL(ufshcd_init);
>
> +void ufshcd_resume_complete(struct device *dev)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + pm_runtime_put_noidle(&hba->sdev_ufs_device->sdev_gendev);
pm_runtime_put() instead of pm_runtime_put_noidle() would be
more correct here.
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
> +
> +int ufshcd_suspend_prepare(struct device *dev)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + /*
> + * SCSI assumes that runtime-pm and system-pm for scsi drivers
> + * are same. And it doesn't wake up the device for system-suspend
> + * if it's runtime suspended. But ufs doesn't follow that.
> + * The rpm-lvl and spm-lvl can be different in ufs.
> + * Force it to honor system-suspend.
> + */
> + scsi_autopm_get_device(hba->sdev_ufs_device);
> + /* Refer ufshcd_resume_complete() */
> + pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev);
> + scsi_autopm_put_device(hba->sdev_ufs_device);
This could simply be:
pm_runtime_get_sync(&hba->sdev_ufs_device->sdev_gendev);
or new wrapper.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ufshcd_wl_poweroff(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
> + struct ufs_hba *hba = shost_priv(sdev->host);
> +
> + __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> + return 0;
> +}
> +#endif
> +
> +static int ufshcd_wl_probe(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
> +
> + if (!is_device_wlun(sdev))
> + return -ENODEV;
> +
> + blk_pm_runtime_init(sdev->request_queue, dev);
> + pm_runtime_set_autosuspend_delay(dev, 0);
> + pm_runtime_allow(dev);
> +
> + return 0;
> +}
> +
> +static int ufshcd_wl_remove(struct device *dev)
> +{
> + pm_runtime_forbid(dev);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ufshcd_wl_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> + .suspend = ufshcd_wl_suspend,
> + .resume = ufshcd_wl_resume,
> + .freeze = ufshcd_wl_suspend,
> + .thaw = ufshcd_wl_resume,
> + .poweroff = ufshcd_wl_poweroff,
> + .restore = ufshcd_wl_resume,
> +#endif
> + SET_RUNTIME_PM_OPS(ufshcd_wl_runtime_suspend, ufshcd_wl_runtime_resume, NULL)
> +};
> +
> +/**
> + * ufs_dev_wlun_template - describes ufs device wlun
> + * ufs-device wlun - used to send pm commands
> + * All luns are consumers of ufs-device wlun.
> + *
> + * Currently, no sd driver is present for wluns.
> + * Hence the no specific pm operations are performed.
> + * With ufs design, SSU should be sent to ufs-device wlun.
> + * Hence register a scsi driver for ufs wluns only.
> + */
> +static struct scsi_driver ufs_dev_wlun_template = {
> + .gendrv = {
> + .name = "ufs_device_wlun",
> + .owner = THIS_MODULE,
> + .probe = ufshcd_wl_probe,
> + .remove = ufshcd_wl_remove,
> + .pm = &ufshcd_wl_pm_ops,
> + .shutdown = ufshcd_wl_shutdown,
> + },
> +};
> +
> +static int ufshcd_rpmb_probe(struct device *dev)
> +{
> + return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
> +}
> +
> +static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
> +{
> + int ret = 0;
> +
> + if (!hba->wlun_rpmb_clr_ua)
> + return 0;
> + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> + if (!ret)
> + hba->wlun_rpmb_clr_ua = 0;
> + return ret;
> +}
> +
> +static int ufshcd_rpmb_runtime_resume(struct device *dev)
> +{
> + struct ufs_hba *hba = wlun_dev_to_hba(dev);
> +
> + if (hba->sdev_rpmb)
> + return ufshcd_clear_rpmb_uac(hba);
> + return 0;
> +}
> +
> +static int ufshcd_rpmb_resume(struct device *dev)
> +{
> + struct ufs_hba *hba = wlun_dev_to_hba(dev);
> +
> + if (hba->sdev_rpmb && !pm_runtime_suspended(dev))
> + return ufshcd_clear_rpmb_uac(hba);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ufs_rpmb_pm_ops = {
> + SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
> +};
> +
> +/**
> + * Describes the ufs rpmb wlun.
> + * Used only to send uac.
> + */
> +static struct scsi_driver ufs_rpmb_wlun_template = {
> + .gendrv = {
> + .name = "ufs_rpmb_wlun",
> + .owner = THIS_MODULE,
> + .probe = ufshcd_rpmb_probe,
> + .pm = &ufs_rpmb_pm_ops,
> + },
> +};
> +
> static int __init ufshcd_core_init(void)
> {
> + int ret;
> +
> ufs_debugfs_init();
> +
> + ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
> + if (ret) {
> + ufs_debugfs_exit();
> + return ret;
> + }
> + ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
> + if (ret) {
> + ufs_debugfs_exit();
> + scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> + return ret;
> + }
> return 0;
A separated error path is preferred i.e.
ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
if (ret)
goto debugfs_exit;
ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
if (ret)
goto unregister;
return 0;
unregister:
scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
debugfs_exit:
ufs_debugfs_exit();
return ret;
> }
>
> static void __exit ufshcd_core_exit(void)
> {
> ufs_debugfs_exit();
> + scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
> + scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> }
>
> module_init(ufshcd_core_init);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 5eb66a8..40436e3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -72,6 +72,8 @@ enum ufs_event_type {
> UFS_EVT_LINK_STARTUP_FAIL,
> UFS_EVT_RESUME_ERR,
> UFS_EVT_SUSPEND_ERR,
> + UFS_EVT_WL_SUSP_ERR,
> + UFS_EVT_WL_RES_ERR,
>
> /* abnormal events */
> UFS_EVT_DEV_RESET,
> @@ -807,6 +809,7 @@ struct ufs_hba {
> struct list_head clk_list_head;
>
> bool wlun_dev_clr_ua;
> + bool wlun_rpmb_clr_ua;
>
> /* Number of requests aborts */
> int req_abort_count;
> @@ -846,6 +849,7 @@ struct ufs_hba {
> struct delayed_work debugfs_ee_work;
> u32 debugfs_ee_rate_limit_ms;
> #endif
> + u32 luns_avail;
> };
>
> /* Returns true if clocks can be gated. Otherwise false */
> @@ -1105,6 +1109,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
> enum query_opcode desc_op);
>
> int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
> +int ufshcd_suspend_prepare(struct device *dev);
> +void ufshcd_resume_complete(struct device *dev);
>
> /* Wrapper functions for safely calling variant operations */
> static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 1cb6f1a..599739e 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -246,6 +246,26 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
> int dev_state, int link_state),
> TP_ARGS(dev_name, err, usecs, dev_state, link_state));
>
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_suspend,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_resume,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_suspend,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_resume,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> TRACE_EVENT(ufshcd_command,
> TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t,
> unsigned int tag, u32 doorbell, int transfer_len, u32 intr,
>
WARNING: multiple messages have this Message-ID (diff)
From: Adrian Hunter <adrian.hunter@intel.com>
To: Asutosh Das <asutoshd@codeaurora.org>,
cang@codeaurora.org, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org,
Alim Akhtar <alim.akhtar@samsung.com>,
Avri Altman <avri.altman@wdc.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Stanley Chu <stanley.chu@mediatek.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Lee Jones <lee.jones@linaro.org>, Bean Huo <beanhuo@micron.com>,
Kiwoong Kim <kwmad.kim@samsung.com>,
Wei Yongjun <weiyongjun1@huawei.com>,
Dan Carpenter <dan.carpenter@oracle.com>,
Yue Hu <huyue2@yulong.com>, Dinghao Liu <dinghao.liu@zju.edu.cn>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Satya Tangirala <satyat@google.com>,
open list <linux-kernel@vger.kernel.org>,
"moderated list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES"
<linux-arm-kernel@lists.infradead.org>,
"open list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES"
<linux-samsung-soc@vger.kernel.org>,
"moderated list:UNIVERSAL FLASH STORAGE HOST CONTROLLER
DRIVER..." <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
Date: Sun, 11 Apr 2021 20:52:04 +0300 [thread overview]
Message-ID: <0e967ede-e233-8202-d638-5b1d24144192@intel.com> (raw)
In-Reply-To: <1b3d53dad245a7166f3f67a4c65f3a731e6600b3.1617893198.git.asutoshd@codeaurora.org>
On 8/04/21 5:49 pm, Asutosh Das wrote:
> During runtime-suspend of ufs host, the scsi devices are
> already suspended and so are the queues associated with them.
> But the ufs host sends SSU (START_STOP_UNIT) to wlun
> during its runtime-suspend.
> During the process blk_queue_enter checks if the queue is not in
> suspended state. If so, it waits for the queue to resume, and never
> comes out of it.
> The commit
> (d55d15a33: scsi: block: Do not accept any requests while suspended)
> adds the check if the queue is in suspended state in blk_queue_enter().
>
> Call trace:
> __switch_to+0x174/0x2c4
> __schedule+0x478/0x764
> schedule+0x9c/0xe0
> blk_queue_enter+0x158/0x228
> blk_mq_alloc_request+0x40/0xa4
> blk_get_request+0x2c/0x70
> __scsi_execute+0x60/0x1c4
> ufshcd_set_dev_pwr_mode+0x124/0x1e4
> ufshcd_suspend+0x208/0x83c
> ufshcd_runtime_suspend+0x40/0x154
> ufshcd_pltfrm_runtime_suspend+0x14/0x20
> pm_generic_runtime_suspend+0x28/0x3c
> __rpm_callback+0x80/0x2a4
> rpm_suspend+0x308/0x614
> rpm_idle+0x158/0x228
> pm_runtime_work+0x84/0xac
> process_one_work+0x1f0/0x470
> worker_thread+0x26c/0x4c8
> kthread+0x13c/0x320
> ret_from_fork+0x10/0x18
>
> Fix this by registering ufs device wlun as a scsi driver and
> registering it for block runtime-pm. Also make this as a
> supplier for all other luns. That way, this device wlun
> suspends after all the consumers and resumes after
> hba resumes.
>
> Co-developed-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
<SNIP>
> @@ -5815,14 +5857,14 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba)
>
> hba->is_sys_suspended = false;
> /*
> - * Set RPM status of hba device to RPM_ACTIVE,
> + * Set RPM status of wlun device to RPM_ACTIVE,
> * this also clears its runtime error.
> */
> - ret = pm_runtime_set_active(hba->dev);
> + ret = pm_runtime_set_active(&hba->sdev_ufs_device->sdev_gendev);
It may be that it was hba->dev with a runtime error. So,
also need here:
if (ret)
ret = pm_runtime_set_active(hba->dev);
> @@ -8671,7 +8701,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> enum ufs_dev_pwr_mode req_dev_pwr_mode;
> enum uic_link_state req_link_state;
>
> - hba->pm_op_in_progress = 1;
> + hba->pm_op_in_progress = true;
> if (!ufshcd_is_shutdown_pm(pm_op)) {
> pm_lvl = ufshcd_is_runtime_pm(pm_op) ?
> hba->rpm_lvl : hba->spm_lvl;
> @@ -8694,17 +8724,17 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>
> if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
> req_link_state == UIC_LINK_ACTIVE_STATE) {
> - goto disable_clks;
> + goto enable_scaling;
Previously "goto disable_clks" would go to the vendor callback
ufshcd_vops_suspend(), but now it is skipped.
> }
>
> if ((req_dev_pwr_mode == hba->curr_dev_pwr_mode) &&
> (req_link_state == hba->uic_link_state))
> - goto enable_gating;
> + goto enable_scaling;
>
> /* UFS device & link must be active before we enter in this function */
> if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba)) {
> ret = -EINVAL;
> - goto enable_gating;
> + goto enable_scaling;
> }
<SNIP>
> +/**
> + * ufshcd_suspend - helper function for suspend operations
> + * @hba: per adapter instance
> + *
> + * This function will put disable irqs, turn off clocks
> + * and set vreg and hba-vreg in lpm mode.
> + * Also check the description of __ufshcd_wl_suspend().
> + */
> +static void ufshcd_suspend(struct ufs_hba *hba)
> +{
> + hba->pm_op_in_progress = 1;
Seems like pm_op_in_progress wouldn't be needed in this function,
nor ufshcd_resume().
You could probably simplify the callers slighly by
putting the "if (!hbs->is_powered) return" check here and in
ufshcd_resume().
> +
> + /*
> + * Disable the host irq as host controller as there won't be any
> + * host controller transaction expected till resume.
> + */
> ufshcd_disable_irq(hba);
> ufshcd_setup_clocks(hba, false);
> if (ufshcd_is_clkgating_allowed(hba)) {
> @@ -8948,6 +9054,43 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> trace_ufshcd_clk_gating(dev_name(hba->dev),
> hba->clk_gating.state);
> }
> +
> + ufshcd_vreg_set_lpm(hba);
> + /* Put the host controller in low power mode if possible */
> + ufshcd_hba_vreg_set_lpm(hba);
> + hba->pm_op_in_progress = 0;
> +}
> +
> +/**
> + * ufshcd_resume - helper function for resume operations
> + * @hba: per adapter instance
> + *
> + * This function basically turns on the regulators, clocks and
> + * irqs of the hba.
> + * Also check the description of __ufshcd_wl_resume().
> + *
> + * Returns 0 for success and non-zero for failure
> + */
> +static int ufshcd_resume(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + hba->pm_op_in_progress = 1;
> +
> + ufshcd_hba_vreg_set_hpm(hba);
> + ret = ufshcd_vreg_set_hpm(hba);
> + if (ret)
> + goto out;
> +
> + /* Make sure clocks are enabled before accessing controller */
> + ret = ufshcd_setup_clocks(hba, true);
> + if (ret)
> + goto disable_vreg;
> +
> + /* enable the host irq as host controller would be active soon */
> + ufshcd_enable_irq(hba);
> + goto out;
> +
> disable_vreg:
> ufshcd_vreg_set_lpm(hba);
> out:
> @@ -8962,6 +9105,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> * @hba: per adapter instance
> *
> * Check the description of ufshcd_suspend() function for more details.
> + * Also check the description of __ufshcd_wl_suspend().
> *
> * Returns 0 for success and non-zero for failure
> */
> @@ -8982,21 +9126,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
> hba->curr_dev_pwr_mode) &&
> (ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl) ==
> hba->uic_link_state) &&
> !hba->dev_info.b_rpm_dev_flush_capable)
> goto out;
The logic above can be removed.
>
> - if (pm_runtime_suspended(hba->dev)) {
> - /*
> - * UFS device and/or UFS link low power states during runtime
> - * suspend seems to be different than what is expected during
> - * system suspend. Hence runtime resume the devic & link and
> - * let the system suspend low power states to take effect.
> - * TODO: If resume takes longer time, we might have optimize
> - * it in future by not resuming everything if possible.
> - */
> - ret = ufshcd_runtime_resume(hba);
> - if (ret)
> - goto out;
> - }
System suspend/resume and runtime suspend/resume are identical
for hba->dev, so the logic becomes:
if (pm_runtime_suspended(hba->dev))
goto out;
> -
> - ret = ufshcd_suspend(hba, UFS_SYSTEM_PM);
> + ufshcd_suspend(hba);
> out:
> trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> @@ -9018,25 +9148,20 @@ EXPORT_SYMBOL(ufshcd_system_suspend);
>
> int ufshcd_system_resume(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> - if (!hba->is_powered || pm_runtime_suspended(hba->dev))
> - /*
> - * Let the runtime resume take care of resuming
> - * if runtime suspended.
> - */
> + if (!hba->is_powered)
System suspend/resume and runtime suspend/resume are identical
for hba->dev, so the original logic is fine here.
> goto out;
> else
> - ret = ufshcd_resume(hba, UFS_SYSTEM_PM);
> + ufshcd_resume(hba);
> out:
> - trace_ufshcd_system_resume(dev_name(hba->dev), ret,
> + trace_ufshcd_system_resume(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - if (!ret)
> - hba->is_sys_suspended = false;
> +
> + hba->is_sys_suspended = false;
> up(&hba->host_sem);
It seems likely that hba->host_sem down / up should be in
ufshcd_wl_suspend() / ufshcd_wl_resume() respectively, instead
of here.
Ditto hba->is_sys_suspended
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_system_resume);
>
> @@ -9045,23 +9170,23 @@ EXPORT_SYMBOL(ufshcd_system_resume);
> * @hba: per adapter instance
> *
> * Check the description of ufshcd_suspend() function for more details.
> + * Also check the description of __ufshcd_wl_suspend().
> *
> * Returns 0 for success and non-zero for failure
> */
> int ufshcd_runtime_suspend(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> if (!hba->is_powered)
> goto out;
> else
> - ret = ufshcd_suspend(hba, UFS_RUNTIME_PM);
> + ufshcd_suspend(hba);
> out:
> - trace_ufshcd_runtime_suspend(dev_name(hba->dev), ret,
> + trace_ufshcd_runtime_suspend(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_runtime_suspend);
>
> @@ -9069,37 +9194,25 @@ EXPORT_SYMBOL(ufshcd_runtime_suspend);
> * ufshcd_runtime_resume - runtime resume routine
> * @hba: per adapter instance
> *
> - * This function basically brings the UFS device, UniPro link and controller
> + * This function basically brings controller
> * to active state. Following operations are done in this function:
> *
> * 1. Turn on all the controller related clocks
> - * 2. Bring the UniPro link out of Hibernate state
> - * 3. If UFS device is in sleep state, turn ON VCC rail and bring the UFS device
> - * to active state.
> - * 4. If auto-bkops is enabled on the device, disable it.
> - *
> - * So following would be the possible power state after this function return
> - * successfully:
> - * S1: UFS device in Active state with VCC rail ON
> - * UniPro link in Active state
> - * All the UFS/UniPro controller clocks are ON
> - *
> - * Returns 0 for success and non-zero for failure
> + * 2. Turn ON VCC rail
> */
> int ufshcd_runtime_resume(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> if (!hba->is_powered)
> goto out;
> else
> - ret = ufshcd_resume(hba, UFS_RUNTIME_PM);
> + ufshcd_resume(hba);
> out:
> - trace_ufshcd_runtime_resume(dev_name(hba->dev), ret,
> + trace_ufshcd_runtime_resume(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_runtime_resume);
>
> @@ -9113,18 +9226,13 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
> * ufshcd_shutdown - shutdown routine
> * @hba: per adapter instance
> *
> - * This function would power off both UFS device and UFS link.
> + * This function would turn off both UFS device and UFS hba
> + * regulators. It would also disable clocks.
> *
> * Returns 0 always to allow force shutdown even in case of errors.
> */
> int ufshcd_shutdown(struct ufs_hba *hba)
> {
> - int ret = 0;
> -
> - down(&hba->host_sem);
> - hba->shutting_down = true;
> - up(&hba->host_sem);
> -
> if (!hba->is_powered)
> goto out;
>
> @@ -9133,10 +9241,8 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>
> pm_runtime_get_sync(hba->dev);
>
> - ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
> + ufshcd_suspend(hba);
> out:
> - if (ret)
> - dev_err(hba->dev, "%s failed, err %d\n", __func__, ret);
> hba->is_powered = false;
> /* allow force shutdown even in case of errors */
> return 0;
> @@ -9150,6 +9256,8 @@ EXPORT_SYMBOL(ufshcd_shutdown);
> */
> void ufshcd_remove(struct ufs_hba *hba)
> {
> + if (hba->sdev_ufs_device)
> + scsi_autopm_get_device(hba->sdev_ufs_device);
> ufs_bsg_remove(hba);
> ufs_sysfs_remove_nodes(hba->dev);
> blk_cleanup_queue(hba->tmf_queue);
> @@ -9453,15 +9561,175 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> }
> EXPORT_SYMBOL_GPL(ufshcd_init);
>
> +void ufshcd_resume_complete(struct device *dev)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + pm_runtime_put_noidle(&hba->sdev_ufs_device->sdev_gendev);
pm_runtime_put() instead of pm_runtime_put_noidle() would be
more correct here.
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
> +
> +int ufshcd_suspend_prepare(struct device *dev)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + /*
> + * SCSI assumes that runtime-pm and system-pm for scsi drivers
> + * are same. And it doesn't wake up the device for system-suspend
> + * if it's runtime suspended. But ufs doesn't follow that.
> + * The rpm-lvl and spm-lvl can be different in ufs.
> + * Force it to honor system-suspend.
> + */
> + scsi_autopm_get_device(hba->sdev_ufs_device);
> + /* Refer ufshcd_resume_complete() */
> + pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev);
> + scsi_autopm_put_device(hba->sdev_ufs_device);
This could simply be:
pm_runtime_get_sync(&hba->sdev_ufs_device->sdev_gendev);
or new wrapper.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ufshcd_wl_poweroff(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
> + struct ufs_hba *hba = shost_priv(sdev->host);
> +
> + __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> + return 0;
> +}
> +#endif
> +
> +static int ufshcd_wl_probe(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
> +
> + if (!is_device_wlun(sdev))
> + return -ENODEV;
> +
> + blk_pm_runtime_init(sdev->request_queue, dev);
> + pm_runtime_set_autosuspend_delay(dev, 0);
> + pm_runtime_allow(dev);
> +
> + return 0;
> +}
> +
> +static int ufshcd_wl_remove(struct device *dev)
> +{
> + pm_runtime_forbid(dev);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ufshcd_wl_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> + .suspend = ufshcd_wl_suspend,
> + .resume = ufshcd_wl_resume,
> + .freeze = ufshcd_wl_suspend,
> + .thaw = ufshcd_wl_resume,
> + .poweroff = ufshcd_wl_poweroff,
> + .restore = ufshcd_wl_resume,
> +#endif
> + SET_RUNTIME_PM_OPS(ufshcd_wl_runtime_suspend, ufshcd_wl_runtime_resume, NULL)
> +};
> +
> +/**
> + * ufs_dev_wlun_template - describes ufs device wlun
> + * ufs-device wlun - used to send pm commands
> + * All luns are consumers of ufs-device wlun.
> + *
> + * Currently, no sd driver is present for wluns.
> + * Hence the no specific pm operations are performed.
> + * With ufs design, SSU should be sent to ufs-device wlun.
> + * Hence register a scsi driver for ufs wluns only.
> + */
> +static struct scsi_driver ufs_dev_wlun_template = {
> + .gendrv = {
> + .name = "ufs_device_wlun",
> + .owner = THIS_MODULE,
> + .probe = ufshcd_wl_probe,
> + .remove = ufshcd_wl_remove,
> + .pm = &ufshcd_wl_pm_ops,
> + .shutdown = ufshcd_wl_shutdown,
> + },
> +};
> +
> +static int ufshcd_rpmb_probe(struct device *dev)
> +{
> + return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
> +}
> +
> +static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
> +{
> + int ret = 0;
> +
> + if (!hba->wlun_rpmb_clr_ua)
> + return 0;
> + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> + if (!ret)
> + hba->wlun_rpmb_clr_ua = 0;
> + return ret;
> +}
> +
> +static int ufshcd_rpmb_runtime_resume(struct device *dev)
> +{
> + struct ufs_hba *hba = wlun_dev_to_hba(dev);
> +
> + if (hba->sdev_rpmb)
> + return ufshcd_clear_rpmb_uac(hba);
> + return 0;
> +}
> +
> +static int ufshcd_rpmb_resume(struct device *dev)
> +{
> + struct ufs_hba *hba = wlun_dev_to_hba(dev);
> +
> + if (hba->sdev_rpmb && !pm_runtime_suspended(dev))
> + return ufshcd_clear_rpmb_uac(hba);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ufs_rpmb_pm_ops = {
> + SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
> +};
> +
> +/**
> + * Describes the ufs rpmb wlun.
> + * Used only to send uac.
> + */
> +static struct scsi_driver ufs_rpmb_wlun_template = {
> + .gendrv = {
> + .name = "ufs_rpmb_wlun",
> + .owner = THIS_MODULE,
> + .probe = ufshcd_rpmb_probe,
> + .pm = &ufs_rpmb_pm_ops,
> + },
> +};
> +
> static int __init ufshcd_core_init(void)
> {
> + int ret;
> +
> ufs_debugfs_init();
> +
> + ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
> + if (ret) {
> + ufs_debugfs_exit();
> + return ret;
> + }
> + ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
> + if (ret) {
> + ufs_debugfs_exit();
> + scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> + return ret;
> + }
> return 0;
A separated error path is preferred i.e.
ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
if (ret)
goto debugfs_exit;
ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
if (ret)
goto unregister;
return 0;
unregister:
scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
debugfs_exit:
ufs_debugfs_exit();
return ret;
> }
>
> static void __exit ufshcd_core_exit(void)
> {
> ufs_debugfs_exit();
> + scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
> + scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> }
>
> module_init(ufshcd_core_init);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 5eb66a8..40436e3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -72,6 +72,8 @@ enum ufs_event_type {
> UFS_EVT_LINK_STARTUP_FAIL,
> UFS_EVT_RESUME_ERR,
> UFS_EVT_SUSPEND_ERR,
> + UFS_EVT_WL_SUSP_ERR,
> + UFS_EVT_WL_RES_ERR,
>
> /* abnormal events */
> UFS_EVT_DEV_RESET,
> @@ -807,6 +809,7 @@ struct ufs_hba {
> struct list_head clk_list_head;
>
> bool wlun_dev_clr_ua;
> + bool wlun_rpmb_clr_ua;
>
> /* Number of requests aborts */
> int req_abort_count;
> @@ -846,6 +849,7 @@ struct ufs_hba {
> struct delayed_work debugfs_ee_work;
> u32 debugfs_ee_rate_limit_ms;
> #endif
> + u32 luns_avail;
> };
>
> /* Returns true if clocks can be gated. Otherwise false */
> @@ -1105,6 +1109,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
> enum query_opcode desc_op);
>
> int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
> +int ufshcd_suspend_prepare(struct device *dev);
> +void ufshcd_resume_complete(struct device *dev);
>
> /* Wrapper functions for safely calling variant operations */
> static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 1cb6f1a..599739e 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -246,6 +246,26 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
> int dev_state, int link_state),
> TP_ARGS(dev_name, err, usecs, dev_state, link_state));
>
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_suspend,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_resume,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_suspend,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_resume,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> TRACE_EVENT(ufshcd_command,
> TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t,
> unsigned int tag, u32 doorbell, int transfer_len, u32 intr,
>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Adrian Hunter <adrian.hunter@intel.com>
To: Asutosh Das <asutoshd@codeaurora.org>,
cang@codeaurora.org, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org,
Alim Akhtar <alim.akhtar@samsung.com>,
Avri Altman <avri.altman@wdc.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Stanley Chu <stanley.chu@mediatek.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Lee Jones <lee.jones@linaro.org>, Bean Huo <beanhuo@micron.com>,
Kiwoong Kim <kwmad.kim@samsung.com>,
Wei Yongjun <weiyongjun1@huawei.com>,
Dan Carpenter <dan.carpenter@oracle.com>,
Yue Hu <huyue2@yulong.com>, Dinghao Liu <dinghao.liu@zju.edu.cn>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Satya Tangirala <satyat@google.com>,
open list <linux-kernel@vger.kernel.org>,
"moderated list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES"
<linux-arm-kernel@lists.infradead.org>,
"open list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES"
<linux-samsung-soc@vger.kernel.org>,
"moderated list:UNIVERSAL FLASH STORAGE HOST CONTROLLER
DRIVER..." <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
Date: Sun, 11 Apr 2021 20:52:04 +0300 [thread overview]
Message-ID: <0e967ede-e233-8202-d638-5b1d24144192@intel.com> (raw)
In-Reply-To: <1b3d53dad245a7166f3f67a4c65f3a731e6600b3.1617893198.git.asutoshd@codeaurora.org>
On 8/04/21 5:49 pm, Asutosh Das wrote:
> During runtime-suspend of ufs host, the scsi devices are
> already suspended and so are the queues associated with them.
> But the ufs host sends SSU (START_STOP_UNIT) to wlun
> during its runtime-suspend.
> During the process blk_queue_enter checks if the queue is not in
> suspended state. If so, it waits for the queue to resume, and never
> comes out of it.
> The commit
> (d55d15a33: scsi: block: Do not accept any requests while suspended)
> adds the check if the queue is in suspended state in blk_queue_enter().
>
> Call trace:
> __switch_to+0x174/0x2c4
> __schedule+0x478/0x764
> schedule+0x9c/0xe0
> blk_queue_enter+0x158/0x228
> blk_mq_alloc_request+0x40/0xa4
> blk_get_request+0x2c/0x70
> __scsi_execute+0x60/0x1c4
> ufshcd_set_dev_pwr_mode+0x124/0x1e4
> ufshcd_suspend+0x208/0x83c
> ufshcd_runtime_suspend+0x40/0x154
> ufshcd_pltfrm_runtime_suspend+0x14/0x20
> pm_generic_runtime_suspend+0x28/0x3c
> __rpm_callback+0x80/0x2a4
> rpm_suspend+0x308/0x614
> rpm_idle+0x158/0x228
> pm_runtime_work+0x84/0xac
> process_one_work+0x1f0/0x470
> worker_thread+0x26c/0x4c8
> kthread+0x13c/0x320
> ret_from_fork+0x10/0x18
>
> Fix this by registering ufs device wlun as a scsi driver and
> registering it for block runtime-pm. Also make this as a
> supplier for all other luns. That way, this device wlun
> suspends after all the consumers and resumes after
> hba resumes.
>
> Co-developed-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
<SNIP>
> @@ -5815,14 +5857,14 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba)
>
> hba->is_sys_suspended = false;
> /*
> - * Set RPM status of hba device to RPM_ACTIVE,
> + * Set RPM status of wlun device to RPM_ACTIVE,
> * this also clears its runtime error.
> */
> - ret = pm_runtime_set_active(hba->dev);
> + ret = pm_runtime_set_active(&hba->sdev_ufs_device->sdev_gendev);
It may be that it was hba->dev with a runtime error. So,
also need here:
if (ret)
ret = pm_runtime_set_active(hba->dev);
> @@ -8671,7 +8701,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> enum ufs_dev_pwr_mode req_dev_pwr_mode;
> enum uic_link_state req_link_state;
>
> - hba->pm_op_in_progress = 1;
> + hba->pm_op_in_progress = true;
> if (!ufshcd_is_shutdown_pm(pm_op)) {
> pm_lvl = ufshcd_is_runtime_pm(pm_op) ?
> hba->rpm_lvl : hba->spm_lvl;
> @@ -8694,17 +8724,17 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>
> if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
> req_link_state == UIC_LINK_ACTIVE_STATE) {
> - goto disable_clks;
> + goto enable_scaling;
Previously "goto disable_clks" would go to the vendor callback
ufshcd_vops_suspend(), but now it is skipped.
> }
>
> if ((req_dev_pwr_mode == hba->curr_dev_pwr_mode) &&
> (req_link_state == hba->uic_link_state))
> - goto enable_gating;
> + goto enable_scaling;
>
> /* UFS device & link must be active before we enter in this function */
> if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba)) {
> ret = -EINVAL;
> - goto enable_gating;
> + goto enable_scaling;
> }
<SNIP>
> +/**
> + * ufshcd_suspend - helper function for suspend operations
> + * @hba: per adapter instance
> + *
> + * This function will put disable irqs, turn off clocks
> + * and set vreg and hba-vreg in lpm mode.
> + * Also check the description of __ufshcd_wl_suspend().
> + */
> +static void ufshcd_suspend(struct ufs_hba *hba)
> +{
> + hba->pm_op_in_progress = 1;
Seems like pm_op_in_progress wouldn't be needed in this function,
nor ufshcd_resume().
You could probably simplify the callers slighly by
putting the "if (!hbs->is_powered) return" check here and in
ufshcd_resume().
> +
> + /*
> + * Disable the host irq as host controller as there won't be any
> + * host controller transaction expected till resume.
> + */
> ufshcd_disable_irq(hba);
> ufshcd_setup_clocks(hba, false);
> if (ufshcd_is_clkgating_allowed(hba)) {
> @@ -8948,6 +9054,43 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> trace_ufshcd_clk_gating(dev_name(hba->dev),
> hba->clk_gating.state);
> }
> +
> + ufshcd_vreg_set_lpm(hba);
> + /* Put the host controller in low power mode if possible */
> + ufshcd_hba_vreg_set_lpm(hba);
> + hba->pm_op_in_progress = 0;
> +}
> +
> +/**
> + * ufshcd_resume - helper function for resume operations
> + * @hba: per adapter instance
> + *
> + * This function basically turns on the regulators, clocks and
> + * irqs of the hba.
> + * Also check the description of __ufshcd_wl_resume().
> + *
> + * Returns 0 for success and non-zero for failure
> + */
> +static int ufshcd_resume(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + hba->pm_op_in_progress = 1;
> +
> + ufshcd_hba_vreg_set_hpm(hba);
> + ret = ufshcd_vreg_set_hpm(hba);
> + if (ret)
> + goto out;
> +
> + /* Make sure clocks are enabled before accessing controller */
> + ret = ufshcd_setup_clocks(hba, true);
> + if (ret)
> + goto disable_vreg;
> +
> + /* enable the host irq as host controller would be active soon */
> + ufshcd_enable_irq(hba);
> + goto out;
> +
> disable_vreg:
> ufshcd_vreg_set_lpm(hba);
> out:
> @@ -8962,6 +9105,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> * @hba: per adapter instance
> *
> * Check the description of ufshcd_suspend() function for more details.
> + * Also check the description of __ufshcd_wl_suspend().
> *
> * Returns 0 for success and non-zero for failure
> */
> @@ -8982,21 +9126,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
> hba->curr_dev_pwr_mode) &&
> (ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl) ==
> hba->uic_link_state) &&
> !hba->dev_info.b_rpm_dev_flush_capable)
> goto out;
The logic above can be removed.
>
> - if (pm_runtime_suspended(hba->dev)) {
> - /*
> - * UFS device and/or UFS link low power states during runtime
> - * suspend seems to be different than what is expected during
> - * system suspend. Hence runtime resume the devic & link and
> - * let the system suspend low power states to take effect.
> - * TODO: If resume takes longer time, we might have optimize
> - * it in future by not resuming everything if possible.
> - */
> - ret = ufshcd_runtime_resume(hba);
> - if (ret)
> - goto out;
> - }
System suspend/resume and runtime suspend/resume are identical
for hba->dev, so the logic becomes:
if (pm_runtime_suspended(hba->dev))
goto out;
> -
> - ret = ufshcd_suspend(hba, UFS_SYSTEM_PM);
> + ufshcd_suspend(hba);
> out:
> trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> @@ -9018,25 +9148,20 @@ EXPORT_SYMBOL(ufshcd_system_suspend);
>
> int ufshcd_system_resume(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> - if (!hba->is_powered || pm_runtime_suspended(hba->dev))
> - /*
> - * Let the runtime resume take care of resuming
> - * if runtime suspended.
> - */
> + if (!hba->is_powered)
System suspend/resume and runtime suspend/resume are identical
for hba->dev, so the original logic is fine here.
> goto out;
> else
> - ret = ufshcd_resume(hba, UFS_SYSTEM_PM);
> + ufshcd_resume(hba);
> out:
> - trace_ufshcd_system_resume(dev_name(hba->dev), ret,
> + trace_ufshcd_system_resume(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - if (!ret)
> - hba->is_sys_suspended = false;
> +
> + hba->is_sys_suspended = false;
> up(&hba->host_sem);
It seems likely that hba->host_sem down / up should be in
ufshcd_wl_suspend() / ufshcd_wl_resume() respectively, instead
of here.
Ditto hba->is_sys_suspended
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_system_resume);
>
> @@ -9045,23 +9170,23 @@ EXPORT_SYMBOL(ufshcd_system_resume);
> * @hba: per adapter instance
> *
> * Check the description of ufshcd_suspend() function for more details.
> + * Also check the description of __ufshcd_wl_suspend().
> *
> * Returns 0 for success and non-zero for failure
> */
> int ufshcd_runtime_suspend(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> if (!hba->is_powered)
> goto out;
> else
> - ret = ufshcd_suspend(hba, UFS_RUNTIME_PM);
> + ufshcd_suspend(hba);
> out:
> - trace_ufshcd_runtime_suspend(dev_name(hba->dev), ret,
> + trace_ufshcd_runtime_suspend(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_runtime_suspend);
>
> @@ -9069,37 +9194,25 @@ EXPORT_SYMBOL(ufshcd_runtime_suspend);
> * ufshcd_runtime_resume - runtime resume routine
> * @hba: per adapter instance
> *
> - * This function basically brings the UFS device, UniPro link and controller
> + * This function basically brings controller
> * to active state. Following operations are done in this function:
> *
> * 1. Turn on all the controller related clocks
> - * 2. Bring the UniPro link out of Hibernate state
> - * 3. If UFS device is in sleep state, turn ON VCC rail and bring the UFS device
> - * to active state.
> - * 4. If auto-bkops is enabled on the device, disable it.
> - *
> - * So following would be the possible power state after this function return
> - * successfully:
> - * S1: UFS device in Active state with VCC rail ON
> - * UniPro link in Active state
> - * All the UFS/UniPro controller clocks are ON
> - *
> - * Returns 0 for success and non-zero for failure
> + * 2. Turn ON VCC rail
> */
> int ufshcd_runtime_resume(struct ufs_hba *hba)
> {
> - int ret = 0;
> ktime_t start = ktime_get();
>
> if (!hba->is_powered)
> goto out;
> else
> - ret = ufshcd_resume(hba, UFS_RUNTIME_PM);
> + ufshcd_resume(hba);
> out:
> - trace_ufshcd_runtime_resume(dev_name(hba->dev), ret,
> + trace_ufshcd_runtime_resume(dev_name(hba->dev), 0,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> hba->curr_dev_pwr_mode, hba->uic_link_state);
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(ufshcd_runtime_resume);
>
> @@ -9113,18 +9226,13 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
> * ufshcd_shutdown - shutdown routine
> * @hba: per adapter instance
> *
> - * This function would power off both UFS device and UFS link.
> + * This function would turn off both UFS device and UFS hba
> + * regulators. It would also disable clocks.
> *
> * Returns 0 always to allow force shutdown even in case of errors.
> */
> int ufshcd_shutdown(struct ufs_hba *hba)
> {
> - int ret = 0;
> -
> - down(&hba->host_sem);
> - hba->shutting_down = true;
> - up(&hba->host_sem);
> -
> if (!hba->is_powered)
> goto out;
>
> @@ -9133,10 +9241,8 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>
> pm_runtime_get_sync(hba->dev);
>
> - ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
> + ufshcd_suspend(hba);
> out:
> - if (ret)
> - dev_err(hba->dev, "%s failed, err %d\n", __func__, ret);
> hba->is_powered = false;
> /* allow force shutdown even in case of errors */
> return 0;
> @@ -9150,6 +9256,8 @@ EXPORT_SYMBOL(ufshcd_shutdown);
> */
> void ufshcd_remove(struct ufs_hba *hba)
> {
> + if (hba->sdev_ufs_device)
> + scsi_autopm_get_device(hba->sdev_ufs_device);
> ufs_bsg_remove(hba);
> ufs_sysfs_remove_nodes(hba->dev);
> blk_cleanup_queue(hba->tmf_queue);
> @@ -9453,15 +9561,175 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> }
> EXPORT_SYMBOL_GPL(ufshcd_init);
>
> +void ufshcd_resume_complete(struct device *dev)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + pm_runtime_put_noidle(&hba->sdev_ufs_device->sdev_gendev);
pm_runtime_put() instead of pm_runtime_put_noidle() would be
more correct here.
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
> +
> +int ufshcd_suspend_prepare(struct device *dev)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + /*
> + * SCSI assumes that runtime-pm and system-pm for scsi drivers
> + * are same. And it doesn't wake up the device for system-suspend
> + * if it's runtime suspended. But ufs doesn't follow that.
> + * The rpm-lvl and spm-lvl can be different in ufs.
> + * Force it to honor system-suspend.
> + */
> + scsi_autopm_get_device(hba->sdev_ufs_device);
> + /* Refer ufshcd_resume_complete() */
> + pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev);
> + scsi_autopm_put_device(hba->sdev_ufs_device);
This could simply be:
pm_runtime_get_sync(&hba->sdev_ufs_device->sdev_gendev);
or new wrapper.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ufshcd_wl_poweroff(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
> + struct ufs_hba *hba = shost_priv(sdev->host);
> +
> + __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> + return 0;
> +}
> +#endif
> +
> +static int ufshcd_wl_probe(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
> +
> + if (!is_device_wlun(sdev))
> + return -ENODEV;
> +
> + blk_pm_runtime_init(sdev->request_queue, dev);
> + pm_runtime_set_autosuspend_delay(dev, 0);
> + pm_runtime_allow(dev);
> +
> + return 0;
> +}
> +
> +static int ufshcd_wl_remove(struct device *dev)
> +{
> + pm_runtime_forbid(dev);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ufshcd_wl_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> + .suspend = ufshcd_wl_suspend,
> + .resume = ufshcd_wl_resume,
> + .freeze = ufshcd_wl_suspend,
> + .thaw = ufshcd_wl_resume,
> + .poweroff = ufshcd_wl_poweroff,
> + .restore = ufshcd_wl_resume,
> +#endif
> + SET_RUNTIME_PM_OPS(ufshcd_wl_runtime_suspend, ufshcd_wl_runtime_resume, NULL)
> +};
> +
> +/**
> + * ufs_dev_wlun_template - describes ufs device wlun
> + * ufs-device wlun - used to send pm commands
> + * All luns are consumers of ufs-device wlun.
> + *
> + * Currently, no sd driver is present for wluns.
> + * Hence the no specific pm operations are performed.
> + * With ufs design, SSU should be sent to ufs-device wlun.
> + * Hence register a scsi driver for ufs wluns only.
> + */
> +static struct scsi_driver ufs_dev_wlun_template = {
> + .gendrv = {
> + .name = "ufs_device_wlun",
> + .owner = THIS_MODULE,
> + .probe = ufshcd_wl_probe,
> + .remove = ufshcd_wl_remove,
> + .pm = &ufshcd_wl_pm_ops,
> + .shutdown = ufshcd_wl_shutdown,
> + },
> +};
> +
> +static int ufshcd_rpmb_probe(struct device *dev)
> +{
> + return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
> +}
> +
> +static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
> +{
> + int ret = 0;
> +
> + if (!hba->wlun_rpmb_clr_ua)
> + return 0;
> + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> + if (!ret)
> + hba->wlun_rpmb_clr_ua = 0;
> + return ret;
> +}
> +
> +static int ufshcd_rpmb_runtime_resume(struct device *dev)
> +{
> + struct ufs_hba *hba = wlun_dev_to_hba(dev);
> +
> + if (hba->sdev_rpmb)
> + return ufshcd_clear_rpmb_uac(hba);
> + return 0;
> +}
> +
> +static int ufshcd_rpmb_resume(struct device *dev)
> +{
> + struct ufs_hba *hba = wlun_dev_to_hba(dev);
> +
> + if (hba->sdev_rpmb && !pm_runtime_suspended(dev))
> + return ufshcd_clear_rpmb_uac(hba);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ufs_rpmb_pm_ops = {
> + SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
> +};
> +
> +/**
> + * Describes the ufs rpmb wlun.
> + * Used only to send uac.
> + */
> +static struct scsi_driver ufs_rpmb_wlun_template = {
> + .gendrv = {
> + .name = "ufs_rpmb_wlun",
> + .owner = THIS_MODULE,
> + .probe = ufshcd_rpmb_probe,
> + .pm = &ufs_rpmb_pm_ops,
> + },
> +};
> +
> static int __init ufshcd_core_init(void)
> {
> + int ret;
> +
> ufs_debugfs_init();
> +
> + ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
> + if (ret) {
> + ufs_debugfs_exit();
> + return ret;
> + }
> + ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
> + if (ret) {
> + ufs_debugfs_exit();
> + scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> + return ret;
> + }
> return 0;
A separated error path is preferred i.e.
ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
if (ret)
goto debugfs_exit;
ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
if (ret)
goto unregister;
return 0;
unregister:
scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
debugfs_exit:
ufs_debugfs_exit();
return ret;
> }
>
> static void __exit ufshcd_core_exit(void)
> {
> ufs_debugfs_exit();
> + scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
> + scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> }
>
> module_init(ufshcd_core_init);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 5eb66a8..40436e3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -72,6 +72,8 @@ enum ufs_event_type {
> UFS_EVT_LINK_STARTUP_FAIL,
> UFS_EVT_RESUME_ERR,
> UFS_EVT_SUSPEND_ERR,
> + UFS_EVT_WL_SUSP_ERR,
> + UFS_EVT_WL_RES_ERR,
>
> /* abnormal events */
> UFS_EVT_DEV_RESET,
> @@ -807,6 +809,7 @@ struct ufs_hba {
> struct list_head clk_list_head;
>
> bool wlun_dev_clr_ua;
> + bool wlun_rpmb_clr_ua;
>
> /* Number of requests aborts */
> int req_abort_count;
> @@ -846,6 +849,7 @@ struct ufs_hba {
> struct delayed_work debugfs_ee_work;
> u32 debugfs_ee_rate_limit_ms;
> #endif
> + u32 luns_avail;
> };
>
> /* Returns true if clocks can be gated. Otherwise false */
> @@ -1105,6 +1109,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
> enum query_opcode desc_op);
>
> int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
> +int ufshcd_suspend_prepare(struct device *dev);
> +void ufshcd_resume_complete(struct device *dev);
>
> /* Wrapper functions for safely calling variant operations */
> static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 1cb6f1a..599739e 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -246,6 +246,26 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
> int dev_state, int link_state),
> TP_ARGS(dev_name, err, usecs, dev_state, link_state));
>
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_suspend,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_resume,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_suspend,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> +DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_resume,
> + TP_PROTO(const char *dev_name, int err, s64 usecs,
> + int dev_state, int link_state),
> + TP_ARGS(dev_name, err, usecs, dev_state, link_state));
> +
> TRACE_EVENT(ufshcd_command,
> TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t,
> unsigned int tag, u32 doorbell, int transfer_len, u32 intr,
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-04-11 17:51 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-08 14:49 [PATCH v17 0/2] Enable power management for ufs wlun Asutosh Das
2021-04-08 14:49 ` Asutosh Das
2021-04-08 14:49 ` Asutosh Das
2021-04-08 14:49 ` [PATCH v17 1/2] scsi: ufs: Enable power management for wlun Asutosh Das
2021-04-08 14:49 ` Asutosh Das
2021-04-08 14:49 ` Asutosh Das
2021-04-09 2:27 ` Daejun Park
2021-04-09 10:07 ` Adrian Hunter
2021-04-09 17:15 ` Asutosh Das (asd)
2021-04-09 17:30 ` Adrian Hunter
2021-04-11 17:52 ` Adrian Hunter [this message]
2021-04-11 17:52 ` Adrian Hunter
2021-04-11 17:52 ` Adrian Hunter
2021-04-08 14:49 ` [PATCH v17 2/2] ufs: sysfs: Resume the proper scsi device Asutosh Das
2021-04-10 8:57 ` Adrian Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0e967ede-e233-8202-d638-5b1d24144192@intel.com \
--to=adrian.hunter@intel.com \
--cc=agross@kernel.org \
--cc=alim.akhtar@samsung.com \
--cc=asutoshd@codeaurora.org \
--cc=avri.altman@wdc.com \
--cc=beanhuo@micron.com \
--cc=bjorn.andersson@linaro.org \
--cc=cang@codeaurora.org \
--cc=dan.carpenter@oracle.com \
--cc=dinghao.liu@zju.edu.cn \
--cc=gustavoars@kernel.org \
--cc=huyue2@yulong.com \
--cc=jaegeuk@kernel.org \
--cc=jejb@linux.ibm.com \
--cc=krzk@kernel.org \
--cc=kwmad.kim@samsung.com \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=matthias.bgg@gmail.com \
--cc=mingo@redhat.com \
--cc=rostedt@goodmis.org \
--cc=satyat@google.com \
--cc=stanley.chu@mediatek.com \
--cc=weiyongjun1@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.