[PATCH v7 00/11] Add Host control mode to HPB

[PATCH v7 00/11] Add Host control mode to HPB

[Avri Altman]

v6 -> v7:
 - attend CanG's comments
 - add one more patch to transform set_dirty to iterate_rgn
 - rebase on Daejun's v32

v5 -> v6:
 - attend CanG's comments
 - rebase on Daejun's v29

v4 -> v5:
 - attend Daejun's comments
 - Control the number of inflight map requests

v3 -> v4:
 - rebase on Daejun's v25

v2 -> v3:
 - Attend Greg's and Can's comments
 - rebase on Daejun's v21

v1 -> v2:
 - attend Greg's and Daejun's comments
 - add patch 9 making host mode parameters configurable
 - rebase on Daejun's v19


The HPB spec defines 2 control modes - device control mode and host
control mode. In oppose to device control mode, in which the host obey
to whatever recommendation received from the device - In host control
mode, the host uses its own algorithms to decide which regions should
be activated or inactivated.

We kept the host managed heuristic simple and concise.

Aside from adding a by-spec functionality, host control mode entails
some further potential benefits: makes the hpb logic transparent and
readable, while allow tuning / scaling its various parameters, and
utilize system-wide info to optimize HPB potential.

This series is based on Samsung's V32 device-control HPB2.0 driver

This version was tested on Galaxy S20, and Xiaomi Mi10 pro.
Your meticulous review and testing is mostly welcome and appreciated.

Thanks,
Avri


Avri Altman (11):
  scsi: ufshpb: Cache HPB Control mode on init
  scsi: ufshpb: Add host control mode support to rsp_upiu
  scsi: ufshpb: Transform set_dirty to iterate_rgn
  scsi: ufshpb: Add reads counter
  scsi: ufshpb: Make eviction depends on region's reads
  scsi: ufshpb: Region inactivation in host mode
  scsi: ufshpb: Add hpb dev reset response
  scsi: ufshpb: Add "Cold" regions timer
  scsi: ufshpb: Limit the number of inflight map requests
  scsi: ufshpb: Add support for host control mode
  scsi: ufshpb: Make host mode parameters configurable

 Documentation/ABI/testing/sysfs-driver-ufs |  84 ++-
 drivers/scsi/ufs/ufshcd.h                  |   2 +
 drivers/scsi/ufs/ufshpb.c                  | 568 ++++++++++++++++++++-
 drivers/scsi/ufs/ufshpb.h                  |  44 ++
 4 files changed, 663 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v7 01/11] scsi: ufshpb: Cache HPB Control mode on init

[Avri Altman]

We will use it later, when we'll need to differentiate between device
and host control modes.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshcd.h | 2 ++
 drivers/scsi/ufs/ufshpb.c | 8 +++++---
 drivers/scsi/ufs/ufshpb.h | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 4dbe9bc60e85..c01f75963750 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -656,6 +656,7 @@ struct ufs_hba_variant_params {
  * @hpb_disabled: flag to check if HPB is disabled
  * @max_hpb_single_cmd: maximum size of single HPB command
  * @is_legacy: flag to check HPB 1.0
+ * @control_mode: either host or device
  */
 struct ufshpb_dev_info {
 	int num_lu;
@@ -665,6 +666,7 @@ struct ufshpb_dev_info {
 	bool hpb_disabled;
 	int max_hpb_single_cmd;
 	bool is_legacy;
+	u8 control_mode;
 };
 #endif
 
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 86805af9abe7..5285a50b05dd 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1615,6 +1615,9 @@ static void ufshpb_lu_parameter_init(struct ufs_hba *hba,
 				 % (hpb->srgn_mem_size / HPB_ENTRY_SIZE);
 
 	hpb->pages_per_srgn = DIV_ROUND_UP(hpb->srgn_mem_size, PAGE_SIZE);
+
+	if (hpb_dev_info->control_mode == HPB_HOST_CONTROL)
+		hpb->is_hcm = true;
 }
 
 static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
@@ -2308,11 +2311,10 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
 {
 	struct ufshpb_dev_info *hpb_dev_info = &hba->ufshpb_dev;
 	int version, ret;
-	u8 hpb_mode;
 	u32 max_hpb_single_cmd = HPB_MULTI_CHUNK_LOW;
 
-	hpb_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
-	if (hpb_mode == HPB_HOST_CONTROL) {
+	hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
+	if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
 		dev_err(hba->dev, "%s: host control mode is not supported.\n",
 			__func__);
 		hpb_dev_info->hpb_disabled = true;
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index b1128b0ce486..7df30340386a 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -228,6 +228,8 @@ struct ufshpb_lu {
 	u32 entries_per_srgn_shift;
 	u32 pages_per_srgn;
 
+	bool is_hcm;
+
 	struct ufshpb_stats stats;
 	struct ufshpb_params params;
 
-- 
2.25.1

[PATCH v7 02/11] scsi: ufshpb: Add host control mode support to rsp_upiu

[Avri Altman]

In device control mode, the device may recommend the host to either
activate or inactivate a region, and the host should follow. Meaning
those are not actually recommendations, but more of instructions.

On the contrary, in host control mode, the recommendation protocol is
slightly changed:
a) The device may only recommend the host to update a subregion of an
   already-active region. And,
b) The device may *not* recommend to inactivate a region.

Furthermore, in host control mode, the host may choose not to follow any
of the device's recommendations. However, in case of a recommendation to
update an active and clean subregion, it is better to follow those
recommendation because otherwise the host has no other way to know that
some internal relocation took place.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 34 +++++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshpb.h |  2 ++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 5285a50b05dd..6111019ca31a 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -166,6 +166,8 @@ static void ufshpb_set_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
 	else
 		set_bit_len = cnt;
 
+	set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+
 	if (rgn->rgn_state != HPB_RGN_INACTIVE &&
 	    srgn->srgn_state == HPB_SRGN_VALID)
 		bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
@@ -235,6 +237,11 @@ static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
 	return false;
 }
 
+static inline bool is_rgn_dirty(struct ufshpb_region *rgn)
+{
+	return test_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+}
+
 static int ufshpb_fill_ppn_from_page(struct ufshpb_lu *hpb,
 				     struct ufshpb_map_ctx *mctx, int pos,
 				     int len, u64 *ppn_buf)
@@ -712,6 +719,7 @@ static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
 static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
 				     struct ufshpb_subregion *srgn)
 {
+	struct ufshpb_region *rgn;
 	u32 num_entries = hpb->entries_per_srgn;
 
 	if (!srgn->mctx) {
@@ -725,6 +733,10 @@ static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
 		num_entries = hpb->last_srgn_entries;
 
 	bitmap_zero(srgn->mctx->ppn_dirty, num_entries);
+
+	rgn = hpb->rgn_tbl + srgn->rgn_idx;
+	clear_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+
 	return 0;
 }
 
@@ -1244,6 +1256,18 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		srgn_i =
 			be16_to_cpu(rsp_field->hpb_active_field[i].active_srgn);
 
+		rgn = hpb->rgn_tbl + rgn_i;
+		if (hpb->is_hcm &&
+		    (rgn->rgn_state != HPB_RGN_ACTIVE || is_rgn_dirty(rgn))) {
+			/*
+			 * in host control mode, subregion activation
+			 * recommendations are only allowed to active regions.
+			 * Also, ignore recommendations for dirty regions - the
+			 * host will make decisions concerning those by himself
+			 */
+			continue;
+		}
+
 		dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
 			"activate(%d) region %d - %d\n", i, rgn_i, srgn_i);
 
@@ -1251,7 +1275,6 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		ufshpb_update_active_info(hpb, rgn_i, srgn_i);
 		spin_unlock(&hpb->rsp_list_lock);
 
-		rgn = hpb->rgn_tbl + rgn_i;
 		srgn = rgn->srgn_tbl + srgn_i;
 
 		/* blocking HPB_READ */
@@ -1262,6 +1285,14 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		hpb->stats.rb_active_cnt++;
 	}
 
+	if (hpb->is_hcm) {
+		/*
+		 * in host control mode the device is not allowed to inactivate
+		 * regions
+		 */
+		goto out;
+	}
+
 	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,
@@ -1286,6 +1317,7 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		hpb->stats.rb_inactive_cnt++;
 	}
 
+out:
 	dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "Noti: #ACT %u #INACT %u\n",
 		rsp_field->active_rgn_cnt, rsp_field->inactive_rgn_cnt);
 
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 7df30340386a..032672114881 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -121,6 +121,8 @@ struct ufshpb_region {
 
 	/* below information is used by lru */
 	struct list_head list_lru_rgn;
+	unsigned long rgn_flags;
+#define RGN_FLAG_DIRTY 0
 };
 
 #define for_each_sub_region(rgn, i, srgn)				\
-- 
2.25.1

[PATCH v7 03/11] scsi: ufshpb: Transform set_dirty to iterate_rgn

[Avri Altman]

Given a transfer length, set_dirty meticulously runs over all the
entries, across subregions and regions if needed. Currently its only use
is to mark dirty blocks, but soon HCM may profit from it as well, when
managing its read counters.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 6111019ca31a..252fcfb48862 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -144,13 +144,14 @@ static bool ufshpb_is_hpb_rsp_valid(struct ufs_hba *hba,
 	return true;
 }
 
