[PATCH v3 0/6] Several changes for UFSHPB

[PATCH v3 0/6] Several changes for UFSHPB

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Hi UFS driver developers/reviewers

Here are some changes to the UFS HPB driver. They are all based on Martin's Git repo
5.18/scsi-staging branch. Please refer to the patch submission information for the
specific purpose of each patch. I tested them on my own platform. Please have a review
and any comments and suggestions are welcome.

v2--v3:
     1. Fix two comment issues in patch 5/6
     2. change function ufshpb_toggle_state() in patch 1/6
     3. Add new pach 4/6
v1--v2:
     1. Increase the submission information of the cover letter.
     2. Fix coding style issues in patch 4/5
     3. Add new patch 5/5.

Bean Huo (6):
  scsi: ufshpb: Merge ufshpb_reset() and ufshpb_reset_host()
  scsi: ufshpb: Remove 0 assignment for enum value
  scsi: ufshpb: Cleanup the handler when device reset HPB information
  scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with
    rcmd_*
  scsi: ufshpb: Add handing of device reset HPB regions Infos in HPB
    device mode
  scsi: ufshpb: Cleanup ufshpb_suspend/resume

 Documentation/ABI/testing/sysfs-driver-ufs |  18 +-
 drivers/scsi/ufs/ufshcd.c                  |   4 +-
 drivers/scsi/ufs/ufshpb.c                  | 182 ++++++++++++---------
 drivers/scsi/ufs/ufshpb.h                  |  16 +-
 4 files changed, 125 insertions(+), 95 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] scsi: ufshpb: Merge ufshpb_reset() and ufshpb_reset_host()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

There is no functional change in this patch, just merge ufshpb_reset()
and ufshpb_reset_host() into one function ufshpb_toggle_state().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c |  4 ++--
 drivers/scsi/ufs/ufshpb.c | 36 +++++++++++++-----------------------
 drivers/scsi/ufs/ufshpb.h |  6 ++----
 3 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3f9caafa91bf..62c7f5404353 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	 * Stop the host controller and complete the requests
 	 * cleared by h/w
 	 */
-	ufshpb_reset_host(hba);
+	ufshpb_toggle_state(hba, HPB_PRESENT, HPB_RESET);
 	ufshcd_hba_stop(hba);
 	hba->silence_err_logs = true;
 	ufshcd_complete_requests(hba);
@@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	/* Enable Auto-Hibernate if configured */
 	ufshcd_auto_hibern8_enable(hba);
 
-	ufshpb_reset(hba);
+	ufshpb_toggle_state(hba, HPB_RESET, HPB_PRESENT);
 out:
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (ret)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index fd31e516e491..167643969165 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2272,38 +2272,28 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba)
 	return flag_res;
 }
 
-void ufshpb_reset(struct ufs_hba *hba)
+/**
+ * ufshpb_toggle_state - switch HPB state of all LUs
+ * @hba: per-adapter instance
+ * @src: expected current HPB state
+ * @dest: target HPB state to switch to
+ */
+void ufshpb_toggle_state(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest)
 {
 	struct ufshpb_lu *hpb;
 	struct scsi_device *sdev;
 
 	shost_for_each_device(sdev, hba->host) {
 		hpb = ufshpb_get_hpb_data(sdev);
-		if (!hpb)
-			continue;
-
-		if (ufshpb_get_state(hpb) != HPB_RESET)
-			continue;
-
-		ufshpb_set_state(hpb, HPB_PRESENT);
-	}
-}
-
-void ufshpb_reset_host(struct ufs_hba *hba)
-{
-	struct ufshpb_lu *hpb;
-	struct scsi_device *sdev;
 
-	shost_for_each_device(sdev, hba->host) {
-		hpb = ufshpb_get_hpb_data(sdev);
-		if (!hpb)
+		if (!hpb || ufshpb_get_state(hpb) != src)
 			continue;
+		ufshpb_set_state(hpb, dest);
 
-		if (ufshpb_get_state(hpb) != HPB_PRESENT)
-			continue;
-		ufshpb_set_state(hpb, HPB_RESET);
-		ufshpb_cancel_jobs(hpb);
-		ufshpb_discard_rsp_lists(hpb);
+		if (dest == HPB_RESET) {
+			ufshpb_cancel_jobs(hpb);
+			ufshpb_discard_rsp_lists(hpb);
+		}
 	}
 }
 
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index b475dbd78988..7aa0dc58ee9f 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) { return 0;
 static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) {}
 static void ufshpb_resume(struct ufs_hba *hba) {}
 static void ufshpb_suspend(struct ufs_hba *hba) {}
-static void ufshpb_reset(struct ufs_hba *hba) {}
-static void ufshpb_reset_host(struct ufs_hba *hba) {}
+static void ufshpb_toggle_state(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest) {}
 static void ufshpb_init(struct ufs_hba *hba) {}
 static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev) {}
 static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev) {}
@@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
 void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
 void ufshpb_resume(struct ufs_hba *hba);
 void ufshpb_suspend(struct ufs_hba *hba);
-void ufshpb_reset(struct ufs_hba *hba);
-void ufshpb_reset_host(struct ufs_hba *hba);
+void ufshpb_toggle_state(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest);
 void ufshpb_init(struct ufs_hba *hba);
 void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev);
 void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);
-- 
2.34.1

[PATCH v3 2/6] scsi: ufshpb: Remove 0 assignment for enum value

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

If the first enumerator has no initializer, the value of the corresponding
constant is zero.

Reviewed-by: Keoseong Park <keosung.park@samsung.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshpb.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 7aa0dc58ee9f..b83b9ec9044a 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -59,8 +59,8 @@ enum UFSHPB_MODE {
 };
 
 enum UFSHPB_STATE {
-	HPB_INIT = 0,
-	HPB_PRESENT = 1,
+	HPB_INIT,
+	HPB_PRESENT,
 	HPB_SUSPEND,
 	HPB_FAILED,
 	HPB_RESET,
-- 
2.34.1

[PATCH v3 3/6] scsi: ufshpb: Cleanup the handler when device reset HPB information

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

"When the device is powered off by the host, the device may restore L2P map data
upon power up or build from the host’s HPB READ command. In case device powered
up and lost HPB information, device can signal to the host through HPB Sense data,
by setting HPB Operation as ‘2’ which will inform the host that device reset HPB
information."

This patch is to clean up the handler and make the intent of this handler more
readable, no functional change.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshpb.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 167643969165..f1f30d4c3d65 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1225,7 +1225,10 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		queue_work(ufshpb_wq, &hpb->map_work);
 }
 
