diff mbox series

[v3] scsi: ufs: wb: Add explicit flush_threshold sysfs attribute

Message ID 20230609030357.64360-1-luhongfei@vivo.com
State Superseded
Headers show
Series [v3] scsi: ufs: wb: Add explicit flush_threshold sysfs attribute | expand

Commit Message

Lu Hongfei June 9, 2023, 3:03 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 <luhongfei@vivo.com>
---
The modifications made compared to the previous version are as follows:
1. Using 'wb_flush_threshold == 0' insted of 'wb_flush_threshold <= 0'.

 drivers/ufs/core/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Avri Altman June 9, 2023, 7:35 a.m. UTC | #1 
> 
> 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 <luhongfei@vivo.com>
> ---
> The modifications made compared to the previous version are as follows:
> 1. Using 'wb_flush_threshold == 0' insted of 'wb_flush_threshold <= 0'.
> 
>  drivers/ufs/core/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

You are missing sysfs documentation.
> 
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index
> cdf3d5f2b77b..347207f4e8ee
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -298,6 +298,37 @@ 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, "%u\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 (kstrtouint(buf, 0, &wb_flush_threshold))
> +               return -EINVAL;
Maybe also make note if wb not supported:
if (!ufshcd_is_wb_allowed(hba)  {
               dev_info(dev, "It is not allowed to configure WB buf flushing!\n");
               return -EOPNOTSUPP;
       }

Thanks,
Avri
> +
> +       /* The range of values for wb_flush_threshold is (0,10] */
> +       if (wb_flush_threshold > UFS_WB_BUF_REMAIN_PERCENT(100) ||
> +               wb_flush_threshold == 0) {
> +               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 +338,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 +350,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
>  };
> 
> --
> 2.39.0
Avri Altman June 9, 2023, 8:29 a.m. UTC | #2 
> >
> > 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 <luhongfei@vivo.com>
> > ---
> > The modifications made compared to the previous version are as follows:
> > 1. Using 'wb_flush_threshold == 0' insted of 'wb_flush_threshold <= 0'.
> >
> >  drivers/ufs/core/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> 
> You are missing sysfs documentation.
> >
> > diff --git a/drivers/ufs/core/ufs-sysfs.c
> > b/drivers/ufs/core/ufs-sysfs.c index cdf3d5f2b77b..347207f4e8ee
> > --- a/drivers/ufs/core/ufs-sysfs.c
> > +++ b/drivers/ufs/core/ufs-sysfs.c
> > @@ -298,6 +298,37 @@ 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, "%u\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 (kstrtouint(buf, 0, &wb_flush_threshold))
> > +               return -EINVAL;
> Maybe also make note if wb not supported:
> if (!ufshcd_is_wb_allowed(hba)  {
>                dev_info(dev, "It is not allowed to configure WB buf flushing!\n");
>                return -EOPNOTSUPP;
>        }
I just noticed that Bart asked you to leave it out - sorry.  Please ignore.

Thanks,
Avri
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index cdf3d5f2b77b..347207f4e8ee
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -298,6 +298,37 @@  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, "%u\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 (kstrtouint(buf, 0, &wb_flush_threshold))
+		return -EINVAL;
+
+	/* The range of values for wb_flush_threshold is (0,10] */
+	if (wb_flush_threshold > UFS_WB_BUF_REMAIN_PERCENT(100) ||
+		wb_flush_threshold == 0) {
+		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 +338,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 +350,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
 };