-static void ufshpb_set_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
-				 int srgn_idx, int srgn_offset, int cnt)
+static void ufshpb_iterate_rgn(struct ufshpb_lu *hpb, int rgn_idx, int srgn_idx,
+			       int srgn_offset, int cnt, bool set_dirty)
 {
 	struct ufshpb_region *rgn;
 	struct ufshpb_subregion *srgn;
 	int set_bit_len;
 	int bitmap_len;
+	unsigned long flags;
 
 next_srgn:
 	rgn = hpb->rgn_tbl + rgn_idx;
@@ -166,11 +167,14 @@ static void ufshpb_set_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
 	else
 		set_bit_len = cnt;
 
-	set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+	if (set_dirty)
+		set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
 
-	if (rgn->rgn_state != HPB_RGN_INACTIVE &&
+	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+	if (set_dirty && rgn->rgn_state != HPB_RGN_INACTIVE &&
 	    srgn->srgn_state == HPB_SRGN_VALID)
 		bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
+	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
 
 	srgn_offset = 0;
 	if (++srgn_idx == hpb->srgns_per_rgn) {
@@ -592,10 +596,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 
 	/* If command type is WRITE or DISCARD, set bitmap as drity */
 	if (ufshpb_is_write_or_discard_cmd(cmd)) {
-		spin_lock_irqsave(&hpb->rgn_state_lock, flags);
-		ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
-				 transfer_len);
-		spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+		ufshpb_iterate_rgn(hpb, rgn_idx, srgn_idx, srgn_offset,
+				   transfer_len, true);
 		return 0;
 	}
 
-- 
2.25.1

[PATCH v7 04/11] scsi: ufshpb: Add reads counter

[Avri Altman]

In host control mode, reads are the major source of activation trials.
Keep track of those reads counters, for both active as well inactive
regions.

We reset the read counter upon write - we are only interested in "clean"
reads.

Keep those counters normalized, as we are using those reads as a
comparative score, to make various decisions.
If during consecutive normalizations an active region has exhaust its
reads - inactivate it.

while at it, protect the {active,inactive}_count stats by adding them
into the applicable handler.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 94 ++++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufshpb.h |  9 ++++
 2 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 252fcfb48862..3ab66421dc00 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -16,6 +16,8 @@
 #include "ufshpb.h"
 #include "../sd.h"
 
+#define ACTIVATION_THRESHOLD 8 /* 8 IOs */
+
 /* memory management */
 static struct kmem_cache *ufshpb_mctx_cache;
 static mempool_t *ufshpb_mctx_pool;
@@ -26,6 +28,9 @@ static int tot_active_srgn_pages;
 
 static struct workqueue_struct *ufshpb_wq;
 
+static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
+				      int srgn_idx);
+
 bool ufshpb_is_allowed(struct ufs_hba *hba)
 {
 	return !(hba->ufshpb_dev.hpb_disabled);
@@ -148,7 +153,7 @@ static void ufshpb_iterate_rgn(struct ufshpb_lu *hpb, int rgn_idx, int srgn_idx,
 			       int srgn_offset, int cnt, bool set_dirty)
 {
 	struct ufshpb_region *rgn;
-	struct ufshpb_subregion *srgn;
+	struct ufshpb_subregion *srgn, *prev_srgn = NULL;
 	int set_bit_len;
 	int bitmap_len;
 	unsigned long flags;
@@ -167,15 +172,39 @@ static void ufshpb_iterate_rgn(struct ufshpb_lu *hpb, int rgn_idx, int srgn_idx,
 	else
 		set_bit_len = cnt;
 
-	if (set_dirty)
-		set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
-
 	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
 	if (set_dirty && rgn->rgn_state != HPB_RGN_INACTIVE &&
 	    srgn->srgn_state == HPB_SRGN_VALID)
 		bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
 
+	if (hpb->is_hcm && prev_srgn != srgn) {
+		bool activate = false;
+
+		spin_lock(&rgn->rgn_lock);
+		if (set_dirty) {
+			rgn->reads -= srgn->reads;
+			srgn->reads = 0;
+			set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+		} else {
+			srgn->reads++;
+			rgn->reads++;
+			if (srgn->reads == ACTIVATION_THRESHOLD)
+				activate = true;
+		}
+		spin_unlock(&rgn->rgn_lock);
+
+		if (activate) {
+			spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+			ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+			spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+			dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
+				"activate region %d-%d\n", rgn_idx, srgn_idx);
+		}
+
+		prev_srgn = srgn;
+	}
+
 	srgn_offset = 0;
 	if (++srgn_idx == hpb->srgns_per_rgn) {
 		srgn_idx = 0;
@@ -604,6 +633,19 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	if (!ufshpb_is_support_chunk(hpb, transfer_len))
 		return 0;
 
+	if (hpb->is_hcm) {
+		/*
+		 * in host control mode, reads are the main source for
+		 * activation trials.
+		 */
+		ufshpb_iterate_rgn(hpb, rgn_idx, srgn_idx, srgn_offset,
+				   transfer_len, false);
+
+		/* keep those counters normalized */
+		if (rgn->reads > hpb->entries_per_srgn)
+			schedule_work(&hpb->ufshpb_normalization_work);
+	}
+
 	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
 	if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
 				   transfer_len)) {
@@ -755,6 +797,8 @@ 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++;
 }
 
 static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
@@ -770,6 +814,8 @@ 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++;
 }
 
 static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
@@ -1090,6 +1136,7 @@ static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			 rgn->rgn_idx);
 		goto out;
 	}
+
 	if (!list_empty(&rgn->list_lru_rgn)) {
 		if (ufshpb_check_srgns_issue_state(hpb, rgn)) {
 			ret = -EBUSY;
@@ -1284,7 +1331,6 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		if (srgn->srgn_state == HPB_SRGN_VALID)
 			srgn->srgn_state = HPB_SRGN_INVALID;
 		spin_unlock(&hpb->rgn_state_lock);
-		hpb->stats.rb_active_cnt++;
 	}
 
 	if (hpb->is_hcm) {
@@ -1316,7 +1362,6 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		}
 		spin_unlock(&hpb->rgn_state_lock);
 
-		hpb->stats.rb_inactive_cnt++;
 	}
 
 out:
@@ -1515,6 +1560,36 @@ static void ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb)
 	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
 }
 
+static void ufshpb_normalization_work_handler(struct work_struct *work)
+{
+	struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu,
+					     ufshpb_normalization_work);
+	int rgn_idx;
+
+	for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
+		struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
+		int srgn_idx;
+
+		spin_lock(&rgn->rgn_lock);
+		rgn->reads = 0;
+		for (srgn_idx = 0; srgn_idx < hpb->srgns_per_rgn; srgn_idx++) {
+			struct ufshpb_subregion *srgn = rgn->srgn_tbl + srgn_idx;
+
+			srgn->reads >>= 1;
+			rgn->reads += srgn->reads;
+		}
+		spin_unlock(&rgn->rgn_lock);
+
+		if (rgn->rgn_state != HPB_RGN_ACTIVE || rgn->reads)
+			continue;
+
+		/* if region is active but has no reads - inactivate it */
+		spin_lock(&hpb->rsp_list_lock);
+		ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
+		spin_unlock(&hpb->rsp_list_lock);
+	}
+}
+
 static void ufshpb_map_work_handler(struct work_struct *work)
 {
 	struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu, map_work);
@@ -1674,6 +1749,8 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 		rgn = rgn_table + rgn_idx;
 		rgn->rgn_idx = rgn_idx;
 
+		spin_lock_init(&rgn->rgn_lock);
+
 		INIT_LIST_HEAD(&rgn->list_inact_rgn);
 		INIT_LIST_HEAD(&rgn->list_lru_rgn);
 
@@ -1915,6 +1992,9 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 	INIT_LIST_HEAD(&hpb->list_hpb_lu);
 
 	INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
+	if (hpb->is_hcm)
+		INIT_WORK(&hpb->ufshpb_normalization_work,
+			  ufshpb_normalization_work_handler);
 
 	hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
 			  sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -2014,6 +2094,8 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
 
 static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
 {
+	if (hpb->is_hcm)
+		cancel_work_sync(&hpb->ufshpb_normalization_work);
 	cancel_work_sync(&hpb->map_work);
 }
 
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 032672114881..87495e59fcf1 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -106,6 +106,10 @@ struct ufshpb_subregion {
 	int rgn_idx;
 	int srgn_idx;
 	bool is_last;
+
+	/* subregion reads - for host mode */
+	unsigned int reads;
+
 	/* below information is used by rsp_list */
 	struct list_head list_act_srgn;
 };
@@ -123,6 +127,10 @@ struct ufshpb_region {
 	struct list_head list_lru_rgn;
 	unsigned long rgn_flags;
 #define RGN_FLAG_DIRTY 0
+
+	/* region reads - for host mode */
+	spinlock_t rgn_lock;
+	unsigned int reads;
 };
 
 #define for_each_sub_region(rgn, i, srgn)				\
@@ -212,6 +220,7 @@ struct ufshpb_lu {
 
 	/* for selecting victim */
 	struct victim_select_info lru_info;
+	struct work_struct ufshpb_normalization_work;
 
 	/* pinned region information */
 	u32 lu_pinned_start;
-- 
2.25.1

[PATCH v7 05/11] scsi: ufshpb: Make eviction depends on region's reads

[Avri Altman]

In host mode, eviction is considered an extreme measure.
verify that the entering region has enough reads, and the exiting
region has much less reads.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 3ab66421dc00..aefb6dc160ee 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,6 +17,7 @@
 #include "../sd.h"
 
 #define ACTIVATION_THRESHOLD 8 /* 8 IOs */
+#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 5) /* 256 IOs */
 
 /* memory management */
 static struct kmem_cache *ufshpb_mctx_cache;
@@ -1057,6 +1058,13 @@ static struct ufshpb_region *ufshpb_victim_lru_info(struct ufshpb_lu *hpb)
 		if (ufshpb_check_srgns_issue_state(hpb, rgn))
 			continue;
 
+		/*
+		 * in host control mode, verify that the exiting region
+		 * has less reads
+		 */
+		if (hpb->is_hcm && rgn->reads > (EVICTION_THRESHOLD >> 1))
+			continue;
+
 		victim_rgn = rgn;
 		break;
 	}
@@ -1229,7 +1237,7 @@ static int ufshpb_issue_map_req(struct ufshpb_lu *hpb,
 
 static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 {
-	struct ufshpb_region *victim_rgn;
+	struct ufshpb_region *victim_rgn = NULL;
 	struct victim_select_info *lru_info = &hpb->lru_info;
 	unsigned long flags;
 	int ret = 0;
@@ -1256,7 +1264,15 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			 * It is okay to evict the least recently used region,
 			 * because the device could detect this region
 			 * by not issuing HPB_READ
+			 *
+			 * in host control mode, verify that the entering
+			 * region has enough reads
 			 */
+			if (hpb->is_hcm && rgn->reads < EVICTION_THRESHOLD) {
+				ret = -EACCES;
+				goto out;
+			}
+
 			victim_rgn = ufshpb_victim_lru_info(hpb);
 			if (!victim_rgn) {
 				dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
-- 
2.25.1

[PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Avri Altman]

In host mode, the host is expected to send HPB-WRITE-BUFFER with
buffer-id = 0x1 when it inactivates a region.

Use the map-requests pool as there is no point in assigning a
designated cache for umap-requests.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 35 +++++++++++++++++++++++++++++++----
 drivers/scsi/ufs/ufshpb.h |  1 +
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index aefb6dc160ee..fcc954f51bcf 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
 
 	blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn);
 
+	hpb->stats.umap_req_cnt++;
 	return 0;
 }
 
@@ -1110,18 +1111,37 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb,
 	return -EAGAIN;
 }
 
+static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb,
+					struct ufshpb_region *rgn)
+{
+	return ufshpb_issue_umap_req(hpb, rgn);
+}
+
 static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
 {
 	return ufshpb_issue_umap_req(hpb, NULL);
 }
 