-static void ufshpb_dev_reset_handler(struct ufshpb_lu *hpb)
+/*
+ * Set the flags of all active regions to RGN_FLAG_UPDATE to let host side reload L2P entries later
+ */
+static void ufshpb_set_regions_update(struct ufshpb_lu *hpb)
 {
 	struct victim_select_info *lru_info = &hpb->lru_info;
 	struct ufshpb_region *rgn;
@@ -1239,6 +1242,24 @@ static void ufshpb_dev_reset_handler(struct ufshpb_lu *hpb)
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
 }
 
+static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
+{
+	struct scsi_device *sdev;
+	struct ufshpb_lu *hpb;
+
+	__shost_for_each_device(sdev, hba->host) {
+		hpb = ufshpb_get_hpb_data(sdev);
+		if (hpb && hpb->is_hcm)
+			/*
+			 * For the HPB host mode, in case device powered up and lost HPB
+			 * information, we will set the region flag to be RGN_FLAG_UPDATE,
+			 * it will let host reload its L2P entries(re-activate the region
+			 * in the UFS device).
+			 */
+			ufshpb_set_regions_update(hpb);
+	}
+}
+
 /*
  * This function will parse recommended active subregion information in sense
  * data field of response UPIU with SAM_STAT_GOOD state.
@@ -1313,17 +1334,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	case HPB_RSP_DEV_RESET:
 		dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
 			 "UFS device lost HPB information during PM.\n");
-
-		if (hpb->is_hcm) {
-			struct scsi_device *sdev;
-
-			__shost_for_each_device(sdev, hba->host) {
-				struct ufshpb_lu *h = sdev->hostdata;
-
-				if (h)
-					ufshpb_dev_reset_handler(h);
-			}
-		}
+		ufshpb_dev_reset_handler(hba);
 
 		break;
 	default:
-- 
2.34.1

[PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with rcmd_*

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
and rb_inactive_cnt, they are all related to HPB recommendation in UPIU response
packet. I don't know what 'rb' refers to, I think 'rcmd'
(recommendation) should be the correct abbreviation.

Change the sysfs documentation about these sysfs nodes to highlight what they mean
under different HPB control modes.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 18 +++++++++-------
 drivers/scsi/ufs/ufshpb.c                  | 24 +++++++++++-----------
 drivers/scsi/ufs/ufshpb.h                  |  6 +++---
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index a44ef8bfbadf..6b248abb1bd7 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1518,7 +1518,7 @@ Description:	This entry shows the number of reads that cannot be changed to
 
 		The file is read only.
 
-What:		/sys/class/scsi_device/*/device/hpb_stats/rb_noti_cnt
+What:		/sys/class/scsi_device/*/device/hpb_stats/rcmd_noti_cnt
 Date:		June 2021
 Contact:	Daejun Park <daejun7.park@samsung.com>
 Description:	This entry shows the number of response UPIUs that has
@@ -1526,19 +1526,23 @@ Description:	This entry shows the number of response UPIUs that has
 
 		The file is read only.
 
-What:		/sys/class/scsi_device/*/device/hpb_stats/rb_active_cnt
+What:		/sys/class/scsi_device/*/device/hpb_stats/rcmd_active_cnt
 Date:		June 2021
 Contact:	Daejun Park <daejun7.park@samsung.com>
-Description:	This entry shows the number of active sub-regions recommended by
-		response UPIUs.
+Description:	For the HPB device control mode, this entry shows the number of
+        active sub-regions recommended by response UPIUs. For the HPB host control
+        mode, this entry shows the number of active sub-regions recommended by the
+        HPB host control mode heuristic algorithm.
 
 		The file is read only.
 
-What:		/sys/class/scsi_device/*/device/hpb_stats/rb_inactive_cnt
+What:		/sys/class/scsi_device/*/device/hpb_stats/rcmd_inactive_cnt
 Date:		June 2021
 Contact:	Daejun Park <daejun7.park@samsung.com>
-Description:	This entry shows the number of inactive regions recommended by
-		response UPIUs.
+Description:	For the HPB device control mode, this entry shows the number of
+        inactive regions recommended by response UPIUs. For the HPB host control
+        mode, this entry shows the number of inactive regions recommended by the
+        HPB host control mode heuristic algorithm.
 
 		The file is read only.
 
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index f1f30d4c3d65..e7f311bb4401 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -563,7 +563,7 @@ static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
 	if (list_empty(&srgn->list_act_srgn))
 		list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
 
-	hpb->stats.rb_active_cnt++;
+	hpb->stats.rcmd_active_cnt++;
 }
 
 static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
@@ -580,7 +580,7 @@ static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
 	if (list_empty(&rgn->list_inact_rgn))
 		list_add_tail(&rgn->list_inact_rgn, &hpb->lh_inact_rgn);
 
-	hpb->stats.rb_inactive_cnt++;
+	hpb->stats.rcmd_inactive_cnt++;
 }
 
 static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
@@ -1321,7 +1321,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
 		return;
 
-	hpb->stats.rb_noti_cnt++;
+	hpb->stats.rcmd_noti_cnt++;
 
 	switch (rsp_field->hpb_op) {
 	case HPB_RSP_REQ_REGION_UPDATE:
@@ -1724,18 +1724,18 @@ static DEVICE_ATTR_RO(__name)
 
 ufshpb_sysfs_attr_show_func(hit_cnt);
 ufshpb_sysfs_attr_show_func(miss_cnt);
-ufshpb_sysfs_attr_show_func(rb_noti_cnt);
-ufshpb_sysfs_attr_show_func(rb_active_cnt);
-ufshpb_sysfs_attr_show_func(rb_inactive_cnt);
+ufshpb_sysfs_attr_show_func(rcmd_noti_cnt);
+ufshpb_sysfs_attr_show_func(rcmd_active_cnt);
+ufshpb_sysfs_attr_show_func(rcmd_inactive_cnt);
 ufshpb_sysfs_attr_show_func(map_req_cnt);
 ufshpb_sysfs_attr_show_func(umap_req_cnt);
 
 static struct attribute *hpb_dev_stat_attrs[] = {
 	&dev_attr_hit_cnt.attr,
 	&dev_attr_miss_cnt.attr,
-	&dev_attr_rb_noti_cnt.attr,
-	&dev_attr_rb_active_cnt.attr,
-	&dev_attr_rb_inactive_cnt.attr,
+	&dev_attr_rcmd_noti_cnt.attr,
+	&dev_attr_rcmd_active_cnt.attr,
+	&dev_attr_rcmd_inactive_cnt.attr,
 	&dev_attr_map_req_cnt.attr,
 	&dev_attr_umap_req_cnt.attr,
 	NULL,
@@ -2098,9 +2098,9 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
 {
 	hpb->stats.hit_cnt = 0;
 	hpb->stats.miss_cnt = 0;
-	hpb->stats.rb_noti_cnt = 0;
-	hpb->stats.rb_active_cnt = 0;
-	hpb->stats.rb_inactive_cnt = 0;
+	hpb->stats.rcmd_noti_cnt = 0;
+	hpb->stats.rcmd_active_cnt = 0;
+	hpb->stats.rcmd_inactive_cnt = 0;
 	hpb->stats.map_req_cnt = 0;
 	hpb->stats.umap_req_cnt = 0;
 }
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index b83b9ec9044a..0d6e6004d783 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -211,9 +211,9 @@ struct ufshpb_params {
 struct ufshpb_stats {
 	u64 hit_cnt;
 	u64 miss_cnt;
-	u64 rb_noti_cnt;
-	u64 rb_active_cnt;
-	u64 rb_inactive_cnt;
+	u64 rcmd_noti_cnt;
+	u64 rcmd_active_cnt;
+	u64 rcmd_inactive_cnt;
 	u64 map_req_cnt;
 	u64 pre_req_cnt;
 	u64 umap_req_cnt;
-- 
2.34.1

[PATCH v3 5/6] scsi: ufshpb: Add handing of device reset HPB regions Infos in HPB device mode

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

In UFS HPB Spec JESD220-3A,

"5.8. Active and inactive information upon power cycle
...
When the device is powered off by the host, the device may restore L2P map data
upon power up or build from the host’s HPB READ command. In case device powered
up and lost HPB information, device can signal to the host through HPB Sense data,
by setting HPB Operation as ‘2’ which will inform the host that device reset HPB
information."

Therefore, for HPB device control mode, if the UFS device is reset via the RST_N
pin, the active region information in the device will be reset. If the host side
receives this notification from the device side, it is recommended to inactivate
all active regions in the host's HPB cache.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshpb.c | 82 +++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index e7f311bb4401..7868412054bf 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1137,6 +1137,39 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
 	return ret;
 }
+/**
+ *ufshpb_submit_region_inactive() - submit a region to be inactivated later
+ *@hpb: per-LU HPB instance
+ *@region_index: the index associated with the region that will be inactivated later
+ */
+static void ufshpb_submit_region_inactive(struct ufshpb_lu *hpb, int region_index)
+{
+	int subregion_index;
+	struct ufshpb_region *rgn;
+	struct ufshpb_subregion *srgn;
+
+	/*
+	 * Remove this region from active region list and add it to inactive list
+	 */
+	spin_lock(&hpb->rsp_list_lock);
+	ufshpb_update_inactive_info(hpb, region_index);
+	spin_unlock(&hpb->rsp_list_lock);
+
+	rgn = hpb->rgn_tbl + region_index;
+
+	/*
+	 * Set subregion state to be HPB_SRGN_INVALID, there will no HPB read on this subregion
+	 */
+	spin_lock(&hpb->rgn_state_lock);
+	if (rgn->rgn_state != HPB_RGN_INACTIVE) {
+		for (subregion_index = 0; subregion_index < rgn->srgn_cnt; subregion_index++) {
+			srgn = rgn->srgn_tbl + subregion_index;
+			if (srgn->srgn_state == HPB_SRGN_VALID)
+				srgn->srgn_state = HPB_SRGN_INVALID;
+		}
+	}
+	spin_unlock(&hpb->rgn_state_lock);
+}
 
 static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 					 struct utp_hpb_rsp *rsp_field)
