[PATCH v2 0/9] Add Host control mode to HPB

[PATCH v2 0/9] Add Host control mode to HPB

[Avri Altman]

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 V19 device-control HPB1.0 driver, see
msg-id: 20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p6
in lore.kernel.org. The patches are also available in wdc ufs repo:
https://github.com/westerndigitalcorporation/WDC-UFS-REPO/tree/hpb-v19

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 (9):
  scsi: ufshpb: Cache HPB Control mode on init
  scsi: ufshpb: Add host control mode support to rsp_upiu
  scsi: ufshpb: Add region's 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: Add support for host control mode
  scsi: ufshpb: Make host mode parameters configurable

 drivers/scsi/ufs/ufshcd.c |   1 +
 drivers/scsi/ufs/ufshcd.h |   2 +
 drivers/scsi/ufs/ufshpb.c | 697 +++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufshpb.h |  47 +++
 4 files changed, 697 insertions(+), 50 deletions(-)

-- 
2.25.1

[PATCH v2 1/9] 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 961fc5b77943..7aeb83b10c37 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -654,6 +654,7 @@ struct ufs_hba_variant_params {
  * @srgn_size: device reported HPB sub-region size
  * @slave_conf_cnt: counter to check all lu finished initialization
  * @hpb_disabled: flag to check if HPB is disabled
+ * @control_mode: either host or device
  */
 struct ufshpb_dev_info {
 	int num_lu;
@@ -661,6 +662,7 @@ struct ufshpb_dev_info {
 	int srgn_size;
 	atomic_t slave_conf_cnt;
 	bool hpb_disabled;
+	u8 control_mode;
 };
 #endif
 
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 73e7b3ed04a4..46f6a7104e7e 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1128,6 +1128,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)
@@ -1694,10 +1697,9 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
 {
 	struct ufshpb_dev_info *hpb_dev_info = &hba->ufshpb_dev;
 	int version;
-	u8 hpb_mode;
 
-	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 2c43a03b66b6..afeb6365daf8 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -186,6 +186,8 @@ struct ufshpb_lu {
 	u32 entries_per_srgn_shift;
 	u32 pages_per_srgn;
 
+	bool is_hcm;
+
 	struct ufshpb_stats stats;
 
 	struct kmem_cache *map_req_cache;
-- 
2.25.1

[PATCH v2 2/9] 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 | 35 +++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshpb.h |  6 ++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 46f6a7104e7e..61de80a778a7 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -138,6 +138,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);
@@ -199,6 +201,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 u64 ufshpb_get_ppn(struct ufshpb_lu *hpb,
 			  struct ufshpb_map_ctx *mctx, int pos, int *error)
 {
@@ -380,8 +387,12 @@ 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;
+
 	WARN_ON(!srgn->mctx);
 	bitmap_zero(srgn->mctx->ppn_dirty, hpb->entries_per_srgn);
+	rgn = hpb->rgn_tbl + srgn->rgn_idx;
+	clear_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
 	return 0;
 }
 
@@ -814,17 +825,39 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 	 */
 	spin_lock(&hpb->rsp_list_lock);
 	for (i = 0; i < rsp_field->active_rgn_cnt; i++) {
+		struct ufshpb_region *rgn;
+
 		rgn_idx =
 			be16_to_cpu(rsp_field->hpb_active_field[i].active_rgn);
 		srgn_idx =
 			be16_to_cpu(rsp_field->hpb_active_field[i].active_srgn);
 
+		rgn = hpb->rgn_tbl + rgn_idx;
+		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_idx, srgn_idx);
 		ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
 		hpb->stats.rb_active_cnt++;
 	}
 
+	if (hpb->is_hcm) {
+		/*
+		 * in host control mode the device is not allowed to inactivate
+		 * regions
+		 */
+		goto out_unlock;
+	}
+
 	for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
 		rgn_idx = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
 		dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
@@ -832,6 +865,8 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
 		ufshpb_update_inactive_info(hpb, rgn_idx);
 		hpb->stats.rb_inactive_cnt++;
 	}
+
+out_unlock:
 	spin_unlock(&hpb->rsp_list_lock);
 
 	dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "Noti: #ACT %u #INACT %u\n",
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index afeb6365daf8..5ec4023db74d 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -48,6 +48,11 @@ enum UFSHPB_MODE {
 	HPB_DEVICE_CONTROL,
 };
 