-static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
-				  struct ufshpb_region *rgn)
+static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
+				 struct ufshpb_region *rgn)
 {
 	struct victim_select_info *lru_info;
 	struct ufshpb_subregion *srgn;
 	int srgn_idx;
 
+	lockdep_assert_held(&hpb->rgn_state_lock);
+
+	if (hpb->is_hcm) {
+		unsigned long flags;
+		int ret;
+
+		spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+		ret = ufshpb_issue_umap_single_req(hpb, rgn);
+		spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+		if (ret)
+			return ret;
+	}
+
 	lru_info = &hpb->lru_info;
 
 	dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx);
@@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
 
 	for_each_sub_region(rgn, srgn_idx, srgn)
 		ufshpb_purge_active_subregion(hpb, srgn);
+
+	return 0;
 }
 
 static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
@@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			goto out;
 		}
 
-		__ufshpb_evict_region(hpb, rgn);
+		ret = __ufshpb_evict_region(hpb, rgn);
 	}
 out:
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
@@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 				"LRU full (%d), choose victim %d\n",
 				atomic_read(&lru_info->active_cnt),
 				victim_rgn->rgn_idx);
-			__ufshpb_evict_region(hpb, victim_rgn);
+			ret = __ufshpb_evict_region(hpb, victim_rgn);
+			if (ret)
+				goto out;
 		}
 
 		/*
@@ -1856,6 +1880,7 @@ 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(map_req_cnt);
+ufshpb_sysfs_attr_show_func(umap_req_cnt);
 
 static struct attribute *hpb_dev_stat_attrs[] = {
 	&dev_attr_hit_cnt.attr,
@@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
 	&dev_attr_rb_active_cnt.attr,
 	&dev_attr_rb_inactive_cnt.attr,
 	&dev_attr_map_req_cnt.attr,
+	&dev_attr_umap_req_cnt.attr,
 	NULL,
 };
 
@@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
 	hpb->stats.rb_active_cnt = 0;
 	hpb->stats.rb_inactive_cnt = 0;
 	hpb->stats.map_req_cnt = 0;
+	hpb->stats.umap_req_cnt = 0;
 }
 
 static void ufshpb_param_init(struct ufshpb_lu *hpb)
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 87495e59fcf1..1ea58c17a4de 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -191,6 +191,7 @@ struct ufshpb_stats {
 	u64 rb_inactive_cnt;
 	u64 map_req_cnt;
 	u64 pre_req_cnt;
+	u64 umap_req_cnt;
 };
 
 struct ufshpb_lu {
-- 
2.25.1

Re: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Can Guo]

On 2021-03-31 15:39, Avri Altman wrote:
> In host mode, the host is expected to send HPB-WRITE-BUFFER with
> buffer-id = 0x1 when it inactivates a region.
> 
> Use the map-requests pool as there is no point in assigning a
> designated cache for umap-requests.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/scsi/ufs/ufshpb.c | 35 +++++++++++++++++++++++++++++++----
>  drivers/scsi/ufs/ufshpb.h |  1 +
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index aefb6dc160ee..fcc954f51bcf 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu 
> *hpb,
> 
>  	blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn);
> 
> +	hpb->stats.umap_req_cnt++;
>  	return 0;
>  }
> 
> @@ -1110,18 +1111,37 @@ static int ufshpb_issue_umap_req(struct 
> ufshpb_lu *hpb,
>  	return -EAGAIN;
>  }
> 
> +static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb,
> +					struct ufshpb_region *rgn)
> +{
> +	return ufshpb_issue_umap_req(hpb, rgn);
> +}
> +
>  static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
>  {
>  	return ufshpb_issue_umap_req(hpb, NULL);
>  }
> 
> -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
> -				  struct ufshpb_region *rgn)
> +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
> +				 struct ufshpb_region *rgn)
>  {
>  	struct victim_select_info *lru_info;
>  	struct ufshpb_subregion *srgn;
>  	int srgn_idx;
> 
> +	lockdep_assert_held(&hpb->rgn_state_lock);
> +
> +	if (hpb->is_hcm) {
> +		unsigned long flags;
> +		int ret;
> +
> +		spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);

Never seen a usage like this... Here flags is used without being 
intialized.
The flag is needed when spin_unlock_irqrestore -> 
local_irq_restore(flags) to
restore the DAIF register (in terms of ARM).

Thanks,

Can Guo.

> +		ret = ufshpb_issue_umap_single_req(hpb, rgn);
> +		spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	lru_info = &hpb->lru_info;
> 
>  	dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", 
> rgn->rgn_idx);
> @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct 
> ufshpb_lu *hpb,
> 
>  	for_each_sub_region(rgn, srgn_idx, srgn)
>  		ufshpb_purge_active_subregion(hpb, srgn);
> +
> +	return 0;
>  }
> 
>  static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct
> ufshpb_region *rgn)
> @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu
> *hpb, struct ufshpb_region *rgn)
>  			goto out;
>  		}
> 
> -		__ufshpb_evict_region(hpb, rgn);
> +		ret = __ufshpb_evict_region(hpb, rgn);
>  	}
>  out:
>  	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu
> *hpb, struct ufshpb_region *rgn)
>  				"LRU full (%d), choose victim %d\n",
>  				atomic_read(&lru_info->active_cnt),
>  				victim_rgn->rgn_idx);
> -			__ufshpb_evict_region(hpb, victim_rgn);
> +			ret = __ufshpb_evict_region(hpb, victim_rgn);
> +			if (ret)
> +				goto out;
>  		}
> 
>  		/*
> @@ -1856,6 +1880,7 @@ 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(map_req_cnt);
> +ufshpb_sysfs_attr_show_func(umap_req_cnt);
> 
>  static struct attribute *hpb_dev_stat_attrs[] = {
>  	&dev_attr_hit_cnt.attr,
> @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
>  	&dev_attr_rb_active_cnt.attr,
>  	&dev_attr_rb_inactive_cnt.attr,
>  	&dev_attr_map_req_cnt.attr,
> +	&dev_attr_umap_req_cnt.attr,
>  	NULL,
>  };
> 
> @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu 
> *hpb)
>  	hpb->stats.rb_active_cnt = 0;
>  	hpb->stats.rb_inactive_cnt = 0;
>  	hpb->stats.map_req_cnt = 0;
> +	hpb->stats.umap_req_cnt = 0;
>  }
> 
>  static void ufshpb_param_init(struct ufshpb_lu *hpb)
> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> index 87495e59fcf1..1ea58c17a4de 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -191,6 +191,7 @@ struct ufshpb_stats {
>  	u64 rb_inactive_cnt;
>  	u64 map_req_cnt;
>  	u64 pre_req_cnt;
> +	u64 umap_req_cnt;
>  };
> 
>  struct ufshpb_lu {

RE: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Avri Altman]

> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
> > -                               struct ufshpb_region *rgn)
> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
> > +                              struct ufshpb_region *rgn)
> >  {
> >       struct victim_select_info *lru_info;
> >       struct ufshpb_subregion *srgn;
> >       int srgn_idx;
> >
> > +     lockdep_assert_held(&hpb->rgn_state_lock);
> > +
> > +     if (hpb->is_hcm) {
> > +             unsigned long flags;
> > +             int ret;
> > +
> > +             spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> 
> Never seen a usage like this... Here flags is used without being
> intialized.
> The flag is needed when spin_unlock_irqrestore ->
> local_irq_restore(flags) to
> restore the DAIF register (in terms of ARM).
OK.

Thanks,
Avri

> 
> Thanks,
> 
> Can Guo.
> 
> > +             ret = ufshpb_issue_umap_single_req(hpb, rgn);
> > +             spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       lru_info = &hpb->lru_info;
> >
> >       dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n",
> > rgn->rgn_idx);
> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct
> > ufshpb_lu *hpb,
> >
> >       for_each_sub_region(rgn, srgn_idx, srgn)
> >               ufshpb_purge_active_subregion(hpb, srgn);
> > +
> > +     return 0;
> >  }
> >
> >  static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct
> > ufshpb_region *rgn)
> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu
> > *hpb, struct ufshpb_region *rgn)
> >                       goto out;
> >               }
> >
> > -             __ufshpb_evict_region(hpb, rgn);
> > +             ret = __ufshpb_evict_region(hpb, rgn);
> >       }
> >  out:
> >       spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu
> > *hpb, struct ufshpb_region *rgn)
> >                               "LRU full (%d), choose victim %d\n",
> >                               atomic_read(&lru_info->active_cnt),
> >                               victim_rgn->rgn_idx);
> > -                     __ufshpb_evict_region(hpb, victim_rgn);
> > +                     ret = __ufshpb_evict_region(hpb, victim_rgn);
> > +                     if (ret)
> > +                             goto out;
> >               }
> >
> >               /*
> > @@ -1856,6 +1880,7 @@ 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(map_req_cnt);
> > +ufshpb_sysfs_attr_show_func(umap_req_cnt);
> >
> >  static struct attribute *hpb_dev_stat_attrs[] = {
> >       &dev_attr_hit_cnt.attr,
> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
> >       &dev_attr_rb_active_cnt.attr,
> >       &dev_attr_rb_inactive_cnt.attr,
> >       &dev_attr_map_req_cnt.attr,
> > +     &dev_attr_umap_req_cnt.attr,
> >       NULL,
> >  };
> >
> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu
> > *hpb)
> >       hpb->stats.rb_active_cnt = 0;
> >       hpb->stats.rb_inactive_cnt = 0;
> >       hpb->stats.map_req_cnt = 0;
> > +     hpb->stats.umap_req_cnt = 0;
> >  }
> >
> >  static void ufshpb_param_init(struct ufshpb_lu *hpb)
> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > index 87495e59fcf1..1ea58c17a4de 100644
> > --- a/drivers/scsi/ufs/ufshpb.h
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -191,6 +191,7 @@ struct ufshpb_stats {
> >       u64 rb_inactive_cnt;
> >       u64 map_req_cnt;
> >       u64 pre_req_cnt;
> > +     u64 umap_req_cnt;
> >  };
> >
> >  struct ufshpb_lu {

Re: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Can Guo]

On 2021-04-06 13:20, Avri Altman wrote:
>> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
>> > -                               struct ufshpb_region *rgn)
>> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
>> > +                              struct ufshpb_region *rgn)
>> >  {
>> >       struct victim_select_info *lru_info;
>> >       struct ufshpb_subregion *srgn;
>> >       int srgn_idx;
>> >
>> > +     lockdep_assert_held(&hpb->rgn_state_lock);
>> > +
>> > +     if (hpb->is_hcm) {
>> > +             unsigned long flags;
>> > +             int ret;
>> > +
>> > +             spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
>> 
>> Never seen a usage like this... Here flags is used without being
>> intialized.
>> The flag is needed when spin_unlock_irqrestore ->
>> local_irq_restore(flags) to
>> restore the DAIF register (in terms of ARM).
> OK.

Hi Avri,

Checked on my setup, this lead to compilation error. Will you fix it in 
next version?

warning: variable 'flags' is uninitialized when used here 
[-Wuninitialized]

Thanks,
Can Guo.

> 
> Thanks,
> Avri
> 
>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>> > +             ret = ufshpb_issue_umap_single_req(hpb, rgn);
>> > +             spin_lock_irqsave(&hpb->rgn_state_lock, flags);
>> > +             if (ret)
>> > +                     return ret;
>> > +     }
>> > +
>> >       lru_info = &hpb->lru_info;
>> >
>> >       dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n",
>> > rgn->rgn_idx);
>> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct
>> > ufshpb_lu *hpb,
>> >
>> >       for_each_sub_region(rgn, srgn_idx, srgn)
>> >               ufshpb_purge_active_subregion(hpb, srgn);
>> > +
>> > +     return 0;
>> >  }
>> >
>> >  static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct
>> > ufshpb_region *rgn)
>> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu
>> > *hpb, struct ufshpb_region *rgn)
>> >                       goto out;
>> >               }
>> >
>> > -             __ufshpb_evict_region(hpb, rgn);
>> > +             ret = __ufshpb_evict_region(hpb, rgn);
>> >       }
>> >  out:
>> >       spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
>> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu
>> > *hpb, struct ufshpb_region *rgn)
>> >                               "LRU full (%d), choose victim %d\n",
>> >                               atomic_read(&lru_info->active_cnt),
>> >                               victim_rgn->rgn_idx);
>> > -                     __ufshpb_evict_region(hpb, victim_rgn);
>> > +                     ret = __ufshpb_evict_region(hpb, victim_rgn);
>> > +                     if (ret)
>> > +                             goto out;
>> >               }
>> >
>> >               /*
>> > @@ -1856,6 +1880,7 @@ 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(map_req_cnt);
>> > +ufshpb_sysfs_attr_show_func(umap_req_cnt);
>> >
>> >  static struct attribute *hpb_dev_stat_attrs[] = {
>> >       &dev_attr_hit_cnt.attr,
>> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
>> >       &dev_attr_rb_active_cnt.attr,
>> >       &dev_attr_rb_inactive_cnt.attr,
>> >       &dev_attr_map_req_cnt.attr,
>> > +     &dev_attr_umap_req_cnt.attr,
>> >       NULL,
>> >  };
>> >
>> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu
>> > *hpb)
>> >       hpb->stats.rb_active_cnt = 0;
>> >       hpb->stats.rb_inactive_cnt = 0;
>> >       hpb->stats.map_req_cnt = 0;
>> > +     hpb->stats.umap_req_cnt = 0;
>> >  }
>> >
>> >  static void ufshpb_param_init(struct ufshpb_lu *hpb)
>> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
>> > index 87495e59fcf1..1ea58c17a4de 100644
>> > --- a/drivers/scsi/ufs/ufshpb.h
>> > +++ b/drivers/scsi/ufs/ufshpb.h
>> > @@ -191,6 +191,7 @@ struct ufshpb_stats {
>> >       u64 rb_inactive_cnt;
>> >       u64 map_req_cnt;
>> >       u64 pre_req_cnt;
>> > +     u64 umap_req_cnt;
>> >  };
>> >
>> >  struct ufshpb_lu {

RE: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Avri Altman]

 
> 
> On 2021-04-06 13:20, Avri Altman wrote:
> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
> >> > -                               struct ufshpb_region *rgn)
> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
> >> > +                              struct ufshpb_region *rgn)
> >> >  {
> >> >       struct victim_select_info *lru_info;
> >> >       struct ufshpb_subregion *srgn;
> >> >       int srgn_idx;
> >> >
> >> > +     lockdep_assert_held(&hpb->rgn_state_lock);
> >> > +
> >> > +     if (hpb->is_hcm) {
> >> > +             unsigned long flags;
> >> > +             int ret;
> >> > +
> >> > +             spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> >>
> >> Never seen a usage like this... Here flags is used without being
> >> intialized.
> >> The flag is needed when spin_unlock_irqrestore ->
> >> local_irq_restore(flags) to
> >> restore the DAIF register (in terms of ARM).
> > OK.
> 
> Hi Avri,
> 
> Checked on my setup, this lead to compilation error. Will you fix it in
> next version?
> 
> warning: variable 'flags' is uninitialized when used here
> [-Wuninitialized]
Yeah - I will pass it to __ufshpb_evict_region and drop the lockdep_assert call.

I don't want to block your testing - are there any other things you want me to change?

Thanks,
Avri

> 
> Thanks,
> Can Guo.
> 
> >
> > Thanks,
> > Avri
> >
> >>
> >> Thanks,
> >>
> >> Can Guo.
> >>
> >> > +             ret = ufshpb_issue_umap_single_req(hpb, rgn);
> >> > +             spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> >> > +             if (ret)
> >> > +                     return ret;
> >> > +     }
> >> > +
> >> >       lru_info = &hpb->lru_info;
> >> >
> >> >       dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n",
> >> > rgn->rgn_idx);
> >> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct
> >> > ufshpb_lu *hpb,
> >> >
> >> >       for_each_sub_region(rgn, srgn_idx, srgn)
> >> >               ufshpb_purge_active_subregion(hpb, srgn);
> >> > +
> >> > +     return 0;
> >> >  }
> >> >
> >> >  static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct
> >> > ufshpb_region *rgn)
> >> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu
> >> > *hpb, struct ufshpb_region *rgn)
> >> >                       goto out;
> >> >               }
> >> >
> >> > -             __ufshpb_evict_region(hpb, rgn);
> >> > +             ret = __ufshpb_evict_region(hpb, rgn);
> >> >       }
> >> >  out:
> >> >       spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> >> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu
> >> > *hpb, struct ufshpb_region *rgn)
> >> >                               "LRU full (%d), choose victim %d\n",
> >> >                               atomic_read(&lru_info->active_cnt),
> >> >                               victim_rgn->rgn_idx);
> >> > -                     __ufshpb_evict_region(hpb, victim_rgn);
> >> > +                     ret = __ufshpb_evict_region(hpb, victim_rgn);
> >> > +                     if (ret)
> >> > +                             goto out;
> >> >               }
> >> >
> >> >               /*
> >> > @@ -1856,6 +1880,7 @@ 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(map_req_cnt);
> >> > +ufshpb_sysfs_attr_show_func(umap_req_cnt);
> >> >
> >> >  static struct attribute *hpb_dev_stat_attrs[] = {
> >> >       &dev_attr_hit_cnt.attr,
> >> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
> >> >       &dev_attr_rb_active_cnt.attr,
> >> >       &dev_attr_rb_inactive_cnt.attr,
> >> >       &dev_attr_map_req_cnt.attr,
> >> > +     &dev_attr_umap_req_cnt.attr,
> >> >       NULL,
> >> >  };
> >> >
> >> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu
> >> > *hpb)
> >> >       hpb->stats.rb_active_cnt = 0;
> >> >       hpb->stats.rb_inactive_cnt = 0;
> >> >       hpb->stats.map_req_cnt = 0;
> >> > +     hpb->stats.umap_req_cnt = 0;
> >> >  }
> >> >
> >> >  static void ufshpb_param_init(struct ufshpb_lu *hpb)
> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> >> > index 87495e59fcf1..1ea58c17a4de 100644
> >> > --- a/drivers/scsi/ufs/ufshpb.h
> >> > +++ b/drivers/scsi/ufs/ufshpb.h
> >> > @@ -191,6 +191,7 @@ struct ufshpb_stats {
> >> >       u64 rb_inactive_cnt;
> >> >       u64 map_req_cnt;
> >> >       u64 pre_req_cnt;
> >> > +     u64 umap_req_cnt;
> >> >  };
> >> >
> >> >  struct ufshpb_lu {

Re: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Can Guo]

On 2021-04-06 14:16, Avri Altman wrote:
>> 
>> On 2021-04-06 13:20, Avri Altman wrote:
>> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
>> >> > -                               struct ufshpb_region *rgn)
>> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
>> >> > +                              struct ufshpb_region *rgn)
>> >> >  {
>> >> >       struct victim_select_info *lru_info;
>> >> >       struct ufshpb_subregion *srgn;
>> >> >       int srgn_idx;
>> >> >
>> >> > +     lockdep_assert_held(&hpb->rgn_state_lock);
>> >> > +
>> >> > +     if (hpb->is_hcm) {
>> >> > +             unsigned long flags;
>> >> > +             int ret;
>> >> > +
>> >> > +             spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
>> >>
>> >> Never seen a usage like this... Here flags is used without being
>> >> intialized.
>> >> The flag is needed when spin_unlock_irqrestore ->
>> >> local_irq_restore(flags) to
>> >> restore the DAIF register (in terms of ARM).
>> > OK.
>> 
>> Hi Avri,
>> 
>> Checked on my setup, this lead to compilation error. Will you fix it 
>> in
>> next version?
>> 
>> warning: variable 'flags' is uninitialized when used here
>> [-Wuninitialized]
> Yeah - I will pass it to __ufshpb_evict_region and drop the 
> lockdep_assert call.
> 

Please paste the sample code/change here so that I can move forward 
quickly.

> I don't want to block your testing - are there any other things you
> want me to change?

Currently, no. I will try to review and test this series these days and
post comments at once.

Thanks,
Can Guo.

> 
> Thanks,
> Avri
> 
>> 
>> Thanks,
>> Can Guo.
>> 
>> >
>> > Thanks,
>> > Avri
>> >
>> >>
>> >> Thanks,
>> >>
>> >> Can Guo.
>> >>
>> >> > +             ret = ufshpb_issue_umap_single_req(hpb, rgn);
>> >> > +             spin_lock_irqsave(&hpb->rgn_state_lock, flags);
>> >> > +             if (ret)
>> >> > +                     return ret;
>> >> > +     }
>> >> > +
>> >> >       lru_info = &hpb->lru_info;
>> >> >
>> >> >       dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n",
>> >> > rgn->rgn_idx);
>> >> > @@ -1130,6 +1150,8 @@ static void __ufshpb_evict_region(struct
>> >> > ufshpb_lu *hpb,
>> >> >
>> >> >       for_each_sub_region(rgn, srgn_idx, srgn)
>> >> >               ufshpb_purge_active_subregion(hpb, srgn);
>> >> > +
>> >> > +     return 0;
>> >> >  }
>> >> >
>> >> >  static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct
>> >> > ufshpb_region *rgn)
>> >> > @@ -1151,7 +1173,7 @@ static int ufshpb_evict_region(struct ufshpb_lu
>> >> > *hpb, struct ufshpb_region *rgn)
>> >> >                       goto out;
>> >> >               }
>> >> >
>> >> > -             __ufshpb_evict_region(hpb, rgn);
>> >> > +             ret = __ufshpb_evict_region(hpb, rgn);
>> >> >       }
>> >> >  out:
>> >> >       spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
>> >> > @@ -1285,7 +1307,9 @@ static int ufshpb_add_region(struct ufshpb_lu
>> >> > *hpb, struct ufshpb_region *rgn)
>> >> >                               "LRU full (%d), choose victim %d\n",
>> >> >                               atomic_read(&lru_info->active_cnt),
>> >> >                               victim_rgn->rgn_idx);
>> >> > -                     __ufshpb_evict_region(hpb, victim_rgn);
>> >> > +                     ret = __ufshpb_evict_region(hpb, victim_rgn);
>> >> > +                     if (ret)
>> >> > +                             goto out;
>> >> >               }
>> >> >
>> >> >               /*
>> >> > @@ -1856,6 +1880,7 @@ 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(map_req_cnt);
>> >> > +ufshpb_sysfs_attr_show_func(umap_req_cnt);
>> >> >
>> >> >  static struct attribute *hpb_dev_stat_attrs[] = {
>> >> >       &dev_attr_hit_cnt.attr,
>> >> > @@ -1864,6 +1889,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
>> >> >       &dev_attr_rb_active_cnt.attr,
>> >> >       &dev_attr_rb_inactive_cnt.attr,
>> >> >       &dev_attr_map_req_cnt.attr,
>> >> > +     &dev_attr_umap_req_cnt.attr,
>> >> >       NULL,
>> >> >  };
>> >> >
>> >> > @@ -1988,6 +2014,7 @@ static void ufshpb_stat_init(struct ufshpb_lu
>> >> > *hpb)
>> >> >       hpb->stats.rb_active_cnt = 0;
>> >> >       hpb->stats.rb_inactive_cnt = 0;
>> >> >       hpb->stats.map_req_cnt = 0;
>> >> > +     hpb->stats.umap_req_cnt = 0;
>> >> >  }
>> >> >
>> >> >  static void ufshpb_param_init(struct ufshpb_lu *hpb)
>> >> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
>> >> > index 87495e59fcf1..1ea58c17a4de 100644
>> >> > --- a/drivers/scsi/ufs/ufshpb.h
>> >> > +++ b/drivers/scsi/ufs/ufshpb.h
>> >> > @@ -191,6 +191,7 @@ struct ufshpb_stats {
>> >> >       u64 rb_inactive_cnt;
>> >> >       u64 map_req_cnt;
>> >> >       u64 pre_req_cnt;
>> >> > +     u64 umap_req_cnt;
>> >> >  };
>> >> >
>> >> >  struct ufshpb_lu {

RE: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Avri Altman]

[-- Attachment #1: Type: text/plain, Size: 6291 bytes --]

> >>
> >> On 2021-04-06 13:20, Avri Altman wrote:
> >> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
> >> >> > -                               struct ufshpb_region *rgn)
> >> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
> >> >> > +                              struct ufshpb_region *rgn)
> >> >> >  {
> >> >> >       struct victim_select_info *lru_info;
> >> >> >       struct ufshpb_subregion *srgn;
> >> >> >       int srgn_idx;
> >> >> >
> >> >> > +     lockdep_assert_held(&hpb->rgn_state_lock);
> >> >> > +
> >> >> > +     if (hpb->is_hcm) {
> >> >> > +             unsigned long flags;
> >> >> > +             int ret;
> >> >> > +
> >> >> > +             spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> >> >>
> >> >> Never seen a usage like this... Here flags is used without being
> >> >> intialized.
> >> >> The flag is needed when spin_unlock_irqrestore ->
> >> >> local_irq_restore(flags) to
> >> >> restore the DAIF register (in terms of ARM).
> >> > OK.
> >>
> >> Hi Avri,
> >>
> >> Checked on my setup, this lead to compilation error. Will you fix it
> >> in
> >> next version?
> >>
> >> warning: variable 'flags' is uninitialized when used here
> >> [-Wuninitialized]
> > Yeah - I will pass it to __ufshpb_evict_region and drop the
> > lockdep_assert call.
> >
> 
> Please paste the sample code/change here so that I can move forward
> quickly.
Thanks a lot.  Also attaching the patch if its more convenient.

Thanks,
Avri

$ git show 5d33d36e8704
commit 5d33d36e87047d27a546ad3529cb7837186b47b2
Author: Avri Altman <avri.altman@wdc.com>
Date:   Tue Jun 30 15:14:31 2020 +0300

    scsi: ufshpb: Region inactivation in host mode
    
    In host mode, the host is expected to send HPB-WRITE-BUFFER with
    buffer-id = 0x1 when it inactivates a region.
    
    Use the map-requests pool as there is no point in assigning a
    designated cache for umap-requests.
    
    Signed-off-by: Avri Altman <avri.altman@wdc.com>
    Reviewed-by: Daejun Park <daejun7.park@samsung.com>
    Change-Id: I1a6696b38d4abfb4d9fbe44e84016a6238825125

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index aefb6dc160ee..54a3ea9f5732 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
 
        blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn);
 
+       hpb->stats.umap_req_cnt++;
        return 0;
 }
 
@@ -1110,18 +1111,35 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb,
        return -EAGAIN;
 }
 
+static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb,
+                                       struct ufshpb_region *rgn)
+{
+       return ufshpb_issue_umap_req(hpb, rgn);
+}
+
 static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
 {
        return ufshpb_issue_umap_req(hpb, NULL);
 }
 
-static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
-                                 struct ufshpb_region *rgn)
+static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
+                                struct ufshpb_region *rgn,
+                                unsigned long flags)
 {
        struct victim_select_info *lru_info;
        struct ufshpb_subregion *srgn;
        int srgn_idx;
 
+       if (hpb->is_hcm) {
+               int ret;
+
+               spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+               ret = ufshpb_issue_umap_single_req(hpb, rgn);
+               spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+               if (ret)
+                       return ret;
+       }
+
        lru_info = &hpb->lru_info;
 
        dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx);
@@ -1130,6 +1148,8 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
 
        for_each_sub_region(rgn, srgn_idx, srgn)
                ufshpb_purge_active_subregion(hpb, srgn);
+
+       return 0;
 }
 
 static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
@@ -1151,7 +1171,7 @@ static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
                        goto out;
                }
 
-               __ufshpb_evict_region(hpb, rgn);
+               ret = __ufshpb_evict_region(hpb, rgn, flags);
        }
 out:
        spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
@@ -1285,7 +1305,9 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
                                "LRU full (%d), choose victim %d\n",
                                atomic_read(&lru_info->active_cnt),
                                victim_rgn->rgn_idx);
-                       __ufshpb_evict_region(hpb, victim_rgn);
+                       ret = __ufshpb_evict_region(hpb, victim_rgn, flags);
+                       if (ret)
+                               goto out;
                }
 
                /*
@@ -1856,6 +1878,7 @@ 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(map_req_cnt);
+ufshpb_sysfs_attr_show_func(umap_req_cnt);
 
 static struct attribute *hpb_dev_stat_attrs[] = {
        &dev_attr_hit_cnt.attr,
@@ -1864,6 +1887,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
        &dev_attr_rb_active_cnt.attr,
        &dev_attr_rb_inactive_cnt.attr,
        &dev_attr_map_req_cnt.attr,
+       &dev_attr_umap_req_cnt.attr,
        NULL,
 };
 
@@ -1988,6 +2012,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
        hpb->stats.rb_active_cnt = 0;
        hpb->stats.rb_inactive_cnt = 0;
        hpb->stats.map_req_cnt = 0;
+       hpb->stats.umap_req_cnt = 0;
 }
 
 static void ufshpb_param_init(struct ufshpb_lu *hpb)
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 87495e59fcf1..1ea58c17a4de 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -191,6 +191,7 @@ struct ufshpb_stats {
        u64 rb_inactive_cnt;
        u64 map_req_cnt;
        u64 pre_req_cnt;
+       u64 umap_req_cnt;
 };
 
 struct ufshpb_lu {

[-- Attachment #2: v8-0006-scsi-ufshpb-Region-inactivation-in-host-mode.patch --]
[-- Type: application/octet-stream, Size: 4074 bytes --]

From 5d33d36e87047d27a546ad3529cb7837186b47b2 Mon Sep 17 00:00:00 2001
From: Avri Altman <avri.altman@wdc.com>
Date: Tue, 30 Jun 2020 15:14:31 +0300
Subject: [PATCH v8 06/11] scsi: ufshpb: Region inactivation in host mode

In host mode, the host is expected to send HPB-WRITE-BUFFER with
buffer-id = 0x1 when it inactivates a region.

Use the map-requests pool as there is no point in assigning a
designated cache for umap-requests.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/scsi/ufs/ufshpb.c | 33 +++++++++++++++++++++++++++++----
 drivers/scsi/ufs/ufshpb.h |  1 +
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index aefb6dc160ee..54a3ea9f5732 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -914,6 +914,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
 
 	blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn);
 
+	hpb->stats.umap_req_cnt++;
 	return 0;
 }
 
@@ -1110,18 +1111,35 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb,
 	return -EAGAIN;
 }
 
+static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb,
+					struct ufshpb_region *rgn)
+{
+	return ufshpb_issue_umap_req(hpb, rgn);
+}
+
 static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
 {
 	return ufshpb_issue_umap_req(hpb, NULL);
 }
 
-static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
-				  struct ufshpb_region *rgn)
+static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
+				 struct ufshpb_region *rgn,
+				 unsigned long flags)
 {
 	struct victim_select_info *lru_info;
 	struct ufshpb_subregion *srgn;
 	int srgn_idx;
 
+	if (hpb->is_hcm) {
+		int ret;
+
+		spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+		ret = ufshpb_issue_umap_single_req(hpb, rgn);
+		spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+		if (ret)
+			return ret;
+	}
+
 	lru_info = &hpb->lru_info;
 
 	dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx);
@@ -1130,6 +1148,8 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
 
 	for_each_sub_region(rgn, srgn_idx, srgn)
 		ufshpb_purge_active_subregion(hpb, srgn);
+
+	return 0;
 }
 
 static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
@@ -1151,7 +1171,7 @@ static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			goto out;
 		}
 
-		__ufshpb_evict_region(hpb, rgn);
+		ret = __ufshpb_evict_region(hpb, rgn, flags);
 	}
 out:
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
@@ -1285,7 +1305,9 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 				"LRU full (%d), choose victim %d\n",
 				atomic_read(&lru_info->active_cnt),
 				victim_rgn->rgn_idx);
-			__ufshpb_evict_region(hpb, victim_rgn);
+			ret = __ufshpb_evict_region(hpb, victim_rgn, flags);
+			if (ret)
+				goto out;
 		}
 
 		/*
@@ -1856,6 +1878,7 @@ 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(map_req_cnt);
+ufshpb_sysfs_attr_show_func(umap_req_cnt);
 
 static struct attribute *hpb_dev_stat_attrs[] = {
 	&dev_attr_hit_cnt.attr,
@@ -1864,6 +1887,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
 	&dev_attr_rb_active_cnt.attr,
 	&dev_attr_rb_inactive_cnt.attr,
 	&dev_attr_map_req_cnt.attr,
+	&dev_attr_umap_req_cnt.attr,
 	NULL,
 };
 
@@ -1988,6 +2012,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
 	hpb->stats.rb_active_cnt = 0;
 	hpb->stats.rb_inactive_cnt = 0;
 	hpb->stats.map_req_cnt = 0;
+	hpb->stats.umap_req_cnt = 0;
 }
 
 static void ufshpb_param_init(struct ufshpb_lu *hpb)
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 87495e59fcf1..1ea58c17a4de 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -191,6 +191,7 @@ struct ufshpb_stats {
 	u64 rb_inactive_cnt;
 	u64 map_req_cnt;
 	u64 pre_req_cnt;
+	u64 umap_req_cnt;
 };
 
 struct ufshpb_lu {
-- 
2.25.1

RE: [PATCH v7 06/11] scsi: ufshpb: Region inactivation in host mode

[Avri Altman]

> > >> On 2021-04-06 13:20, Avri Altman wrote:
> > >> >> > -static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
> > >> >> > -                               struct ufshpb_region *rgn)
> > >> >> > +static int __ufshpb_evict_region(struct ufshpb_lu *hpb,
> > >> >> > +                              struct ufshpb_region *rgn)
> > >> >> >  {
> > >> >> >       struct victim_select_info *lru_info;
> > >> >> >       struct ufshpb_subregion *srgn;
> > >> >> >       int srgn_idx;
> > >> >> >
> > >> >> > +     lockdep_assert_held(&hpb->rgn_state_lock);
> > >> >> > +
> > >> >> > +     if (hpb->is_hcm) {
> > >> >> > +             unsigned long flags;
> > >> >> > +             int ret;
> > >> >> > +
> > >> >> > +             spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> > >> >>
> > >> >> Never seen a usage like this... Here flags is used without being
> > >> >> intialized.
> > >> >> The flag is needed when spin_unlock_irqrestore ->
> > >> >> local_irq_restore(flags) to
> > >> >> restore the DAIF register (in terms of ARM).
> > >> > OK.
> > >>
> > >> Hi Avri,
> > >>
> > >> Checked on my setup, this lead to compilation error. Will you fix it
> > >> in
> > >> next version?
> > >>
> > >> warning: variable 'flags' is uninitialized when used here
> > >> [-Wuninitialized]
> > > Yeah - I will pass it to __ufshpb_evict_region and drop the
> > > lockdep_assert call.
Please ignore it.  This of course won't do.
Will fix it in v8.

Thanks,
Avri

[PATCH v7 07/11] scsi: ufshpb: Add hpb dev reset response

[Avri Altman]

The spec does not define what is the host's recommended response when
the device send hpb dev reset response (oper 0x2).

We will update all active hpb regions: mark them and do that on the next
read.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 32 +++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshpb.h |  1 +
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index fcc954f51bcf..1d99099ebd41 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -195,7 +195,8 @@ static void ufshpb_iterate_rgn(struct ufshpb_lu *hpb, int rgn_idx, int srgn_idx,
 		}
 		spin_unlock(&rgn->rgn_lock);
 
-		if (activate) {
+		if (activate ||
+		    test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
 			spin_lock_irqsave(&hpb->rsp_list_lock, flags);
 			ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
 			spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
@@ -1412,6 +1413,20 @@ 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)
+{
+	struct victim_select_info *lru_info = &hpb->lru_info;
+	struct ufshpb_region *rgn;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+	list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
+		set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags);
+
+	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+}
+
 /*
  * This function will parse recommended active subregion information in sense
  * data field of response UPIU with SAM_STAT_GOOD state.
@@ -1486,6 +1501,18 @@ 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);
+			}
+		}
+
 		break;
 	default:
 		dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1812,6 +1839,8 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 		} else {
 			rgn->rgn_state = HPB_RGN_INACTIVE;
 		}
+
+		rgn->rgn_flags = 0;
 	}
 
 	return 0;
@@ -2139,6 +2168,7 @@ static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
 {
 	if (hpb->is_hcm)
 		cancel_work_sync(&hpb->ufshpb_normalization_work);
+
 	cancel_work_sync(&hpb->map_work);
 }
 
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 1ea58c17a4de..b863540e28d6 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -127,6 +127,7 @@ struct ufshpb_region {
 	struct list_head list_lru_rgn;
 	unsigned long rgn_flags;
 #define RGN_FLAG_DIRTY 0
+#define RGN_FLAG_UPDATE 1
 
 	/* region reads - for host mode */
 	spinlock_t rgn_lock;
