[PATCH v2 0/3] Three changes for UFS WriteBooster

[PATCH v2 0/3] Three changes for UFS WriteBooster

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Changelog:
v1--v2:
  1. Take is_hibern8_wb_flush checkup out from function
     ufshcd_wb_need_flush() in patch 2/3
  2. Add UFSHCD_CAP_CLK_SCALING checkup in patch 1/3. that means
     only for the platform, which doesn't support UFSHCD_CAP_CLK_SCALING,
     can control WB through "wb_on".

Bean Huo (3):
  scsi: ufs: Add "wb_on" sysfs node to control WB on/off
  scsi: ufs: Keep device active mode only
    fWriteBoosterBufferFlushDuringHibernate == 1
  scsi: ufs: Changes comment in the function ufshcd_wb_probe()

 drivers/scsi/ufs/ufs-sysfs.c | 40 ++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs.h       |  2 ++
 drivers/scsi/ufs/ufshcd.c    | 30 +++++++++++++++++----------
 drivers/scsi/ufs/ufshcd.h    |  2 ++
 4 files changed, 63 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Currently UFS WriteBooster driver uses clock scaling up/down to set
WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
WB will be always on. Provide a sysfs attribute to enable/disable WB
during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs-sysfs.c | 40 ++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c    |  3 +--
 drivers/scsi/ufs/ufshcd.h    |  2 ++
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..b3bf7fca00e5 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,44 @@ static ssize_t auto_hibern8_store(struct device *dev,
 	return count;
 }
 
+static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+}
+
+static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned int wb_enable;
+	ssize_t res;
+
+	if (ufshcd_is_clkscaling_supported(hba)) {
+		/* If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND, 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");
+		return -EOPNOTSUPP;
+	}
+	if (!ufshcd_is_wb_allowed(hba))
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &wb_enable))
+		return -EINVAL;
+
+	if (wb_enable != 0 && wb_enable != 1)
+		return -EINVAL;
+
+	pm_runtime_get_sync(hba->dev);
+	res = ufshcd_wb_ctrl(hba, wb_enable);
+	pm_runtime_put_sync(hba->dev);
+
+	return res < 0 ? res : count;
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -196,6 +234,7 @@ static DEVICE_ATTR_RW(spm_lvl);
 static DEVICE_ATTR_RO(spm_target_dev_state);
 static DEVICE_ATTR_RO(spm_target_link_state);
 static DEVICE_ATTR_RW(auto_hibern8);
+static DEVICE_ATTR_RW(wb_on);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -205,6 +244,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_spm_target_dev_state.attr,
 	&dev_attr_spm_target_link_state.attr,
 	&dev_attr_auto_hibern8.attr,
+	&dev_attr_wb_on.attr,
 	NULL
 };
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 92d433d5f3ca..30332592e624 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -247,7 +247,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
 static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
 static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
 static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
 static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
@@ -5307,7 +5306,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
 				__func__, err);
 }
 
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 	u8 index;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 61344c49c2cc..c61584dff74a 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     u8 *desc_buff, int *buff_len,
 			     enum query_opcode desc_op);
 
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
 {
-- 
2.17.1

[PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
is set to one, the device flushes the WriteBooster Buffer data automatically
whenever the link enters the hibernate (HIBERN8) state. While the flushing
operation is in progress, the device should be kept in Active power mode.
Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
with its programming failure. Even this failure is less likely to occur, but
still it is possible.
This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
keep the device as "active power mode" only when this flag be successfully set
to 1.

Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h    |  2 ++
 drivers/scsi/ufs/ufshcd.c | 20 +++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index d593edb48767..311d5f7a024d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -530,6 +530,8 @@ struct ufs_dev_info {
 	bool f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
 	bool is_lu_power_on_wp;
+	/* Indicates if flush WB buffer during hibern8 successfully enabled */
+	bool is_hibern8_wb_flush;
 	/* Maximum number of general LU supported by the UFS device */
 	u8 max_lu_supported;
 	u8 wb_dedicated_lu;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 30332592e624..da38d760944b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -285,10 +285,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
 	else
 		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
+
 	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
-	if (ret)
+	if (ret) {
 		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
 			__func__, ret);
+		hba->dev_info.is_hibern8_wb_flush = false;
+	} else {
+		hba->dev_info.is_hibern8_wb_flush = true;
+	}
+
 	ufshcd_wb_toggle_flush(hba, true);
 }
 