@@ -1196,25 +1229,8 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 
 	for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
 		rgn_i = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
-		dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
-			"inactivate(%d) region %d\n", i, rgn_i);
-
-		spin_lock(&hpb->rsp_list_lock);
-		ufshpb_update_inactive_info(hpb, rgn_i);
-		spin_unlock(&hpb->rsp_list_lock);
-
-		rgn = hpb->rgn_tbl + rgn_i;
-
-		spin_lock(&hpb->rgn_state_lock);
-		if (rgn->rgn_state != HPB_RGN_INACTIVE) {
-			for (srgn_i = 0; srgn_i < rgn->srgn_cnt; srgn_i++) {
-				srgn = rgn->srgn_tbl + srgn_i;
-				if (srgn->srgn_state == HPB_SRGN_VALID)
-					srgn->srgn_state = HPB_SRGN_INVALID;
-			}
-		}
-		spin_unlock(&hpb->rgn_state_lock);
-
+		dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "inactivate(%d) region %d\n", i, rgn_i);
+		ufshpb_submit_region_inactive(hpb, rgn_i);
 	}
 
 out:
@@ -1249,14 +1265,32 @@ static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
 
 	__shost_for_each_device(sdev, hba->host) {
 		hpb = ufshpb_get_hpb_data(sdev);
-		if (hpb && hpb->is_hcm)
+		if (!hpb)
+			continue;
+
+		if (hpb->is_hcm) {
 			/*
-			 * For the HPB host mode, in case device powered up and lost HPB
-			 * information, we will set the region flag to be RGN_FLAG_UPDATE,
-			 * it will let host reload its L2P entries(re-activate the region
-			 * in the UFS device).
+			 * For the HPB host control mode, in case device powered up and lost HPB
+			 * information, we will set the region flag to be RGN_FLAG_UPDATE, it will
+			 * let host reload its L2P entries(reactivate region in the UFS device).
 			 */
 			ufshpb_set_regions_update(hpb);
+		} else {
+			/*
+			 * For the HPB device control mode, if host side receives 02h:HPB Operation
+			 * in UPIU response, which means device recommends the host side should
+			 * inactivate all active regions. Here we add all active regions to inactive
+			 * list, they will be inactivated later in ufshpb_map_work_handler().
+			 */
+			struct victim_select_info *lru_info = &hpb->lru_info;
+			struct ufshpb_region *rgn;
+
+			list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
+				ufshpb_submit_region_inactive(hpb, rgn->rgn_idx);
+
+			if (ufshpb_get_state(hpb) == HPB_PRESENT)
+				queue_work(ufshpb_wq, &hpb->map_work);
+		}
 	}
 }
 
