[PATCH v5 0/7] Several changes for UFS WriteBooster

[PATCH v5 0/7] Several changes for UFS WriteBooster

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Changelog:
V--V5:
  1. Add patch "docs: ABI: Add wb_on documentation for UFS sysfs"
  2. Unify WB related flags with wb_* prefix (Stanley Chu)
  3. Delete d_ext_ufs_feature_sup (Stanley Chu)
  4. Incorporate Stanley's suggestion to patch 6/7
  5. Replace scnprintf() with sysfs_emit() in 1/7 (Greg KH)

v3--v4:
  1. Rebase patch on 5.11/scsi-staging
  2. Add WB cleanup patches 3/6, 4/6 adn 5/6

v2--v3:
  1. Change multi-line comments style in patch 1/3 (Can Guo)

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 (7):
  scsi: ufs: Add "wb_on" sysfs node to control WB on/off
  docs: ABI: Add wb_on documentation for UFS sysfs
  scsi: ufs: Changes comment in the function ufshcd_wb_probe()
  scsi: ufs: Remove two WB related fields from struct ufs_dev_info
  scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
  scsi: ufs: Cleanup WB buffer flush toggle implementation
  scsi: ufs: Keep device active mode only
    fWriteBoosterBufferFlushDuringHibernate == 1

 Documentation/ABI/testing/sysfs-driver-ufs |   8 ++
 drivers/scsi/ufs/ufs-sysfs.c               |  41 +++++++
 drivers/scsi/ufs/ufs.h                     |  30 ++---
 drivers/scsi/ufs/ufshcd.c                  | 124 +++++++++------------
 drivers/scsi/ufs/ufshcd.h                  |   6 +-
 5 files changed, 122 insertions(+), 87 deletions(-)

-- 
2.17.1

[PATCH v5 1/7] 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.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c    |  3 +--
 drivers/scsi/ufs/ufshcd.h    |  2 ++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..f3ca3d6b82c4 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,45 @@ 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 sysfs_emit(buf, "%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_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");
+		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 +235,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 +245,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 e221add25a7e..5e1dcf4de67e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -246,7 +246,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);
@@ -5351,7 +5350,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 9bb5f0ed4124..2a97006a2c93 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 v5 2/7] docs: ABI: Add wb_on documentation for UFS sysfs

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Adds UFS sysfs documentation for new entry wb_on.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index d1a352194d2e..5a70d541b2f2 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1019,3 +1019,11 @@ Contact:	Asutosh Das <asutoshd@codeaurora.org>
 Description:	This entry shows the configured size of WriteBooster buffer.
 		0400h corresponds to 4GB.
 		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/wb_on
+Date:		December 2020
+Contact:	Bean Huo <beanhuo@micron.com>
+Description:	This sets or displays whether UFS WriteBooster is enabled. Echo
+        0 into this file to disable UFS WriteBooster or 1 to enable it. Note,
+        this is only allowed for the platform doesen't support
+        UFSHCD_CAP_CLK_SCALING.
-- 
2.17.1

[PATCH v5 3/7] 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().

Reviewed-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 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 5e1dcf4de67e..6a5532b752aa 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7232,10 +7232,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

[PATCH v5 4/7] scsi: ufs: Remove two WB related fields from struct ufs_dev_info

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

d_wb_alloc_units and d_ext_ufs_feature_sup only be used while WB probe.
They are just used to confirm the condition that "if bWriteBoosterBufferType
is set to 01h but dNumSharedWriteBoosterBufferAllocUnits is set to zero,
the WriteBooster feature is disabled", and if UFS device supports WB.
After that, no user uses them any more. So, don't need to keep time in
runtime.

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

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14dfda735adf..a789e074ae3f 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -538,9 +538,7 @@ struct ufs_dev_info {
 	u8 *model;
 	u16 wspecversion;
 	u32 clk_gating_wait_us;
-	u32 d_ext_ufs_feature_sup;
 	u8 b_wb_buffer_type;
-	u32 d_wb_alloc_units;
 	bool b_rpm_dev_flush_capable;
 	u8 b_presrv_uspc_en;
 };
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6a5532b752aa..5f08f4a59a17 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7207,6 +7207,7 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 	struct ufs_dev_info *dev_info = &hba->dev_info;
 	u8 lun;
 	u32 d_lu_wb_buf_alloc;