-- 
2.25.1

[PATCH v7 08/11] scsi: ufshpb: Add "Cold" regions timer

[Avri Altman]

In order not to hang on to “cold” regions, we shall inactivate a
region that has no READ access for a predefined amount of time -
READ_TO_MS. For that purpose we shall monitor the active regions list,
polling it on every POLLING_INTERVAL_MS. On timeout expiry we shall add
the region to the "to-be-inactivated" list, unless it is clean and did
not exhaust its READ_TO_EXPIRIES - another parameter.

All this does not apply to pinned regions.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 74 +++++++++++++++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshpb.h |  8 +++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 1d99099ebd41..8dbeaf948afa 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,6 +18,9 @@
 
 #define ACTIVATION_THRESHOLD 8 /* 8 IOs */
 #define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 5) /* 256 IOs */
+#define READ_TO_MS 1000
+#define READ_TO_EXPIRIES 100
+#define POLLING_INTERVAL_MS 200
 
 /* memory management */
 static struct kmem_cache *ufshpb_mctx_cache;
@@ -1031,12 +1034,63 @@ static int ufshpb_check_srgns_issue_state(struct ufshpb_lu *hpb,
 	return 0;
 }
 
+static void ufshpb_read_to_handler(struct work_struct *work)
+{
+	struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu,
+					     ufshpb_read_to_work.work);
+	struct victim_select_info *lru_info = &hpb->lru_info;
+	struct ufshpb_region *rgn, *next_rgn;
+	unsigned long flags;
+	LIST_HEAD(expired_list);
+
+	if (test_and_set_bit(TIMEOUT_WORK_RUNNING, &hpb->work_data_bits))
+		return;
+
+	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+	list_for_each_entry_safe(rgn, next_rgn, &lru_info->lh_lru_rgn,
+				 list_lru_rgn) {
+		bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
+
+		if (timedout) {
+			rgn->read_timeout_expiries--;
+			if (is_rgn_dirty(rgn) ||
+			    rgn->read_timeout_expiries == 0)
+				list_add(&rgn->list_expired_rgn, &expired_list);
+			else
+				rgn->read_timeout = ktime_add_ms(ktime_get(),
+							 READ_TO_MS);
+		}
+	}
+
+	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+	list_for_each_entry_safe(rgn, next_rgn, &expired_list,
+				 list_expired_rgn) {
+		list_del_init(&rgn->list_expired_rgn);
+		spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+		ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
+		spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+	}
+
+	ufshpb_kick_map_work(hpb);
+
+	clear_bit(TIMEOUT_WORK_RUNNING, &hpb->work_data_bits);
+
+	schedule_delayed_work(&hpb->ufshpb_read_to_work,
+			      msecs_to_jiffies(POLLING_INTERVAL_MS));
+}
+
 static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
 				struct ufshpb_region *rgn)
 {
 	rgn->rgn_state = HPB_RGN_ACTIVE;
 	list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
 	atomic_inc(&lru_info->active_cnt);
+	if (rgn->hpb->is_hcm) {
+		rgn->read_timeout = ktime_add_ms(ktime_get(), READ_TO_MS);
+		rgn->read_timeout_expiries = READ_TO_EXPIRIES;
+	}
 }
 
 static void ufshpb_hit_lru_info(struct victim_select_info *lru_info,
@@ -1820,6 +1874,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 
 		INIT_LIST_HEAD(&rgn->list_inact_rgn);
 		INIT_LIST_HEAD(&rgn->list_lru_rgn);
+		INIT_LIST_HEAD(&rgn->list_expired_rgn);
 
 		if (rgn_idx == hpb->rgns_per_lu - 1) {
 			srgn_cnt = ((hpb->srgns_per_lu - 1) %
@@ -1841,6 +1896,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 		}
 
 		rgn->rgn_flags = 0;
+		rgn->hpb = hpb;
 	}
 
 	return 0;
@@ -2064,9 +2120,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 	INIT_LIST_HEAD(&hpb->list_hpb_lu);
 
 	INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
-	if (hpb->is_hcm)
+	if (hpb->is_hcm) {
 		INIT_WORK(&hpb->ufshpb_normalization_work,
 			  ufshpb_normalization_work_handler);
+		INIT_DELAYED_WORK(&hpb->ufshpb_read_to_work,
+				  ufshpb_read_to_handler);
+	}
 
 	hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
 			  sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -2100,6 +2159,10 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 	ufshpb_stat_init(hpb);
 	ufshpb_param_init(hpb);
 
+	if (hpb->is_hcm)
+		schedule_delayed_work(&hpb->ufshpb_read_to_work,
+				      msecs_to_jiffies(POLLING_INTERVAL_MS));
+
 	return 0;
 
 release_pre_req_mempool:
@@ -2166,9 +2229,10 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
 
 static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
 {
-	if (hpb->is_hcm)
+	if (hpb->is_hcm) {
+		cancel_delayed_work_sync(&hpb->ufshpb_read_to_work);
 		cancel_work_sync(&hpb->ufshpb_normalization_work);
-
+	}
 	cancel_work_sync(&hpb->map_work);
 }
 
@@ -2276,6 +2340,10 @@ void ufshpb_resume(struct ufs_hba *hba)
 			continue;
 		ufshpb_set_state(hpb, HPB_PRESENT);
 		ufshpb_kick_map_work(hpb);
+		if (hpb->is_hcm)
+			schedule_delayed_work(&hpb->ufshpb_read_to_work,
+				msecs_to_jiffies(POLLING_INTERVAL_MS));
+
 	}
 }
 
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index b863540e28d6..448062a94760 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -115,6 +115,7 @@ struct ufshpb_subregion {
 };
 
 struct ufshpb_region {
+	struct ufshpb_lu *hpb;
 	struct ufshpb_subregion *srgn_tbl;
 	enum HPB_RGN_STATE rgn_state;
 	int rgn_idx;
@@ -132,6 +133,10 @@ struct ufshpb_region {
 	/* region reads - for host mode */
 	spinlock_t rgn_lock;
 	unsigned int reads;
+	/* region "cold" timer - for host mode */
+	ktime_t read_timeout;
+	unsigned int read_timeout_expiries;
+	struct list_head list_expired_rgn;
 };
 
 #define for_each_sub_region(rgn, i, srgn)				\
@@ -223,6 +228,9 @@ struct ufshpb_lu {
 	/* for selecting victim */
 	struct victim_select_info lru_info;
 	struct work_struct ufshpb_normalization_work;
+	struct delayed_work ufshpb_read_to_work;
+	unsigned long work_data_bits;
+#define TIMEOUT_WORK_RUNNING 0
 
 	/* pinned region information */
 	u32 lu_pinned_start;
-- 
2.25.1

[PATCH v7 09/11] scsi: ufshpb: Limit the number of inflight map requests

[Avri Altman]

In host control mode the host is the originator of map requests. To not
flood the device with map requests, use a simple throttling mechanism
that limits the number of inflight map requests.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 11 +++++++++++
 drivers/scsi/ufs/ufshpb.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 8dbeaf948afa..c07da481ff4e 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -21,6 +21,7 @@
 #define READ_TO_MS 1000
 #define READ_TO_EXPIRIES 100
 #define POLLING_INTERVAL_MS 200
+#define THROTTLE_MAP_REQ_DEFAULT 1
 
 /* memory management */
 static struct kmem_cache *ufshpb_mctx_cache;
@@ -740,6 +741,14 @@ static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 	struct ufshpb_req *map_req;
 	struct bio *bio;
 
+	if (hpb->is_hcm &&
+	    hpb->num_inflight_map_req >= THROTTLE_MAP_REQ_DEFAULT) {
+		dev_info(&hpb->sdev_ufs_lu->sdev_dev,
+			 "map_req throttle. inflight %d throttle %d",
+			 hpb->num_inflight_map_req, THROTTLE_MAP_REQ_DEFAULT);
+		return NULL;
+	}
+
 	map_req = ufshpb_get_req(hpb, srgn->rgn_idx, REQ_OP_SCSI_IN);
 	if (!map_req)
 		return NULL;
@@ -754,6 +763,7 @@ static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 
 	map_req->rb.srgn_idx = srgn->srgn_idx;
 	map_req->rb.mctx = srgn->mctx;
+	hpb->num_inflight_map_req++;
 
 	return map_req;
 }
@@ -763,6 +773,7 @@ static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
 {
 	bio_put(map_req->bio);
 	ufshpb_put_req(hpb, map_req);
+	hpb->num_inflight_map_req--;
 }
 
 static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 448062a94760..cfa0abac21db 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -217,6 +217,7 @@ struct ufshpb_lu {
 	struct ufshpb_req *pre_req;
 	int num_inflight_pre_req;
 	int throttle_pre_req;
+	int num_inflight_map_req;
 	struct list_head lh_pre_req_free;
 	int cur_read_id;
 	int pre_req_min_tr_len;
-- 
2.25.1

[PATCH v7 10/11] scsi: ufshpb: Add support for host control mode

[Avri Altman]

Support devices that report they are using host control mode.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index c07da481ff4e..08066bb6da65 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2582,12 +2582,6 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
 	u32 max_hpb_single_cmd = HPB_MULTI_CHUNK_LOW;
 
 	hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
-	if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
-		dev_err(hba->dev, "%s: host control mode is not supported.\n",
-			__func__);
-		hpb_dev_info->hpb_disabled = true;
-		return;
-	}
 
 	version = get_unaligned_be16(desc_buf + DEVICE_DESC_PARAM_HPB_VER);
 	if ((version != HPB_SUPPORT_VERSION) &&
-- 
2.25.1

[PATCH v7 11/11] scsi: ufshpb: Make host mode parameters configurable

[Avri Altman]

We can make use of this commit, to elaborate some more of the host
control mode logic, explaining what role play each and every variable.

While at it, allow those parameters to be configurable.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs |  84 +++++-
 drivers/scsi/ufs/ufshpb.c                  | 288 +++++++++++++++++++--
 drivers/scsi/ufs/ufshpb.h                  |  20 ++
 3 files changed, 365 insertions(+), 27 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 419adf450b89..133af2114165 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1323,14 +1323,76 @@ Description:	This entry shows the maximum HPB data size for using single HPB
 
 		The file is read only.
 
-What:		/sys/bus/platform/drivers/ufshcd/*/flags/wb_enable
-Date:		March 2021
-Contact:	Daejun Park <daejun7.park@samsung.com>
-Description:	This entry shows the status of HPB.
-
-		== ============================
-		0  HPB is not enabled.
-		1  HPB is enabled
-		== ============================
-
-		The file is read only.
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/activation_thld
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	In host control mode, reads are the major source of activation
+		trials.  once this threshold hs met, the region is added to the
+		"to-be-activated" list.  Since we reset the read counter upon
+		write, this include sending a rb command updating the region
+		ppn as well.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/normalization_factor
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	In host control mode, We think of the regions as "buckets".
+		Those buckets are being filled with reads, and emptied on write.
+		We use entries_per_srgn - the amount of blocks in a subregion as
+		our bucket size.  This applies because HPB1.0 only concern a
+		single-block reads.  Once the bucket size is crossed, we trigger
+		a normalization work - not only to avoid overflow, but mainly
+		because we want to keep those counters normalized, as we are
+		using those reads as a comparative score, to make various decisions.
+		The normalization is dividing (shift right) the read counter by
+		the normalization_factor. If during consecutive normalizations
+		an active region has exhaust its reads - inactivate it.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/eviction_thld_enter
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	Region deactivation is often due to the fact that eviction took
+		place: a region become active on the expense of another. This is
+		happening when the max-active-regions limit has crossed.
+		In host mode, eviction is considered an extreme measure. We
+		want to verify that the entering region has enough reads, and
+		the exiting region has much less reads.  eviction_thld_enter is
+		the min reads that a region must have in order to be considered
+		as a candidate to evict other region.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/eviction_thld_exit
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	same as above for the exiting region. A region is consider to
+		be a candidate to be evicted, only if it has less reads than
+		eviction_thld_exit.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/read_timeout_ms
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	In order not to hang on to “cold” regions, we shall inactivate
+		a region that has no READ access for a predefined amount of
+		time - read_timeout_ms. If read_timeout_ms has expired, and the
+		region is dirty - it is less likely that we can make any use of
+		HPB-READing it.  So we inactivate it.  Still, deactivation has
+		its overhead, and we may still benefit from HPB-READing this
+		region if it is clean - see read_timeout_expiries.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/read_timeout_expiries
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	if the region read timeout has expired, but the region is clean,
+		just re-wind its timer for another spin.  Do that as long as it
+		is clean and did not exhaust its read_timeout_expiries threshold.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/timeout_polling_interval_ms
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	the frequency in which the delayed worker that checks the
+		read_timeouts is awaken.
+
+What:		/sys/class/scsi_device/*/device/hpb_param_sysfs/inflight_map_req
+Date:		February 2021
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:	in host control mode the host is the originator of map requests.
+		To not flood the device with map requests, use a simple throttling
+		mechanism that limits the number of inflight map requests.
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 08066bb6da65..c9fa2d9ccc2c 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,7 +17,6 @@
 #include "../sd.h"
 
 #define ACTIVATION_THRESHOLD 8 /* 8 IOs */
-#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 5) /* 256 IOs */
 #define READ_TO_MS 1000
 #define READ_TO_EXPIRIES 100
 #define POLLING_INTERVAL_MS 200
@@ -194,7 +193,7 @@ static void ufshpb_iterate_rgn(struct ufshpb_lu *hpb, int rgn_idx, int srgn_idx,
 		} else {
 			srgn->reads++;
 			rgn->reads++;
-			if (srgn->reads == ACTIVATION_THRESHOLD)
+			if (srgn->reads == hpb->params.activation_thld)
 				activate = true;
 		}
 		spin_unlock(&rgn->rgn_lock);
@@ -742,10 +741,11 @@ static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 	struct bio *bio;
 
 	if (hpb->is_hcm &&
-	    hpb->num_inflight_map_req >= THROTTLE_MAP_REQ_DEFAULT) {
+	    hpb->num_inflight_map_req >= hpb->params.inflight_map_req) {
 		dev_info(&hpb->sdev_ufs_lu->sdev_dev,
 			 "map_req throttle. inflight %d throttle %d",
-			 hpb->num_inflight_map_req, THROTTLE_MAP_REQ_DEFAULT);
+			 hpb->num_inflight_map_req,
+			 hpb->params.inflight_map_req);
 		return NULL;
 	}
 
@@ -1052,6 +1052,7 @@ static void ufshpb_read_to_handler(struct work_struct *work)
 	struct victim_select_info *lru_info = &hpb->lru_info;
 	struct ufshpb_region *rgn, *next_rgn;
 	unsigned long flags;
+	unsigned int poll;
 	LIST_HEAD(expired_list);
 
 	if (test_and_set_bit(TIMEOUT_WORK_RUNNING, &hpb->work_data_bits))
@@ -1070,7 +1071,7 @@ static void ufshpb_read_to_handler(struct work_struct *work)
 				list_add(&rgn->list_expired_rgn, &expired_list);
 			else
 				rgn->read_timeout = ktime_add_ms(ktime_get(),
-							 READ_TO_MS);
+						hpb->params.read_timeout_ms);
 		}
 	}
 