@@ -5448,6 +5454,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return false;
+
 	/*
 	 * The ufs device needs the vcc to be ON to flush.
 	 * With user-space reduction enabled, it's enough to enable flush
@@ -8540,6 +8547,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	enum ufs_pm_level pm_lvl;
 	enum ufs_dev_pwr_mode req_dev_pwr_mode;
 	enum uic_link_state req_link_state;
+	bool hibern8;
 
 	hba->pm_op_in_progress = 1;
 	if (!ufshcd_is_shutdown_pm(pm_op)) {
@@ -8599,11 +8607,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		 * Hibern8, keep device power mode as "active power mode"
 		 * and VCC supply.
 		 */
+		hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
+			(req_link_state == UIC_LINK_ACTIVE_STATE &&
+			 ufshcd_is_auto_hibern8_enabled(hba));
+
 		hba->dev_info.b_rpm_dev_flush_capable =
-			hba->auto_bkops_enabled ||
-			(((req_link_state == UIC_LINK_HIBERN8_STATE) ||
-			((req_link_state == UIC_LINK_ACTIVE_STATE) &&
-			ufshcd_is_auto_hibern8_enabled(hba))) &&
+			hba->auto_bkops_enabled || (hibern8 &&
+			hba->dev_info.is_hibern8_wb_flush &&
 			ufshcd_wb_need_flush(hba));
 	}
 
-- 
2.17.1

[PATCH v2 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

USFHCD supports WriteBooster "LU dedicated buffer” mode and
“shared buffer” mode both, so changes the comment in the
function ufshcd_wb_probe().

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index da38d760944b..ceb562accc39 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7171,10 +7171,9 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 		goto wb_disabled;
 
 	/*
-	 * WB may be supported but not configured while provisioning.
-	 * The spec says, in dedicated wb buffer mode,
-	 * a max of 1 lun would have wb buffer configured.
-	 * Now only shared buffer mode is supported.
+	 * WB may be supported but not configured while provisioning. The spec
+	 * says, in dedicated wb buffer mode, a max of 1 lun would have wb
+	 * buffer configured.
 	 */
 	dev_info->b_wb_buffer_type =
 		desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
-- 
2.17.1

Re: [PATCH v2 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe()

[Stanley Chu]

On Sun, 2020-12-06 at 11:13 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> USFHCD supports WriteBooster "LU dedicated buffer” mode and
> “shared buffer” mode both, so changes the comment in the
> function ufshcd_wb_probe().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Can Guo <cang@codeaurora.org>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Stanley Chu]

On Sun, 2020-12-06 at 11:13 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
> is set to one, the device flushes the WriteBooster Buffer data automatically
> whenever the link enters the hibernate (HIBERN8) state. While the flushing
> operation is in progress, the device should be kept in Active power mode.
> Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
> with its programming failure. Even this failure is less likely to occur, but
> still it is possible.
> This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
> keep the device as "active power mode" only when this flag be successfully set
> to 1.
> 
> Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs.h    |  2 ++
>  drivers/scsi/ufs/ufshcd.c | 20 +++++++++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index d593edb48767..311d5f7a024d 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -530,6 +530,8 @@ struct ufs_dev_info {
>  	bool f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
>  	bool is_lu_power_on_wp;
> +	/* Indicates if flush WB buffer during hibern8 successfully enabled */
> +	bool is_hibern8_wb_flush;

Perhaps a more comprehensive name?
For example, wb_flush_during_hibern8?
>  	/* Maximum number of general LU supported by the UFS device */
>  	u8 max_lu_supported;
>  	u8 wb_dedicated_lu;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 30332592e624..da38d760944b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -285,10 +285,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
>  		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
>  	else
>  		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
> +
>  	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
> -	if (ret)
> +	if (ret) {
>  		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
>  			__func__, ret);
> +		hba->dev_info.is_hibern8_wb_flush = false;

Perhaps this statement could be dummy because
hba->dev_info.is_hibern8_wb_flush is zero-initialized and
ufshcd_wb_config() is invoked only once during ufs initialization.

Thanks,
Stanley Chu

> +	} else {
> +		hba->dev_info.is_hibern8_wb_flush = true;
> +	}
> +
>  	ufshcd_wb_toggle_flush(hba, true);
>  }
>  
> @@ -5448,6 +5454,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
>  
>  	if (!ufshcd_is_wb_allowed(hba))
>  		return false;
> +
>  	/*
>  	 * The ufs device needs the vcc to be ON to flush.
>  	 * With user-space reduction enabled, it's enough to enable flush
> @@ -8540,6 +8547,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	enum ufs_pm_level pm_lvl;
>  	enum ufs_dev_pwr_mode req_dev_pwr_mode;
>  	enum uic_link_state req_link_state;
> +	bool hibern8;
>  
>  	hba->pm_op_in_progress = 1;
>  	if (!ufshcd_is_shutdown_pm(pm_op)) {
> @@ -8599,11 +8607,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		 * Hibern8, keep device power mode as "active power mode"
>  		 * and VCC supply.
>  		 */
> +		hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
> +			(req_link_state == UIC_LINK_ACTIVE_STATE &&
> +			 ufshcd_is_auto_hibern8_enabled(hba));
> +
>  		hba->dev_info.b_rpm_dev_flush_capable =
> -			hba->auto_bkops_enabled ||
> -			(((req_link_state == UIC_LINK_HIBERN8_STATE) ||
> -			((req_link_state == UIC_LINK_ACTIVE_STATE) &&
> -			ufshcd_is_auto_hibern8_enabled(hba))) &&
> +			hba->auto_bkops_enabled || (hibern8 &&
> +			hba->dev_info.is_hibern8_wb_flush &&
>  			ufshcd_wb_need_flush(hba));
>  	}
>  