-- 
2.34.1

[PATCH v3 6/6] scsi: ufshpb: Cleanup ufshpb_suspend/resume

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

ufshpb_resume() is only called when the HPB state is HPB_SUSPEND, so
the check statement for "ufshpb_get_state(hpb) != HPB_PRESENT" is useless.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshpb.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 7868412054bf..10f40e390dc5 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2349,11 +2349,9 @@ void ufshpb_suspend(struct ufs_hba *hba)
 
 	shost_for_each_device(sdev, hba->host) {
 		hpb = ufshpb_get_hpb_data(sdev);
-		if (!hpb)
+		if (!hpb || ufshpb_get_state(hpb) != HPB_PRESENT)
 			continue;
 
-		if (ufshpb_get_state(hpb) != HPB_PRESENT)
-			continue;
 		ufshpb_set_state(hpb, HPB_SUSPEND);
 		ufshpb_cancel_jobs(hpb);
 	}
@@ -2366,20 +2364,15 @@ void ufshpb_resume(struct ufs_hba *hba)
 
 	shost_for_each_device(sdev, hba->host) {
 		hpb = ufshpb_get_hpb_data(sdev);
-		if (!hpb)
+		if (!hpb || ufshpb_get_state(hpb) != HPB_SUSPEND)
 			continue;
 
-		if ((ufshpb_get_state(hpb) != HPB_PRESENT) &&
-		    (ufshpb_get_state(hpb) != HPB_SUSPEND))
-			continue;
 		ufshpb_set_state(hpb, HPB_PRESENT);
 		ufshpb_kick_map_work(hpb);
 		if (hpb->is_hcm) {
-			unsigned int poll =
-				hpb->params.timeout_polling_interval_ms;
+			unsigned int poll = hpb->params.timeout_polling_interval_ms;
 
-			schedule_delayed_work(&hpb->ufshpb_read_to_work,
-				msecs_to_jiffies(poll));
+			schedule_delayed_work(&hpb->ufshpb_read_to_work, msecs_to_jiffies(poll));
 		}
 	}
 }
-- 
2.34.1

RE: [PATCH v3 1/6] scsi: ufshpb: Merge ufshpb_reset() and ufshpb_reset_host()

[Keoseong Park]

Hi Bean,

>From: Bean Huo <beanhuo@micron.com>
> 
>There is no functional change in this patch, just merge ufshpb_reset()
>and ufshpb_reset_host() into one function ufshpb_toggle_state().
> 
>Signed-off-by: Bean Huo <beanhuo@micron.com>
Looks good to me.

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park

>---
> drivers/scsi/ufs/ufshcd.c |  4 ++--
> drivers/scsi/ufs/ufshpb.c | 36 +++++++++++++-----------------------
> drivers/scsi/ufs/ufshpb.h |  6 ++----
> 3 files changed, 17 insertions(+), 29 deletions(-)
> 
>diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>index 3f9caafa91bf..62c7f5404353 100644
>--- a/drivers/scsi/ufs/ufshcd.c
>+++ b/drivers/scsi/ufs/ufshcd.c
>@@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>          * Stop the host controller and complete the requests
>          * cleared by h/w
>          */
>-        ufshpb_reset_host(hba);
>+        ufshpb_toggle_state(hba, HPB_PRESENT, HPB_RESET);
>         ufshcd_hba_stop(hba);
>         hba->silence_err_logs = true;
>         ufshcd_complete_requests(hba);
>@@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>         /* Enable Auto-Hibernate if configured */
>         ufshcd_auto_hibern8_enable(hba);
> 
>-        ufshpb_reset(hba);
>+        ufshpb_toggle_state(hba, HPB_RESET, HPB_PRESENT);
> out:
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         if (ret)
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index fd31e516e491..167643969165 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -2272,38 +2272,28 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba)
>         return flag_res;
> }
> 
>-void ufshpb_reset(struct ufs_hba *hba)
>+/**
>+ * ufshpb_toggle_state - switch HPB state of all LUs
>+ * @hba: per-adapter instance
>+ * @src: expected current HPB state
>+ * @dest: target HPB state to switch to
>+ */
>+void ufshpb_toggle_state(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest)
> {
>         struct ufshpb_lu *hpb;
>         struct scsi_device *sdev;
> 
>         shost_for_each_device(sdev, hba->host) {
>                 hpb = ufshpb_get_hpb_data(sdev);
>-                if (!hpb)
>-                        continue;
>-
>-                if (ufshpb_get_state(hpb) != HPB_RESET)
>-                        continue;
>-
>-                ufshpb_set_state(hpb, HPB_PRESENT);
>-        }
>-}
>-
>-void ufshpb_reset_host(struct ufs_hba *hba)
>-{
>-        struct ufshpb_lu *hpb;
>-        struct scsi_device *sdev;
> 
>-        shost_for_each_device(sdev, hba->host) {
>-                hpb = ufshpb_get_hpb_data(sdev);
>-                if (!hpb)
>+                if (!hpb || ufshpb_get_state(hpb) != src)
>                         continue;
>+                ufshpb_set_state(hpb, dest);
> 
>-                if (ufshpb_get_state(hpb) != HPB_PRESENT)
>-                        continue;
>-                ufshpb_set_state(hpb, HPB_RESET);
>-                ufshpb_cancel_jobs(hpb);
>-                ufshpb_discard_rsp_lists(hpb);
>+                if (dest == HPB_RESET) {
>+                        ufshpb_cancel_jobs(hpb);
>+                        ufshpb_discard_rsp_lists(hpb);
>+                }
>         }
> }
> 
>diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
>index b475dbd78988..7aa0dc58ee9f 100644
>--- a/drivers/scsi/ufs/ufshpb.h
>+++ b/drivers/scsi/ufs/ufshpb.h
>@@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) { return 0;
> static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) {}
> static void ufshpb_resume(struct ufs_hba *hba) {}
> static void ufshpb_suspend(struct ufs_hba *hba) {}
>-static void ufshpb_reset(struct ufs_hba *hba) {}
>-static void ufshpb_reset_host(struct ufs_hba *hba) {}
>+static void ufshpb_toggle_state(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest) {}
> static void ufshpb_init(struct ufs_hba *hba) {}
> static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev) {}
> static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev) {}
>@@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
> void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
> void ufshpb_resume(struct ufs_hba *hba);
> void ufshpb_suspend(struct ufs_hba *hba);
>-void ufshpb_reset(struct ufs_hba *hba);
>-void ufshpb_reset_host(struct ufs_hba *hba);
>+void ufshpb_toggle_state(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest);
> void ufshpb_init(struct ufs_hba *hba);
> void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev);
> void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);
>-- 
>2.34.1
> 
> 