+enum HPB_RGN_FLAGS {
+	RGN_FLAG_UPDATE = 0,
+	RGN_FLAG_DIRTY,
+};
+
 enum UFSHPB_STATE {
 	HPB_PRESENT = 1,
 	HPB_SUSPEND,
@@ -109,6 +114,7 @@ struct ufshpb_region {
 
 	/* below information is used by lru */
 	struct list_head list_lru_rgn;
+	unsigned long rgn_flags;
 };
 
 #define for_each_sub_region(rgn, i, srgn)				\
-- 
2.25.1

Re: [PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu

[Greg KH]

On Tue, Feb 02, 2021 at 10:30:00AM +0200, Avri Altman wrote:
> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> index afeb6365daf8..5ec4023db74d 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -48,6 +48,11 @@ enum UFSHPB_MODE {
>  	HPB_DEVICE_CONTROL,
>  };
>  
> +enum HPB_RGN_FLAGS {
> +	RGN_FLAG_UPDATE = 0,
> +	RGN_FLAG_DIRTY,
> +};
> +
>  enum UFSHPB_STATE {
>  	HPB_PRESENT = 1,
>  	HPB_SUSPEND,
> @@ -109,6 +114,7 @@ struct ufshpb_region {
>  
>  	/* below information is used by lru */
>  	struct list_head list_lru_rgn;
> +	unsigned long rgn_flags;

Why an unsigned long for a simple enumerated type?  And why not make
this "type safe" by explicitly saying this is an enumerated type
variable?

thanks,

greg k-h

RE: [PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu

[Avri Altman]

 
> 
> On Tue, Feb 02, 2021 at 10:30:00AM +0200, Avri Altman wrote:
> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > index afeb6365daf8..5ec4023db74d 100644
> > --- a/drivers/scsi/ufs/ufshpb.h
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -48,6 +48,11 @@ enum UFSHPB_MODE {
> >       HPB_DEVICE_CONTROL,
> >  };
> >
> > +enum HPB_RGN_FLAGS {
> > +     RGN_FLAG_UPDATE = 0,
> > +     RGN_FLAG_DIRTY,
> > +};
> > +
> >  enum UFSHPB_STATE {
> >       HPB_PRESENT = 1,
> >       HPB_SUSPEND,
> > @@ -109,6 +114,7 @@ struct ufshpb_region {
> >
> >       /* below information is used by lru */
> >       struct list_head list_lru_rgn;
> > +     unsigned long rgn_flags;
> 
> Why an unsigned long for a simple enumerated type?  And why not make
> this "type safe" by explicitly saying this is an enumerated type
> variable?
I am using it for atomic bit operations.

Thanks,
Avri

Re: [PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu

[Greg KH]

On Tue, Feb 02, 2021 at 11:24:04AM +0000, Avri Altman wrote:
>  
> > 
> > On Tue, Feb 02, 2021 at 10:30:00AM +0200, Avri Altman wrote:
> > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > > index afeb6365daf8..5ec4023db74d 100644
> > > --- a/drivers/scsi/ufs/ufshpb.h
> > > +++ b/drivers/scsi/ufs/ufshpb.h
> > > @@ -48,6 +48,11 @@ enum UFSHPB_MODE {
> > >       HPB_DEVICE_CONTROL,
> > >  };
> > >
> > > +enum HPB_RGN_FLAGS {
> > > +     RGN_FLAG_UPDATE = 0,
> > > +     RGN_FLAG_DIRTY,
> > > +};
> > > +
> > >  enum UFSHPB_STATE {
> > >       HPB_PRESENT = 1,
> > >       HPB_SUSPEND,
> > > @@ -109,6 +114,7 @@ struct ufshpb_region {
> > >
> > >       /* below information is used by lru */
> > >       struct list_head list_lru_rgn;
> > > +     unsigned long rgn_flags;
> > 
> > Why an unsigned long for a simple enumerated type?  And why not make
> > this "type safe" by explicitly saying this is an enumerated type
> > variable?
> I am using it for atomic bit operations.

That's not obvious given you have an enumerated type above.  Seems like
an odd mix...

RE: [PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu

[Avri Altman]

> On Tue, Feb 02, 2021 at 11:24:04AM +0000, Avri Altman wrote:
> >
> > >
> > > On Tue, Feb 02, 2021 at 10:30:00AM +0200, Avri Altman wrote:
> > > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > > > index afeb6365daf8..5ec4023db74d 100644
> > > > --- a/drivers/scsi/ufs/ufshpb.h
> > > > +++ b/drivers/scsi/ufs/ufshpb.h
> > > > @@ -48,6 +48,11 @@ enum UFSHPB_MODE {
> > > >       HPB_DEVICE_CONTROL,
> > > >  };
> > > >
> > > > +enum HPB_RGN_FLAGS {
> > > > +     RGN_FLAG_UPDATE = 0,
> > > > +     RGN_FLAG_DIRTY,
> > > > +};
> > > > +
> > > >  enum UFSHPB_STATE {
> > > >       HPB_PRESENT = 1,
> > > >       HPB_SUSPEND,
> > > > @@ -109,6 +114,7 @@ struct ufshpb_region {
> > > >
> > > >       /* below information is used by lru */
> > > >       struct list_head list_lru_rgn;
> > > > +     unsigned long rgn_flags;
> > >
> > > Why an unsigned long for a simple enumerated type?  And why not make
> > > this "type safe" by explicitly saying this is an enumerated type
> > > variable?
> > I am using it for atomic bit operations.
> 
> That's not obvious given you have an enumerated type above.  Seems like
> an odd mix...
Done.
Will make it clear that those are bit indices.

[PATCH v2 3/9] scsi: ufshpb: Add region's 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.  less intuitive however, is that we also reset it upon region's
deactivation.  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. If we
don’t reset the counter, we will trigger a lot of trashing of the HPB
database, since few reads (or even one) to the region that was
deactivated, will trigger a re-activation trial.

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.

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

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 61de80a778a7..de4866d42df0 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -16,6 +16,9 @@
 #include "ufshpb.h"
 #include "../sd.h"
 
+#define WORK_PENDING 0
+#define ACTIVATION_THRSHLD 4 /* 4 IOs */
+
 /* memory management */
 static struct kmem_cache *ufshpb_mctx_cache;
 static mempool_t *ufshpb_mctx_pool;
@@ -261,6 +264,21 @@ ufshpb_set_hpb_read_to_upiu(struct ufshpb_lu *hpb, struct ufshcd_lrb *lrbp,
 	lrbp->cmd->cmd_len = UFS_CDB_SIZE;
 }
 
+static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
+				      int srgn_idx)
+{
+	struct ufshpb_region *rgn;
+	struct ufshpb_subregion *srgn;
+
+	rgn = hpb->rgn_tbl + rgn_idx;
+	srgn = rgn->srgn_tbl + srgn_idx;
+
+	list_del_init(&rgn->list_inact_rgn);
+
+	if (list_empty(&srgn->list_act_srgn))
+		list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+}
+
 /*
  * This function will set up HPB read command using host-side L2P map data.
  * In HPB v1.0, maximum size of HPB read command is 4KB.
@@ -306,12 +324,45 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
 				 transfer_len);
 		spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+		if (hpb->is_hcm) {
+			spin_lock_irqsave(&rgn->rgn_lock, flags);
+			rgn->reads = 0;
+			spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+		}
+
 		return;
 	}
 
 	if (!ufshpb_is_support_chunk(transfer_len))
 		return;
 
+	if (hpb->is_hcm) {
+		bool activate = false;
+		/*
+		 * in host control mode, reads are the main source for
+		 * activation trials.
+		 */
+		spin_lock_irqsave(&rgn->rgn_lock, flags);
+		rgn->reads++;
+		if (rgn->reads == ACTIVATION_THRSHLD)
+			activate = true;
+		spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+		if (activate) {
+			spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+			ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+			hpb->stats.rb_active_cnt++;
+			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);
+		}
+
+		/* keep those counters normalized */
+		if (rgn->reads > hpb->entries_per_srgn &&
+		    !test_and_set_bit(WORK_PENDING, &hpb->work_data_bits))
+			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)) {
@@ -396,21 +447,6 @@ static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
 	return 0;
 }
 
-static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
-				      int srgn_idx)
-{
-	struct ufshpb_region *rgn;
-	struct ufshpb_subregion *srgn;
-
-	rgn = hpb->rgn_tbl + rgn_idx;
-	srgn = rgn->srgn_tbl + srgn_idx;
-
-	list_del_init(&rgn->list_inact_rgn);
-
-	if (list_empty(&srgn->list_act_srgn))
-		list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
-}
-
 static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
 {
 	struct ufshpb_region *rgn;
@@ -646,6 +682,14 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
 
 	ufshpb_cleanup_lru_info(lru_info, rgn);
 
+	if (hpb->is_hcm) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&rgn->rgn_lock, flags);
+		rgn->reads = 0;
+		spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+	}
+
 	for_each_sub_region(rgn, srgn_idx, srgn)
 		ufshpb_purge_active_subregion(hpb, srgn);
 }
@@ -1044,6 +1088,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;
+	int rgn_idx;
+
+	hpb = container_of(work, struct ufshpb_lu, ufshpb_normalization_work);
+
+	for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
+		struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
+
+		if (rgn->reads) {
+			unsigned long flags;
+
+			spin_lock_irqsave(&rgn->rgn_lock, flags);
+			rgn->reads = (rgn->reads >> 1);
+			spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+		}
+
+		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);
+	}
+
+	clear_bit(WORK_PENDING, &hpb->work_data_bits);
+}
+
 static void ufshpb_map_work_handler(struct work_struct *work)
 {
 	struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu, map_work);
@@ -1313,6 +1387,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);
@@ -1399,6 +1476,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 5ec4023db74d..381b5fed61a5 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -115,6 +115,10 @@ struct ufshpb_region {
 	/* below information is used by lru */
 	struct list_head list_lru_rgn;
 	unsigned long rgn_flags;
+
+	/* region reads - for host mode */
+	spinlock_t rgn_lock;
+	unsigned int reads;
 };
 
 #define for_each_sub_region(rgn, i, srgn)				\
@@ -175,6 +179,8 @@ struct ufshpb_lu {
 
 	/* for selecting victim */
 	struct victim_select_info lru_info;
+	struct work_struct ufshpb_normalization_work;
+	unsigned long work_data_bits;
 
 	/* pinned region information */
 	u32 lu_pinned_start;
-- 
2.25.1

Re: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter

[Greg KH]

On Tue, Feb 02, 2021 at 10:30:01AM +0200, Avri Altman wrote:
> @@ -175,6 +179,8 @@ struct ufshpb_lu {
>  
>  	/* for selecting victim */
>  	struct victim_select_info lru_info;
> +	struct work_struct ufshpb_normalization_work;
> +	unsigned long work_data_bits;

You only have 1 "bit" being used here, so perhaps just a u8?  Please
don't use things like "unsigned long" for types, this isn't Windows :)

thanks,

greg k-h

RE: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter

[Avri Altman]

> 
> 
> On Tue, Feb 02, 2021 at 10:30:01AM +0200, Avri Altman wrote:
> > @@ -175,6 +179,8 @@ struct ufshpb_lu {
> >
> >       /* for selecting victim */
> >       struct victim_select_info lru_info;
> > +     struct work_struct ufshpb_normalization_work;
> > +     unsigned long work_data_bits;
> 
> You only have 1 "bit" being used here, so perhaps just a u8?  Please
> don't use things like "unsigned long" for types, this isn't Windows :)
I am using it for atomic bit operations.

Thanks,
Avri

Re: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter

[Greg KH]

On Tue, Feb 02, 2021 at 11:23:17AM +0000, Avri Altman wrote:
> > 
> > 
> > On Tue, Feb 02, 2021 at 10:30:01AM +0200, Avri Altman wrote:
> > > @@ -175,6 +179,8 @@ struct ufshpb_lu {
> > >
> > >       /* for selecting victim */
> > >       struct victim_select_info lru_info;
> > > +     struct work_struct ufshpb_normalization_work;
> > > +     unsigned long work_data_bits;
> > 
> > You only have 1 "bit" being used here, so perhaps just a u8?  Please
> > don't use things like "unsigned long" for types, this isn't Windows :)
> I am using it for atomic bit operations.

Ah, and that requires "unsigned long"?  Ok, nevermind :)

Re: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter

[Greg KH]

On Tue, Feb 02, 2021 at 10:30:01AM +0200, Avri Altman wrote:
> 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.  less intuitive however, is that we also reset it upon region's
> deactivation.  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. If we
> don’t reset the counter, we will trigger a lot of trashing of the HPB
> database, since few reads (or even one) to the region that was
> deactivated, will trigger a re-activation trial.
> 
> 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.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/scsi/ufs/ufshpb.c | 109 ++++++++++++++++++++++++++++++++------
>  drivers/scsi/ufs/ufshpb.h |   6 +++
>  2 files changed, 100 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index 61de80a778a7..de4866d42df0 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -16,6 +16,9 @@
>  #include "ufshpb.h"
>  #include "../sd.h"
>  
> +#define WORK_PENDING 0

This should be next to the variable you define that uses this, right?
Otherwise we would think this is a valid value, when in reality it is
the bit number, correct?

> +#define ACTIVATION_THRSHLD 4 /* 4 IOs */

You can spell things out "ACTIVATION_THRESHOLD" :)

thanks,

greg k-h

RE: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter

[Avri Altman]

> >
> > +#define WORK_PENDING 0
> 
> This should be next to the variable you define that uses this, right?
> Otherwise we would think this is a valid value, when in reality it is
> the bit number, correct?
Done.

> 
> > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */
> 
> You can spell things out "ACTIVATION_THRESHOLD" :)
Done.

