lkml.org 
[lkml]   [2022]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
From
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.

\
 
 \ /
  Last update: 2022-04-22 19:32    [W:0.065 / U:4.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site