RE: [PATCH v3 6/6] scsi: ufshpb: Cleanup ufshpb_suspend/resume

[Keoseong Park]

>From: Bean Huo <beanhuo@micron.com>
> 
>ufshpb_resume() is only called when the HPB state is HPB_SUSPEND, so
>the check statement for "ufshpb_get_state(hpb) != HPB_PRESENT" is useless.
> 
>Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park

>---
> drivers/scsi/ufs/ufshpb.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
> 
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index 7868412054bf..10f40e390dc5 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -2349,11 +2349,9 @@ void ufshpb_suspend(struct ufs_hba *hba)
> 
>         shost_for_each_device(sdev, hba->host) {
>                 hpb = ufshpb_get_hpb_data(sdev);
>-                if (!hpb)
>+                if (!hpb || ufshpb_get_state(hpb) != HPB_PRESENT)
>                         continue;
> 
>-                if (ufshpb_get_state(hpb) != HPB_PRESENT)
>-                        continue;
>                 ufshpb_set_state(hpb, HPB_SUSPEND);
>                 ufshpb_cancel_jobs(hpb);
>         }
>@@ -2366,20 +2364,15 @@ void ufshpb_resume(struct ufs_hba *hba)
> 
>         shost_for_each_device(sdev, hba->host) {
>                 hpb = ufshpb_get_hpb_data(sdev);
>-                if (!hpb)
>+                if (!hpb || ufshpb_get_state(hpb) != HPB_SUSPEND)
>                         continue;
> 
>-                if ((ufshpb_get_state(hpb) != HPB_PRESENT) &&
>-                    (ufshpb_get_state(hpb) != HPB_SUSPEND))
>-                        continue;
>                 ufshpb_set_state(hpb, HPB_PRESENT);
>                 ufshpb_kick_map_work(hpb);
>                 if (hpb->is_hcm) {
>-                        unsigned int poll =
>-                                hpb->params.timeout_polling_interval_ms;
>+                        unsigned int poll = hpb->params.timeout_polling_interval_ms;
> 
>-                        schedule_delayed_work(&hpb->ufshpb_read_to_work,
>-                                msecs_to_jiffies(poll));
>+                        schedule_delayed_work(&hpb->ufshpb_read_to_work, msecs_to_jiffies(poll));
>                 }
>         }
> }
>-- 
>2.34.1
> 
> 

RE: [PATCH v3 3/6] scsi: ufshpb: Cleanup the handler when device reset HPB information

[Keoseong Park]

>From: Bean Huo <beanhuo@micron.com>
> 
>"When the device is powered off by the host, the device may restore L2P map data
>upon power up or build from the host’s HPB READ command. In case device powered
>up and lost HPB information, device can signal to the host through HPB Sense data,
>by setting HPB Operation as ‘2’ which will inform the host that device reset HPB
>information."
> 
>This patch is to clean up the handler and make the intent of this handler more
>readable, no functional change.
> 
>Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park

