Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions

[Bart Van Assche <bvanassche@acm.org>]

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.