+	u32 ext_ufs_feature;
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return;
@@ -7224,11 +7225,10 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 	    DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
 		goto wb_disabled;
 
-	dev_info->d_ext_ufs_feature_sup =
-		get_unaligned_be32(desc_buf +
-				   DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
+	ext_ufs_feature = get_unaligned_be32(desc_buf +
+					DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
 
-	if (!(dev_info->d_ext_ufs_feature_sup & UFS_DEV_WRITE_BOOSTER_SUP))
+	if (!(ext_ufs_feature & UFS_DEV_WRITE_BOOSTER_SUP))
 		goto wb_disabled;
 
 	/*
@@ -7243,10 +7243,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 		desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
 
 	if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
-		dev_info->d_wb_alloc_units =
-		get_unaligned_be32(desc_buf +
-				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
-		if (!dev_info->d_wb_alloc_units)
+		if (!get_unaligned_be32(desc_buf +
+				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
 			goto wb_disabled;
 	} else {
 		for (lun = 0; lun < UFS_UPIU_MAX_WB_LUN_ID; lun++) {
-- 
2.17.1

[PATCH v5 5/7] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

UFS device-related flags should be grouped in ufs_dev_info. Take
wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
group them to struct ufs_dev_info, and align the names of the structure
members vertically.

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

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index f3ca3d6b82c4..9a9acc722a37 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%d\n", hba->wb_enabled);
+	return sysfs_emit(buf, "%d\n", hba->dev_info.wb_enabled);
 }
 
 static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index a789e074ae3f..ec74cf360b1f 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -527,20 +527,25 @@ struct ufs_vreg_info {
 };
 
 struct ufs_dev_info {
-	bool f_power_on_wp_en;
+	bool	f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
-	bool is_lu_power_on_wp;
+	bool	is_lu_power_on_wp;
 	/* Maximum number of general LU supported by the UFS device */
-	u8 max_lu_supported;
-	u8 wb_dedicated_lu;
-	u16 wmanufacturerid;
+	u8	max_lu_supported;
+	u16	wmanufacturerid;
 	/*UFS device Product Name */
-	u8 *model;
-	u16 wspecversion;
-	u32 clk_gating_wait_us;
-	u8 b_wb_buffer_type;
-	bool b_rpm_dev_flush_capable;
-	u8 b_presrv_uspc_en;
+	u8	*model;
+	u16	wspecversion;
+	u32	clk_gating_wait_us;
+
+	/* UFS WB related flags */
+	bool    wb_enabled;
+	bool    wb_buf_flush_enabled;
+	u8	wb_dedicated_lu;
+	u8      wb_buffer_type;
+
+	bool	b_rpm_dev_flush_capable;
+	u8	b_presrv_uspc_en;
 };
 
 /**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5f08f4a59a17..466a85051d54 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
 	if (!err) {
 		ufshcd_set_ufs_dev_active(hba);
 		if (ufshcd_is_wb_allowed(hba)) {
-			hba->wb_enabled = false;
-			hba->wb_buf_flush_enabled = false;
+			hba->dev_info.wb_enabled = false;
+			hba->dev_info.wb_buf_flush_enabled = false;
 		}
 	}
 	if (err != -EOPNOTSUPP)
@@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 	if (!ufshcd_is_wb_allowed(hba))
 		return 0;
 
-	if (!(enable ^ hba->wb_enabled))
+	if (!(enable ^ hba->dev_info.wb_enabled))
 		return 0;
 	if (enable)
 		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
@@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 		return ret;
 	}
 
-	hba->wb_enabled = enable;
+	hba->dev_info.wb_enabled = enable;
 	dev_dbg(hba->dev, "%s write booster %s %d\n",
 			__func__, enable ? "enable" : "disable", ret);
 
@@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
 	int ret;
 	u8 index;
 
-	if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
+	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
 		return 0;
 
 	index = ufshcd_wb_get_query_index(hba);
@@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
 			__func__, ret);
 	else
-		hba->wb_buf_flush_enabled = true;
+		hba->dev_info.wb_buf_flush_enabled = true;
 
 	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
 	return ret;
@@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
 	int ret;
 	u8 index;
 
-	if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
+	if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
 		return 0;
 
 	index = ufshcd_wb_get_query_index(hba);
@@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
 		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
 			 __func__, ret);
 	} else {
-		hba->wb_buf_flush_enabled = false;
+		hba->dev_info.wb_buf_flush_enabled = false;
 		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
 	}
 
@@ -7236,13 +7236,12 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 	 * 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];
+	dev_info->wb_buffer_type = desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
 
 	dev_info->b_presrv_uspc_en =
 		desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
 
-	if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
+	if (dev_info->wb_buffer_type == WB_BUF_MODE_SHARED) {
 		if (!get_unaligned_be32(desc_buf +
 				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
 			goto wb_disabled;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2a97006a2c93..ee97068158e2 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -805,8 +805,6 @@ struct ufs_hba {
 
 	struct device		bsg_dev;
 	struct request_queue	*bsg_queue;
-	bool wb_buf_flush_enabled;
-	bool wb_enabled;
 	struct delayed_work rpm_dev_flush_recheck_work;
 
 #ifdef CONFIG_SCSI_UFS_CRYPTO
@@ -946,7 +944,7 @@ static inline bool ufshcd_keep_autobkops_enabled_except_suspend(
 
 static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba)
 {
-	if (hba->dev_info.b_wb_buffer_type == WB_BUF_MODE_LU_DEDICATED)
+	if (hba->dev_info.wb_buffer_type == WB_BUF_MODE_LU_DEDICATED)
 		return hba->dev_info.wb_dedicated_lu;
 	return 0;
 }
-- 
2.17.1

[PATCH v5 6/7] scsi: ufs: Cleanup WB buffer flush toggle implementation

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
move the implementation into ufshcd_wb_toggle_flush().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 66 +++++++++++++--------------------------
 1 file changed, 21 insertions(+), 45 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 466a85051d54..ba8298f0d017 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
 static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 					 struct ufs_vreg *vreg);
 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_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
 static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
 
@@ -5398,60 +5396,38 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
 				index, NULL);
 }
 
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
-{
-	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
-		return;
-
-	if (enable)
-		ufshcd_wb_buf_flush_enable(hba);
-	else
-		ufshcd_wb_buf_flush_disable(hba);
-
-}
-
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 	u8 index;
+	enum query_opcode opcode;
 
-	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
+	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
 		return 0;
 
-	index = ufshcd_wb_get_query_index(hba);
-	ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
-				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
-				      index, NULL);
-	if (ret)
-		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
-			__func__, ret);
-	else
-		hba->dev_info.wb_buf_flush_enabled = true;
-
-	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
-	return ret;
-}
-
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
-{
-	int ret;
-	u8 index;
-
-	if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
+	if (!ufshcd_is_wb_allowed(hba) ||
+	    hba->dev_info.wb_buf_flush_enabled == enable)
 		return 0;
 
+	if (enable)
+		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
+	else
+		opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
+
 	index = ufshcd_wb_get_query_index(hba);
-	ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
-				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
-				      index, NULL);
+	ret = ufshcd_query_flag_retry(hba, opcode,
+				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
+				      NULL);
 	if (ret) {
-		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
-			 __func__, ret);
-	} else {
-		hba->dev_info.wb_buf_flush_enabled = false;
-		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
+		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
+			enable ? "enable" : "disable", ret);
+		goto out;
 	}
 
+	hba->dev_info.wb_buf_flush_enabled = enable;
+
+	dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
+out:
 	return ret;
 }
 
-- 
2.17.1

[PATCH v5 7/7] 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    |  1 +
 drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index ec74cf360b1f..506ae8fab558 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -541,6 +541,7 @@ struct ufs_dev_info {
 	/* UFS WB related flags */
 	bool    wb_enabled;
 	bool    wb_buf_flush_enabled;
+	bool    wb_buf_flush_in_hibern8;
 	u8	wb_dedicated_lu;
 	u8      wb_buffer_type;
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ba8298f0d017..3d9e6f5a7ffe 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -282,10 +282,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.wb_buf_flush_in_hibern8 = false;
+	} else {
+		hba->dev_info.wb_buf_flush_in_hibern8 = true;
+	}
+
 	ufshcd_wb_toggle_flush(hba, true);
 }
 
