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

[Avri Altman]

> >
> > 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@xxxxxxxx>
> > ---
> > 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