[PATCH v2 4/9] 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 | 42 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index de4866d42df0..bae7dca105da 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,6 +18,7 @@
 
 #define WORK_PENDING 0
 #define ACTIVATION_THRSHLD 4 /* 4 IOs */
+#define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 256 IOs */
 
 /* memory management */
 static struct kmem_cache *ufshpb_mctx_cache;
@@ -644,6 +645,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_THRSHLD >> 1))
+			continue;
+
 		victim_rgn = rgn;
 		break;
 	}
@@ -799,7 +807,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;
@@ -827,6 +835,16 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
 			 * 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_THRSHLD) {
+				ret = -EACCES;
+				goto out;
+			}
+
 			victim_rgn = ufshpb_victim_lru_info(hpb);
 			if (!victim_rgn) {
 				dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1034,8 +1052,13 @@ static void ufshpb_run_active_subregion_list(struct ufshpb_lu *hpb)
 
 		rgn = hpb->rgn_tbl + srgn->rgn_idx;
 		ret = ufshpb_add_region(hpb, rgn);
-		if (ret)
-			goto active_failed;
+		if (ret) {
+			if (ret == -EACCES) {
+				spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+				continue;
+			}
+			goto add_region_failed;
+		}
 
 		ret = ufshpb_issue_map_req(hpb, rgn, srgn);
 		if (ret) {
@@ -1049,9 +1072,22 @@ static void ufshpb_run_active_subregion_list(struct ufshpb_lu *hpb)
 	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
 	return;
 
+add_region_failed:
+	if (hpb->is_hcm) {
+		/*
+		 * In host control mode, it is common that eviction trials
+		 * fail, either because the entering region didn't have enough
+		 * reads, or a poor-performing exiting region wasn't found.
+		 * No need to re-insert those regions to the "to-be-activated"
+		 * list.
+		 */
+		return;
+	}
+
 active_failed:
 	dev_err(&hpb->sdev_ufs_lu->sdev_dev, "failed to activate region %d - %d, will retry\n",
 		   rgn->rgn_idx, srgn->srgn_idx);
+
 	spin_lock_irqsave(&hpb->rsp_list_lock, flags);
 	ufshpb_add_active_list(hpb, rgn, srgn);
 	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
-- 
2.25.1

[PATCH v2 5/9] scsi: ufshpb: Region inactivation in host mode

[Avri Altman]

I 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 | 119 +++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshpb.h |   4 ++
 2 files changed, 103 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index bae7dca105da..49c74de539b7 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -391,49 +391,66 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	hpb->stats.hit_cnt++;
 }
 
+static struct ufshpb_req *ufshpb_get_req(struct ufshpb_lu *hpb,
+					 int rgn_idx, enum req_opf dir)
+{
+	struct ufshpb_req *rq;
+	struct request *req;
+
+	rq = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL);
+	if (!rq)
+		return NULL;
+
+	req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
+			      dir, 0);
+	if (IS_ERR(req))
+		goto free_rq;
+
+	rq->hpb = hpb;
+	rq->req = req;
+	rq->rgn_idx = rgn_idx;
+
+	return rq;
+
+free_rq:
+	kmem_cache_free(hpb->map_req_cache, rq);
+	return NULL;
+}
+
+static void ufshpb_put_req(struct ufshpb_lu *hpb, struct ufshpb_req *rq)
+{
+	blk_put_request(rq->req);
+	kmem_cache_free(hpb->map_req_cache, rq);
+}
+
 static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 					     struct ufshpb_subregion *srgn)
 {
 	struct ufshpb_req *map_req;
-	struct request *req;
 	struct bio *bio;
 
-	map_req = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL);
+	map_req = ufshpb_get_req(hpb, srgn->rgn_idx, REQ_OP_SCSI_IN);
 	if (!map_req)
 		return NULL;
 