@@ -589,6 +595,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
 		if (ufshcd_is_wb_allowed(hba)) {
 			hba->dev_info.wb_enabled = false;
 			hba->dev_info.wb_buf_flush_enabled = false;
+			hba->dev_info.wb_buf_flush_in_hibern8 = false;
 		}
 	}
 	if (err != -EOPNOTSUPP)
@@ -5468,6 +5475,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
@@ -8567,6 +8575,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)) {
@@ -8626,11 +8635,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.wb_buf_flush_in_hibern8 &&
 			ufshcd_wb_need_flush(hba));
 	}
 
-- 
2.17.1

Re: [PATCH v5 4/7] scsi: ufs: Remove two WB related fields from struct ufs_dev_info

[Stanley Chu]

On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> d_wb_alloc_units and d_ext_ufs_feature_sup only be used while WB probe.
> They are just used to confirm the condition that "if bWriteBoosterBufferType
> is set to 01h but dNumSharedWriteBoosterBufferAllocUnits is set to zero,
> the WriteBooster feature is disabled", and if UFS device supports WB.
> After that, no user uses them any more. So, don't need to keep time in
> runtime.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>

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

Re: [PATCH v5 5/7] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Stanley Chu]

On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> UFS device-related flags should be grouped in ufs_dev_info. Take
> wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
> group them to struct ufs_dev_info, and align the names of the structure
> members vertically.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>

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

