Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7c13f4f8-0921-5a15-70bc-248550f1efa4@acm.org>
Date:   Thu, 8 Jun 2023 09:44:26 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] scsi: ufs: wb: Add explicit flush_threshold sysfs
 attribute
Content-Language: en-US
To:     Lu Hongfei <luhongfei@vivo.com>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Bean Huo <beanhuo@micron.com>,
        Jinyoung Choi <j-young.choi@samsung.com>,
        Peter Wang <peter.wang@mediatek.com>,
        Daniil Lunev <dlunev@chromium.org>,
        "open list:UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER" 
        <linux-scsi@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Cc:     opensource.kernel@vivo.com
References: <20230608101642.40086-1-luhongfei@vivo.com>
From:   Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <20230608101642.40086-1-luhongfei@vivo.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/8/23 03:16, Lu Hongfei wrote:
> +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, "%d\n", hba->vps->wb_flush_threshold);
> +}

Since wb_flush_threshold is unsigned, please change %d into %u.

> +	if (!ufshcd_is_wb_allowed(hba)) {
> +		dev_warn(dev, "It is not allowed to configure WB buf flush threshold!\n");
> +		return -EOPNOTSUPP;
> +	}

The above check prevents configuring the flush threshold before enabling 
the write booster mechanism. I don't think users will like this. Please 
leave out the above check.

> +	if (kstrtouint(buf, 0, &wb_flush_threshold))
> +		return -EINVAL;
> +
> +	/* The range of values for wb_flush_threshold is (0,10] */
> +	if (wb_flush_threshold <= 0 || wb_flush_threshold > 10) {
> +		dev_err(dev, "The value of wb_flush_threshold is invalid!\n");
> +		return -EINVAL;
> +	}

Please change '10' in the above code into UFS_WB_BUF_REMAIN_PERCENT(100) 
to make the above code easier to read.

Thanks,

Bart.