-	req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
-			      REQ_OP_SCSI_IN, 0);
-	if (IS_ERR(req))
-		goto free_map_req;
-
 	bio = bio_alloc(GFP_KERNEL, hpb->pages_per_srgn);
 	if (!bio) {
-		blk_put_request(req);
-		goto free_map_req;
+		ufshpb_put_req(hpb, map_req);
+		return NULL;
 	}
 
-	map_req->hpb = hpb;
-	map_req->req = req;
 	map_req->bio = bio;
-
-	map_req->rgn_idx = srgn->rgn_idx;
 	map_req->srgn_idx = srgn->srgn_idx;
 	map_req->mctx = srgn->mctx;
 
 	return map_req;
-
-free_map_req:
-	kmem_cache_free(hpb->map_req_cache, map_req);
-	return NULL;
 }
 
 static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
-				      struct ufshpb_req *map_req)
+			       struct ufshpb_req *map_req)
 {
 	bio_put(map_req->bio);
-	blk_put_request(map_req->req);
-	kmem_cache_free(hpb->map_req_cache, map_req);
+	ufshpb_put_req(hpb, map_req);
 }
 
 static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
@@ -488,6 +505,13 @@ static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
 	srgn->srgn_state = HPB_SRGN_VALID;
 }
 
+static void ufshpb_umap_req_compl_fn(struct request *req, blk_status_t error)
+{
+	struct ufshpb_req *umap_req = (struct ufshpb_req *)req->end_io_data;
+
+	ufshpb_put_req(umap_req->hpb, umap_req);
+}
+
 static void ufshpb_map_req_compl_fn(struct request *req, blk_status_t error)
 {
 	struct ufshpb_req *map_req = (struct ufshpb_req *) req->end_io_data;
@@ -506,6 +530,14 @@ static void ufshpb_map_req_compl_fn(struct request *req, blk_status_t error)
 	ufshpb_put_map_req(map_req->hpb, map_req);
 }
 
+static void ufshpb_set_write_buf_cmd(unsigned char *cdb, int rgn_idx)
+{
+	cdb[0] = UFSHPB_WRITE_BUFFER;
+	cdb[1] = UFSHPB_WRITE_BUFFER_ID;
+	put_unaligned_be16(rgn_idx, &cdb[2]);
+	cdb[9] = 0x00;
+}
+
 static void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
 					   int srgn_idx, int srgn_mem_size)
 {
@@ -519,6 +551,27 @@ static void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
 	cdb[9] = 0x00;
 }
 
+static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
+				   struct ufshpb_req *umap_req)
+{
+	struct request_queue *q;
+	struct request *req;
+	struct scsi_request *rq;
+
+	q = hpb->sdev_ufs_lu->request_queue;
+	req = umap_req->req;
+	req->timeout = 0;
+	req->end_io_data = (void *)umap_req;
+	rq = scsi_req(req);
+	ufshpb_set_write_buf_cmd(rq->cmd, umap_req->rgn_idx);
+	rq->cmd_len = HPB_WRITE_BUFFER_CMD_LENGTH;
+
+	blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn);
+
+	hpb->stats.umap_req_cnt++;
+	return 0;
+}
+
 static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
 				  struct ufshpb_req *map_req)
 {
@@ -677,6 +730,25 @@ static void ufshpb_purge_active_subregion(struct ufshpb_lu *hpb,
 	}
 }
 