Re: [PATCH v5 6/7] scsi: ufs: Cleanup WB buffer flush toggle implementation

[Stanley Chu]

On Wed, 2020-12-16 at 00:05 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
> move the implementation into ufshcd_wb_toggle_flush().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---

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

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

[Stanley Chu]

Hi Bean,

On Wed, 2020-12-16 at 00:05 +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.
> 
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.c    |  3 +--
>  drivers/scsi/ufs/ufshcd.h    |  2 ++
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..f3ca3d6b82c4 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,45 @@ 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 sysfs_emit(buf, "%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_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");
> +		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);

May this operation race with UFS shutdown flow?

To be more clear, ufshcd_wb_ctrl() here may be executed after host clock
is disabled by shutdown flow?

If yes, we need to avoid it.

Thanks,
Stanley Chu

> +	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 +235,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 +245,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 e221add25a7e..5e1dcf4de67e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -246,7 +246,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);
> @@ -5351,7 +5350,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 9bb5f0ed4124..2a97006a2c93 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 v5 1/7] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Can Guo]

On 2020-12-22 14:08, Stanley Chu wrote:
> Hi Bean,
> 
> On Wed, 2020-12-16 at 00:05 +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.
>> 
>> Reviewed-by: Avri Altman <avri.altman@wdc.com>
>> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
>> Signed-off-by: Bean Huo <beanhuo@micron.com>
>> ---
>>  drivers/scsi/ufs/ufs-sysfs.c | 41 
>> ++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/ufs/ufshcd.c    |  3 +--
>>  drivers/scsi/ufs/ufshcd.h    |  2 ++
>>  3 files changed, 44 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
>> b/drivers/scsi/ufs/ufs-sysfs.c
>> index 08e72b7eef6a..f3ca3d6b82c4 100644
>> --- a/drivers/scsi/ufs/ufs-sysfs.c
>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
>> @@ -189,6 +189,45 @@ 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 sysfs_emit(buf, "%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_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");
>> +		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);
> 
> May this operation race with UFS shutdown flow?
> 
> To be more clear, ufshcd_wb_ctrl() here may be executed after host 
> clock
> is disabled by shutdown flow?
> 
> If yes, we need to avoid it.

I have the same doubt - can user still access sysfs nodes after system
starts to run shutdown routines? If yes, then we need to remove all UFS
sysfs nodes in ufshcd_shutdown().

Thanks,

Can Guo.

> 
> Thanks,
> Stanley Chu
> 
>> +	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 +235,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 +245,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 e221add25a7e..5e1dcf4de67e 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -246,7 +246,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);
>> @@ -5351,7 +5350,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 9bb5f0ed4124..2a97006a2c93 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 v5 6/7] scsi: ufs: Cleanup WB buffer flush toggle implementation

[Can Guo]

On 2020-12-16 07:05, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
> move the implementation into ufshcd_wb_toggle_flush().
> 

