[v6,3/6] scsi: ufs: wb: Add explicit flush sysfs attribute

Commit Message

Jinyoung Choi Aug. 2, 2022, 8:07 a.m. UTC

There is the following quirk to bypass "WB Flush" in Write Booster.

	- UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL

If this quirk is not set, there is no knob that can control "WB Flush".

There are three flags that control Write Booster Feature.
	1. WB ON/OFF
	2. WB Hibern Flush ON/OFF (implicitly)
	3. WB Flush ON/OFF (explicit)

The sysfs attribute that controls the WB was implemented. (1)

In the case of "Hibern Flush", it is always good to turn on.
Control may not be required. (2)

Finally, "Flush" may be necessary because the Auto-Hibern8 is not
supported in a specific environment.
So the sysfs attribute that controls this is necessary. (3)

Reviewed-by: Avri Altman <[e-mail]>
Signed-off-by: Jinyoung Choi <[e-mail]>
---
 Documentation/ABI/testing/sysfs-driver-ufs |  9 +++++
 drivers/ufs/core/ufs-sysfs.c               | 46 ++++++++++++++++++++++
 drivers/ufs/core/ufshcd.c                  |  9 +++--
 include/ufs/ufshcd.h                       |  1 +
 4 files changed, 61 insertions(+), 4 deletions(-)

Comments

Bart Van Assche Aug. 3, 2022, 5:38 p.m. UTC | #1

On 8/2/22 01:07, Jinyoung CHOI wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en
> +What:		/sys/bus/platform/devices/*.ufs/wb_buf_flush_en
> +Date:		July 2022
> +Contact:	Jinyoung Choi <[e-mail]>
> +Description:	This entry shows the status of WriteBooster buffer flushing

Can we rename this attribute into something that has a word order that 
is grammatically correct, e.g. enable_wb_buf_flush?

> +		and it can be used to allow or disallow the flushing.
> +		If the flushing is allowed, the device executes the flush
> +		operation when the command queue is empty.

The attribute has "enabled" in its name while the above text uses the 
verb "allowed". Consider changing "allowed" into "enabled". Please also 
change "If the flushing" into "If flushing".

Thanks,

Bart.

Jinyoung Choi Aug. 4, 2022, 5:49 a.m. UTC | #2

>On 8/2/22 01:07, Jinyoung CHOI wrote:
>> +What:		/sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en
>> +What:		/sys/bus/platform/devices/*.ufs/wb_buf_flush_en
>> +Date:		July 2022
>> +Contact:	Jinyoung Choi <[e-mail]>
>> +Description:	This entry shows the status of WriteBooster buffer flushing
>
>Can we rename this attribute into something that has a word order that 
>is grammatically correct, e.g. enable_wb_buf_flush?
>

OK, I will replace it.
Instead, When the list is printed through "ls",
it may be difficult to check because the prefix is different.

>> +		and it can be used to allow or disallow the flushing.
>> +		If the flushing is allowed, the device executes the flush
>> +		operation when the command queue is empty.
>
>The attribute has "enabled" in its name while the above text uses the 
>verb "allowed". Consider changing "allowed" into "enabled". Please also 
>change "If the flushing" into "If flushing".
>
>Thanks,
>
>Bart

OK, I will apply next patch.

Warm Regards,
Jinyoung.

Patch

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 6b248abb1bd7..1255cbda0ac9 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1417,6 +1417,15 @@  Description:	This node is used to set or display whether UFS WriteBooster is
 		platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can
 		disable/enable WriteBooster through this sysfs node.
 
+What:		/sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en
+What:		/sys/bus/platform/devices/*.ufs/wb_buf_flush_en
+Date:		July 2022
+Contact:	Jinyoung Choi <[e-mail]>
+Description:	This entry shows the status of WriteBooster buffer flushing
+		and it can be used to allow or disallow the flushing.
+		If the flushing is allowed, the device executes the flush
+		operation when the command queue is empty.
+
 What:		/sys/bus/platform/drivers/ufshcd/*/device_descriptor/hpb_version
 What:		/sys/bus/platform/devices/*.ufs/device_descriptor/hpb_version
 Date:		June 2021
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 0a088b47d557..69cc9b26c762 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -254,6 +254,50 @@  static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 	return res < 0 ? res : count;
 }
 
+static ssize_t wb_buf_flush_en_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", hba->dev_info.wb_buf_flush_enabled);
+}
+
+static ssize_t wb_buf_flush_en_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned int wb_buf_flush_en;
+	ssize_t res;
+
+	if (!ufshcd_is_wb_allowed(hba) ||
+	    (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) {
+		dev_warn(dev, "It is not allowed to configure WB buf flushing!\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (kstrtouint(buf, 0, &wb_buf_flush_en))
+		return -EINVAL;
+
+	if (wb_buf_flush_en != 0 && wb_buf_flush_en != 1)
+		return -EINVAL;
+
+	down(&hba->host_sem);
+	if (!ufshcd_is_user_access_allowed(hba)) {
+		res = -EBUSY;
+		goto out;
+	}
+
+	ufshcd_rpm_get_sync(hba);
+	res = ufshcd_wb_toggle_buf_flush(hba, wb_buf_flush_en);
+	ufshcd_rpm_put_sync(hba);
+
+out:
+	up(&hba->host_sem);
+	return res < 0 ? res : count;
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -262,6 +306,7 @@  static DEVICE_ATTR_RO(spm_target_dev_state);
 static DEVICE_ATTR_RO(spm_target_link_state);
 static DEVICE_ATTR_RW(auto_hibern8);
 static DEVICE_ATTR_RW(wb_on);
+static DEVICE_ATTR_RW(wb_buf_flush_en);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -272,6 +317,7 @@  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_spm_target_link_state.attr,
 	&dev_attr_auto_hibern8.attr,
 	&dev_attr_wb_on.attr,
+	&dev_attr_wb_buf_flush_en.attr,
 	NULL
 };
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index df00441f89a1..b7b8efd17659 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -267,7 +267,6 @@  static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
 static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
 						 bool enable);
-static void ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
 static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
 
@@ -5765,25 +5764,27 @@  static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
 			__func__, enable ? "enabled" : "disabled");
 }
 
-static void ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 
 	if (!ufshcd_is_wb_allowed(hba) ||
 	    hba->dev_info.wb_buf_flush_enabled == enable)
-		return;
+		return 0;
 
 	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);
 	if (ret) {
 		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
 			enable ? "enable" : "disable", ret);
-		return;
+		return ret;
 	}
 
 	hba->dev_info.wb_buf_flush_enabled = enable;
 
 	dev_dbg(hba->dev, "%s WB-Buf Flush %s\n",
 			__func__, enable ? "enabled" : "disabled");
+
+	return ret;
 }
 
 static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7fe1a926cd99..94bcfec98fb8 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1211,6 +1211,7 @@  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_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable);
 int ufshcd_suspend_prepare(struct device *dev);
 int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm);
 void ufshcd_resume_complete(struct device *dev);