+static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb,
+				 struct ufshpb_region *rgn)
+{
+	struct ufshpb_req *umap_req;
+
+	umap_req = ufshpb_get_req(hpb, rgn->rgn_idx, REQ_OP_SCSI_OUT);
+	if (!umap_req)
+		return -ENOMEM;
+
+	if (ufshpb_execute_umap_req(hpb, umap_req))
+		goto free_umap_req;
+
+	return 0;
+
+free_umap_req:
+	ufshpb_put_req(hpb, umap_req);
+	return -EAGAIN;
+}
+
 static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
 				  struct ufshpb_region *rgn)
 {
@@ -684,6 +756,10 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
 	struct ufshpb_subregion *srgn;
 	int srgn_idx;
 
+
+	if (hpb->is_hcm && ufshpb_issue_umap_req(hpb, rgn))
+		return;
+
 	lru_info = &hpb->lru_info;
 
 	dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx);
@@ -1384,6 +1460,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_attrs[] = {
 	&dev_attr_hit_cnt.attr,
@@ -1392,6 +1469,7 @@ static struct attribute *hpb_dev_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,
 };
 
@@ -1408,6 +1486,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 int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 381b5fed61a5..71b082ee7876 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -35,8 +35,11 @@
 /* hpb vender defined opcode */
 #define UFSHPB_READ				0xF8
 #define UFSHPB_READ_BUFFER			0xF9
+#define UFSHPB_WRITE_BUFFER			0xFA
 #define UFSHPB_READ_BUFFER_ID			0x01
+#define UFSHPB_WRITE_BUFFER_ID			0x01
 #define HPB_READ_BUFFER_CMD_LENGTH		10
+#define HPB_WRITE_BUFFER_CMD_LENGTH		10
 #define LU_ENABLED_HPB_FUNC			0x02
 
 #define HPB_RESET_REQ_RETRIES			10
@@ -159,6 +162,7 @@ struct ufshpb_stats {
 	u64 rb_active_cnt;
 	u64 rb_inactive_cnt;
 	u64 map_req_cnt;
+	u64 umap_req_cnt;
 };
 
 struct ufshpb_lu {
-- 
2.25.1

[PATCH v2 6/9] 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 | 54 ++++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufshpb.h |  1 +
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 49c74de539b7..28e0025507a1 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,6 +17,7 @@
 #include "../sd.h"
 
 #define WORK_PENDING 0
+#define RESET_PENDING 1
 #define ACTIVATION_THRSHLD 4 /* 4 IOs */
 #define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 256 IOs */
 
@@ -349,7 +350,8 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		if (rgn->reads == ACTIVATION_THRSHLD)
 			activate = true;
 		spin_unlock_irqrestore(&rgn->rgn_lock, flags);
-		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);
 			hpb->stats.rb_active_cnt++;
@@ -1068,6 +1070,24 @@ 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 ufshpb_lu *h;
+			struct scsi_device *sdev;
+
+			shost_for_each_device(sdev, hba->host) {
+				h = sdev->hostdata;
+				if (!h)
+					continue;
+
+				if (test_and_set_bit(RESET_PENDING,
+						     &h->work_data_bits))
+					continue;
+
+				schedule_work(&h->ufshpb_lun_reset_work);
+			}
+		}
+
 		break;
 	default:
 		dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1200,6 +1220,27 @@ static void ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb)
 	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
 }
 
