All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 0/3] Three changes related with UFS clock scaling
@ 2020-12-30 11:29 Can Guo
  2020-12-30 11:29 ` [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected " Can Guo
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Can Guo @ 2020-12-30 11:29 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang

This series is made based on 5.10/scsi-fixes branch.

Current devfreq framework allows sysfs nodes like governor, min_freq and max_freq to be changed even after devfreq device is suspended.
Meanwhile, devfreq_suspend_device() cannot/wouldn't synchronize clock scaling which has already been invoked through devfreq sysfs nodes menitioned above.
It means that clock scaling invoked through these devfreq sysfs nodes can happen at any time regardless of the state of UFS host and/or device.
We need to control and synchronize clock scaling in this scenario.

The 1st change allows contexts to prevent clock scaling from being invoked through devfreq sysfs nodes.
The 2nd change is just a code cleanup for clk_scaling/gating initialization routine.
The 3rd change reverts one old change which can be covered by the 1st change. For branches which do not have this change yet, it can be ignored.

Change since v7:
- Slightly updated the 1st change: changed the up_write() before ufshcd_wb_ctrl() to downgrade_write() in ufshcd_devfreq_scale(),
  so that ufshcd_wb_ctrl() is called with clk_scale_lock held, this is to make sure race condition won't happen to ufshcd_wb_ctrl().

Change since v6:
- Updated the 2nd change

Change since v5:
- Reomved the code change in ufshcd_shutdown() since it is not quite relevant with this fix

Change since v4:
- Updated some comment lines as requested by Stanley

Change since v3:
- Slightly updated the 1st change

Change since v2:
- Split the 1st change to two changes, which become the 1st change and the 3rd change

Change since v1:
- Updated the 2nd change

Can Guo (3):
  scsi: ufs: Protect some contexts from unexpected clock scaling
  scsi: ufs: Refactor ufshcd_init/exit_clk_scaling/gating()
  scsi: ufs: Revert "Make sure clk scaling happens only when HBA is
    runtime ACTIVE"

 drivers/scsi/ufs/ufshcd.c | 216 ++++++++++++++++++++++++++--------------------
 drivers/scsi/ufs/ufshcd.h |  10 ++-
 2 files changed, 132 insertions(+), 94 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected clock scaling
  2020-12-30 11:29 [PATCH v8 0/3] Three changes related with UFS clock scaling Can Guo
@ 2020-12-30 11:29 ` Can Guo
  2020-12-31  4:39   ` Can Guo
  2020-12-30 11:29 ` [PATCH v8 2/3] scsi: ufs: Refactor ufshcd_init/exit_clk_scaling/gating() Can Guo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Can Guo @ 2020-12-30 11:29 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Bean Huo, Bart Van Assche,
	Satya Tangirala, open list

In contexts like suspend, shutdown and error handling, we need to suspend
devfreq to make sure these contexts won't be disturbed by clock scaling.
However, suspending devfreq is not enough since users can still trigger a
clock scaling by manipulating the devfreq sysfs nodes like min/max_freq and
governor even after devfreq is suspended. Moreover, mere suspending devfreq
cannot synchroinze a clock scaling which has already been invoked through
these sysfs nodes. Add one more flag in struct clk_scaling and wrap the
entire func ufshcd_devfreq_scale() with the clk_scaling_lock, so that we
can use this flag and clk_scaling_lock to control and synchronize clock
scaling invoked through devfreq sysfs nodes.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 87 ++++++++++++++++++++++++++++++-----------------
 drivers/scsi/ufs/ufshcd.h |  6 +++-
 2 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0c148fc..44cbee1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1147,19 +1147,33 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 	 */
 	ufshcd_scsi_block_requests(hba);
 	down_write(&hba->clk_scaling_lock);
-	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
+
+	if (!hba->clk_scaling.is_allowed)
+		ret = -EAGAIN;
+	else if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US))
 		ret = -EBUSY;
+
+	if (ret) {
 		up_write(&hba->clk_scaling_lock);
 		ufshcd_scsi_unblock_requests(hba);
+		goto out;
 	}
 
+	/* let's not get into low power until clock scaling is completed */
+	ufshcd_hold(hba, false);
+
+out:
 	return ret;
 }
 
-static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
+static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
 {
-	up_write(&hba->clk_scaling_lock);
+	if (writelock)
+		up_write(&hba->clk_scaling_lock);
+	else
+		up_read(&hba->clk_scaling_lock);
 	ufshcd_scsi_unblock_requests(hba);
+	ufshcd_release(hba);
 }
 
 /**
@@ -1174,13 +1188,11 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
 static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 {
 	int ret = 0;
-
-	/* let's not get into low power until clock scaling is completed */
-	ufshcd_hold(hba, false);
+	bool is_writelock = true;
 
 	ret = ufshcd_clock_scaling_prepare(hba);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* scale down the gear before scaling down clocks */
 	if (!scale_up) {
@@ -1206,14 +1218,12 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 	}
 
 	/* Enable Write Booster if we have scaled up else disable it */
-	up_write(&hba->clk_scaling_lock);
+	downgrade_write(&hba->clk_scaling_lock);
+	is_writelock = false;
 	ufshcd_wb_ctrl(hba, scale_up);
-	down_write(&hba->clk_scaling_lock);
 
 out_unprepare:
-	ufshcd_clock_scaling_unprepare(hba);
-out:
-	ufshcd_release(hba);
+	ufshcd_clock_scaling_unprepare(hba, is_writelock);
 	return ret;
 }
 
@@ -1487,7 +1497,7 @@ static ssize_t ufshcd_clkscale_enable_show(struct device *dev,
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", hba->clk_scaling.is_allowed);
+	return snprintf(buf, PAGE_SIZE, "%d\n", hba->clk_scaling.is_enabled);
 }
 
 static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