Reviewed-by: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 66 +++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 466a85051d54..ba8298f0d017 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, 
> bool on);
>  static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
>  					 struct ufs_vreg *vreg);
>  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_toggle_flush_during_h8(struct ufs_hba *hba, bool 
> set);
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool 
> enable);
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool 
> enable);
>  static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
>  static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
> 
> @@ -5398,60 +5396,38 @@ static int
> ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
>  				index, NULL);
>  }
> 
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool 
> enable)
> -{
> -	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
> -		return;
> -
> -	if (enable)
> -		ufshcd_wb_buf_flush_enable(hba);
> -	else
> -		ufshcd_wb_buf_flush_disable(hba);
> -
> -}
> -
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool 
> enable)
>  {
>  	int ret;
>  	u8 index;
> +	enum query_opcode opcode;
> 
> -	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
> +	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
>  		return 0;
> 
> -	index = ufshcd_wb_get_query_index(hba);
> -	ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> -				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
> -				      index, NULL);
> -	if (ret)
> -		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
> -			__func__, ret);
> -	else
> -		hba->dev_info.wb_buf_flush_enabled = true;
> -
> -	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
> -	return ret;
> -}
> -
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> -{
> -	int ret;
> -	u8 index;
> -
> -	if (!ufshcd_is_wb_allowed(hba) || 
> !hba->dev_info.wb_buf_flush_enabled)
> +	if (!ufshcd_is_wb_allowed(hba) ||
> +	    hba->dev_info.wb_buf_flush_enabled == enable)
>  		return 0;
> 
> +	if (enable)
> +		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> +	else
> +		opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
> +
>  	index = ufshcd_wb_get_query_index(hba);
> -	ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
> -				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
> -				      index, NULL);
> +	ret = ufshcd_query_flag_retry(hba, opcode,
> +				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
> +				      NULL);
>  	if (ret) {
> -		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
> -			 __func__, ret);
> -	} else {
> -		hba->dev_info.wb_buf_flush_enabled = false;
> -		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> +		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
> +			enable ? "enable" : "disable", ret);
> +		goto out;
>  	}
> 
> +	hba->dev_info.wb_buf_flush_enabled = enable;
> +
> +	dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : 
> "disabled");
> +out:
>  	return ret;
>  }

Re: [PATCH v5 5/7] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Can Guo]

On 2020-12-16 07:05, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> UFS device-related flags should be grouped in ufs_dev_info. Take
> wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
> group them to struct ufs_dev_info, and align the names of the structure
> members vertically.
> 