+static void ufshpb_reset_work_handler(struct work_struct *work)
+{
+	struct ufshpb_lu *hpb;
+	struct victim_select_info *lru_info;
+	struct ufshpb_region *rgn;
+	unsigned long flags;
+
+	hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work);
+
+	lru_info = &hpb->lru_info;
+
+	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);
+
+	clear_bit(RESET_PENDING, &hpb->work_data_bits);
+}
+
 static void ufshpb_normalization_work_handler(struct work_struct *work)
 {
 	struct ufshpb_lu *hpb;
@@ -1392,6 +1433,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;
@@ -1502,9 +1545,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_WORK(&hpb->ufshpb_lun_reset_work,
+			  ufshpb_reset_work_handler);
+	}
 
 	hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
 			  sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -1591,8 +1637,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_work_sync(&hpb->ufshpb_lun_reset_work);
 		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 71b082ee7876..e55892ceb3fc 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -184,6 +184,7 @@ struct ufshpb_lu {
 	/* for selecting victim */
 	struct victim_select_info lru_info;
 	struct work_struct ufshpb_normalization_work;
+	struct work_struct ufshpb_lun_reset_work;
 	unsigned long work_data_bits;
 
 	/* pinned region information */
-- 
2.25.1

Re: [PATCH v2 6/9] scsi: ufshpb: Add hpb dev reset response

[Can Guo]

On 2021-02-02 16:30, Avri Altman wrote:
> 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 | 54 ++++++++++++++++++++++++++++++++++++---
>  drivers/scsi/ufs/ufshpb.h |  1 +
>  2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index 49c74de539b7..28e0025507a1 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -17,6 +17,7 @@
>  #include "../sd.h"
> 
>  #define WORK_PENDING 0
> +#define RESET_PENDING 1
>  #define ACTIVATION_THRSHLD 4 /* 4 IOs */
>  #define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 256 IOs */
> 
> @@ -349,7 +350,8 @@ void ufshpb_prep(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp)
>  		if (rgn->reads == ACTIVATION_THRSHLD)
>  			activate = true;
>  		spin_unlock_irqrestore(&rgn->rgn_lock, flags);
> -		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);
>  			hpb->stats.rb_active_cnt++;
> @@ -1068,6 +1070,24 @@ 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 ufshpb_lu *h;
> +			struct scsi_device *sdev;
> +
> +			shost_for_each_device(sdev, hba->host) {

I haven't test it yet, but this line shall cause recursive spin lock -
in current code base, ufshpb_rsp_upiu() is called with host_lock held.

Regards,

Can Guo.

> +				h = sdev->hostdata;
> +				if (!h)
> +					continue;
> +
> +				if (test_and_set_bit(RESET_PENDING,
> +						     &h->work_data_bits))
> +					continue;
> +
> +				schedule_work(&h->ufshpb_lun_reset_work);
> +			}
> +		}
> +
>  		break;
>  	default:
>  		dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
> @@ -1200,6 +1220,27 @@ static void
> ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb)
>  	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
>  }
> 
> +static void ufshpb_reset_work_handler(struct work_struct *work)
> +{
> +	struct ufshpb_lu *hpb;
> +	struct victim_select_info *lru_info;
> +	struct ufshpb_region *rgn;
> +	unsigned long flags;
> +
> +	hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work);
> +
> +	lru_info = &hpb->lru_info;
> +
> +	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);
> +
> +	clear_bit(RESET_PENDING, &hpb->work_data_bits);
> +}
> +
>  static void ufshpb_normalization_work_handler(struct work_struct 
> *work)
>  {
>  	struct ufshpb_lu *hpb;
> @@ -1392,6 +1433,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;
> @@ -1502,9 +1545,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_WORK(&hpb->ufshpb_lun_reset_work,
> +			  ufshpb_reset_work_handler);
> +	}
> 
>  	hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
>  			  sizeof(struct ufshpb_req), 0, 0, NULL);
> @@ -1591,8 +1637,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_work_sync(&hpb->ufshpb_lun_reset_work);
>  		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 71b082ee7876..e55892ceb3fc 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -184,6 +184,7 @@ struct ufshpb_lu {
>  	/* for selecting victim */
>  	struct victim_select_info lru_info;
>  	struct work_struct ufshpb_normalization_work;
> +	struct work_struct ufshpb_lun_reset_work;
>  	unsigned long work_data_bits;
> 
>  	/* pinned region information */

RE: [PATCH v2 6/9] scsi: ufshpb: Add hpb dev reset response

[Avri Altman]

> > +             if (hpb->is_hcm) {
> > +                     struct ufshpb_lu *h;
> > +                     struct scsi_device *sdev;
> > +
> > +                     shost_for_each_device(sdev, hba->host) {
> 
> I haven't test it yet, but this line shall cause recursive spin lock -
> in current code base, ufshpb_rsp_upiu() is called with host_lock held.
Yayks Ouch.  Will fix.

Thanks,
Avri

[PATCH v2 7/9] 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 | 76 ++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshpb.h |  6 ++++
 2 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 28e0025507a1..c61fda95e35a 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,8 +18,12 @@
 
 #define WORK_PENDING 0
 #define RESET_PENDING 1
+#define TIMEOUT_WORK_PENDING 2
 #define ACTIVATION_THRSHLD 4 /* 4 IOs */
 #define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 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;
@@ -676,12 +680,69 @@ static int ufshpb_check_srgns_issue_state(struct ufshpb_lu *hpb,
 	return 0;
 }
 
+static void ufshpb_read_to_handler(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct ufshpb_lu *hpb;
+	struct victim_select_info *lru_info;
+	struct ufshpb_region *rgn;
+	unsigned long flags;
+	LIST_HEAD(expired_list);
+
+	hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
+
+	if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits))
+		return;
+
+	spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+	lru_info = &hpb->lru_info;
+
+	list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
+		bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
+		bool dirty, expired = false;
+
+		if (!timedout)
+			continue;
+
+		dirty = is_rgn_dirty(rgn);
+		rgn->read_timeout_expiries--;
+		if (rgn->read_timeout_expiries == 0)
+			expired = true;
+
+		if (dirty || expired)
+			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(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);
+		hpb->stats.rb_inactive_cnt++;
+		spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+	}
+
+	clear_bit(TIMEOUT_WORK_PENDING, &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)
+				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,
@@ -1416,6 +1477,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) %
@@ -1435,6 +1497,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 		}
 
 		rgn->rgn_flags = 0;
+		rgn->hpb = hpb;
 	}
 
 	return 0;
@@ -1550,6 +1613,8 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 			  ufshpb_normalization_work_handler);
 		INIT_WORK(&hpb->ufshpb_lun_reset_work,
 			  ufshpb_reset_work_handler);
+		INIT_DELAYED_WORK(&hpb->ufshpb_read_to_work,
+				  ufshpb_read_to_handler);
 	}
 
 	hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
@@ -1576,6 +1641,10 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 
 	ufshpb_stat_init(hpb);
 
+	if (hpb->is_hcm)
+		schedule_delayed_work(&hpb->ufshpb_read_to_work,
+				      msecs_to_jiffies(POLLING_INTERVAL_MS));
+
 	return 0;
 
 release_m_page_cache:
@@ -1638,6 +1707,7 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
 static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
 {
 	if (hpb->is_hcm) {
+		cancel_delayed_work_sync(&hpb->ufshpb_read_to_work);
 		cancel_work_sync(&hpb->ufshpb_lun_reset_work);
 		cancel_work_sync(&hpb->ufshpb_normalization_work);
 	}
@@ -1748,6 +1818,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 e55892ceb3fc..207925cf1f44 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -107,6 +107,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;
@@ -122,6 +123,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)				\
@@ -185,6 +190,7 @@ struct ufshpb_lu {
 	struct victim_select_info lru_info;
 	struct work_struct ufshpb_normalization_work;
 	struct work_struct ufshpb_lun_reset_work;
+	struct delayed_work ufshpb_read_to_work;
 	unsigned long work_data_bits;
 
 	/* pinned region information */
-- 
2.25.1

[PATCH v2 8/9] 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 c61fda95e35a..cec6f641a103 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2050,12 +2050,6 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
 	int version;
 
 	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 v2 9/9] 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:

 - activation_thld - 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.

- normalization_factor - 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.

- eviction_thld_enter - 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.

- eviction_thld_exit - 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.

 - read_timeout_ms - 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.

- read_timeout_expiries - 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.

