Messages in this thread | | | Date | Fri, 22 Apr 2022 10:25:07 -0700 | Subject | Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions | From | Bart Van Assche <> |
| |
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.
> 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.
> + 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).
> 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.
> -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.
| |