RE: [PATCH v2 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Avri Altman]

> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support
> UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH v2 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Stanley Chu]

On Sun, 2020-12-06 at 11:13 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 40 ++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.c    |  3 +--
>  drivers/scsi/ufs/ufshcd.h    |  2 ++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..b3bf7fca00e5 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,44 @@ static ssize_t auto_hibern8_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int wb_enable;
> +	ssize_t res;
> +
> +	if (ufshcd_is_clkscaling_supported(hba)) {
> +		/* If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND, 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");
> +		return -EOPNOTSUPP;
> +	}

Perhaps wb_on shall override clkscaling control. But this could be
future work.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

> +	if (!ufshcd_is_wb_allowed(hba))
> +		return -EOPNOTSUPP;
> +
> +	if (kstrtouint(buf, 0, &wb_enable))
> +		return -EINVAL;
> +
> +	if (wb_enable != 0 && wb_enable != 1)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(hba->dev);
> +	res = ufshcd_wb_ctrl(hba, wb_enable);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	return res < 0 ? res : count;
> +}
> +
>  static DEVICE_ATTR_RW(rpm_lvl);
>  static DEVICE_ATTR_RO(rpm_target_dev_state);
>  static DEVICE_ATTR_RO(rpm_target_link_state);
> @@ -196,6 +234,7 @@ static DEVICE_ATTR_RW(spm_lvl);
>  static DEVICE_ATTR_RO(spm_target_dev_state);
>  static DEVICE_ATTR_RO(spm_target_link_state);
>  static DEVICE_ATTR_RW(auto_hibern8);
> +static DEVICE_ATTR_RW(wb_on);
>  
>  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_rpm_lvl.attr,
> @@ -205,6 +244,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_spm_target_dev_state.attr,
>  	&dev_attr_spm_target_link_state.attr,
>  	&dev_attr_auto_hibern8.attr,
> +	&dev_attr_wb_on.attr,
>  	NULL
>  };
>  
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 92d433d5f3ca..30332592e624 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -247,7 +247,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
>  static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
>  static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
>  static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
>  static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
>  static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
>  static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
> @@ -5307,7 +5306,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
>  				__func__, err);
>  }
>  
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>  {
>  	int ret;
>  	u8 index;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 61344c49c2cc..c61584dff74a 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
>  			     u8 *desc_buff, int *buff_len,
>  			     enum query_opcode desc_op);
>  
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
> +
>  /* Wrapper functions for safely calling variant operations */
>  static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
>  {

RE: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Avri Altman]

> 
> According to the JEDEC UFS 3.1 Spec, If
> fWriteBoosterBufferFlushDuringHibernate
> is set to one, the device flushes the WriteBooster Buffer data automatically
> whenever the link enters the hibernate (HIBERN8) state. While the flushing
> operation is in progress, the device should be kept in Active power mode.
> Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
> with its programming failure. Even this failure is less likely to occur, but
> still it is possible.
> This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate
> setting,
> keep the device as "active power mode" only when this flag be successfully
> set
> to 1.
> 
> Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime
> suspend")
> Signed-off-by: Bean Huo <beanhuo@micron.com>
You've added the fixes tag, but my previous comment is still unanswered:
you are adding protection to a single device management command.
Why this command in particular? 
What makes it so special that it needs this extra care?

Thanks,
Avri

Re: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Mon, 2020-12-07 at 08:02 +0000, Avri Altman wrote:
> > According to the JEDEC UFS 3.1 Spec, If
> > fWriteBoosterBufferFlushDuringHibernate
> > is set to one, the device flushes the WriteBooster Buffer data
> > automatically
> > whenever the link enters the hibernate (HIBERN8) state. While the
> > flushing
> > operation is in progress, the device should be kept in Active power
> > mode.
> > Currently, we set this flag during the UFSHCD probe stage, but we
> > didn't deal
> > with its programming failure. Even this failure is less likely to
> > occur, but
> > still it is possible.
> > This patch is to add checkup of
> > fWriteBoosterBufferFlushDuringHibernate
> > setting,
> > keep the device as "active power mode" only when this flag be
> > successfully
> > set
> > to 1.
> > 
> > Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during
> > runtime
> > suspend")
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> 
> You've added the fixes tag,

yes,it is a bug.

>  but my previous comment is still unanswered:
> you are adding protection to a single device management command.
> Why this command in particular? 
> What makes it so special that it needs this extra care?
> 

see the Spec: 
"
If fWriteBoosterBufferFlushDuringHibernate is set to one, the device
flushes the WriteBooster Buffer data automatically whenever the link
enters the hibernate (HIBERN8) state.

The device shall stop the flushing operation if
fWriteBoosterBufferFlushDuringHibernate are set to zero.
....

"
If fWriteBoosterBufferFlushDuringHibernate ==0, device will not flush
WB, even if you keep device as "active mode" and LINK in hibernate
state.

Bean
Thanks,

Re: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Stanley Chu]