- timeout_polling_interval_ms - the frequency in which the delayed
    worker that checks the read_timeouts is awaken.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1b521b366067..8dac66783c46 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8014,6 +8014,7 @@ static const struct attribute_group *ufshcd_driver_groups[] = {
 	&ufs_sysfs_lun_attributes_group,
 #ifdef CONFIG_SCSI_UFS_HPB
 	&ufs_sysfs_hpb_stat_group,
+	&ufs_sysfs_hpb_param_group,
 #endif
 	NULL,
 };
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index cec6f641a103..69a742acf0ee 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -351,7 +351,7 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		 */
 		spin_lock_irqsave(&rgn->rgn_lock, flags);
 		rgn->reads++;
-		if (rgn->reads == ACTIVATION_THRSHLD)
+		if (rgn->reads == hpb->params.activation_thld)
 			activate = true;
 		spin_unlock_irqrestore(&rgn->rgn_lock, flags);
 		if (activate ||
@@ -687,6 +687,7 @@ static void ufshpb_read_to_handler(struct work_struct *work)
 	struct victim_select_info *lru_info;
 	struct ufshpb_region *rgn;
 	unsigned long flags;
+	unsigned int poll;
 	LIST_HEAD(expired_list);
 
 	hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
@@ -713,8 +714,9 @@ static void ufshpb_read_to_handler(struct work_struct *work)
 		if (dirty || expired)
 			list_add(&rgn->list_expired_rgn, &expired_list);
 		else
-			rgn->read_timeout = ktime_add_ms(ktime_get(),
-							 READ_TO_MS);
+			rgn->read_timeout =
+				ktime_add_ms(ktime_get(),
+					     hpb->params.read_timeout_ms);
 	}
 
 	spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
@@ -729,8 +731,9 @@ static void ufshpb_read_to_handler(struct work_struct *work)
 
 	clear_bit(TIMEOUT_WORK_PENDING, &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,
@@ -740,8 +743,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;
 	}
 }
 
@@ -765,7 +771,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_THRSHLD >> 1))
+		if (hpb->is_hcm &&
+		    rgn->reads > hpb->params.eviction_thld_exit)
 			continue;
 
 		victim_rgn = rgn;
@@ -979,7 +986,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_THRSHLD) {
+			if (hpb->is_hcm &&
+			    rgn->reads < hpb->params.eviction_thld_enter) {
 				ret = -EACCES;
 				goto out;
 			}
@@ -1306,8 +1314,10 @@ static void ufshpb_normalization_work_handler(struct work_struct *work)
 {
 	struct ufshpb_lu *hpb;
 	int rgn_idx;
+	u8 factor;
 
 	hpb = container_of(work, struct ufshpb_lu, ufshpb_normalization_work);
+	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;
@@ -1316,7 +1326,7 @@ static void ufshpb_normalization_work_handler(struct work_struct *work)
 			unsigned long flags;
 
 			spin_lock_irqsave(&rgn->rgn_lock, flags);
-			rgn->reads = (rgn->reads >> 1);
+			rgn->reads = (rgn->reads >> factor);
 			spin_unlock_irqrestore(&rgn->rgn_lock, flags);
 		}
 
@@ -1546,6 +1556,238 @@ static void ufshpb_destroy_region_tbl(struct ufshpb_lu *hpb)
 }
 
 /* SYSFS functions */
+#define ufshpb_sysfs_param_show_func(__name)				\
+static ssize_t __name##_show(struct device *dev,			\
+	struct device_attribute *attr, char *buf)			\
+{									\
+	struct scsi_device *sdev = to_scsi_device(dev);			\
+	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);		\
+	if (!hpb)							\
+		return -ENODEV;						\
+	if (!hpb->is_hcm)						\
+		return -EOPNOTSUPP;					\
+									\
+	return sysfs_emit(buf, "%d\n", hpb->params.__name);		\
+}
+
+
+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;
+
+	if (val <= 0)
+		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;
+
+	if (val <= 0)
+		return -EINVAL;
+
+	hpb->params.timeout_polling_interval_ms = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(timeout_polling_interval_ms);
+
+static struct attribute *hpb_dev_param_attrs[] = {
+	&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,
+	NULL,
+};
+
+struct attribute_group ufs_sysfs_hpb_param_group = {
+	.name = "hpb_param_sysfs",
+	.attrs = hpb_dev_param_attrs,
+};
+
+static void ufshpb_param_init(struct ufshpb_lu *hpb)
+{
+	hpb->params.activation_thld = ACTIVATION_THRSHLD;
+	hpb->params.normalization_factor = 1;
+	hpb->params.eviction_thld_enter = (ACTIVATION_THRSHLD << 6);
+	hpb->params.eviction_thld_exit = (ACTIVATION_THRSHLD << 5);
+	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;
+}
+
 #define ufshpb_sysfs_attr_show_func(__name)				\
 static ssize_t __name##_show(struct device *dev,			\
 	struct device_attribute *attr, char *buf)			\