@@ -1088,8 +1089,9 @@ static void ufshpb_read_to_handler(struct work_struct *work)
 
 	clear_bit(TIMEOUT_WORK_RUNNING, &hpb->work_data_bits);
 
+	poll = hpb->params.timeout_polling_interval_ms;
 	schedule_delayed_work(&hpb->ufshpb_read_to_work,
-			      msecs_to_jiffies(POLLING_INTERVAL_MS));
+			      msecs_to_jiffies(poll));
 }
 
 static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
@@ -1099,8 +1101,11 @@ static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
 	list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
 	atomic_inc(&lru_info->active_cnt);
 	if (rgn->hpb->is_hcm) {
-		rgn->read_timeout = ktime_add_ms(ktime_get(), READ_TO_MS);
-		rgn->read_timeout_expiries = READ_TO_EXPIRIES;
+		rgn->read_timeout =
+			ktime_add_ms(ktime_get(),
+				     rgn->hpb->params.read_timeout_ms);
+		rgn->read_timeout_expiries =
+			rgn->hpb->params.read_timeout_expiries;
 	}
 }
 
@@ -1129,7 +1134,8 @@ static struct ufshpb_region *ufshpb_victim_lru_info(struct ufshpb_lu *hpb)
 		 * in host control mode, verify that the exiting region
 		 * has less reads
 		 */