On Mon, 2020-12-07 at 13:36 +0800, Stanley Chu wrote:
> On Sun, 2020-12-06 at 11:13 +0100, Bean Huo wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
> > is set to one, the device flushes the WriteBooster Buffer data automatically
> > whenever the link enters the hibernate (HIBERN8) state. While the flushing
> > operation is in progress, the device should be kept in Active power mode.
> > Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
> > with its programming failure. Even this failure is less likely to occur, but
> > still it is possible.
> > This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
> > keep the device as "active power mode" only when this flag be successfully set
> > to 1.
> > 
> > Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/scsi/ufs/ufs.h    |  2 ++
> >  drivers/scsi/ufs/ufshcd.c | 20 +++++++++++++++-----
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index d593edb48767..311d5f7a024d 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -530,6 +530,8 @@ struct ufs_dev_info {
> >  	bool f_power_on_wp_en;
> >  	/* Keeps information if any of the LU is power on write protected */
> >  	bool is_lu_power_on_wp;
> > +	/* Indicates if flush WB buffer during hibern8 successfully enabled */
> > +	bool is_hibern8_wb_flush;
> 
> Perhaps a more comprehensive name?
> For example, wb_flush_during_hibern8?
> >  	/* Maximum number of general LU supported by the UFS device */
> >  	u8 max_lu_supported;
> >  	u8 wb_dedicated_lu;
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 30332592e624..da38d760944b 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -285,10 +285,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
> >  		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
> >  	else
> >  		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
> > +
> >  	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
> > -	if (ret)
> > +	if (ret) {
> >  		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
> >  			__func__, ret);
> > +		hba->dev_info.is_hibern8_wb_flush = false;
> 
> Perhaps this statement could be dummy because
> hba->dev_info.is_hibern8_wb_flush is zero-initialized and
> ufshcd_wb_config() is invoked only once during ufs initialization.

Hi Bean,
Sorry to mislead you. ufshcd_wb_config() may be called multiple times by
both initialization and error recovery. Please ignore above suggestion.

Thanks,
Stanley Chu

> 
> Thanks,
> Stanley Chu
> 
> > +	} else {
> > +		hba->dev_info.is_hibern8_wb_flush = true;
> > +	}
> > +
> >  	ufshcd_wb_toggle_flush(hba, true);
> >  }
> >  
> > @@ -5448,6 +5454,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
> >  
> >  	if (!ufshcd_is_wb_allowed(hba))
> >  		return false;
> > +
> >  	/*
> >  	 * The ufs device needs the vcc to be ON to flush.
> >  	 * With user-space reduction enabled, it's enough to enable flush
> > @@ -8540,6 +8547,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >  	enum ufs_pm_level pm_lvl;
> >  	enum ufs_dev_pwr_mode req_dev_pwr_mode;
> >  	enum uic_link_state req_link_state;
> > +	bool hibern8;
> >  
> >  	hba->pm_op_in_progress = 1;
> >  	if (!ufshcd_is_shutdown_pm(pm_op)) {
> > @@ -8599,11 +8607,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >  		 * Hibern8, keep device power mode as "active power mode"
> >  		 * and VCC supply.
> >  		 */
> > +		hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
> > +			(req_link_state == UIC_LINK_ACTIVE_STATE &&
> > +			 ufshcd_is_auto_hibern8_enabled(hba));
> > +
> >  		hba->dev_info.b_rpm_dev_flush_capable =
> > -			hba->auto_bkops_enabled ||
> > -			(((req_link_state == UIC_LINK_HIBERN8_STATE) ||
> > -			((req_link_state == UIC_LINK_ACTIVE_STATE) &&
> > -			ufshcd_is_auto_hibern8_enabled(hba))) &&
> > +			hba->auto_bkops_enabled || (hibern8 &&
> > +			hba->dev_info.is_hibern8_wb_flush &&
> >  			ufshcd_wb_need_flush(hba));
> >  	}
> >  
> 

