scsi: ufs: wb: Add explicit flush_threshold sysfs attribute

Commit Message

Lu Hongfei June 8, 2023, 10:16 a.m. UTC

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)

In the case of "Hibern Flush", one of the conditions for flush WB buffer is
that avail_wb_buff < wb_flush_threshold.

As we know, different users have different requirements for power
consumption and performance. Therefore, we need the ability to manually
set wb_flush_threshold, so that users can easily and flexibly adjust
the wb_flush_threshold value, thereby achieving a balance between power
consumption and performance.

So the sysfs attribute that controls this is necessary.

The meaning of wb_flush_threshold is the percentage of WB's total size,
such as 1 representing 10%, 2 representing 20%, and so on.

Signed-off-by: Lu Hongfei <[e-mail]>
---
 drivers/ufs/core/ufs-sysfs.c | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Bart Van Assche June 8, 2023, 4:44 p.m. UTC | #1

On 6/8/23 03:16, Lu Hongfei wrote:
> +static ssize_t wb_flush_threshold_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->vps->wb_flush_threshold);
> +}

Since wb_flush_threshold is unsigned, please change %d into %u.

> +	if (!ufshcd_is_wb_allowed(hba)) {
> +		dev_warn(dev, "It is not allowed to configure WB buf flush threshold!\n");
> +		return -EOPNOTSUPP;
> +	}

The above check prevents configuring the flush threshold before enabling 
the write booster mechanism. I don't think users will like this. Please 
leave out the above check.

> +	if (kstrtouint(buf, 0, &wb_flush_threshold))
> +		return -EINVAL;
> +
> +	/* The range of values for wb_flush_threshold is (0,10] */
> +	if (wb_flush_threshold <= 0 || wb_flush_threshold > 10) {
> +		dev_err(dev, "The value of wb_flush_threshold is invalid!\n");
> +		return -EINVAL;
> +	}

Please change '10' in the above code into UFS_WB_BUF_REMAIN_PERCENT(100) 
to make the above code easier to read.

Thanks,

Bart.

Patch

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index cdf3d5f2b77b..679f65dc4cab
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -298,6 +298,41 @@  static ssize_t enable_wb_buf_flush_store(struct device *dev,
 	return res < 0 ? res : count;
 }
 
+static ssize_t wb_flush_threshold_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->vps->wb_flush_threshold);
+}
+
+static ssize_t wb_flush_threshold_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_flush_threshold;
+
+	if (!ufshcd_is_wb_allowed(hba)) {
+		dev_warn(dev, "It is not allowed to configure WB buf flush threshold!\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (kstrtouint(buf, 0, &wb_flush_threshold))
+		return -EINVAL;
+
+	/* The range of values for wb_flush_threshold is (0,10] */
+	if (wb_flush_threshold <= 0 || wb_flush_threshold > 10) {
+		dev_err(dev, "The value of wb_flush_threshold is invalid!\n");
+		return -EINVAL;
+	}
+
+	hba->vps->wb_flush_threshold = wb_flush_threshold;
+
+	return count;
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -307,6 +342,7 @@  static DEVICE_ATTR_RO(spm_target_link_state);
 static DEVICE_ATTR_RW(auto_hibern8);
 static DEVICE_ATTR_RW(wb_on);
 static DEVICE_ATTR_RW(enable_wb_buf_flush);
+static DEVICE_ATTR_RW(wb_flush_threshold);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -318,6 +354,7 @@  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_auto_hibern8.attr,
 	&dev_attr_wb_on.attr,
 	&dev_attr_enable_wb_buf_flush.attr,
+	&dev_attr_wb_flush_threshold.attr,
 	NULL
 };