-		if (hpb->is_hcm && rgn->reads > (EVICTION_THRESHOLD >> 1))
+		if (hpb->is_hcm &&
+		    rgn->reads > hpb->params.eviction_thld_exit)
 			continue;
 
 		victim_rgn = rgn;
@@ -1356,7 +1362,8 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			 * in host control mode, verify that the entering
 			 * region has enough reads
 			 */
-			if (hpb->is_hcm && rgn->reads < EVICTION_THRESHOLD) {
+			if (hpb->is_hcm &&
+			    rgn->reads < hpb->params.eviction_thld_enter) {
 				ret = -EACCES;
 				goto out;
 			}
@@ -1697,6 +1704,7 @@ static void ufshpb_normalization_work_handler(struct work_struct *work)
 	struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu,
 					     ufshpb_normalization_work);
 	int rgn_idx;
+	u8 factor = hpb->params.normalization_factor;
 
 	for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
 		struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
@@ -1707,7 +1715,7 @@ static void ufshpb_normalization_work_handler(struct work_struct *work)
 		for (srgn_idx = 0; srgn_idx < hpb->srgns_per_rgn; srgn_idx++) {
 			struct ufshpb_subregion *srgn = rgn->srgn_tbl + srgn_idx;
 
-			srgn->reads >>= 1;
+			srgn->reads >>= factor;
 			rgn->reads += srgn->reads;
 		}
 		spin_unlock(&rgn->rgn_lock);