Reviewed-by: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c |  2 +-
>  drivers/scsi/ufs/ufs.h       | 27 ++++++++++++++++-----------
>  drivers/scsi/ufs/ufshcd.c    | 21 ++++++++++-----------
>  drivers/scsi/ufs/ufshcd.h    |  4 +---
>  4 files changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
> b/drivers/scsi/ufs/ufs-sysfs.c
> index f3ca3d6b82c4..9a9acc722a37 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev,
> struct device_attribute *attr,
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> -	return sysfs_emit(buf, "%d\n", hba->wb_enabled);
> +	return sysfs_emit(buf, "%d\n", hba->dev_info.wb_enabled);
>  }
> 
>  static ssize_t wb_on_store(struct device *dev, struct device_attribute 
> *attr,
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index a789e074ae3f..ec74cf360b1f 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -527,20 +527,25 @@ struct ufs_vreg_info {
>  };
> 
>  struct ufs_dev_info {
> -	bool f_power_on_wp_en;
> +	bool	f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
> -	bool is_lu_power_on_wp;
> +	bool	is_lu_power_on_wp;
>  	/* Maximum number of general LU supported by the UFS device */
> -	u8 max_lu_supported;
> -	u8 wb_dedicated_lu;
> -	u16 wmanufacturerid;
> +	u8	max_lu_supported;
> +	u16	wmanufacturerid;
>  	/*UFS device Product Name */
> -	u8 *model;
> -	u16 wspecversion;
> -	u32 clk_gating_wait_us;
> -	u8 b_wb_buffer_type;
> -	bool b_rpm_dev_flush_capable;
> -	u8 b_presrv_uspc_en;
> +	u8	*model;
> +	u16	wspecversion;
> +	u32	clk_gating_wait_us;
> +
> +	/* UFS WB related flags */
> +	bool    wb_enabled;
> +	bool    wb_buf_flush_enabled;
> +	u8	wb_dedicated_lu;
> +	u8      wb_buffer_type;
> +
> +	bool	b_rpm_dev_flush_capable;
> +	u8	b_presrv_uspc_en;
>  };
> 
>  /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5f08f4a59a17..466a85051d54 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba 
> *hba)
>  	if (!err) {
>  		ufshcd_set_ufs_dev_active(hba);
>  		if (ufshcd_is_wb_allowed(hba)) {
> -			hba->wb_enabled = false;
> -			hba->wb_buf_flush_enabled = false;
> +			hba->dev_info.wb_enabled = false;
> +			hba->dev_info.wb_buf_flush_enabled = false;
>  		}
>  	}
>  	if (err != -EOPNOTSUPP)
> @@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool 
> enable)
>  	if (!ufshcd_is_wb_allowed(hba))
>  		return 0;
> 
> -	if (!(enable ^ hba->wb_enabled))
> +	if (!(enable ^ hba->dev_info.wb_enabled))
>  		return 0;
>  	if (enable)
>  		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> @@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool 
> enable)
>  		return ret;
>  	}
> 
> -	hba->wb_enabled = enable;
> +	hba->dev_info.wb_enabled = enable;
>  	dev_dbg(hba->dev, "%s write booster %s %d\n",
>  			__func__, enable ? "enable" : "disable", ret);
> 
> @@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct 
> ufs_hba *hba)
>  	int ret;
>  	u8 index;
> 
> -	if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
> +	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
>  		return 0;
> 
>  	index = ufshcd_wb_get_query_index(hba);
> @@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct 
> ufs_hba *hba)
>  		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
>  			__func__, ret);
>  	else
> -		hba->wb_buf_flush_enabled = true;
> +		hba->dev_info.wb_buf_flush_enabled = true;
> 
>  	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
>  	return ret;
> @@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct
> ufs_hba *hba)
>  	int ret;
>  	u8 index;
> 
> -	if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
> +	if (!ufshcd_is_wb_allowed(hba) || 
> !hba->dev_info.wb_buf_flush_enabled)
>  		return 0;
> 
>  	index = ufshcd_wb_get_query_index(hba);
> @@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct
> ufs_hba *hba)
>  		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
>  			 __func__, ret);
>  	} else {
> -		hba->wb_buf_flush_enabled = false;
> +		hba->dev_info.wb_buf_flush_enabled = false;
>  		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
>  	}
> 
> @@ -7236,13 +7236,12 @@ static void ufshcd_wb_probe(struct ufs_hba
> *hba, u8 *desc_buf)
>  	 * 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];
> +	dev_info->wb_buffer_type = desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
> 
>  	dev_info->b_presrv_uspc_en =
>  		desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
> 
> -	if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
> +	if (dev_info->wb_buffer_type == WB_BUF_MODE_SHARED) {
>  		if (!get_unaligned_be32(desc_buf +
>  				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
>  			goto wb_disabled;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 2a97006a2c93..ee97068158e2 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -805,8 +805,6 @@ struct ufs_hba {
> 
>  	struct device		bsg_dev;
>  	struct request_queue	*bsg_queue;
> -	bool wb_buf_flush_enabled;
> -	bool wb_enabled;
>  	struct delayed_work rpm_dev_flush_recheck_work;
> 
>  #ifdef CONFIG_SCSI_UFS_CRYPTO
> @@ -946,7 +944,7 @@ static inline bool
> ufshcd_keep_autobkops_enabled_except_suspend(
> 
>  static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba)
>  {
> -	if (hba->dev_info.b_wb_buffer_type == WB_BUF_MODE_LU_DEDICATED)
> +	if (hba->dev_info.wb_buffer_type == WB_BUF_MODE_LU_DEDICATED)
>  		return hba->dev_info.wb_dedicated_lu;
>  	return 0;
>  }

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

[Bean Huo]

On Tue, 2020-12-22 at 14:08 +0800, Stanley Chu wrote:
> > +     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);
> 
> May this operation race with UFS shutdown flow?
> 
> To be more clear, ufshcd_wb_ctrl() here may be executed after host
> clock
> is disabled by shutdown flow?
> 
> If yes, we need to avoid it.
> 
> Thanks,
> Stanley Chu

Yes, it is quite possible, but who will mess up this on purpose?

ok, to counterbalance our concerns, I can add checkup:


/* UFS device & link must be active before we change WB status */
if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba))
	return -EINVAL;


how do you think about?

Bean

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

[Bean Huo]

On Tue, 2020-12-22 at 14:12 +0800, Can Guo wrote:
> > > +            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);
> > 
> > May this operation race with UFS shutdown flow?
> > 
> > To be more clear, ufshcd_wb_ctrl() here may be executed after host 
> > clock
> > is disabled by shutdown flow?
> > 
> > If yes, we need to avoid it.
> 
> I have the same doubt - can user still access sysfs nodes after
> system
> starts to run shutdown routines? If yes, then we need to remove all
> UFS
> sysfs nodes in ufshcd_shutdown().
> 

