>
> On 4/22/22 05:14, Jinyoung CHOI wrote:
> > There is the following quirk to bypass "WB Manual Flush" in Write
> > Booster.
> >
> > - UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
> >
> > If this quirk is not set, there is no knob that can controll "WB Manual Flush".
> >
> > There are three flags that control Write Booster Feature.
> > 1. WB ON/OFF
> > 2. WB Hibern Flush ON/OFF
> > 3. WB Flush ON/OFF
> >
> > The sysfs 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, "Manual flush" may be determined that it can affect
> > performance or power consumption.
> > So the sysfs that controls this may be necessary. (3)
> >
> > In addition, toggle functions for controlling the above flags are cleaned.
>
> Please make all sentences in the patch description start at the left margin.
>
OK. I'll fix it. :)
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > index 5c405ff7b6ea..6bbb56152708 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -229,7 +229,7 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> > * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
> > * on/off will be done while clock scaling up/down.
> > */
> > - dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> > + dev_warn(dev, "To control Write Booster is not allowed!\n");
> > return -EOPNOTSUPP;
> > }
>
> The new error message is grammatically incorrect. Please fix.
>
OK. I'll fix it. :)
> > + if (!ufshcd_is_wb_flush_allowed(hba)) {
> > + dev_warn(dev, "To control WB Flush is not allowed!\n");
>
> Same issue for the above error message.
>
> > +static DEVICE_ATTR_RW(wb_flush_on);
>
> "wb_flush_enabled" is probably a better name than "wb_flush_on".
> Additionally, the "wb_flush_en" is closer to the terminology used in the
> UFS specification (fWriteBoosterBufferFlushEn).
>
'wb_on' sysfs already existed. So I named it in the same format. (_on)
I'll change both. (_on -> _enable)
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index 4a00c24a3209..6c85f512f82f 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -611,7 +611,7 @@ struct ufs_dev_info {
> >
> > /* UFS WB related flags */
> > bool wb_enabled;
> > - bool wb_buf_flush_enabled;
> > + bool wb_flush_enabled;
> > u8 wb_dedicated_lu;
> > u8 wb_buffer_type;
>
> Adding a variable with the name "wb_flush_enabled" next to a variable with
> the name "wb_buf_flush_enabled" is confusing. Please chose better names and
> add comments.
>
Hmm... it would be better not to modify the variable name.
I'll put it back
> > -static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
> > +static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
> > + bool set, enum flag_idn idn)
> > {
> > + int ret;
> > u8 index;
> > enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
> > - UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > + UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > +
> > + if (!ufshcd_is_wb_allowed(hba))
> > + return -EPERM;
> >
> > index = ufshcd_wb_get_query_index(hba);
> > - return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > +
> > + ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > + if (ret) {
> > + dev_err(hba->dev, "%s: %s %s failed %d\n",
> > + __func__, knob, set ? "enable" : "disable", ret);
> > + return ret;
> > + }
> > +
> > + dev_dbg(hba->dev, "%s: %s %s\n",
> > + __func__, knob, set ? "enabled" : "disabled");
> > +
> > + return ret;
> > }
>
> Please leave out the dev_dbg() message and move the dev_err() message to
> the callers of __ufshcd_wb_toggle() such that the 'knob' argument does not
> have to be added to __ufshcd_wb_toggle().
>
> Thanks,
>
> Bart.
OK. I got it.
Regarding this review, I wrote a comment on avri's comment.
Thanks,
Jinyoung