@@ -2031,8 +2039,247 @@ requeue_timeout_ms_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(requeue_timeout_ms);
 
+ufshpb_sysfs_param_show_func(activation_thld);
+static ssize_t
+activation_thld_store(struct device *dev, struct device_attribute *attr,
+		      const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	if (val <= 0)
+		return -EINVAL;
+
+	hpb->params.activation_thld = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(activation_thld);
+
+ufshpb_sysfs_param_show_func(normalization_factor);
+static ssize_t
+normalization_factor_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	if (val <= 0 || val > ilog2(hpb->entries_per_srgn))
+		return -EINVAL;
+
+	hpb->params.normalization_factor = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(normalization_factor);
+
+ufshpb_sysfs_param_show_func(eviction_thld_enter);
+static ssize_t
+eviction_thld_enter_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	if (val <= hpb->params.eviction_thld_exit)
+		return -EINVAL;
+
+	hpb->params.eviction_thld_enter = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(eviction_thld_enter);
+
+ufshpb_sysfs_param_show_func(eviction_thld_exit);
+static ssize_t
+eviction_thld_exit_store(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	if (val <= hpb->params.activation_thld)
+		return -EINVAL;
+
+	hpb->params.eviction_thld_exit = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(eviction_thld_exit);
+
+ufshpb_sysfs_param_show_func(read_timeout_ms);
+static ssize_t
+read_timeout_ms_store(struct device *dev, struct device_attribute *attr,
+		      const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	/* read_timeout >> timeout_polling_interval */
+	if (val < hpb->params.timeout_polling_interval_ms * 2)
+		return -EINVAL;
+
+	hpb->params.read_timeout_ms = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(read_timeout_ms);
+
+ufshpb_sysfs_param_show_func(read_timeout_expiries);
+static ssize_t
+read_timeout_expiries_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	if (val <= 0)
+		return -EINVAL;
+
+	hpb->params.read_timeout_expiries = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(read_timeout_expiries);
+
+ufshpb_sysfs_param_show_func(timeout_polling_interval_ms);
+static ssize_t
+timeout_polling_interval_ms_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	/* timeout_polling_interval << read_timeout */
+	if (val <= 0 || val > hpb->params.read_timeout_ms / 2)
+		return -EINVAL;
+
+	hpb->params.timeout_polling_interval_ms = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(timeout_polling_interval_ms);
+
+ufshpb_sysfs_param_show_func(inflight_map_req);
+static ssize_t inflight_map_req_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+	int val;
+
+	if (!hpb)
+		return -ENODEV;
+
+	if (!hpb->is_hcm)
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+
+	if (val <= 0 || val > hpb->sdev_ufs_lu->queue_depth - 1)
+		return -EINVAL;
+
+	hpb->params.inflight_map_req = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(inflight_map_req);
+
+static void ufshpb_hcm_param_init(struct ufshpb_lu *hpb)
+{
+	hpb->params.activation_thld = ACTIVATION_THRESHOLD;
+	hpb->params.normalization_factor = 1;
+	hpb->params.eviction_thld_enter = (ACTIVATION_THRESHOLD << 5);
+	hpb->params.eviction_thld_exit = (ACTIVATION_THRESHOLD << 4);
+	hpb->params.read_timeout_ms = READ_TO_MS;
+	hpb->params.read_timeout_expiries = READ_TO_EXPIRIES;
+	hpb->params.timeout_polling_interval_ms = POLLING_INTERVAL_MS;
+	hpb->params.inflight_map_req = THROTTLE_MAP_REQ_DEFAULT;
+}
+
 static struct attribute *hpb_dev_param_attrs[] = {
 	&dev_attr_requeue_timeout_ms.attr,
+	&dev_attr_activation_thld.attr,
+	&dev_attr_normalization_factor.attr,
+	&dev_attr_eviction_thld_enter.attr,
+	&dev_attr_eviction_thld_exit.attr,
+	&dev_attr_read_timeout_ms.attr,
+	&dev_attr_read_timeout_expiries.attr,
+	&dev_attr_timeout_polling_interval_ms.attr,
+	&dev_attr_inflight_map_req.attr,
 	NULL,
 };
 
@@ -2116,6 +2363,8 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
 static void ufshpb_param_init(struct ufshpb_lu *hpb)
 {
 	hpb->params.requeue_timeout_ms = HPB_REQUEUE_TIME_MS;
+	if (hpb->is_hcm)
+		ufshpb_hcm_param_init(hpb);
 }
 
 static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
@@ -2170,9 +2419,13 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 	ufshpb_stat_init(hpb);
 	ufshpb_param_init(hpb);
 
-	if (hpb->is_hcm)
+	if (hpb->is_hcm) {
+		unsigned int poll;
+
+		poll = hpb->params.timeout_polling_interval_ms;
 		schedule_delayed_work(&hpb->ufshpb_read_to_work,
-				      msecs_to_jiffies(POLLING_INTERVAL_MS));
+				      msecs_to_jiffies(poll));
+	}
 
 	return 0;
 
@@ -2351,10 +2604,13 @@ void ufshpb_resume(struct ufs_hba *hba)
 			continue;
 		ufshpb_set_state(hpb, HPB_PRESENT);
 		ufshpb_kick_map_work(hpb);
-		if (hpb->is_hcm)
-			schedule_delayed_work(&hpb->ufshpb_read_to_work,
-				msecs_to_jiffies(POLLING_INTERVAL_MS));
+		if (hpb->is_hcm) {
+			unsigned int poll =
+				hpb->params.timeout_polling_interval_ms;
 
+			schedule_delayed_work(&hpb->ufshpb_read_to_work,
+				msecs_to_jiffies(poll));
+		}
 	}
 }
 
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index cfa0abac21db..68a5af0ff682 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -185,8 +185,28 @@ struct victim_select_info {
 	atomic_t active_cnt;
 };
 