No, we shouldn't do in this way, user space complains this. I think
the nodes in the sysfs can be shileded write, but the nodes shouldn't
be flash of its presence frequently.

Thanks,
Bean 


> Thanks,
> 
> Can Guo.

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

[Bean Huo]

On Tue, 2020-12-22 at 21:50 +0100, Bean Huo wrote:
> > 
> > May this operation race with UFS shutdown flow?
> > 
> > To be more clear, ufshcd_wb_ctrl() here may be executed after host
> > clock
> > is disabled by shutdown flow?
> > 
> > If yes, we need to avoid it.
> > 
> > Thanks,
> > Stanley Chu
> 
> Yes, it is quite possible, but who will mess up this on purpose?
> 
> ok, to counterbalance our concerns, I can add checkup:
> 
> 
> /* UFS device & link must be active before we change WB status */
> if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba))
>         return -EINVAL;
> 
> 
> how do you think about?
> 
> Bean

seems there are only auto_hibern8 and wb will issue command to UFS
device. if you think auto_hibern8 also needs this protection.

I will add in the next version patch, otherwise, just add this checkup
in the WB.

Look forward to your feedback!

Thanks,
Bean

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

[Bean Huo]

On Tue, 2020-12-22 at 21:57 +0100, Bean Huo wrote:
> > > May this operation race with UFS shutdown flow?
> > > 
> > > To be more clear, ufshcd_wb_ctrl() here may be executed after
> > > host 
> > > clock
> > > is disabled by shutdown flow?
> > > 
> > > If yes, we need to avoid it.
> > 
> > I have the same doubt - can user still access sysfs nodes after
> > system
> > starts to run shutdown routines? If yes, then we need to remove all
> > UFS
> > sysfs nodes in ufshcd_shutdown().
> > 
> 
> No, we shouldn't do in this way, user space complains this. I think
> the nodes in the sysfs can be shileded write, but the nodes shouldn't
> be flash of its presence frequently.
> 
> Thanks,
> Bean 
> 
> 
> > Thanks,
> > 
> > Can Guo.


Hi Can
Got your point, you don't want user space to interrupt UFS by sysyfs if
UFS is in power down mode. if this is true, insteading of removing all
sysfs node in ufshcd_shutdown, maybe just add this checkup before
accessing UFS device descriptors/flag/attributes/LU:

for example, for the device descriptor:


diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-
sysfs.c       
index b3bf7fca00e5..881fe1c24a9f 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -262,6 +262,9 @@ static ssize_t ufs_sysfs_read_desc_param(struct
ufs_hba *hba,
        u8 desc_buf[8] = {0};
        int ret;
 
+       if (!ufshcd_is_ufs_dev_active(hba) ||
!ufshcd_is_link_active(hba))
+               return -EACCES;
+

Bean

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

[Can Guo]

On 2020-12-23 06:11, Bean Huo wrote:
> On Tue, 2020-12-22 at 21:57 +0100, Bean Huo wrote:
>> > > May this operation race with UFS shutdown flow?
>> > >
>> > > To be more clear, ufshcd_wb_ctrl() here may be executed after
>> > > host
>> > > clock
>> > > is disabled by shutdown flow?
>> > >
>> > > If yes, we need to avoid it.
>> >
>> > I have the same doubt - can user still access sysfs nodes after
>> > system
>> > starts to run shutdown routines? If yes, then we need to remove all
>> > UFS
>> > sysfs nodes in ufshcd_shutdown().
>> >
>> 
>> No, we shouldn't do in this way, user space complains this. I think
>> the nodes in the sysfs can be shileded write, but the nodes shouldn't
>> be flash of its presence frequently.
>> 
>> Thanks,
>> Bean
>> 
>> 
>> > Thanks,
>> >
>> > Can Guo.
> 
> 
> Hi Can
> Got your point, you don't want user space to interrupt UFS by sysyfs if
> UFS is in power down mode. if this is true, insteading of removing all
> sysfs node in ufshcd_shutdown, maybe just add this checkup before
> accessing UFS device descriptors/flag/attributes/LU:
> 
> for example, for the device descriptor:
> 
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-
> sysfs.c
> index b3bf7fca00e5..881fe1c24a9f 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -262,6 +262,9 @@ static ssize_t ufs_sysfs_read_desc_param(struct
> ufs_hba *hba,
>         u8 desc_buf[8] = {0};
>         int ret;
> 
> +       if (!ufshcd_is_ufs_dev_active(hba) ||
> !ufshcd_is_link_active(hba))
> +               return -EACCES;
> +
> 
> Bean

First of all, this check is not helping at all, during 
ufshcd_shutdown(),
both the link and dev can be active for a moment, so this check is not
helping the race condition. And assume you come up with a better check,
you want to add the check everywhere? You must have noticed the fix to
the func ufshcd_clk_gate_enable_store() from Jaegeuk Kim. So, don't
expect any luck from user space, so long the sysfs nodes are available,
users can grasp them and even stress them just for testing purposes.
I don't see why removing the sysfs nodes during ufshcd_shutdown() is a
concern to customer - we should do whatever is needed to protect LLD
contexts from user space intervene. What do you think?

Can Guo.

Thanks,

Can Guo.

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

[Bean Huo]

On Wed, 2020-12-23 at 09:31 +0800, Can Guo wrote:
> I don't see why removing the sysfs nodes during ufshcd_shutdown() is
> a
> concern to customer - we should do whatever is needed to protect LLD
> contexts from user space intervene. What do you think?

The sysfs nodes can be removed only when the device is remvoed.

Bean

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

[Bean Huo]

On Wed, 2020-12-23 at 09:31 +0800, Can Guo wrote:
> First of all, this check is not helping at all, during 
> ufshcd_shutdown(),
> both the link and dev can be active for a moment, so this check is
> not
> helping the race condition.

yes, This checkup doesn't fix race, it is to address your remove of
sysfs nodes in the ufshcd_shutdown().

Bean

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

[Bean Huo]

On Wed, 2020-12-23 at 09:31 +0800, Can Guo wrote:
> And assume you come up with a better check,
> you want to add the check everywhere? You must have noticed the fix
> to
> the func ufshcd_clk_gate_enable_store() from Jaegeuk Kim.

Do you mean lock spin_lock_irqsave(hba->host->host_lock, flags)?
It can completely fix race issue, but it is different with here.
ufshcd_clkgate_enable_store() doesn't call ufshcd_hold().
If you want using this lock, we should change ufshcd_hold() and
ufshcd_query_*().

Bean

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

[Bean Huo]

On Tue, 2020-12-22 at 14:08 +0800, Stanley Chu wrote:
> Hi Bean,
> 
> On Wed, 2020-12-16 at 00:05 +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.
> > 
> > Reviewed-by: Avri Altman <avri.altman@wdc.com>
> > Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/scsi/ufs/ufs-sysfs.c | 41
> > ++++++++++++++++++++++++++++++++++++
> >  drivers/scsi/ufs/ufshcd.c    |  3 +--
> >  drivers/scsi/ufs/ufshcd.h    |  2 ++
> >  3 files changed, 44 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-
> > sysfs.c
> > index 08e72b7eef6a..f3ca3d6b82c4 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -189,6 +189,45 @@ 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 sysfs_emit(buf, "%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_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");
> > +		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);
> 
> May this operation race with UFS shutdown flow?
> 
> To be more clear, ufshcd_wb_ctrl() here may be executed after host
> clock
> is disabled by shutdown flow?
> 
> If yes, we need to avoid it.
> 
> Thanks,
> Stanley Chu
> 



Hi Stanley and Can

I just sent a new patch to address this issue, let's discusss in that
patch set, I added PM maintainer in the mail as well to help us address
concern about pm_runtime_get_sync and shutdown flow. If that way can
fix this issue, then I will update this patch again.

one note:

I didn't add "if (!ufshcd_is_ufs_dev_active(hba) ||
!ufshcd_is_link_active(hba))" checkup, since sysfs node access is a
normal request as well, UFS driver should give correct response as
possbile as it can, even if device/link is lower power mode.

Thanks,
Bean