@@ -1501,7 +1511,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 		return -EINVAL;
 
 	value = !!value;
-	if (value == hba->clk_scaling.is_allowed)
+	if (value == hba->clk_scaling.is_enabled)
 		goto out;
 
 	pm_runtime_get_sync(hba->dev);
@@ -1510,7 +1520,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	cancel_work_sync(&hba->clk_scaling.suspend_work);
 	cancel_work_sync(&hba->clk_scaling.resume_work);
 
-	hba->clk_scaling.is_allowed = value;
+	hba->clk_scaling.is_enabled = value;
 
 	if (value) {
 		ufshcd_resume_clkscaling(hba);
@@ -1845,8 +1855,6 @@ static void ufshcd_init_clk_scaling(struct ufs_hba *hba)
 	snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d",
 		 hba->host->host_no);
 	hba->clk_scaling.workq = create_singlethread_workqueue(wq_name);
-
-	ufshcd_clkscaling_init_sysfs(hba);
 }
 
 static void ufshcd_exit_clk_scaling(struct ufs_hba *hba)
@@ -1854,6 +1862,8 @@ static void ufshcd_exit_clk_scaling(struct ufs_hba *hba)
 	if (!ufshcd_is_clkscaling_supported(hba))
 		return;
 
+	if (hba->clk_scaling.enable_attr.attr.name)
+		device_remove_file(hba->dev, &hba->clk_scaling.enable_attr);
 	destroy_workqueue(hba->clk_scaling.workq);
 	ufshcd_devfreq_remove(hba);
 }
@@ -1918,7 +1928,7 @@ static void ufshcd_clk_scaling_start_busy(struct ufs_hba *hba)
 	if (!hba->clk_scaling.active_reqs++)
 		queue_resume_work = true;
 
-	if (!hba->clk_scaling.is_allowed || hba->pm_op_in_progress)
+	if (!hba->clk_scaling.is_enabled || hba->pm_op_in_progress)
 		return;
 
 	if (queue_resume_work)
@@ -4987,7 +4997,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 				complete(hba->dev_cmd.complete);
 			}
 		}
-		if (ufshcd_is_clkscaling_supported(hba))
+		if (ufshcd_is_clkscaling_supported(hba) &&
+		    hba->clk_scaling.active_reqs > 0)
 			hba->clk_scaling.active_reqs--;
 	}
 
@@ -5650,18 +5661,25 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 		ufshcd_vops_resume(hba, UFS_RUNTIME_PM);
 	} else {
 		ufshcd_hold(hba, false);
-		if (hba->clk_scaling.is_allowed) {
+		if (hba->clk_scaling.is_enabled) {
 			cancel_work_sync(&hba->clk_scaling.suspend_work);
 			cancel_work_sync(&hba->clk_scaling.resume_work);
 			ufshcd_suspend_clkscaling(hba);
 		}
 	}
+	down_write(&hba->clk_scaling_lock);
+	hba->clk_scaling.is_allowed = false;
+	up_write(&hba->clk_scaling_lock);
 }
 
 static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 {
 	ufshcd_release(hba);
-	if (hba->clk_scaling.is_allowed)
+
+	down_write(&hba->clk_scaling_lock);
+	hba->clk_scaling.is_allowed = true;
+	up_write(&hba->clk_scaling_lock);
+	if (hba->clk_scaling.is_enabled)
 		ufshcd_resume_clkscaling(hba);
 	pm_runtime_put(hba->dev);
 }
@@ -7620,12 +7638,14 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 			sizeof(struct ufs_pa_layer_attr));
 		hba->clk_scaling.saved_pwr_info.is_valid = true;
 		if (!hba->devfreq) {
+			hba->clk_scaling.is_allowed = true;
 			ret = ufshcd_devfreq_init(hba);
 			if (ret)
 				goto out;
-		}
 
-		hba->clk_scaling.is_allowed = true;
+			hba->clk_scaling.is_enabled = true;
+			ufshcd_clkscaling_init_sysfs(hba);
+		}
 	}
 
 	ufs_bsg_probe(hba);
@@ -8491,11 +8511,14 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ufshcd_hold(hba, false);
 	hba->clk_gating.is_suspended = true;
 