+/**
+ * ufshpb_params - ufs hpb parameters
+ * @requeue_timeout_ms - requeue threshold of wb command (0x2)
+ * @activation_thld - min reads [IOs] to activate/update a region
+ * @normalization_factor - shift right the region's reads
+ * @eviction_thld_enter - min reads [IOs] for the entering region in eviction
+ * @eviction_thld_exit - max reads [IOs] for the exiting region in eviction
+ * @read_timeout_ms - timeout [ms] from the last read IO to the region
+ * @read_timeout_expiries - amount of allowable timeout expireis
+ * @timeout_polling_interval_ms - frequency in which timeouts are checked
+ * @inflight_map_req - number of inflight map requests
+ */
 struct ufshpb_params {
 	unsigned int requeue_timeout_ms;
+	unsigned int activation_thld;
+	unsigned int normalization_factor;
+	unsigned int eviction_thld_enter;
+	unsigned int eviction_thld_exit;
+	unsigned int read_timeout_ms;
+	unsigned int read_timeout_expiries;
+	unsigned int timeout_polling_interval_ms;
+	unsigned int inflight_map_req;
 };
 
 struct ufshpb_stats {
-- 
2.25.1

RE: [PATCH v7 00/11] Add Host control mode to HPB

[Daejun Park]

> The HPB spec defines 2 control modes - device control mode and host
> control mode. In oppose to device control mode, in which the host obey
> to whatever recommendation received from the device - In host control
> mode, the host uses its own algorithms to decide which regions should
> be activated or inactivated.
>  
> We kept the host managed heuristic simple and concise.
>  
> Aside from adding a by-spec functionality, host control mode entails
> some further potential benefits: makes the hpb logic transparent and
> readable, while allow tuning / scaling its various parameters, and
> utilize system-wide info to optimize HPB potential.
>  
> This series is based on Samsung's V32 device-control HPB2.0 driver
>  
> This version was tested on Galaxy S20, and Xiaomi Mi10 pro.
> Your meticulous review and testing is mostly welcome and appreciated.
>  
> Thanks,
> Avri
>  
>  
> Avri Altman (11):
>   scsi: ufshpb: Cache HPB Control mode on init
>   scsi: ufshpb: Add host control mode support to rsp_upiu
>   scsi: ufshpb: Transform set_dirty to iterate_rgn
>   scsi: ufshpb: Add reads counter
>   scsi: ufshpb: Make eviction depends on region's reads
>   scsi: ufshpb: Region inactivation in host mode
>   scsi: ufshpb: Add hpb dev reset response
>   scsi: ufshpb: Add "Cold" regions timer
>   scsi: ufshpb: Limit the number of inflight map requests
>   scsi: ufshpb: Add support for host control mode
>   scsi: ufshpb: Make host mode parameters configurable
>  
>  Documentation/ABI/testing/sysfs-driver-ufs |  84 ++-
>  drivers/scsi/ufs/ufshcd.h                  |   2 +
>  drivers/scsi/ufs/ufshpb.c                  | 568 ++++++++++++++++++++-
>  drivers/scsi/ufs/ufshpb.h                  |  44 ++
>  4 files changed, 663 insertions(+), 35 deletions(-)
>  

The patches in this series look good to me.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>