Re: [PATCH v2 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Can Guo]

On 2020-12-06 18:13, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support 
> UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS 
> WB.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 40 ++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.c    |  3 +--
>  drivers/scsi/ufs/ufshcd.h    |  2 ++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
> b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..b3bf7fca00e5 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,44 @@ static ssize_t auto_hibern8_store(struct device 
> *dev,
>  	return count;
>  }
> 
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute 
> *attr,
> +			  char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute 
> *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int wb_enable;
> +	ssize_t res;
> +
> +	if (ufshcd_is_clkscaling_supported(hba)) {
> +		/* If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND, turn
> +		 * WB on/off will be done while clock scaling up/down.
> +		 */

Double check comment line format?

> +		dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> +		return -EOPNOTSUPP;
> +	}
> +	if (!ufshcd_is_wb_allowed(hba))
> +		return -EOPNOTSUPP;
> +
> +	if (kstrtouint(buf, 0, &wb_enable))
> +		return -EINVAL;
> +
> +	if (wb_enable != 0 && wb_enable != 1)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(hba->dev);
> +	res = ufshcd_wb_ctrl(hba, wb_enable);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	return res < 0 ? res : count;
> +}
> +
>  static DEVICE_ATTR_RW(rpm_lvl);
>  static DEVICE_ATTR_RO(rpm_target_dev_state);
>  static DEVICE_ATTR_RO(rpm_target_link_state);
> @@ -196,6 +234,7 @@ static DEVICE_ATTR_RW(spm_lvl);
>  static DEVICE_ATTR_RO(spm_target_dev_state);
>  static DEVICE_ATTR_RO(spm_target_link_state);
>  static DEVICE_ATTR_RW(auto_hibern8);
> +static DEVICE_ATTR_RW(wb_on);
> 
>  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_rpm_lvl.attr,
> @@ -205,6 +244,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = 
> {
>  	&dev_attr_spm_target_dev_state.attr,
>  	&dev_attr_spm_target_link_state.attr,
>  	&dev_attr_auto_hibern8.attr,
> +	&dev_attr_wb_on.attr,
>  	NULL
>  };
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 92d433d5f3ca..30332592e624 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -247,7 +247,6 @@ static inline int ufshcd_config_vreg_hpm(struct
> ufs_hba *hba,
>  static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
>  static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
>  static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
>  static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool 
> set);
>  static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool 
> enable);
>  static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
> @@ -5307,7 +5306,7 @@ static void
> ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
>  				__func__, err);
>  }
> 
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>  {
>  	int ret;
>  	u8 index;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 61344c49c2cc..c61584dff74a 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
>  			     u8 *desc_buff, int *buff_len,
>  			     enum query_opcode desc_op);
> 
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
> +
>  /* Wrapper functions for safely calling variant operations */
>  static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
>  {

RE: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Avri Altman]