>---
> drivers/scsi/ufs/ufshpb.c | 35 +++++++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 12 deletions(-)
> 
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index 167643969165..f1f30d4c3d65 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -1225,7 +1225,10 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
>                 queue_work(ufshpb_wq, &hpb->map_work);
> }
> 
>-static void ufshpb_dev_reset_handler(struct ufshpb_lu *hpb)
>+/*
>+ * Set the flags of all active regions to RGN_FLAG_UPDATE to let host side reload L2P entries later
>+ */
>+static void ufshpb_set_regions_update(struct ufshpb_lu *hpb)
> {
>         struct victim_select_info *lru_info = &hpb->lru_info;
>         struct ufshpb_region *rgn;
>@@ -1239,6 +1242,24 @@ static void ufshpb_dev_reset_handler(struct ufshpb_lu *hpb)
>         spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> }
> 
>+static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
>+{
>+        struct scsi_device *sdev;
>+        struct ufshpb_lu *hpb;
>+
>+        __shost_for_each_device(sdev, hba->host) {
>+                hpb = ufshpb_get_hpb_data(sdev);
>+                if (hpb && hpb->is_hcm)
>+                        /*
>+                         * For the HPB host mode, in case device powered up and lost HPB
>+                         * information, we will set the region flag to be RGN_FLAG_UPDATE,
>+                         * it will let host reload its L2P entries(re-activate the region
>+                         * in the UFS device).
>+                         */
>+                        ufshpb_set_regions_update(hpb);
>+        }
>+}
>+
> /*
>  * This function will parse recommended active subregion information in sense
>  * data field of response UPIU with SAM_STAT_GOOD state.
>@@ -1313,17 +1334,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>         case HPB_RSP_DEV_RESET:
>                 dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
>                          "UFS device lost HPB information during PM.\n");
>-
>-                if (hpb->is_hcm) {
>-                        struct scsi_device *sdev;
>-
>-                        __shost_for_each_device(sdev, hba->host) {
>-                                struct ufshpb_lu *h = sdev->hostdata;
>-
>-                                if (h)
>-                                        ufshpb_dev_reset_handler(h);
>-                        }
>-                }
>+                ufshpb_dev_reset_handler(hba);
> 
>                 break;
>         default:
>-- 
>2.34.1
> 
>

RE: [PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with rcmd_*

[Keoseong Park]

>From: Bean Huo <beanhuo@micron.com>
> 
>According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
>and rb_inactive_cnt, they are all related to HPB recommendation in UPIU response
>packet. I don't know what 'rb' refers to, I think 'rcmd'
>(recommendation) should be the correct abbreviation.
> 
>Change the sysfs documentation about these sysfs nodes to highlight what they mean
>under different HPB control modes.
I think it's better than what I suggested.

> 
>Signed-off-by: Bean Huo <beanhuo@micron.com>
>---
> Documentation/ABI/testing/sysfs-driver-ufs | 18 +++++++++-------
> drivers/scsi/ufs/ufshpb.c                  | 24 +++++++++++-----------
> drivers/scsi/ufs/ufshpb.h                  |  6 +++---
> 3 files changed, 26 insertions(+), 22 deletions(-)
> 
>diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
>index a44ef8bfbadf..6b248abb1bd7 100644
>--- a/Documentation/ABI/testing/sysfs-driver-ufs
>+++ b/Documentation/ABI/testing/sysfs-driver-ufs
>@@ -1518,7 +1518,7 @@ Description:        This entry shows the number of reads that cannot be changed to
> 
>                 The file is read only.
> 
>-What:                /sys/class/scsi_device/*/device/hpb_stats/rb_noti_cnt
>+What:                /sys/class/scsi_device/*/device/hpb_stats/rcmd_noti_cnt
> Date:                June 2021
> Contact:        Daejun Park <daejun7.park@samsung.com>
> Description:        This entry shows the number of response UPIUs that has
>@@ -1526,19 +1526,23 @@ Description:        This entry shows the number of response UPIUs that has
> 
>                 The file is read only.
> 
>-What:                /sys/class/scsi_device/*/device/hpb_stats/rb_active_cnt
>+What:                /sys/class/scsi_device/*/device/hpb_stats/rcmd_active_cnt
> Date:                June 2021
> Contact:        Daejun Park <daejun7.park@samsung.com>
>-Description:        This entry shows the number of active sub-regions recommended by
>-                response UPIUs.
>+Description:        For the HPB device control mode, this entry shows the number of
>+        active sub-regions recommended by response UPIUs. For the HPB host control
>+        mode, this entry shows the number of active sub-regions recommended by the
>+        HPB host control mode heuristic algorithm.
Please check indentation.

> 
>                 The file is read only.
> 
>-What:                /sys/class/scsi_device/*/device/hpb_stats/rb_inactive_cnt
>+What:                /sys/class/scsi_device/*/device/hpb_stats/rcmd_inactive_cnt
> Date:                June 2021
> Contact:        Daejun Park <daejun7.park@samsung.com>
>-Description:        This entry shows the number of inactive regions recommended by
>-                response UPIUs.
>+Description:        For the HPB device control mode, this entry shows the number of
>+        inactive regions recommended by response UPIUs. For the HPB host control
>+        mode, this entry shows the number of inactive regions recommended by the
>+        HPB host control mode heuristic algorithm.
Please check indentation.

> 
>                 The file is read only.
> 
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index f1f30d4c3d65..e7f311bb4401 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -563,7 +563,7 @@ static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
>         if (list_empty(&srgn->list_act_srgn))
>                 list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
> 
>-        hpb->stats.rb_active_cnt++;
>+        hpb->stats.rcmd_active_cnt++;
> }
> 
> static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
>@@ -580,7 +580,7 @@ static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
>         if (list_empty(&rgn->list_inact_rgn))
>                 list_add_tail(&rgn->list_inact_rgn, &hpb->lh_inact_rgn);
> 
>-        hpb->stats.rb_inactive_cnt++;
>+        hpb->stats.rcmd_inactive_cnt++;
> }
> 
> static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
>@@ -1321,7 +1321,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>         if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
>                 return;
> 
>-        hpb->stats.rb_noti_cnt++;
>+        hpb->stats.rcmd_noti_cnt++;
> 
>         switch (rsp_field->hpb_op) {
>         case HPB_RSP_REQ_REGION_UPDATE:
>@@ -1724,18 +1724,18 @@ static DEVICE_ATTR_RO(__name)
> 
> ufshpb_sysfs_attr_show_func(hit_cnt);
> ufshpb_sysfs_attr_show_func(miss_cnt);
>-ufshpb_sysfs_attr_show_func(rb_noti_cnt);
>-ufshpb_sysfs_attr_show_func(rb_active_cnt);
>-ufshpb_sysfs_attr_show_func(rb_inactive_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_noti_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_active_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_inactive_cnt);
> ufshpb_sysfs_attr_show_func(map_req_cnt);
> ufshpb_sysfs_attr_show_func(umap_req_cnt);
> 
> static struct attribute *hpb_dev_stat_attrs[] = {
>         &dev_attr_hit_cnt.attr,
>         &dev_attr_miss_cnt.attr,
>-        &dev_attr_rb_noti_cnt.attr,
>-        &dev_attr_rb_active_cnt.attr,
>-        &dev_attr_rb_inactive_cnt.attr,
>+        &dev_attr_rcmd_noti_cnt.attr,
>+        &dev_attr_rcmd_active_cnt.attr,
>+        &dev_attr_rcmd_inactive_cnt.attr,
>         &dev_attr_map_req_cnt.attr,
>         &dev_attr_umap_req_cnt.attr,
>         NULL,
>@@ -2098,9 +2098,9 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
> {
>         hpb->stats.hit_cnt = 0;
>         hpb->stats.miss_cnt = 0;
>-        hpb->stats.rb_noti_cnt = 0;
>-        hpb->stats.rb_active_cnt = 0;
>-        hpb->stats.rb_inactive_cnt = 0;
>+        hpb->stats.rcmd_noti_cnt = 0;
>+        hpb->stats.rcmd_active_cnt = 0;
>+        hpb->stats.rcmd_inactive_cnt = 0;
>         hpb->https://protect2.fireeye.com/v1/url?k=9b807207-fa0b6728-9b81f948-74fe485cbfe7-c8e650b167f03dee&q=1&e=ec9d9a5f-605d-45e5-b145-e3a4c9a96f3e&u=http%3A%2F%2Fstats.map%2F_req_cnt = 0;
It's not related to the patch, but the code seems to be wrong.

>         hpb->stats.umap_req_cnt = 0;
> }
>diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
>index b83b9ec9044a..0d6e6004d783 100644
>--- a/drivers/scsi/ufs/ufshpb.h
>+++ b/drivers/scsi/ufs/ufshpb.h
>@@ -211,9 +211,9 @@ struct ufshpb_params {
> struct ufshpb_stats {
>         u64 hit_cnt;
>         u64 miss_cnt;
>-        u64 rb_noti_cnt;
>-        u64 rb_active_cnt;
>-        u64 rb_inactive_cnt;
>+        u64 rcmd_noti_cnt;
>+        u64 rcmd_active_cnt;
>+        u64 rcmd_inactive_cnt;
>         u64 map_req_cnt;
>         u64 pre_req_cnt;
>         u64 umap_req_cnt;
>-- 
>2.34.1
> 
> 
Apart from the indentation, the patch looks good to me, so once
you've fixed them, feel free to add my:

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park

RE: [PATCH v3 5/6] scsi: ufshpb: Add handing of device reset HPB regions Infos in HPB device mode

[Keoseong Park]

>From: Bean Huo <beanhuo@micron.com>
> 
>In UFS HPB Spec JESD220-3A,
> 
>"5.8. Active and inactive information upon power cycle
>...
>When the device is powered off by the host, the device may restore L2P map data
>upon power up or build from the host’s HPB READ command. In case device powered
>up and lost HPB information, device can signal to the host through HPB Sense data,
>by setting HPB Operation as ‘2’ which will inform the host that device reset HPB
>information."
> 
>Therefore, for HPB device control mode, if the UFS device is reset via the RST_N
>pin, the active region information in the device will be reset. If the host side
>receives this notification from the device side, it is recommended to inactivate
>all active regions in the host's HPB cache.
> 
>Signed-off-by: Bean Huo <beanhuo@micron.com>
>---
> drivers/scsi/ufs/ufshpb.c | 82 +++++++++++++++++++++++++++------------
> 1 file changed, 58 insertions(+), 24 deletions(-)
> 
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index e7f311bb4401..7868412054bf 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -1137,6 +1137,39 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
>         spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
>         return ret;
> }
>+/**
>+ *ufshpb_submit_region_inactive() - submit a region to be inactivated later
>+ *@hpb: per-LU HPB instance
>+ *@region_index: the index associated with the region that will be inactivated later
>+ */
>+static void ufshpb_submit_region_inactive(struct ufshpb_lu *hpb, int region_index)
>+{
>+        int subregion_index;
>+        struct ufshpb_region *rgn;
>+        struct ufshpb_subregion *srgn;
>+
>+        /*
>+         * Remove this region from active region list and add it to inactive list
>+         */
>+        spin_lock(&hpb->rsp_list_lock);
>+        ufshpb_update_inactive_info(hpb, region_index);
>+        spin_unlock(&hpb->rsp_list_lock);
>+
>+        rgn = hpb->rgn_tbl + region_index;
>+
>+        /*
>+         * Set subregion state to be HPB_SRGN_INVALID, there will no HPB read on this subregion
>+         */
>+        spin_lock(&hpb->rgn_state_lock);
>+        if (rgn->rgn_state != HPB_RGN_INACTIVE) {
>+                for (subregion_index = 0; subregion_index < rgn->srgn_cnt; subregion_index++) {
>+                        srgn = rgn->srgn_tbl + subregion_index;
>+                        if (srgn->srgn_state == HPB_SRGN_VALID)
>+                                srgn->srgn_state = HPB_SRGN_INVALID;
>+                }
>+        }
>+        spin_unlock(&hpb->rgn_state_lock);
>+}
> 
> static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
>                                          struct utp_hpb_rsp *rsp_field)
>@@ -1196,25 +1229,8 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
> 
>         for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
>                 rgn_i = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
>-                dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
>-                        "inactivate(%d) region %d\n", i, rgn_i);
>-
>-                spin_lock(&hpb->rsp_list_lock);
>-                ufshpb_update_inactive_info(hpb, rgn_i);
>-                spin_unlock(&hpb->rsp_list_lock);
>-
>-                rgn = hpb->rgn_tbl + rgn_i;
>-
>-                spin_lock(&hpb->rgn_state_lock);
>-                if (rgn->rgn_state != HPB_RGN_INACTIVE) {
>-                        for (srgn_i = 0; srgn_i < rgn->srgn_cnt; srgn_i++) {
>-                                srgn = rgn->srgn_tbl + srgn_i;
>-                                if (srgn->srgn_state == HPB_SRGN_VALID)
>-                                        srgn->srgn_state = HPB_SRGN_INVALID;
>-                        }
>-                }
>-                spin_unlock(&hpb->rgn_state_lock);
>-
>+                dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "inactivate(%d) region %d\n", i, rgn_i);
>+                ufshpb_submit_region_inactive(hpb, rgn_i);
>         }
> 
> out:
>@@ -1249,14 +1265,32 @@ static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
> 
>         __shost_for_each_device(sdev, hba->host) {
>                 hpb = ufshpb_get_hpb_data(sdev);
>-                if (hpb && hpb->is_hcm)
>+                if (!hpb)
>+                        continue;
>+
>+                if (hpb->is_hcm) {
>                         /*
>-                         * For the HPB host mode, in case device powered up and lost HPB
>-                         * information, we will set the region flag to be RGN_FLAG_UPDATE,
>-                         * it will let host reload its L2P entries(re-activate the region
>-                         * in the UFS device).
>+                         * For the HPB host control mode, in case device powered up and lost HPB
>+                         * information, we will set the region flag to be RGN_FLAG_UPDATE, it will
>+                         * let host reload its L2P entries(reactivate region in the UFS device).
>                          */
>                         ufshpb_set_regions_update(hpb);
>+                } else {
>+                        /*
>+                         * For the HPB device control mode, if host side receives 02h:HPB Operation
>+                         * in UPIU response, which means device recommends the host side should
>+                         * inactivate all active regions. Here we add all active regions to inactive
>+                         * list, they will be inactivated later in ufshpb_map_work_handler().
>+                         */
>+                        struct victim_select_info *lru_info = &hpb->lru_info;
>+                        struct ufshpb_region *rgn;
>+
>+                        list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
>+                                ufshpb_submit_region_inactive(hpb, rgn->rgn_idx);
>+
>+                        if (ufshpb_get_state(hpb) == HPB_PRESENT)
>+                                queue_work(ufshpb_wq, &hpb->map_work);
>+                }
>         }
> }
> 
>-- 
>2.34.1
> 
> 
Looks good to me.

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park