-	if (hba->clk_scaling.is_allowed) {
+	if (hba->clk_scaling.is_enabled) {
 		cancel_work_sync(&hba->clk_scaling.suspend_work);
 		cancel_work_sync(&hba->clk_scaling.resume_work);
 		ufshcd_suspend_clkscaling(hba);
 	}
+	down_write(&hba->clk_scaling_lock);
+	hba->clk_scaling.is_allowed = false;
+	up_write(&hba->clk_scaling_lock);
 
 	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
 			req_link_state == UIC_LINK_ACTIVE_STATE) {
@@ -8592,8 +8615,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	goto out;
 
 set_link_active:
-	if (hba->clk_scaling.is_allowed)
-		ufshcd_resume_clkscaling(hba);
 	ufshcd_vreg_set_hpm(hba);
 	if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
 		ufshcd_set_link_active(hba);
@@ -8603,7 +8624,10 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
 		ufshcd_disable_auto_bkops(hba);
 enable_gating:
-	if (hba->clk_scaling.is_allowed)
+	down_write(&hba->clk_scaling_lock);
+	hba->clk_scaling.is_allowed = true;
+	up_write(&hba->clk_scaling_lock);
+	if (hba->clk_scaling.is_enabled)
 		ufshcd_resume_clkscaling(hba);
 	hba->clk_gating.is_suspended = false;
 	hba->dev_info.b_rpm_dev_flush_capable = false;
@@ -8701,7 +8725,10 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 
 	hba->clk_gating.is_suspended = false;
 
-	if (hba->clk_scaling.is_allowed)
+	down_write(&hba->clk_scaling_lock);
+	hba->clk_scaling.is_allowed = true;
+	up_write(&hba->clk_scaling_lock);
+	if (hba->clk_scaling.is_enabled)
 		ufshcd_resume_clkscaling(hba);
 
 	/* Enable Auto-Hibernate if configured */
@@ -8725,8 +8752,6 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ufshcd_vreg_set_lpm(hba);
 disable_irq_and_vops_clks:
 	ufshcd_disable_irq(hba);
-	if (hba->clk_scaling.is_allowed)
-		ufshcd_suspend_clkscaling(hba);
 	ufshcd_setup_clocks(hba, false);
 	if (ufshcd_is_clkgating_allowed(hba)) {
 		hba->clk_gating.state = CLKS_OFF;
@@ -8944,8 +8969,6 @@ void ufshcd_remove(struct ufs_hba *hba)
 
 	ufshcd_exit_clk_scaling(hba);
 	ufshcd_exit_clk_gating(hba);
-	if (ufshcd_is_clkscaling_supported(hba))
-		device_remove_file(hba->dev, &hba->clk_scaling.enable_attr);
 	ufshcd_hba_exit(hba);
 }
 EXPORT_SYMBOL_GPL(ufshcd_remove);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e0f00a4..5737679 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -382,7 +382,10 @@ struct ufs_saved_pwr_info {
  * @workq: workqueue to schedule devfreq suspend/resume work
  * @suspend_work: worker to suspend devfreq
  * @resume_work: worker to resume devfreq
- * @is_allowed: tracks if scaling is currently allowed or not
+ * @is_enabled: tracks if scaling is currently enabled or not, controlled by
+		clkscale_enable sysfs node
+ * @is_allowed: tracks if scaling is currently allowed or not, used to block
+		clock scaling which is not invoked from devfreq governor
  * @is_busy_started: tracks if busy period has started or not
  * @is_suspended: tracks if devfreq is suspended or not
  */
@@ -396,6 +399,7 @@ struct ufs_clk_scaling {
 	struct workqueue_struct *workq;
 	struct work_struct suspend_work;
 	struct work_struct resume_work;
+	bool is_enabled;
 	bool is_allowed;
 	bool is_busy_started;
 	bool is_suspended;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v8 2/3] scsi: ufs: Refactor ufshcd_init/exit_clk_scaling/gating()
  2020-12-30 11:29 [PATCH v8 0/3] Three changes related with UFS clock scaling Can Guo
  2020-12-30 11:29 ` [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected " Can Guo
@ 2020-12-30 11:29 ` Can Guo
  2020-12-30 11:29   ` Can Guo
  2021-01-06  2:45 ` [PATCH v8 0/3] Three changes related with UFS clock scaling Martin K. Petersen
  3 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-12-30 11:29 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Bean Huo, Bart Van Assche,
	Satya Tangirala, open list

ufshcd_hba_exit() is always called after ufshcd_exit_clk_scaling() and
ufshcd_exit_clk_gating(), so move ufshcd_exit_clk_scaling/gating() to
ufshcd_hba_exit(). Meanwhile, add dedicated funcs to init and remove
sysfs nodes of clock scaling/gating to make the code more readable.
Overall functionality remains same.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 128 +++++++++++++++++++++++++---------------------
 drivers/scsi/ufs/ufshcd.h |   4 ++
 2 files changed, 75 insertions(+), 57 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 44cbee1..b7840ef 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1538,7 +1538,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	return count;
 }
 
-static void ufshcd_clkscaling_init_sysfs(struct ufs_hba *hba)
+static void ufshcd_init_clk_scaling_sysfs(struct ufs_hba *hba)
 {
 	hba->clk_scaling.enable_attr.show = ufshcd_clkscale_enable_show;
 	hba->clk_scaling.enable_attr.store = ufshcd_clkscale_enable_store;
@@ -1549,6 +1549,42 @@ static void ufshcd_clkscaling_init_sysfs(struct ufs_hba *hba)
 		dev_err(hba->dev, "Failed to create sysfs for clkscale_enable\n");
 }
 
+static void ufshcd_remove_clk_scaling_sysfs(struct ufs_hba *hba)
+{
+	if (hba->clk_scaling.enable_attr.attr.name)
+		device_remove_file(hba->dev, &hba->clk_scaling.enable_attr);
+}
+
+static void ufshcd_init_clk_scaling(struct ufs_hba *hba)
+{
+	char wq_name[sizeof("ufs_clkscaling_00")];
+
+	if (!ufshcd_is_clkscaling_supported(hba))
+		return;
+
+	INIT_WORK(&hba->clk_scaling.suspend_work,
+		  ufshcd_clk_scaling_suspend_work);
+	INIT_WORK(&hba->clk_scaling.resume_work,
+		  ufshcd_clk_scaling_resume_work);
+
+	snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d",
+		 hba->host->host_no);
+	hba->clk_scaling.workq = create_singlethread_workqueue(wq_name);
+
+	hba->clk_scaling.is_initialized = true;
+}
+
+static void ufshcd_exit_clk_scaling(struct ufs_hba *hba)
+{
+	if (!hba->clk_scaling.is_initialized)
+		return;
+
+	ufshcd_remove_clk_scaling_sysfs(hba);
+	destroy_workqueue(hba->clk_scaling.workq);
+	ufshcd_devfreq_remove(hba);
+	hba->clk_scaling.is_initialized = false;
+}
+
 static void ufshcd_ungate_work(struct work_struct *work)
 {
 	int ret;
@@ -1840,32 +1876,31 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev,
 	return count;
 }
 