> On Mon, 2020-12-07 at 08:02 +0000, Avri Altman wrote:
> > > According to the JEDEC UFS 3.1 Spec, If
> > > fWriteBoosterBufferFlushDuringHibernate
> > > is set to one, the device flushes the WriteBooster Buffer data
> > > automatically
> > > whenever the link enters the hibernate (HIBERN8) state. While the
> > > flushing
> > > operation is in progress, the device should be kept in Active power
> > > mode.
> > > Currently, we set this flag during the UFSHCD probe stage, but we
> > > didn't deal
> > > with its programming failure. Even this failure is less likely to
> > > occur, but
> > > still it is possible.
> > > This patch is to add checkup of
> > > fWriteBoosterBufferFlushDuringHibernate
> > > setting,
> > > keep the device as "active power mode" only when this flag be
> > > successfully
> > > set
> > > to 1.
> > >
> > > Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during
> > > runtime
> > > suspend")
> > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> >
> > You've added the fixes tag,
> 
> yes,it is a bug.
> 
> >  but my previous comment is still unanswered:
> > you are adding protection to a single device management command.
> > Why this command in particular?
> > What makes it so special that it needs this extra care?
> >
> 
> see the Spec:
> "
> If fWriteBoosterBufferFlushDuringHibernate is set to one, the device
> flushes the WriteBooster Buffer data automatically whenever the link
> enters the hibernate (HIBERN8) state.
> 
> The device shall stop the flushing operation if
> fWriteBoosterBufferFlushDuringHibernate are set to zero.
> ....
> 
> "
> If fWriteBoosterBufferFlushDuringHibernate ==0, device will not flush
> WB, even if you keep device as "active mode" and LINK in hibernate
> state.
OK, so what you are actually saying, is that since we are only toggling this flag once per
link startup / recovery, in case of a failure, however rare - the host may be still keep vcc on
for nothing, as the device will do nothing in that extra wake time.  Right?

But every time ufshcd_wb_need_flush() is called we are reading some other flags/attributes?
What about them? Why not protecting them as well?

Sorry - the whole idea doesn't make any sense to me.

Thanks,
Avri
> 
> Bean
> Thanks,
> 

Re: [PATCH v2 2/3] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Tue, 2020-12-08 at 09:05 +0000, Avri Altman wrote:
> > "
> > If fWriteBoosterBufferFlushDuringHibernate ==0, device will not
> > flush
> > WB, even if you keep device as "active mode" and LINK in hibernate
> > state.
> 
> OK, so what you are actually saying, is that since we are only
> toggling this flag once per
> link startup / recovery, in case of a failure, however rare - the
> host may be still keep vcc on
> for nothing, as the device will do nothing in that extra wake time. 
> Right?
> 

again, this is a BUG, BUG...
Bug is a Bug, doesn't matter it is rare or not rare. 

Tell me why we retry three times in ufshcd_query_flag_retry() in case
of failure?


> But every time ufshcd_wb_need_flush() is called we are reading some
> other flags/attributes?
> What about them? Why not protecting them as well?
> 

did you read ufshcd_wb_need_flush() fully? they have been properly
protected.

> Sorry - the whole idea doesn't make any sense to me.
> 

Thanks very much for sharing your opinion, I am very happy to hear your
opinion. Because you don't believe this is a bug, I will ask other UFS
guys to review, let Martin make the last decision.


Thanks agaion for your review.

Bean

Re: [PATCH v2 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Bean Huo]

On Tue, 2020-12-08 at 15:13 +0800, Can Guo wrote:
> > +     if (ufshcd_is_clkscaling_supported(hba)) {
> > +             /* If the platform supports
> > UFSHCD_CAP_AUTO_BKOPS_SUSPEND, turn
> > +              * WB on/off will be done while clock scaling
> > up/down.
> > +              */
> 
> Double check comment line format?

Can,
I will change it to the preferred comment style.

thanks,
Bean