[PATCH v2 0/5] Several changes for UFSHPB

[PATCH v2 0/5] 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.

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 (5):
  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: Add handing of device reset HPB regions Infos in HPB
    device mode
  scsi: ufshpb: Cleanup ufshpb_suspend/resume

 drivers/scsi/ufs/ufshcd.c |   4 +-
 drivers/scsi/ufs/ufshpb.c | 157 ++++++++++++++++++++++----------------
 drivers/scsi/ufs/ufshpb.h |  10 +--
 3 files changed, 98 insertions(+), 73 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] 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()

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 0899d5b8cdad..de0bc53e3ac6 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(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(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 3ca745ad616c..9df032e82ec3 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2278,38 +2278,28 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba)
 	return flag_res;
 }
 
-void ufshpb_reset(struct ufs_hba *hba)
+/**
+ * ufshpb_toggle - 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(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..2825ec69a6a6 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(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(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 v2 2/5] 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.

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 2825ec69a6a6..b7e2817d4e76 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 v2 3/5] 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 9df032e82ec3..4538164fc493 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1231,7 +1231,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;
@@ -1245,6 +1248,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.
@@ -1319,17 +1340,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 v2 4/5] 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 | 73 ++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 4538164fc493..4b15fa862beb 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1143,6 +1143,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)
@@ -1202,25 +1235,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:
@@ -1255,7 +1271,10 @@ 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,
@@ -1263,6 +1282,20 @@ static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
 			 * in the UFS device).
 			 */
 			ufshpb_set_regions_update(hpb);
+		} else {
+			/*
+			 * For the HPB device mode, 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 v2 5/5] 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 4b15fa862beb..0e3bfd241f87 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2354,11 +2354,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);
 	}
@@ -2371,20 +2369,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 v2 1/5] 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()
> 
>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 0899d5b8cdad..de0bc53e3ac6 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(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(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 3ca745ad616c..9df032e82ec3 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -2278,38 +2278,28 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba)
>         return flag_res;
> }
> 
>-void ufshpb_reset(struct ufs_hba *hba)
>+/**
>+ * ufshpb_toggle - 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(struct ufs_hba *hba, enum UFSHPB_STATE src, enum UFSHPB_STATE dest)
How about changing it to something like ufshpb_state_toggle()?

Best Regards,
Keoseong Park

> {
>         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..2825ec69a6a6 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(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(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 v2 1/5] scsi: ufshpb: Merge ufshpb_reset() and ufshpb_reset_host()

[Bean Huo]

On Wed, 2022-04-20 at 11:47 +0900, Keoseong Park wrote:
> > +void ufshpb_toggle(struct ufs_hba *hba, enum UFSHPB_STATE src,
> > enum UFSHPB_STATE dest)
> How about changing it to something like ufshpb_state_toggle()?
> 
> Best Regards,
> Keoseong Park

ufshpb_state_toggle() is much better, I will change it in the next
version. Thanks.

Kind regards,
Bean

Re: [PATCH v2 1/5] scsi: ufshpb: Merge ufshpb_reset() and ufshpb_reset_host()

[Bart Van Assche]

On 4/20/22 05:48, Bean Huo wrote:
> On Wed, 2022-04-20 at 11:47 +0900, Keoseong Park wrote:
>>> +void ufshpb_toggle(struct ufs_hba *hba, enum UFSHPB_STATE src,
>>> enum UFSHPB_STATE dest)
>> How about changing it to something like ufshpb_state_toggle()?
>>
>> Best Regards,
>> Keoseong Park
> 
> ufshpb_state_toggle() is much better, I will change it in the next
> version. Thanks.

How about ufshbp_toggle_state() such that the word order is correct?

Thanks,

Bart.

Re: [PATCH v2 1/5] scsi: ufshpb: Merge ufshpb_reset() and ufshpb_reset_host()

[Bean Huo]

On Wed, 2022-04-20 at 20:48 -0700, Bart Van Assche wrote:
> > > How about changing it to something like ufshpb_state_toggle()?
> > > 
> > > Best Regards,
> > > Keoseong Park
> > 
> > ufshpb_state_toggle() is much better, I will change it in the next
> > version. Thanks.
> 
> How about ufshbp_toggle_state() such that the word order is correct?
> 
> Thanks,
> 
> Bart.
Sound good, I will change to this one. thanks.

Bean

RE: [PATCH v2 2/5] scsi: ufshpb: Remove 0 assignment for enum value

[Keoseong Park]

>From: Bean Huo <beanhuo@micron.com>
> 
>If the first enumerator has no initializer, the value of the corresponding
>constant is zero.
> 
>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 2825ec69a6a6..b7e2817d4e76 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
> 

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

Best Regards,
Keoseong Park

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

[Keoseong Park]

Hi Bean,

>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 | 73 ++++++++++++++++++++++++++++-----------
> 1 file changed, 53 insertions(+), 20 deletions(-)
> 
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index 4538164fc493..4b15fa862beb 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -1143,6 +1143,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);
How about separating the "hpb->stats.rb_inactive_cnt++" code from ufshpb_update_inactive_info()?
Because I think this code should only be used in ufshpb_rsp_req_region_update().

>+        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)
>@@ -1202,25 +1235,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:
>@@ -1255,7 +1271,10 @@ 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
For the HPB host control mode, ...

>                          * information, we will set the region flag to be RGN_FLAG_UPDATE,
>@@ -1263,6 +1282,20 @@ static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
>                          * in the UFS device).
>                          */
>                         ufshpb_set_regions_update(hpb);
>+                } else {
>+                        /*
>+                         * For the HPB device mode, we add all active regions to inactive list,
For the HPB device control mode, ...

>+                         * 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
> 
> 

Best Regards,
Keoseong Park

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

[Bean Huo]

On Wed, 2022-04-20 at 14:31 +0900, Keoseong Park wrote:
> > +/**
> > + *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);
> How about separating the "hpb->stats.rb_inactive_cnt++" code from
> ufshpb_update_inactive_info()?
> Because I think this code should only be used in
> ufshpb_rsp_req_region_update().


based on Documentation/ABI/testing/sysfs-driver-ufs:

"What:       /sys/class/scsi_device/*/device/hpb_stats/rb_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."


This parameter should be increased only when recieving inactive
recommended.

I will change it in the next version, thanks.


Kind regards,
Bean

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

[Bean Huo]

> On Wed, 2022-04-20 at 14:31 +0900, Keoseong Park wrote:
> > > 
> > > +        /*
> > > +         * 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);
> > How about separating the "hpb->stats.rb_inactive_cnt++" code from
> > ufshpb_update_inactive_info()?
> > Because I think this code should only be used in
> > ufshpb_rsp_req_region_update().

Hi Keoseong Park,

I didn't take this hpb->stats.rb_inactive_cnt++ out, since I think if
the host receives HPB operation:02h, which means the device recommends
the host to de-activate all active regions on the host side. we need to
add these de-activations to this parameter because the device has
inactivated all active regions. Otherwise, we will find an inconsistent
inactivation and activation counter. Please have a look a review the
v3. 

Kind regards,
Bean