-static void ufshcd_init_clk_scaling(struct ufs_hba *hba)
+static void ufshcd_init_clk_gating_sysfs(struct ufs_hba *hba)
 {
-	char wq_name[sizeof("ufs_clkscaling_00")];
-
-	if (!ufshcd_is_clkscaling_supported(hba))
-		return;
-
-	INIT_WORK(&hba->clk_scaling.suspend_work,
-		  ufshcd_clk_scaling_suspend_work);
-	INIT_WORK(&hba->clk_scaling.resume_work,
-		  ufshcd_clk_scaling_resume_work);
+	hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
+	hba->clk_gating.delay_attr.store = ufshcd_clkgate_delay_store;
+	sysfs_attr_init(&hba->clk_gating.delay_attr.attr);
+	hba->clk_gating.delay_attr.attr.name = "clkgate_delay_ms";
+	hba->clk_gating.delay_attr.attr.mode = 0644;
+	if (device_create_file(hba->dev, &hba->clk_gating.delay_attr))
+		dev_err(hba->dev, "Failed to create sysfs for clkgate_delay\n");
 
-	snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d",
-		 hba->host->host_no);
-	hba->clk_scaling.workq = create_singlethread_workqueue(wq_name);
+	hba->clk_gating.enable_attr.show = ufshcd_clkgate_enable_show;
+	hba->clk_gating.enable_attr.store = ufshcd_clkgate_enable_store;
+	sysfs_attr_init(&hba->clk_gating.enable_attr.attr);
+	hba->clk_gating.enable_attr.attr.name = "clkgate_enable";
+	hba->clk_gating.enable_attr.attr.mode = 0644;
+	if (device_create_file(hba->dev, &hba->clk_gating.enable_attr))
+		dev_err(hba->dev, "Failed to create sysfs for clkgate_enable\n");
 }
 
-static void ufshcd_exit_clk_scaling(struct ufs_hba *hba)
+static void ufshcd_remove_clk_gating_sysfs(struct ufs_hba *hba)
 {
-	if (!ufshcd_is_clkscaling_supported(hba))
-		return;
-
-	if (hba->clk_scaling.enable_attr.attr.name)
-		device_remove_file(hba->dev, &hba->clk_scaling.enable_attr);
-	destroy_workqueue(hba->clk_scaling.workq);
-	ufshcd_devfreq_remove(hba);
+	if (hba->clk_gating.delay_attr.attr.name)
+		device_remove_file(hba->dev, &hba->clk_gating.delay_attr);
+	if (hba->clk_gating.enable_attr.attr.name)
+		device_remove_file(hba->dev, &hba->clk_gating.enable_attr);
 }
 
 static void ufshcd_init_clk_gating(struct ufs_hba *hba)
@@ -1886,34 +1921,22 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 	hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
 							   WQ_MEM_RECLAIM);
 
-	hba->clk_gating.is_enabled = true;
+	ufshcd_init_clk_gating_sysfs(hba);
 
-	hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
-	hba->clk_gating.delay_attr.store = ufshcd_clkgate_delay_store;
-	sysfs_attr_init(&hba->clk_gating.delay_attr.attr);
-	hba->clk_gating.delay_attr.attr.name = "clkgate_delay_ms";
-	hba->clk_gating.delay_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->clk_gating.delay_attr))
-		dev_err(hba->dev, "Failed to create sysfs for clkgate_delay\n");
-
-	hba->clk_gating.enable_attr.show = ufshcd_clkgate_enable_show;
-	hba->clk_gating.enable_attr.store = ufshcd_clkgate_enable_store;
-	sysfs_attr_init(&hba->clk_gating.enable_attr.attr);
-	hba->clk_gating.enable_attr.attr.name = "clkgate_enable";
-	hba->clk_gating.enable_attr.attr.mode = 0644;
-	if (device_create_file(hba->dev, &hba->clk_gating.enable_attr))
-		dev_err(hba->dev, "Failed to create sysfs for clkgate_enable\n");
+	hba->clk_gating.is_enabled = true;
+	hba->clk_gating.is_initialized = true;
 }
 
 static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
 {
-	if (!ufshcd_is_clkgating_allowed(hba))
+	if (!hba->clk_gating.is_initialized)
 		return;
-	device_remove_file(hba->dev, &hba->clk_gating.delay_attr);
-	device_remove_file(hba->dev, &hba->clk_gating.enable_attr);
+
+	ufshcd_remove_clk_gating_sysfs(hba);
 	cancel_work_sync(&hba->clk_gating.ungate_work);
 	cancel_delayed_work_sync(&hba->clk_gating.gate_work);
 	destroy_workqueue(hba->clk_gating.clk_gating_workq);
+	hba->clk_gating.is_initialized = false;
 }
 
 /* Must be called with host lock acquired */
@@ -7644,7 +7667,7 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 				goto out;
 
 			hba->clk_scaling.is_enabled = true;
-			ufshcd_clkscaling_init_sysfs(hba);
+			ufshcd_init_clk_scaling_sysfs(hba);
 		}
 	}
 
@@ -7773,7 +7796,6 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	 */
 	if (ret) {
 		pm_runtime_put_sync(hba->dev);
-		ufshcd_exit_clk_scaling(hba);
 		ufshcd_hba_exit(hba);
 	}
 }