RE: [PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with rcmd_*

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
> and rb_inactive_cnt, they are all related to HPB recommendation in UPIU
> response packet. I don't know what 'rb' refers to, I think 'rcmd'
> (recommendation) should be the correct abbreviation.
rb stands for READ-BUFFER

> 
> Change the sysfs documentation about these sysfs nodes to highlight what
> they mean under different HPB control modes.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
I don't think it is allowed to change the kernel's ABI.

Thanks,
Avri

RE: [PATCH v3 5/6] scsi: ufshpb: Add handing of device reset HPB regions Infos in HPB device mode

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> In UFS HPB Spec JESD220-3A,
> 
> "5.8. Active and inactive information upon power cycle
> ...
> When the device is powered off by the host, the device may restore L2P map
> data
> upon power up or build from the host’s HPB READ command. In case device
> powered
> up and lost HPB information, device can signal to the host through HPB Sense
> data,
> by setting HPB Operation as ‘2’ which will inform the host that device reset
> HPB
> information."
> 
> Therefore, for HPB device control mode, if the UFS device is reset via the
> RST_N
> pin, the active region information in the device will be reset. If the host side
> receives this notification from the device side, it is recommended to inactivate
> all active regions in the host's HPB cache.
While it makes sense to me, this interpretation of the spec takes extreme action,
Compared to what it is today, and you probably need to get an ack from Daejun. 

If we are entering a zone in which each vendor is applying a different logic - 
I think you need to facilitate that - maybe in a different patch set.

Thanks,
Avri

Re: [PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with rcmd_*

[Bean Huo]

On Mon, 2022-04-25 at 06:19 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > According to the documentation of the sysfs nodes rb_noti_cnt,
> > rb_active_cnt
> > and rb_inactive_cnt, they are all related to HPB recommendation in
> > UPIU
> > response packet. I don't know what 'rb' refers to, I think 'rcmd'
> > (recommendation) should be the correct abbreviation.
> rb stands for READ-BUFFER
> 
Thanks. right. probably it is HPB READ BUFFER.  I think based on the
documentation,rcmd much makes sense, there is region inactive
recommendation, it is no need to issue HPB read buffer.

> > 
> > Change the sysfs documentation about these sysfs nodes to highlight
> > what
> > they mean under different HPB control modes.
> > 
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> I don't think it is allowed to change the kernel's ABI.
> 

I don't know where this limitation is stated, I see a lot of ABI change
patches on the mailing list. Also, I didn't change its original
meaning, I added a new node meaning to make it clearer. According to
the documentation on these nodes, they should be used in HPB host
control mode, right? Otherwise, we should add a new note for HPB host
control mode. Considering we already have these nodes, we don't need to
change the HPB host control mode code just by changing the ABI to have
the HPB host control mode reuse them. Here are my thoughts on change.
Please consider my changes again.


Kind regards,
Bean


> Thanks,
> Avri

Re: [PATCH v3 5/6] scsi: ufshpb: Add handing of device reset HPB regions Infos in HPB device mode

[Bean Huo]

On Mon, 2022-04-25 at 06:43 +0000, Avri Altman wrote:
> > 
> > Therefore, for HPB device control mode, if the UFS device is reset
> > via the
> > RST_N
> > pin, the active region information in the device will be reset. If
> > the host side
> > receives this notification from the device side, it is recommended
> > to inactivate
> > all active regions in the host's HPB cache.
> While it makes sense to me, this interpretation of the spec takes
> extreme action,
> Compared to what it is today, and you probably need to get an ack
> from Daejun. 
> 

Yes, but it seems he is quiet. Keoseong Park is on the email list and
he has looked at the patch. If you have concerns, I can ping Keoseong
Park to sync with Daejun.

> If we are entering a zone in which each vendor is applying a
> different logic - 
> I think you need to facilitate that - maybe in a different patch set.
> 

Yes, it should be a different patch. I must say that this patch is
based on the HPB Spec, say, if the device loses HPB info. The host side
should also reset its HPB information, Otherwise, performance will
degrade. Of course, it depends on the specific implementation of the
vendor. If they don't report 02:HPB operations, this patch still
doesn't change the original behavior.


Kind regards,
Bean

Re: [PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with rcmd_*

[Bean Huo]

On Mon, 2022-04-25 at 12:43 +0900, Keoseong Park wrote:
> > 2.34.1
> > 
> > 
> Apart from the indentation, the patch looks good to me, so once
> you've fixed them, feel free to add my:
> 
> Reviewed-by: Keoseong Park <keosung.park@samsung.com>

thanks for your review, I will change them based on your comments in
the next version.

Kind regards,
Bean