@@ -1568,7 +1810,7 @@ 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_attrs[] = {
+static struct attribute *hpb_dev_stat_attrs[] = {
 	&dev_attr_hit_cnt.attr,
 	&dev_attr_miss_cnt.attr,
 	&dev_attr_rb_noti_cnt.attr,
@@ -1580,8 +1822,8 @@ static struct attribute *hpb_dev_attrs[] = {
 };
 
 struct attribute_group ufs_sysfs_hpb_stat_group = {
-	.name = "hpb_sysfs",
-	.attrs = hpb_dev_attrs,
+	.name = "hpb_stat_sysfs",
+	.attrs = hpb_dev_stat_attrs,
 };
 
 static void ufshpb_stat_init(struct ufshpb_lu *hpb)
@@ -1641,9 +1883,14 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 
 	ufshpb_stat_init(hpb);
 
-	if (hpb->is_hcm)
+	if (hpb->is_hcm) {
+		unsigned int poll;
+
+		ufshpb_param_init(hpb);
+		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;
 
@@ -1818,10 +2065,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 207925cf1f44..fafc64943c53 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -160,6 +160,26 @@ struct victim_select_info {
 	atomic_t active_cnt;
 };
 
+/**
+ * ufshpb_params - parameters for host control logic
+ * @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
+ */
+struct ufshpb_params {
+	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;
+};
+
 struct ufshpb_stats {
 	u64 hit_cnt;
 	u64 miss_cnt;
@@ -212,6 +232,7 @@ struct ufshpb_lu {
 	bool is_hcm;
 
 	struct ufshpb_stats stats;
+	struct ufshpb_params params;
 
 	struct kmem_cache *map_req_cache;
 	struct kmem_cache *m_page_cache;
@@ -251,6 +272,7 @@ bool ufshpb_is_allowed(struct ufs_hba *hba);
 void ufshpb_get_geo_info(struct ufs_hba *hba, u8 *geo_buf);
 void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf);
 extern struct attribute_group ufs_sysfs_hpb_stat_group;
+extern struct attribute_group ufs_sysfs_hpb_param_group;
 #endif
 
 #endif /* End of Header */
-- 
2.25.1

Re: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable

[Greg KH]

On Tue, Feb 02, 2021 at 10:30:07AM +0200, Avri Altman wrote:
> 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:
> 
>  - activation_thld - 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.
> 
> - normalization_factor - 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.
> 
> - eviction_thld_enter - 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.
> 
> - eviction_thld_exit - 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.
> 
>  - read_timeout_ms - 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.
> 
> - read_timeout_expiries - 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.
> 
> - timeout_polling_interval_ms - the frequency in which the delayed
>     worker that checks the read_timeouts is awaken.

You create new sysfs files, but fail to document them in
Documentation/ABI/ which is where the above information needs to go :(

thanks,

greg k-h

RE: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable

[Avri Altman]

> >
> > - timeout_polling_interval_ms - the frequency in which the delayed
> >     worker that checks the read_timeouts is awaken.
> 
> You create new sysfs files, but fail to document them in
> Documentation/ABI/ which is where the above information needs to go :(
Done.
Will wait to see where Daejun chooses to document the stats entries, and follow.

RE: RE: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable

[Daejun Park]

> > > - timeout_polling_interval_ms - the frequency in which the delayed
> > >     worker that checks the read_timeouts is awaken.
> > 
> > You create new sysfs files, but fail to document them in
> > Documentation/ABI/ which is where the above information needs to go :(
> Done.
> Will wait to see where Daejun chooses to document the stats entries, and follow.

I added all sysfs entries about UFS-specific descriptors but not about HPB
related things. I will add HPB related sysfs entries in the 
Documentation/ABI/testing/sysfs-driver-ufs file in the next patch.

Thanks,
Daejun
 
 

Re: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable

[Greg KH]

On Tue, Feb 02, 2021 at 10:30:07AM +0200, Avri Altman wrote:
> +struct attribute_group ufs_sysfs_hpb_param_group = {
> +	.name = "hpb_param_sysfs",

Shouldn't this be "hpb_param"?  Why the trailing "_sysfs", doesn't that
look odd in the directory path?

thanks,

greg k-h

RE: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable

[Avri Altman]

> 
> On Tue, Feb 02, 2021 at 10:30:07AM +0200, Avri Altman wrote:
> > +struct attribute_group ufs_sysfs_hpb_param_group = {
> > +     .name = "hpb_param_sysfs",
> 
> Shouldn't this be "hpb_param"?  Why the trailing "_sysfs", doesn't that
> look odd in the directory path?
Done.

Re: [PATCH v2 0/9] Add Host control mode to HPB

[Bean Huo]

On Tue, 2021-02-02 at 10:29 +0200, Avri Altman wrote:
> 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.

Hi Avri
In addition to the above advantage of HPB device mode, would you please
share the performance comparison data with host mode? that will draw
more attention, since you mentioned "you tested on Galaxy S20, and
Xiaomi Mi10 pro", I think you have this kind of data.

Your HPB host driver sits in the ufs driver. Since my HPB host mode
driver is also implemented in the UFS driver layer, I did test my HPB
driver between device mode and host mode. Saw there is an improvement,
but not significant. If you can share your HPB drviver data, that will
be awesome.

Kind regards,
Bean

Re: [PATCH v2 0/9] Add Host control mode to HPB

[Bean Huo]

On Fri, 2021-02-05 at 12:36 +0100, Bean Huo wrote:
> > 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.
> 
> Hi Avri
> In addition to the above advantage of HPB device mode, would you
> please
> share the performance comparison data with host mode? that will draw
> more attention, since you mentioned "you tested on Galaxy S20, and
> Xiaomi Mi10 pro", I think you have this kind of data.
> 
> Your HPB host driver sits in the ufs driver. Since my HPB host mode
> driver is also implemented in the UFS driver layer, I did test my HPB
> driver between device mode and host mode. Saw there is an
> improvement,
> but not significant. If you can share your HPB drviver data, that
> will
> be awesome.
> 
> Kind regards,
> Bean

So sorry, there are several typos, easily confused you. correct them as
below:

Hi Avri

In addition to the above advantage of HPB "host control mode", would
you please share the performance comparison data with "device control
mode"? that will draw more attention, since you mentioned "you tested
on Galaxy S20, and Xiaomi Mi10 pro", I think you have this kind of
data.

Your HPB "host control mode" driver sits in the ufs driver. Since my
HPB "host control mode" driver is also implemented in the UFS driver
layer, I did test my HPB driver between device mode and host mode. Saw
UFS HPB "host control mode" driver has performance gain comparing to
HPB "device control mode" driver, but not significant. If you can share
your HPB host control mode drviver data, that will be awesome.

Kind regards,
Bean

RE: [PATCH v2 0/9] Add Host control mode to HPB

[Avri Altman]

Hi Bean,
 
> In addition to the above advantage of HPB "host control mode", would
> you please share the performance comparison data with "device control
> mode"? that will draw more attention, since you mentioned "you tested
> on Galaxy S20, and Xiaomi Mi10 pro", I think you have this kind of
> data.
> 
> Your HPB "host control mode" driver sits in the ufs driver. Since my
> HPB "host control mode" driver is also implemented in the UFS driver
> layer, I did test my HPB driver between device mode and host mode. Saw
> UFS HPB "host control mode" driver has performance gain comparing to
> HPB "device control mode" driver, but not significant. If you can share
> your HPB host control mode drviver data, that will be awesome.
Host control mode wasn't standardized to overcome device mode performance.
Instead, host control mode, entails several advantages comparing to device control mode:
- Allow OEM vendors to have a unified HPB approach, with minor to null performance volatility across different storage vendors
- Flexible and transparent logic which doesn’t requires device firmware changes.
Hence, the discussion of host-control vs device control performance gain is chasing the red herring.

Thanks,
Avri