@@ -8210,12 +8232,12 @@ static int ufshcd_hba_init(struct ufs_hba *hba)
 static void ufshcd_hba_exit(struct ufs_hba *hba)
 {
 	if (hba->is_powered) {
+		ufshcd_exit_clk_scaling(hba);
+		ufshcd_exit_clk_gating(hba);
+		if (hba->eh_wq)
+			destroy_workqueue(hba->eh_wq);
 		ufshcd_variant_hba_exit(hba);
 		ufshcd_setup_vreg(hba, false);
-		ufshcd_suspend_clkscaling(hba);
-		if (ufshcd_is_clkscaling_supported(hba))
-			if (hba->devfreq)
-				ufshcd_suspend_clkscaling(hba);
 		ufshcd_setup_clocks(hba, false);
 		ufshcd_setup_hba_vreg(hba, false);
 		hba->is_powered = false;
@@ -8962,13 +8984,9 @@ void ufshcd_remove(struct ufs_hba *hba)
 	blk_mq_free_tag_set(&hba->tmf_tag_set);
 	blk_cleanup_queue(hba->cmd_queue);
 	scsi_remove_host(hba->host);
-	destroy_workqueue(hba->eh_wq);
 	/* disable interrupts */
 	ufshcd_disable_intr(hba, hba->intr_mask);
 	ufshcd_hba_stop(hba);
-
-	ufshcd_exit_clk_scaling(hba);
-	ufshcd_exit_clk_gating(hba);
 	ufshcd_hba_exit(hba);
 }
 EXPORT_SYMBOL_GPL(ufshcd_remove);
@@ -9167,7 +9185,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
 	if (err) {
 		dev_err(hba->dev, "request irq failed\n");
-		goto exit_gating;
+		goto out_disable;
 	} else {
 		hba->is_irq_enabled = true;
 	}
@@ -9175,7 +9193,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	err = scsi_add_host(host, hba->dev);
 	if (err) {
 		dev_err(hba->dev, "scsi_add_host failed\n");
-		goto exit_gating;
+		goto out_disable;
 	}
 
 	hba->cmd_queue = blk_mq_init_queue(&hba->host->tag_set);
@@ -9258,10 +9276,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	blk_cleanup_queue(hba->cmd_queue);
 out_remove_scsi_host:
 	scsi_remove_host(hba->host);
-exit_gating:
-	ufshcd_exit_clk_scaling(hba);
-	ufshcd_exit_clk_gating(hba);
-	destroy_workqueue(hba->eh_wq);
 out_disable:
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 5737679..cb9075d 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -347,6 +347,7 @@ enum clk_gating_state {
  * @delay_attr: sysfs attribute to control delay_attr
  * @enable_attr: sysfs attribute to enable/disable clock gating
  * @is_enabled: Indicates the current status of clock gating
+ * @is_initialized: Indicates whether clock gating is initialized or not
  * @active_reqs: number of requests that are pending and should be waited for
  * completion before gating clocks.
  */
@@ -359,6 +360,7 @@ struct ufs_clk_gating {
 	struct device_attribute delay_attr;
 	struct device_attribute enable_attr;
 	bool is_enabled;
+	bool is_initialized;
 	int active_reqs;
 	struct workqueue_struct *clk_gating_workq;
 };
@@ -386,6 +388,7 @@ struct ufs_saved_pwr_info {
 		clkscale_enable sysfs node
  * @is_allowed: tracks if scaling is currently allowed or not, used to block
 		clock scaling which is not invoked from devfreq governor
+ * @is_initialized: Indicates whether clock scaling is initialized or not
  * @is_busy_started: tracks if busy period has started or not
  * @is_suspended: tracks if devfreq is suspended or not
  */
@@ -401,6 +404,7 @@ struct ufs_clk_scaling {
 	struct work_struct resume_work;
 	bool is_enabled;
 	bool is_allowed;
+	bool is_initialized;
 	bool is_busy_started;
 	bool is_suspended;
 };
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v8 3/3] scsi: ufs: Revert "Make sure clk scaling happens only when HBA is runtime ACTIVE"
  2020-12-30 11:29 [PATCH v8 0/3] Three changes related with UFS clock scaling Can Guo
  2020-12-30 11:29 ` [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected " Can Guo
@ 2020-12-30 11:29   ` Can Guo
  2020-12-30 11:29   ` Can Guo
  2021-01-06  2:45 ` [PATCH v8 0/3] Three changes related with UFS clock scaling Martin K. Petersen
  3 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-12-30 11:29 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Matthias Brugger, Stanley Chu, Bean Huo,
	Bart Van Assche, open list,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support

Commit 73cc291c27024 ("Make sure clk scaling happens only when HBA is
runtime ACTIVE") is no longer needed since commit f7a42540928a8 ("scsi:
ufs: Protect some contexts from unexpected clock scaling") is a more
mature fix to protect UFS LLD stability from clock scaling invoked through
sysfs nodes by users.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b7840ef..3487c56 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1304,15 +1304,8 @@ static int ufshcd_devfreq_target(struct device *dev,
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 
-	pm_runtime_get_noresume(hba->dev);
-	if (!pm_runtime_active(hba->dev)) {
-		pm_runtime_put_noidle(hba->dev);
-		ret = -EAGAIN;
-		goto out;
-	}
 	start = ktime_get();
 	ret = ufshcd_devfreq_scale(hba, scale_up);
-	pm_runtime_put(hba->dev);
 
 	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
 		(scale_up ? "up" : "down"),
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v8 3/3] scsi: ufs: Revert "Make sure clk scaling happens only when HBA is runtime ACTIVE"
@ 2020-12-30 11:29   ` Can Guo
  0 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-12-30 11:29 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Bart Van Assche, Martin K. Petersen, James E.J. Bottomley,
	open list, Avri Altman, moderated list:ARM/Mediatek SoC support,
	Alim Akhtar, Matthias Brugger, Stanley Chu,
	moderated list:ARM/Mediatek SoC support, Bean Huo

Commit 73cc291c27024 ("Make sure clk scaling happens only when HBA is
runtime ACTIVE") is no longer needed since commit f7a42540928a8 ("scsi:
ufs: Protect some contexts from unexpected clock scaling") is a more
mature fix to protect UFS LLD stability from clock scaling invoked through
sysfs nodes by users.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b7840ef..3487c56 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1304,15 +1304,8 @@ static int ufshcd_devfreq_target(struct device *dev,
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 
-	pm_runtime_get_noresume(hba->dev);
-	if (!pm_runtime_active(hba->dev)) {
-		pm_runtime_put_noidle(hba->dev);
-		ret = -EAGAIN;
-		goto out;
-	}
 	start = ktime_get();
 	ret = ufshcd_devfreq_scale(hba, scale_up);
-	pm_runtime_put(hba->dev);
 
 	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
 		(scale_up ? "up" : "down"),
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v8 3/3] scsi: ufs: Revert "Make sure clk scaling happens only when HBA is runtime ACTIVE"
@ 2020-12-30 11:29   ` Can Guo
  0 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-12-30 11:29 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Bart Van Assche, Martin K. Petersen, James E.J. Bottomley,
	open list, Avri Altman, moderated list:ARM/Mediatek SoC support,
	Alim Akhtar, Matthias Brugger, Stanley Chu,
	moderated list:ARM/Mediatek SoC support, Bean Huo

Commit 73cc291c27024 ("Make sure clk scaling happens only when HBA is
runtime ACTIVE") is no longer needed since commit f7a42540928a8 ("scsi:
ufs: Protect some contexts from unexpected clock scaling") is a more
mature fix to protect UFS LLD stability from clock scaling invoked through
sysfs nodes by users.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b7840ef..3487c56 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1304,15 +1304,8 @@ static int ufshcd_devfreq_target(struct device *dev,
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 
-	pm_runtime_get_noresume(hba->dev);
-	if (!pm_runtime_active(hba->dev)) {
-		pm_runtime_put_noidle(hba->dev);
-		ret = -EAGAIN;
-		goto out;
-	}
 	start = ktime_get();
 	ret = ufshcd_devfreq_scale(hba, scale_up);
-	pm_runtime_put(hba->dev);
 
 	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
 		(scale_up ? "up" : "down"),
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected clock scaling
  2020-12-30 11:29 ` [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected " Can Guo
@ 2020-12-31  4:39   ` Can Guo
  0 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2020-12-31  4:39 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn, cang
  Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Stanley Chu, Bean Huo, Bart Van Assche,
	Satya Tangirala, linux-kernel

Hi Stanley,

I slightly updated this change, because I found that in 
ufshcd_devfreq_scale(),
in order to call ufshcd_wb_ctrl(), clk_scale_lock write sem is released 
once,
which breaks the assumption that clk_scale_lock wraps the entire func. 
So,
in ufshcd_devfreq_scale(), I changed the up_write() to downgrade_write() 
so that
the clk_scale_lock does not have to be released.

Thus, I removed your reviewed-by tag. It would be very helpful if you 
can review
it one more time. Thanks.

Happy new year!!!

Regards,
Can Guo.

On 2020-12-30 19:29, Can Guo wrote:
> In contexts like suspend, shutdown and error handling, we need to 
> suspend
> devfreq to make sure these contexts won't be disturbed by clock 
> scaling.
> However, suspending devfreq is not enough since users can still trigger 
> a
> clock scaling by manipulating the devfreq sysfs nodes like min/max_freq 
> and
> governor even after devfreq is suspended. Moreover, mere suspending 
> devfreq
> cannot synchroinze a clock scaling which has already been invoked 
> through
> these sysfs nodes. Add one more flag in struct clk_scaling and wrap the
> entire func ufshcd_devfreq_scale() with the clk_scaling_lock, so that 
> we
> can use this flag and clk_scaling_lock to control and synchronize clock
> scaling invoked through devfreq sysfs nodes.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 87 
> ++++++++++++++++++++++++++++++-----------------
>  drivers/scsi/ufs/ufshcd.h |  6 +++-
>  2 files changed, 60 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0c148fc..44cbee1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1147,19 +1147,33 @@ static int ufshcd_clock_scaling_prepare(struct
> ufs_hba *hba)
>  	 */
>  	ufshcd_scsi_block_requests(hba);
>  	down_write(&hba->clk_scaling_lock);
> -	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
> +
> +	if (!hba->clk_scaling.is_allowed)
> +		ret = -EAGAIN;
> +	else if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US))
>  		ret = -EBUSY;
> +
> +	if (ret) {
>  		up_write(&hba->clk_scaling_lock);
>  		ufshcd_scsi_unblock_requests(hba);
> +		goto out;
>  	}
> 
> +	/* let's not get into low power until clock scaling is completed */
> +	ufshcd_hold(hba, false);
> +
> +out:
>  	return ret;
>  }
> 
> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool 
> writelock)
>  {
> -	up_write(&hba->clk_scaling_lock);
> +	if (writelock)
> +		up_write(&hba->clk_scaling_lock);
> +	else
> +		up_read(&hba->clk_scaling_lock);
>  	ufshcd_scsi_unblock_requests(hba);
> +	ufshcd_release(hba);
>  }
> 
>  /**
> @@ -1174,13 +1188,11 @@ static void
> ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
>  static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
>  {
>  	int ret = 0;
> -
> -	/* let's not get into low power until clock scaling is completed */
> -	ufshcd_hold(hba, false);
> +	bool is_writelock = true;
> 
>  	ret = ufshcd_clock_scaling_prepare(hba);
>  	if (ret)
> -		goto out;
> +		return ret;
> 
>  	/* scale down the gear before scaling down clocks */
>  	if (!scale_up) {
> @@ -1206,14 +1218,12 @@ static int ufshcd_devfreq_scale(struct ufs_hba
> *hba, bool scale_up)
>  	}
> 
>  	/* Enable Write Booster if we have scaled up else disable it */
> -	up_write(&hba->clk_scaling_lock);
> +	downgrade_write(&hba->clk_scaling_lock);
> +	is_writelock = false;
>  	ufshcd_wb_ctrl(hba, scale_up);
> -	down_write(&hba->clk_scaling_lock);
> 
>  out_unprepare:
> -	ufshcd_clock_scaling_unprepare(hba);
> -out:
> -	ufshcd_release(hba);
> +	ufshcd_clock_scaling_unprepare(hba, is_writelock);
>  	return ret;
>  }
> 
> @@ -1487,7 +1497,7 @@ static ssize_t
> ufshcd_clkscale_enable_show(struct device *dev,
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> -	return snprintf(buf, PAGE_SIZE, "%d\n", hba->clk_scaling.is_allowed);
> +	return snprintf(buf, PAGE_SIZE, "%d\n", hba->clk_scaling.is_enabled);
>  }
> 
>  static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
> @@ -1501,7 +1511,7 @@ static ssize_t
> ufshcd_clkscale_enable_store(struct device *dev,
>  		return -EINVAL;
> 
>  	value = !!value;
> -	if (value == hba->clk_scaling.is_allowed)
> +	if (value == hba->clk_scaling.is_enabled)
>  		goto out;
> 
>  	pm_runtime_get_sync(hba->dev);
> @@ -1510,7 +1520,7 @@ static ssize_t
> ufshcd_clkscale_enable_store(struct device *dev,
>  	cancel_work_sync(&hba->clk_scaling.suspend_work);
>  	cancel_work_sync(&hba->clk_scaling.resume_work);
> 
> -	hba->clk_scaling.is_allowed = value;
> +	hba->clk_scaling.is_enabled = value;
> 
>  	if (value) {
>  		ufshcd_resume_clkscaling(hba);
> @@ -1845,8 +1855,6 @@ static void ufshcd_init_clk_scaling(struct 
> ufs_hba *hba)
>  	snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d",
>  		 hba->host->host_no);
>  	hba->clk_scaling.workq = create_singlethread_workqueue(wq_name);
> -
> -	ufshcd_clkscaling_init_sysfs(hba);
>  }
> 
>  static void ufshcd_exit_clk_scaling(struct ufs_hba *hba)
> @@ -1854,6 +1862,8 @@ static void ufshcd_exit_clk_scaling(struct 
> ufs_hba *hba)
>  	if (!ufshcd_is_clkscaling_supported(hba))
>  		return;
> 
> +	if (hba->clk_scaling.enable_attr.attr.name)
> +		device_remove_file(hba->dev, &hba->clk_scaling.enable_attr);
>  	destroy_workqueue(hba->clk_scaling.workq);
>  	ufshcd_devfreq_remove(hba);
>  }
> @@ -1918,7 +1928,7 @@ static void ufshcd_clk_scaling_start_busy(struct
> ufs_hba *hba)
>  	if (!hba->clk_scaling.active_reqs++)
>  		queue_resume_work = true;
> 
> -	if (!hba->clk_scaling.is_allowed || hba->pm_op_in_progress)
> +	if (!hba->clk_scaling.is_enabled || hba->pm_op_in_progress)
>  		return;
> 
>  	if (queue_resume_work)
> @@ -4987,7 +4997,8 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  				complete(hba->dev_cmd.complete);
>  			}
>  		}
> -		if (ufshcd_is_clkscaling_supported(hba))
> +		if (ufshcd_is_clkscaling_supported(hba) &&
> +		    hba->clk_scaling.active_reqs > 0)
>  			hba->clk_scaling.active_reqs--;
>  	}
> 
> @@ -5650,18 +5661,25 @@ static void ufshcd_err_handling_prepare(struct
> ufs_hba *hba)
>  		ufshcd_vops_resume(hba, UFS_RUNTIME_PM);
>  	} else {
>  		ufshcd_hold(hba, false);
> -		if (hba->clk_scaling.is_allowed) {
> +		if (hba->clk_scaling.is_enabled) {
>  			cancel_work_sync(&hba->clk_scaling.suspend_work);
>  			cancel_work_sync(&hba->clk_scaling.resume_work);
>  			ufshcd_suspend_clkscaling(hba);
>  		}
>  	}
> +	down_write(&hba->clk_scaling_lock);
> +	hba->clk_scaling.is_allowed = false;
> +	up_write(&hba->clk_scaling_lock);
>  }
> 
>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  {
>  	ufshcd_release(hba);
> -	if (hba->clk_scaling.is_allowed)
> +
> +	down_write(&hba->clk_scaling_lock);
> +	hba->clk_scaling.is_allowed = true;
> +	up_write(&hba->clk_scaling_lock);
> +	if (hba->clk_scaling.is_enabled)
>  		ufshcd_resume_clkscaling(hba);
>  	pm_runtime_put(hba->dev);
>  }
> @@ -7620,12 +7638,14 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>  			sizeof(struct ufs_pa_layer_attr));
>  		hba->clk_scaling.saved_pwr_info.is_valid = true;
>  		if (!hba->devfreq) {
> +			hba->clk_scaling.is_allowed = true;
>  			ret = ufshcd_devfreq_init(hba);
>  			if (ret)
>  				goto out;
> -		}
> 
> -		hba->clk_scaling.is_allowed = true;
> +			hba->clk_scaling.is_enabled = true;
> +			ufshcd_clkscaling_init_sysfs(hba);
> +		}
>  	}
> 
>  	ufs_bsg_probe(hba);
> @@ -8491,11 +8511,14 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	ufshcd_hold(hba, false);
>  	hba->clk_gating.is_suspended = true;
> 
> -	if (hba->clk_scaling.is_allowed) {
> +	if (hba->clk_scaling.is_enabled) {
>  		cancel_work_sync(&hba->clk_scaling.suspend_work);
>  		cancel_work_sync(&hba->clk_scaling.resume_work);
>  		ufshcd_suspend_clkscaling(hba);
>  	}
> +	down_write(&hba->clk_scaling_lock);
> +	hba->clk_scaling.is_allowed = false;
> +	up_write(&hba->clk_scaling_lock);
> 
>  	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
>  			req_link_state == UIC_LINK_ACTIVE_STATE) {
> @@ -8592,8 +8615,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	goto out;
> 
>  set_link_active:
> -	if (hba->clk_scaling.is_allowed)
> -		ufshcd_resume_clkscaling(hba);
>  	ufshcd_vreg_set_hpm(hba);
>  	if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
>  		ufshcd_set_link_active(hba);
> @@ -8603,7 +8624,10 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
>  		ufshcd_disable_auto_bkops(hba);
>  enable_gating:
> -	if (hba->clk_scaling.is_allowed)
> +	down_write(&hba->clk_scaling_lock);
> +	hba->clk_scaling.is_allowed = true;
> +	up_write(&hba->clk_scaling_lock);
> +	if (hba->clk_scaling.is_enabled)
>  		ufshcd_resume_clkscaling(hba);
>  	hba->clk_gating.is_suspended = false;
>  	hba->dev_info.b_rpm_dev_flush_capable = false;
> @@ -8701,7 +8725,10 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> 
>  	hba->clk_gating.is_suspended = false;
> 
> -	if (hba->clk_scaling.is_allowed)
> +	down_write(&hba->clk_scaling_lock);
> +	hba->clk_scaling.is_allowed = true;
> +	up_write(&hba->clk_scaling_lock);
> +	if (hba->clk_scaling.is_enabled)
>  		ufshcd_resume_clkscaling(hba);
> 
>  	/* Enable Auto-Hibernate if configured */
> @@ -8725,8 +8752,6 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	ufshcd_vreg_set_lpm(hba);
>  disable_irq_and_vops_clks:
>  	ufshcd_disable_irq(hba);
> -	if (hba->clk_scaling.is_allowed)
> -		ufshcd_suspend_clkscaling(hba);
>  	ufshcd_setup_clocks(hba, false);
>  	if (ufshcd_is_clkgating_allowed(hba)) {
>  		hba->clk_gating.state = CLKS_OFF;
> @@ -8944,8 +8969,6 @@ void ufshcd_remove(struct ufs_hba *hba)
> 
>  	ufshcd_exit_clk_scaling(hba);
>  	ufshcd_exit_clk_gating(hba);
> -	if (ufshcd_is_clkscaling_supported(hba))
> -		device_remove_file(hba->dev, &hba->clk_scaling.enable_attr);
>  	ufshcd_hba_exit(hba);
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_remove);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e0f00a4..5737679 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -382,7 +382,10 @@ struct ufs_saved_pwr_info {
>   * @workq: workqueue to schedule devfreq suspend/resume work
>   * @suspend_work: worker to suspend devfreq
>   * @resume_work: worker to resume devfreq
> - * @is_allowed: tracks if scaling is currently allowed or not
> + * @is_enabled: tracks if scaling is currently enabled or not, 
> controlled by
> +		clkscale_enable sysfs node
> + * @is_allowed: tracks if scaling is currently allowed or not, used to 
> block
> +		clock scaling which is not invoked from devfreq governor
>   * @is_busy_started: tracks if busy period has started or not
>   * @is_suspended: tracks if devfreq is suspended or not
>   */
> @@ -396,6 +399,7 @@ struct ufs_clk_scaling {
>  	struct workqueue_struct *workq;
>  	struct work_struct suspend_work;
>  	struct work_struct resume_work;
> +	bool is_enabled;
>  	bool is_allowed;
>  	bool is_busy_started;
>  	bool is_suspended;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v8 0/3] Three changes related with UFS clock scaling
  2020-12-30 11:29 [PATCH v8 0/3] Three changes related with UFS clock scaling Can Guo
                   ` (2 preceding siblings ...)
  2020-12-30 11:29   ` Can Guo
@ 2021-01-06  2:45 ` Martin K. Petersen
  2021-01-06  4:02   ` Can Guo
  3 siblings, 1 reply; 9+ messages in thread
From: Martin K. Petersen @ 2021-01-06  2:45 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn


Can,

> This series is made based on 5.10/scsi-fixes branch.

Please rebase on top of 5.12/scsi-queue and resubmit. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v8 0/3] Three changes related with UFS clock scaling
  2021-01-06  2:45 ` [PATCH v8 0/3] Three changes related with UFS clock scaling Martin K. Petersen
@ 2021-01-06  4:02   ` Can Guo
  0 siblings, 0 replies; 9+ messages in thread
From: Can Guo @ 2021-01-06  4:02 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: asutoshd, nguyenb, hongwus, ziqichen, rnayak, linux-scsi,
	kernel-team, saravanak, salyzyn

On 2021-01-06 10:45, Martin K. Petersen wrote:
> Can,
> 
>> This series is made based on 5.10/scsi-fixes branch.
> 
> Please rebase on top of 5.12/scsi-queue and resubmit. Thanks!

Sure Martin.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-01-06  4:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-30 11:29 [PATCH v8 0/3] Three changes related with UFS clock scaling Can Guo
2020-12-30 11:29 ` [PATCH v8 1/3] scsi: ufs: Protect some contexts from unexpected " Can Guo
2020-12-31  4:39   ` Can Guo
2020-12-30 11:29 ` [PATCH v8 2/3] scsi: ufs: Refactor ufshcd_init/exit_clk_scaling/gating() Can Guo
2020-12-30 11:29 ` [PATCH v8 3/3] scsi: ufs: Revert "Make sure clk scaling happens only when HBA is runtime ACTIVE" Can Guo
2020-12-30 11:29   ` Can Guo
2020-12-30 11:29   ` Can Guo
2021-01-06  2:45 ` [PATCH v8 0/3] Three changes related with UFS clock scaling Martin K. Petersen
2021-01-06  4:02   ` Can Guo

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.