[RFC PATCH 0/5] scsi: ufs: Add Host Performance Booster Support

[RFC PATCH 0/5] scsi: ufs: Add Host Performance Booster Support

[Daejun Park]

NAND flash memory-based storage devices use Flash Translation Layer (FTL)
to translate logical addresses of I/O requests to corresponding flash
memory addresses. Mobile storage devices typically have RAM with
constrained size, thus lack in memory to keep the whole mapping table.
Therefore, mapping tables are partially retrieved from NAND flash on
demand, causing random-read performance degradation.

To improve random read performance, we propose HPB (Host Performance
Booster) which uses host system memory as a cache for the FTL mapping
table. By using HPB, FTL data can be read from host memory faster than from
NAND flash memory. 

The current version only supports the DCM (device control mode).
This patch consists of 4 parts to support HPB feature.

1) UFS-feature layer
2) HPB probe and initialization process
3) READ -> HPB READ using cached map information
4) L2P (logical to physical) map management

The UFS-feature is an additional layer to avoid the structure in which the
UFS-core driver and the UFS-feature are entangled with each other in a 
single module.
By adding the layer, UFS-features composed of various combinations can be
supported. Also, even if a new feature is added, modification of the 
UFS-core driver can be minimized.

In the HPB probe and init process, the device information of the UFS is
queried. After checking supported features, the data structure for the HPB
is initialized according to the device information.

A read I/O in the active sub-region where the map is cached is changed to
HPB READ by the HPB module.

The HPB module manages the L2P map using information received from the
device. For active sub-region, the HPB module caches through ufshpb_map
request. For the in-active region, the HPB module discards the L2P map.
When a write I/O occurs in an active sub-region area, associated dirty
bitmap checked as dirty for preventing stale read.

HPB is shown to have a performance improvement of 58 - 67% for random read
workload. [1]

This series patches are based on the "5.8/scsi-queue" branch.

[1]:
https://www.usenix.org/conference/hotstorage17/program/presentation/jeong

Daejun park (5):
 scsi: ufs: Add UFS feature related parameter
 scsi: ufs: Add UFS feature layer
 scsi: ufs: Introduce HPB module
 scsi: ufs: L2P map management for HPB read
 scsi: ufs: Prepare HPB read for cached sub-region
 
 drivers/scsi/ufs/Kconfig      |    8 +
 drivers/scsi/ufs/Makefile     |    3 +-
 drivers/scsi/ufs/ufs.h        |   11 +
 drivers/scsi/ufs/ufsfeature.c |  178 ++++
 drivers/scsi/ufs/ufsfeature.h |   95 ++
 drivers/scsi/ufs/ufshcd.c     |   19 +
 drivers/scsi/ufs/ufshcd.h     |    3 +
 drivers/scsi/ufs/ufshpb.c     | 2029 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshpb.h     |  257 ++++++
 9 files changed, 2602 insertions(+), 1 deletion(-)
 created mode 100644 drivers/scsi/ufs/ufsfeature.c
 created mode 100644 drivers/scsi/ufs/ufsfeature.h
 created mode 100644 drivers/scsi/ufs/ufshpb.c
 created mode 100644 drivers/scsi/ufs/ufshpb.h

[RFC PATCH 1/5] scsi: ufs: Add UFS feature related parameter

[Daejun Park]

This is a patch for parameters to be used for UFS features layer and HPB
module.

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/scsi/ufs/ufs.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index c70845d41449..4a4cb790e34c 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -146,6 +146,7 @@ enum flag_idn {
 	QUERY_FLAG_IDN_WB_EN                            = 0x0E,
 	QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN                 = 0x0F,
 	QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8     = 0x10,
+	QUERY_FLAG_IDN_HPB_RESET                        = 0x11,
 };
 
 /* Attribute idn for Query requests */
@@ -229,6 +230,9 @@ enum unit_desc_param {
 	UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT	= 0x18,
 	UNIT_DESC_PARAM_CTX_CAPABILITIES	= 0x20,
 	UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1	= 0x22,
+	UNIT_DESC_HPB_LU_MAX_ACTIVE_REGIONS	= 0x23,
+	UNIT_DESC_HPB_LU_PIN_REGION_START_OFFSET	= 0x25,
+	UNIT_DESC_HPB_LU_NUM_PIN_REGIONS	= 0x27,
 	UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS	= 0x29,
 };
 
@@ -269,6 +273,8 @@ enum device_desc_param {
 	DEVICE_DESC_PARAM_PSA_MAX_DATA		= 0x25,
 	DEVICE_DESC_PARAM_PSA_TMT		= 0x29,
 	DEVICE_DESC_PARAM_PRDCT_REV		= 0x2A,
+	DEVICE_DESC_PARAM_HPB_VER		= 0x40,
+	DEVICE_DESC_PARAM_HPB_CONTROL		= 0x42,
 	DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP	= 0x4F,
 	DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN	= 0x53,
 	DEVICE_DESC_PARAM_WB_TYPE		= 0x54,
@@ -317,6 +323,10 @@ enum geometry_desc_param {
 	GEOMETRY_DESC_PARAM_ENM4_MAX_NUM_UNITS	= 0x3E,
 	GEOMETRY_DESC_PARAM_ENM4_CAP_ADJ_FCTR	= 0x42,
 	GEOMETRY_DESC_PARAM_OPT_LOG_BLK_SIZE	= 0x44,
+	GEOMETRY_DESC_HPB_REGION_SIZE		= 0x48,
+	GEOMETRY_DESC_HPB_NUMBER_LU		= 0x49,
+	GEOMETRY_DESC_HPB_SUBREGION_SIZE	= 0x4A,
+	GEOMETRY_DESC_HPB_DEVICE_MAX_ACTIVE_REGIONS	= 0x4B,
 	GEOMETRY_DESC_PARAM_WB_MAX_ALLOC_UNITS	= 0x4F,
 	GEOMETRY_DESC_PARAM_WB_MAX_WB_LUNS	= 0x53,
 	GEOMETRY_DESC_PARAM_WB_BUFF_CAP_ADJ	= 0x54,
@@ -571,6 +581,7 @@ struct ufs_dev_info {
 	u8 *model;
 	u16 wspecversion;
 	u32 clk_gating_wait_us;
+	u8 b_ufs_feature_sup;
 	u32 d_ext_ufs_feature_sup;
 	u8 b_wb_buffer_type;
 	u32 d_wb_alloc_units;
-- 
2.17.1

RE: [RFC PATCH 1/5] scsi: ufs: Add UFS feature related parameter

[Avri Altman]

OMETRY_DESC_PARAM_ENM4_CAP_ADJ_FCTR   = 0x42,
>         GEOMETRY_DESC_PARAM_OPT_LOG_BLK_SIZE    = 0x44,
> +       GEOMETRY_DESC_HPB_REGION_SIZE           = 0x48,
> +       GEOMETRY_DESC_HPB_NUMBER_LU             = 0x49,
> +       GEOMETRY_DESC_HPB_SUBREGION_SIZE        = 0x4A,
> +       GEOMETRY_DESC_HPB_DEVICE_MAX_ACTIVE_REGIONS     = 0x4B,
>         GEOMETRY_DESC_PARAM_WB_MAX_ALLOC_UNITS  = 0x4F,
>         GEOMETRY_DESC_PARAM_WB_MAX_WB_LUNS      = 0x53,
>         GEOMETRY_DESC_PARAM_WB_BUFF_CAP_ADJ     = 0x54,

Maybe also add bit7 to the enum of dExtendedUFSFeaturesSupport ?

> @@ -571,6 +581,7 @@ struct ufs_dev_info {
>         u8 *model;
>         u16 wspecversion;
>         u32 clk_gating_wait_us;
> +       u8 b_ufs_feature_sup;
>         u32 d_ext_ufs_feature_sup;
>         u8 b_wb_buffer_type;
>         u32 d_wb_alloc_units;
> --
> 2.17.1

RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Daejun Park]

> The spec does not define what the host should do in this case,
> e.g. when the device informs it that the entire db is no longer valid.
> What are you planning to do?
In Jedec spec, there is no decription about what the driver should do.
So, I will just inform to user about the "HPB reset" happening with kernel message.

Thanks,
Daejun

Re: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Daejun Park]

> > +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);
> > +	if (!map_req)
> > +		return NULL;
> > +
> > +	req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
> > +			      REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
> > +	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;
> > +	}
> > +
> > +	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;
> > +	map_req->lun = hpb->lun;
> > +
> > +	return map_req;
> > +free_map_req:
> > +	kmem_cache_free(hpb->map_req_cache, map_req);
> > +	return NULL;
> > +}

> Will blk_get_request() fail if all tags have been allocated? Can that
> cause a deadlock or infinite loop?
If the worker fails to receive the tag, it stops and exits. The remained
lists are processed again at the next work. Therefore, no deadlock or
infinite loop occurs.

> > +static inline void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
> > +					   int srgn_idx, int srgn_mem_size)
> > +{
> > +	cdb[0] = UFSHPB_READ_BUFFER;
> > +	cdb[1] = UFSHPB_READ_BUFFER_ID;
> > +
> > +	put_unaligned_be32(srgn_mem_size, &cdb[5]);
> > +	/* cdb[5] = 0x00; */
> > +	put_unaligned_be16(rgn_idx, &cdb[2]);
> > +	put_unaligned_be16(srgn_idx, &cdb[4]);
> > +
> > +	cdb[9] = 0x00;
> > +}

> So the put_unaligned_be32(srgn_mem_size, &cdb[5]) comes first because
> the put_unaligned_be16(srgn_idx, &cdb[4]) overwrites byte cdb[5]? That
> is really ugly. Please use put_unaligned_be24() instead if that is what
> you meant and keep the put_*() calls in increasing cdb offset order.
OK, I will.

> > +static int ufshpb_map_req_add_bio_page(struct ufshpb_lu *hpb,
> > +				       struct request_queue *q, struct bio *bio,
> > +				       struct ufshpb_map_ctx *mctx)
> > +{
> > +	int i, ret = 0;
> > +
> > +	for (i = 0; i < hpb->pages_per_srgn; i++) {
> > +		ret = bio_add_pc_page(q, bio, mctx->m_page[i], PAGE_SIZE, 0);
> > +		if (ret != PAGE_SIZE) {
> > +			dev_notice(&hpb->hpb_lu_dev,
> > +				   "bio_add_pc_page fail %d\n", ret);
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
	
> Why bio_add_pc_page() instead of bio_add_page()?
Since this map request is created under the block layer and it is a
passthrough command, I think bio_add_pc_page is a more suitable API than
bio_add_page. If bio_add_page is used in scsi LLD, the checking codes that
examine the max segment size in the block layer is not performed.

> > +static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
> > +				  struct ufshpb_req *map_req)
> > +{
> > +	struct request_queue *q;
> > +	struct request *req;
> > +	struct scsi_request *rq;
> > +	int ret = 0;
> > +
> > +	q = hpb->sdev_ufs_lu->request_queue;
> > +	ret = ufshpb_map_req_add_bio_page(hpb, q, map_req->bio,
> > +					  map_req->mctx);
> > +	if (ret) {
> > +		dev_notice(&hpb->hpb_lu_dev,
> > +			   "map_req_add_bio_page fail %d - %d\n",
> > +			   map_req->rgn_idx, map_req->srgn_idx);
> > +		return ret;
> > +	}
> > +
> > +	req = map_req->req;
> > +
> > +	blk_rq_append_bio(req, &map_req->bio);
> > +	req->rq_flags |= RQF_QUIET;
> > +	req->timeout = MAP_REQ_TIMEOUT;
> > +	req->end_io_data = (void *)map_req;
> > +
> > +	rq = scsi_req(req);
> > +	ufshpb_set_read_buf_cmd(rq->cmd, map_req->rgn_idx,
> > +				map_req->srgn_idx, hpb->srgn_mem_size);
> > +	rq->cmd_len = HPB_READ_BUFFER_CMD_LENGTH;
> > +
> > +	blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_map_req_compl_fn);
> > +
> > +	atomic_inc(&hpb->stats.map_req_cnt);
> > +	return 0;
> > +}

> Why RQF_QUIET?
I refered scsi execute function. I will delete the needless flag.

> Why a custom timeout instead of the SCSI LUN timeout?
There was no suitable timeout value to use. I've included sd.h, so I'll
use sd_timeout.

> Can this function be made asynchronous such that it does not have to be
> executed on the context of a workqueue?
If this code doesn't work in your workq, map related task is handled in
interrupt context. Using workq, it avoids frequent active/inactive requests
to UFS devices by batched manner.

Thanks,

Daejun.

Re: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Bart Van Assche]

On 2020-06-11 20:37, Daejun Park wrote:
>>> +static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
>>> +				  struct ufshpb_req *map_req)
>>> +{
>>> +	struct request_queue *q;
>>> +	struct request *req;
>>> +	struct scsi_request *rq;
>>> +	int ret = 0;
>>> +
>>> +	q = hpb->sdev_ufs_lu->request_queue;
>>> +	ret = ufshpb_map_req_add_bio_page(hpb, q, map_req->bio,
>>> +					  map_req->mctx);
>>> +	if (ret) {
>>> +		dev_notice(&hpb->hpb_lu_dev,
>>> +			   "map_req_add_bio_page fail %d - %d\n",
>>> +			   map_req->rgn_idx, map_req->srgn_idx);
>>> +		return ret;
>>> +	}
>>> +
>>> +	req = map_req->req;
>>> +
>>> +	blk_rq_append_bio(req, &map_req->bio);
>>> +	req->rq_flags |= RQF_QUIET;
>>> +	req->timeout = MAP_REQ_TIMEOUT;
>>> +	req->end_io_data = (void *)map_req;
>>> +
>>> +	rq = scsi_req(req);
>>> +	ufshpb_set_read_buf_cmd(rq->cmd, map_req->rgn_idx,
>>> +				map_req->srgn_idx, hpb->srgn_mem_size);
>>> +	rq->cmd_len = HPB_READ_BUFFER_CMD_LENGTH;
>>> +
>>> +	blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_map_req_compl_fn);
>>> +
>>> +	atomic_inc(&hpb->stats.map_req_cnt);
>>> +	return 0;
>>> +}
>> 
>> Why a custom timeout instead of the SCSI LUN timeout?
>
> There was no suitable timeout value to use. I've included sd.h, so I'll
> use sd_timeout.

Wouldn't that be a layering violation? The UFS driver is a SCSI LLD
driver and the sd driver is a SCSI ULD. A SCSI LLD must not make any
assumptions about which ULD driver has been attached.

How about leaving req->timeout zero such that blk_add_timer() sets it?
blk_add_timer() is called by blk_mq_start_request(). From blk_add_timer():

	if (!req->timeout)
		req->timeout = q->rq_timeout;

Thanks,

Bart.

[RFC PATCH 2/5] scsi: ufs: Add UFS-feature layer

[Daejun Park]

This patch is adding UFS feature layer to UFS core driver.

UFS Driver data structure (struct ufs_hba)
	│
┌--------------┐
│ UFS feature  │ <-- HPB module
│    layer     │ <-- other extended feature module
└--------------┘
Each extended UFS-Feature module has a bus of ufs-ext feature type.
The UFS feature layer manages common APIs used by each extended feature
module. The APIs are set of UFS Query requests and UFS Vendor commands
related to each extended feature module.

The following 6 callback functions have been added to "ufshcd.c".
prep_fn: called after construct upiu structure
reset: called after proving hba
reset_host: called before ufshcd_host_reset_and_restore
suspend: called before ufshcd_suspend
resume: called after ufshcd_resume
rsp_upiu: called in ufshcd_transfer_rsp_status with SAM_STAT_GOOD state

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/scsi/ufs/Makefile     |   2 +-
 drivers/scsi/ufs/ufsfeature.c | 178 ++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufsfeature.h |  95 ++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c     |  17 ++++
 drivers/scsi/ufs/ufshcd.h     |   3 +
 5 files changed, 294 insertions(+), 1 deletion(-)
 create mode 100644 drivers/scsi/ufs/ufsfeature.c
 create mode 100644 drivers/scsi/ufs/ufsfeature.h

diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 94c6c5d7334b..fe3a92b06c87 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -5,7 +5,7 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
 obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
-ufshcd-core-y				+= ufshcd.o ufs-sysfs.o
+ufshcd-core-y				+= ufshcd.o ufs-sysfs.o ufsfeature.o
 ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
 obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
diff --git a/drivers/scsi/ufs/ufsfeature.c b/drivers/scsi/ufs/ufsfeature.c
new file mode 100644
index 000000000000..a6671962fad2
--- /dev/null
+++ b/drivers/scsi/ufs/ufsfeature.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Universal Flash Storage Feature Support
+ *
+ * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *	Yongmyung Lee <ymhungry.lee@samsung.com>
+ *	Jinyoung Choi <j-young.choi@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * See the COPYING file in the top-level directory or visit
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This program is provided "AS IS" and "WITH ALL FAULTS" and
+ * without warranty of any kind. You are solely responsible for
+ * determining the appropriateness of using and distributing
+ * the program and assume all risks associated with your exercise
+ * of rights with respect to the program, including but not limited
+ * to infringement of third party rights, the risks and costs of
+ * program errors, damage to or loss of data, programs or equipment,
+ * and unavailability or interruption of operations. Under no
+ * circumstances will the contributor of this Program be liable for
+ * any damages of any kind arising from your use or distribution of
+ * this program.
+ *
+ * The Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
+ */
+
+#include "ufshcd.h"
+#include "ufsfeature.h"
+
+inline void ufsf_slave_configure(struct ufs_hba *hba,
+				 struct scsi_device *sdev)
+{
+	/* skip well-known LU */
+	if (sdev->lun >= UFS_UPIU_MAX_UNIT_NUM_ID)
+		return;
+
+	if (!(hba->dev_info.b_ufs_feature_sup & UFS_FEATURE_SUPPORT_HPB_BIT))
+		return;
+
+	atomic_inc(&hba->ufsf.slave_conf_cnt);
+	smp_mb__after_atomic(); /* for slave_conf_cnt */
+
+	/* waiting sdev init.*/
+	if (waitqueue_active(&hba->ufsf.sdev_wait))
+		wake_up(&hba->ufsf.sdev_wait);
+}
+
+inline void ufsf_ops_prep_fn(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+	struct ufshpb_driver *ufshpb_drv;
+
+	ufshpb_drv = dev_get_drvdata(&hba->ufsf.hpb_dev);
+
+	if (ufshpb_drv && ufshpb_drv->ufshpb_ops.prep_fn)
+		ufshpb_drv->ufshpb_ops.prep_fn(hba, lrbp);
+}
+
+inline void ufsf_ops_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+	struct ufshpb_driver *ufshpb_drv;
+
+	ufshpb_drv = dev_get_drvdata(&hba->ufsf.hpb_dev);
+
+	if (ufshpb_drv && ufshpb_drv->ufshpb_ops.rsp_upiu)
+		ufshpb_drv->ufshpb_ops.rsp_upiu(hba, lrbp);
+}
+
+inline void ufsf_ops_reset_host(struct ufs_hba *hba)
+{
+	struct ufshpb_driver *ufshpb_drv;
+
+	ufshpb_drv = dev_get_drvdata(&hba->ufsf.hpb_dev);
+
+	if (ufshpb_drv && ufshpb_drv->ufshpb_ops.reset_host)
+		ufshpb_drv->ufshpb_ops.reset_host(hba);
+}
+
+inline void ufsf_ops_reset(struct ufs_hba *hba)
+{
+	struct ufshpb_driver *ufshpb_drv;
+
+	ufshpb_drv = dev_get_drvdata(&hba->ufsf.hpb_dev);
+
+	if (ufshpb_drv && ufshpb_drv->ufshpb_ops.reset)
+		ufshpb_drv->ufshpb_ops.reset(hba);
+}
+
+inline void ufsf_ops_suspend(struct ufs_hba *hba)
+{
+	struct ufshpb_driver *ufshpb_drv;
+
+	ufshpb_drv = dev_get_drvdata(&hba->ufsf.hpb_dev);
+
+	if (ufshpb_drv && ufshpb_drv->ufshpb_ops.suspend)
+		ufshpb_drv->ufshpb_ops.suspend(hba);
+}
+
+inline void ufsf_ops_resume(struct ufs_hba *hba)
+{
+	struct ufshpb_driver *ufshpb_drv;
+
+	ufshpb_drv = dev_get_drvdata(&hba->ufsf.hpb_dev);
+
+	if (ufshpb_drv && ufshpb_drv->ufshpb_ops.resume)
+		ufshpb_drv->ufshpb_ops.resume(hba);
+}
+
+struct device_type ufshpb_dev_type = {
+	.name = "ufshpb_device"
+};
+EXPORT_SYMBOL(ufshpb_dev_type);
+
+static int ufsf_bus_match(struct device *dev,
+			 struct device_driver *gendrv)
+{
+	if (dev->type == &ufshpb_dev_type)
+		return 1;
+
+	return 0;
+}
+
+struct bus_type ufsf_bus_type = {
+	.name = "ufsf_bus",
+	.match = ufsf_bus_match,
+};
+EXPORT_SYMBOL(ufsf_bus_type);
+
+static void ufsf_dev_release(struct device *dev)
+{
+	put_device(dev->parent);
+}
+
+void ufsf_scan_features(struct ufs_hba *hba)
+{
+	int ret;
+
+	init_waitqueue_head(&hba->ufsf.sdev_wait);
+	atomic_set(&hba->ufsf.slave_conf_cnt, 0);
+
+	if (hba->dev_info.wspecversion >= HPB_SUPPORTED_VERSION &&
+	    (hba->dev_info.b_ufs_feature_sup & UFS_FEATURE_SUPPORT_HPB_BIT)) {
+		device_initialize(&hba->ufsf.hpb_dev);
+
+		hba->ufsf.hpb_dev.bus = &ufsf_bus_type;
+		hba->ufsf.hpb_dev.type = &ufshpb_dev_type;
+		hba->ufsf.hpb_dev.parent = get_device(hba->dev);
+		hba->ufsf.hpb_dev.release = ufsf_dev_release;
+
+		dev_set_name(&hba->ufsf.hpb_dev, "ufshpb");
+		ret = device_add(&hba->ufsf.hpb_dev);
+		if (ret)
+			dev_warn(hba->dev, "ufshpb: failed to add device\n");
+	}
+}
+
+static int __init ufsf_init(void)
+{
+	int ret;
+
+	ret = bus_register(&ufsf_bus_type);
+	if (ret)
+		pr_err("%s bus_register failed\n", __func__);
+
+	return ret;
+}
+device_initcall(ufsf_init);
diff --git a/drivers/scsi/ufs/ufsfeature.h b/drivers/scsi/ufs/ufsfeature.h
new file mode 100644
index 000000000000..cbac848ec6c6
--- /dev/null
+++ b/drivers/scsi/ufs/ufsfeature.h
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Universal Flash Storage Feature Support
+ *
+ * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *	Yongmyung Lee <ymhungry.lee@samsung.com>
+ *	Jinyoung Choi <j-young.choi@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * See the COPYING file in the top-level directory or visit
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This program is provided "AS IS" and "WITH ALL FAULTS" and
+ * without warranty of any kind. You are solely responsible for
+ * determining the appropriateness of using and distributing
+ * the program and assume all risks associated with your exercise
+ * of rights with respect to the program, including but not limited
+ * to infringement of third party rights, the risks and costs of
+ * program errors, damage to or loss of data, programs or equipment,
+ * and unavailability or interruption of operations. Under no
+ * circumstances will the contributor of this Program be liable for
+ * any damages of any kind arising from your use or distribution of
+ * this program.
+ *
+ * The Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
+ */
+
+#ifndef _UFSFEATURE_H_
+#define _UFSFEATURE_H_
+
+#define HPB_SUPPORTED_VERSION			0x0310
+#define UFS_FEATURE_SUPPORT_HPB_BIT		0x80
+
+struct ufs_hba;
+struct ufshcd_lrb;
+
+/**
+ * struct ufsf_operation - UFS feature specific callbacks
+ * @prep_fn: called after construct upiu structure
+ * @reset: called after proving hba
+ * @reset_host: called before ufshcd_host_reset_and_restore
+ * @suspend: called before ufshcd_suspend
+ * @resume: called after ufshcd_resume
+ * @rsp_upiu: called in ufshcd_transfer_rsp_status with SAM_STAT_GOOD state
+ */
+struct ufsf_operation {
+	void (*prep_fn)(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
+	void (*reset)(struct ufs_hba *hba);
+	void (*reset_host)(struct ufs_hba *hba);
+	void (*suspend)(struct ufs_hba *hba);
+	void (*resume)(struct ufs_hba *hba);
+	void (*rsp_upiu)(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
+};
+
+struct ufshpb_driver {
+	struct device_driver drv;
+	struct list_head lh_hpb_lu;
+
+	struct ufsf_operation ufshpb_ops;
+
+	/* memory management */
+	struct kmem_cache *ufshpb_mctx_cache;
+	mempool_t *ufshpb_mctx_pool;
+	mempool_t *ufshpb_page_pool;
+
+	struct workqueue_struct *ufshpb_wq;
+};
+
+struct ufsf_feature_info {
+	atomic_t slave_conf_cnt;
+	wait_queue_head_t sdev_wait;
+	struct device hpb_dev;
+};
+
+void ufsf_slave_configure(struct ufs_hba *hba, struct scsi_device *sdev);
+void ufsf_scan_features(struct ufs_hba *hba);
+void ufsf_ops_prep_fn(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
+void ufsf_ops_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
+void ufsf_ops_reset_host(struct ufs_hba *hba);
+void ufsf_ops_reset(struct ufs_hba *hba);
+void ufsf_ops_suspend(struct ufs_hba *hba);
+void ufsf_ops_resume(struct ufs_hba *hba);
+
+#endif /* End of Header */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5db18f444ea9..de57ba2a0b03 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2525,6 +2525,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	ufshcd_comp_scsi_upiu(hba, lrbp);
 
+	ufsf_ops_prep_fn(hba, lrbp);
+
 	err = ufshcd_map_sg(hba, lrbp);
 	if (err) {
 		lrbp->cmd = NULL;
@@ -4645,6 +4647,8 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	struct ufs_hba *hba = shost_priv(sdev->host);
 	struct request_queue *q = sdev->request_queue;
 
+	ufsf_slave_configure(hba, sdev);
+
 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
 
 	if (ufshcd_is_rpm_autosuspend_allowed(hba))
@@ -4765,6 +4769,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 				 */
 				pm_runtime_get_noresume(hba->dev);
 			}
+
+			if (scsi_status == SAM_STAT_GOOD)
+				ufsf_ops_rsp_upiu(hba, lrbp);
 			break;
 		case UPIU_TRANSACTION_REJECT_UPIU:
 			/* TODO: handle Reject UPIU Response */
@@ -6508,6 +6515,8 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	 * Stop the host controller and complete the requests
 	 * cleared by h/w
 	 */
+	ufsf_ops_reset_host(hba);
+
 	ufshcd_hba_stop(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
@@ -6934,6 +6943,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	/* getting Specification Version in big endian format */
 	dev_info->wspecversion = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
 				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
+	dev_info->b_ufs_feature_sup = desc_buf[DEVICE_DESC_PARAM_UFS_FEAT];
 
 	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
 
@@ -7350,6 +7360,7 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 	}
 
 	ufs_bsg_probe(hba);
+	ufsf_scan_features(hba);
 	scsi_scan_host(hba->host);
 	pm_runtime_put_sync(hba->dev);
 
@@ -7442,6 +7453,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
 	/* Enable Auto-Hibernate if configured */
 	ufshcd_auto_hibern8_enable(hba);
 
+	ufsf_ops_reset(hba);
 out:
 
 	trace_ufshcd_init(dev_name(hba->dev), ret,
@@ -8199,6 +8211,8 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		req_link_state = UIC_LINK_OFF_STATE;
 	}
 
+	ufsf_ops_suspend(hba);
+
 	/*
 	 * If we can't transition into any of the low power modes
 	 * just gate the clocks.
@@ -8320,6 +8334,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	hba->clk_gating.is_suspended = false;
 	hba->dev_info.b_rpm_dev_flush_capable = false;
 	ufshcd_release(hba);
+	ufsf_ops_resume(hba);
 out:
 	if (hba->dev_info.b_rpm_dev_flush_capable) {
 		schedule_delayed_work(&hba->rpm_dev_flush_recheck_work,
@@ -8416,6 +8431,8 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	/* Enable Auto-Hibernate if configured */
 	ufshcd_auto_hibern8_enable(hba);
 
+	ufsf_ops_resume(hba);
+
 	if (hba->dev_info.b_rpm_dev_flush_capable) {
 		hba->dev_info.b_rpm_dev_flush_capable = false;
 		cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index bf97d616e597..47866ab722ff 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -71,6 +71,7 @@
 #include "ufs.h"
 #include "ufs_quirks.h"
 #include "ufshci.h"
+#include "ufsfeature.h"
 
 #define UFSHCD "ufshcd"
 #define UFSHCD_DRIVER_VERSION "0.2"
@@ -746,6 +747,8 @@ struct ufs_hba {
 	bool wb_buf_flush_enabled;
 	bool wb_enabled;
 	struct delayed_work rpm_dev_flush_recheck_work;
+
+	struct ufsf_feature_info ufsf;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
-- 
2.17.1

Re: [RFC PATCH 2/5] scsi: ufs: Add UFS-feature layer

[Bart Van Assche]

On 2020-06-04 18:30, Daejun Park wrote:
> +inline void ufsf_slave_configure(struct ufs_hba *hba,
> +				 struct scsi_device *sdev)
> +{
> +	/* skip well-known LU */
> +	if (sdev->lun >= UFS_UPIU_MAX_UNIT_NUM_ID)
> +		return;
> +
> +	if (!(hba->dev_info.b_ufs_feature_sup & UFS_FEATURE_SUPPORT_HPB_BIT))
> +		return;
> +
> +	atomic_inc(&hba->ufsf.slave_conf_cnt);
> +	smp_mb__after_atomic(); /* for slave_conf_cnt */
> +
> +	/* waiting sdev init.*/
> +	if (waitqueue_active(&hba->ufsf.sdev_wait))
> +		wake_up(&hba->ufsf.sdev_wait);
> +}

Guarding a wake_up() call with a waitqueue_active() check is an
anti-pattern. Please don't do that and call wake_up() directly.
Additionally, wake_up() includes a barrier if it wakes up a kernel
thread so the smp_mb__after_atomic() can be left out if the
waitqueue_active() call is removed.

> +/**
> + * struct ufsf_operation - UFS feature specific callbacks
> + * @prep_fn: called after construct upiu structure
> + * @reset: called after proving hba
                           ^^^^^^^
Is this a typo? Should "proving" perhaps be changed into "probing"?

> +struct ufshpb_driver {
> +	struct device_driver drv;
> +	struct list_head lh_hpb_lu;
> +
> +	struct ufsf_operation ufshpb_ops;
> +
> +	/* memory management */
> +	struct kmem_cache *ufshpb_mctx_cache;
> +	mempool_t *ufshpb_mctx_pool;
> +	mempool_t *ufshpb_page_pool;
> +
> +	struct workqueue_struct *ufshpb_wq;
> +};

Why is a dedicated workqueue needed? Why are the standard workqueues not
good enough?

> @@ -2525,6 +2525,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  
>  	ufshcd_comp_scsi_upiu(hba, lrbp);
>  
> +	ufsf_ops_prep_fn(hba, lrbp);
> +
>  	err = ufshcd_map_sg(hba, lrbp);
>  	if (err) {
>  		lrbp->cmd = NULL;

What happens if a SCSI command is retried and hence ufsf_ops_prep_fn()
is called multiple times for the same SCSI command?

Thanks,

Bart.

Re: [RFC PATCH 2/5] scsi: ufs: Add UFS-feature layer

[Bart Van Assche]

On 2020-06-04 18:30, Daejun Park wrote:
> This patch is adding UFS feature layer to UFS core driver.
> 
> UFS Driver data structure (struct ufs_hba)
> 	│
> ┌--------------┐
> │ UFS feature  │ <-- HPB module
> │    layer     │ <-- other extended feature module
> └--------------┘
> Each extended UFS-Feature module has a bus of ufs-ext feature type.
> The UFS feature layer manages common APIs used by each extended feature
> module. The APIs are set of UFS Query requests and UFS Vendor commands
> related to each extended feature module.

Personally I'm less than enthusiast that this new feature layer has been
implemented using the driver/bus model. But it seems like nobody else
objects against this model ...

Bart.

RE: [RFC PATCH 1/5] scsi: ufs: Add UFS feature related parameter

[Daejun Park]

> Maybe also add bit7 to the enum of dExtendedUFSFeaturesSupport ?
OK, I will.

Thanks,
Daejun.

RE: [RFC PATCH 0/5] scsi: ufs: Add Host Performance Booster Support

[Avri Altman]

Hi,
> 
> NAND flash memory-based storage devices use Flash Translation Layer (FTL)
> to translate logical addresses of I/O requests to corresponding flash
> memory addresses. Mobile storage devices typically have RAM with
> constrained size, thus lack in memory to keep the whole mapping table.
> Therefore, mapping tables are partially retrieved from NAND flash on
> demand, causing random-read performance degradation.
> 
> To improve random read performance, we propose HPB (Host Performance
we propose  --> jedec spec XXX proposes …
and here you also disclose what version of the spec are you supporting

> Booster) which uses host system memory as a cache for the FTL mapping
> table. By using HPB, FTL data can be read from host memory faster than from
> NAND flash memory.
> 
> The current version only supports the DCM (device control mode).
> This patch consists of 4 parts to support HPB feature.
> 
> 1) UFS-feature layer
> 2) HPB probe and initialization process
> 3) READ -> HPB READ using cached map information
> 4) L2P (logical to physical) map management
> 
> The UFS-feature is an additional layer to avoid the structure in which the
> UFS-core driver and the UFS-feature are entangled with each other in a
> single module.
> By adding the layer, UFS-features composed of various combinations can be
> supported. Also, even if a new feature is added, modification of the
> UFS-core driver can be minimized.
Like Bart, I am not sure that this extra module is needed.
It only makes sense if indeed there are some common calls that can be shared by several features.
There are up to now 10 extended features defined, but none of them can share a common api.
What other features can share this additional layer?  And how those ops can be reused?
If you have some future implementations in mind, you should add this api once you'll add those.

> 
> In the HPB probe and init process, the device information of the UFS is
> queried. After checking supported features, the data structure for the HPB
> is initialized according to the device information.
> 
> A read I/O in the active sub-region where the map is cached is changed to
> HPB READ by the HPB module.
> 
> The HPB module manages the L2P map using information received from the
> device. For active sub-region, the HPB module caches through ufshpb_map
> request. For the in-active region, the HPB module discards the L2P map.
> When a write I/O occurs in an active sub-region area, associated dirty
> bitmap checked as dirty for preventing stale read.
> 
> HPB is shown to have a performance improvement of 58 - 67% for random
> read
> workload. [1]
> 
> This series patches are based on the "5.8/scsi-queue" branch.
> 
> [1]:
> https://www.usenix.org/conference/hotstorage17/program/presentation/jeo
> ng
This 2017 study, is being cited by everyone, but does not really describes it's test setup to its details.
It  does say however that they used a 16MB subregions over a range of 1GB,
which can be covered by a 64 active regions, Even for a single subregion per region.
Meaning no eviction should take place, thus HPB overhead is minimized.
Do we have a more recent public studies that supports those impressive figures?

Thanks,
Avri

RE: RE: [RFC PATCH 0/5] scsi: ufs: Add Host Performance Booster Support

[Daejun Park]

Hi,

I appreciate your insightful comments.
  
> we propose  --> jedec spec XXX proposes …
> and here you also disclose what version of the spec are you supporting
I will change to "JESD220-3 (HPB v1.0) proposes".
This patch supports HPB version 1.0.

> Like Bart, I am not sure that this extra module is needed.
> It only makes sense if indeed there are some common calls that can be shared by several features.
> There are up to now 10 extended features defined, but none of them can share a common api.
> What other features can share this additional layer?  And how those ops can be reused?
> If you have some future implementations in mind, you should add this api once you'll add those.
We added UFS feature layer with several callbacks to important parts of the UFS control flow.
Other extended features can also be implemented using the proposed APIs.
For example, in WB, "prep_fn" can be used to guarantee the lifetime of UFS by updating the amount of write IO used as WB.
And reset/reset_host/suspend/resume can be used to manage the kernel task for checking lifetime of UFS.

> This 2017 study, is being cited by everyone, but does not really describes it's test setup to its details.
> It  does say however that they used a 16MB subregions over a range of 1GB,
> which can be covered by a 64 active regions, Even for a single subregion per region.
> Meaning no eviction should take place, thus HPB overhead is minimized.
> Do we have a more recent public studies that supports those impressive figures?
There are no other public studies currently.
However, when using HPB, there is an internal report that read latency is improved in android 
user-case scenarios, as well as in the benchmarks.

Thanks,
Daejun

RE: RE: [RFC PATCH 0/5] scsi: ufs: Add Host Performance Booster Support

[Avri Altman]

> > Like Bart, I am not sure that this extra module is needed.
> > It only makes sense if indeed there are some common calls that can be
> shared by several features.
> > There are up to now 10 extended features defined, but none of them can
> share a common api.
> > What other features can share this additional layer?  And how those ops
> can be reused?
> > If you have some future implementations in mind, you should add this api
> once you'll add those.
> We added UFS feature layer with several callbacks to important parts of the
> UFS control flow.
> Other extended features can also be implemented using the proposed APIs.
> For example, in WB, "prep_fn" can be used to guarantee the lifetime of UFS
> by updating the amount of write IO used as WB.
This is an interesting idea.

> And reset/reset_host/suspend/resume can be used to manage the kernel task
> for checking lifetime of UFS.
Another interesting idea.

Fair enough. Please share in the commit log of patch 2/5 your plans,
Otherwise, just for HPB - It seems excessive.

RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Daejun Park]

> > The data structure for map data request and L2P map uses mempool API,
> > minimizing allocation overhead while avoiding static allocation.
> Maybe one or two more sentences to explain the L2P framework:
> Each hpb lun maintains 2 "to-do" lists: 
>  - hpb->lh_inact_rgn - regions to be inactivated, and 
>  - hpb->lh_act_srgn - subregions to be activated
> Those lists are being checked on every resume and completion interrupt.
OK, I will add more description of L2P framework.

> > 
> > Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> > ---
> > +       for (i = 0; i < hpb->pages_per_srgn; i++) {
> > +               mctx->m_page[i] = mempool_alloc(ufshpb_drv.ufshpb_page_pool,
> > +                                               GFP_KERNEL);
> > +               memset(page_address(mctx->m_page[i]), 0, PAGE_SIZE);
> Better move this memset after if (!mctx->m_page[i]).
> And maybe use clear_page instead?
OK, I will change the code.

> > +               if (!mctx->m_page[i]) {
> > +                       for (j = 0; j < i; j++)
> > +                               mempool_free(mctx->m_page[j],
> > +                                            ufshpb_drv.ufshpb_page_pool);
> > +                       goto release_ppn_dirty;
> > +               }
> > +static inline int ufshpb_add_region(struct ufshpb_lu *hpb,
> > +                                   struct ufshpb_region *rgn)
> > +{
> Maybe better describe what this function does - ufshpb_get_rgn_map_ctx ?
Yes, I think "ufshpb_get_rgn_map_ctx" is better name.

> > +       if (!list_empty(&rgn->list_lru_rgn)) {
> > +               if (ufshpb_check_issue_state_srgns(hpb, rgn)) {
> So if one of its subregions has inflight map request - you add it to the "starved" list?
> Why call it starved?
"starved list" was wrong name. I will change it to "postponed_evict_list".

> > +        * Since the region state change occurs only in the hpb task-work,
> > +        * the state of the region cannot HPB_RGN_INACTIVE at this point.
> > +        * The region state must be changed in the hpb task-work
> I think that you called this worker map_work?
Yes, "the hpb task-work" will be changed to the map_work.

> > +               spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> > +               ret = ufshpb_add_region(hpb, rgn);
> If this is not an active region,
> Although the device indicated to activate a specific subregion, 
> You are activating all the subregions of that region.
> You should elaborate on that in your commit log,
> and explain why this is the correct activation course.
Yes, I'm going to change the code to activate only the subregions that are "activate state".

> get_unaligned instead of be16_to_cpu ?
Yes, I will change.

> > +
> > +       if (!data_seg_len) {
> data_seg_len should be DEV_DATA_SEG_LEN, and you should also check HPB_UPDATE_ALERT,
> which you might want to do here and not in ufshpb_may_field_valid
Yes, I will change.

> > +       switch (rsp_field->hpb_type) {
> > +       case HPB_RSP_REQ_REGION_UPDATE:
> > +               WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
> > +               ufshpb_rsp_req_region_update(hpb, rsp_field);
> > +               break;
> What about hpb dev reset - oper 0x2?
Yes, I will change.

> > +static void ufshpb_add_active_list(struct ufshpb_lu *hpb,
> > +                                  struct ufshpb_region *rgn,
> > +                                  struct ufshpb_subregion *srgn)
> > +{
> > +       if (!list_empty(&rgn->list_inact_rgn))
> > +               return;
> > +
> > +       if (!list_empty(&srgn->list_act_srgn)) {
> > +               list_move(&srgn->list_act_srgn, &hpb->lh_act_srgn);
> Why is this needed?
> Why updating this subregion position?
The "ufshpb_add_active_list()" is called from "ufshpb_run_active_subregion_list()" to retry activating subregion that failed to activate.
Therefore, it requeues the subregion to activate region list head.

> > @@ -195,8 +1047,15 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba
> > *hba, struct ufshpb_lu *hpb)
> >  release_srgn_table:
> >         for (i = 0; i < rgn_idx; i++) {
> >                 rgn = rgn_table + i;
> > -               if (rgn->srgn_tbl)
> > +               if (rgn->srgn_tbl) {
> > +                       for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt;
> > +                            srgn_idx++) {
> > +                               srgn = rgn->srgn_tbl + srgn_idx;
> > +                               if (srgn->mctx)
> How is it even possible that on init there is an active subregion?
> ufshpb_init_pinned_active_region does its own cleanup.
I will fix the duplicated cleanup codes.

> > +       hpb->m_page_cache = kmem_cache_create("ufshpb_m_page_cache",
> > +                         sizeof(struct page *) * hpb->pages_per_srgn,
> > +                         0, 0, NULL);
> What is the advantage in using an array of page pointers,
> Instead of a single pointer to pages_per_srgn?
To minimize memory fragmentation problem, I used pointer + single page rather than single array of pages. 

> > @@ -398,6 +1326,9 @@ static void ufshpb_resume(struct ufs_hba *hba)
> > 
> >                 dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
> >                 ufshpb_set_state(hpb, HPB_PRESENT);
> > +               if (!ufshpb_is_empty_rsp_lists(hpb))
> > +                       queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
> Ahha - so you are using the ufs driver pm flows to poll your work queue.
> Why device recommendations isn't enough?
I don't understand this comment. The code resumes map_work that was stopped by PM during the map request.
Please explain your concerns.

Thanks,
Avri

RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Avri Altman]

> > > +       switch (rsp_field->hpb_type) {
> > > +       case HPB_RSP_REQ_REGION_UPDATE:
> > > +               WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
> > > +               ufshpb_rsp_req_region_update(hpb, rsp_field);
> > > +               break;
> > What about hpb dev reset - oper 0x2?
> Yes, I will change.
The spec does not define what the host should do in this case,
e.g. when the device informs it that the entire db is no longer valid.
What are you planning to do?

RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Avri Altman]

> > >                 dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
> > >                 ufshpb_set_state(hpb, HPB_PRESENT);
> > > +               if (!ufshpb_is_empty_rsp_lists(hpb))
> > > +                       queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
> > Ahha - so you are using the ufs driver pm flows to poll your work queue.
> > Why device recommendations isn't enough?
> I don't understand this comment. The code resumes map_work that was
> stopped by PM during the map request.
> Please explain your concerns.
This is not a concern, just a question.
If a map request started while runtime/system suspend, can you share its flow?

RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

> I think this module parameter makes its first appearance in patch 4/5 - so maybe there?

OK, I will write module parameter in patch message 4/5.

Thanks,
Daejun

Re: [RFC PATCH 2/5] scsi: ufs: Add UFS-feature layer

[Daejun Park]

Hi Bart,

On 2020-06-04 18:30, Daejun Park wrote:
> > +inline void ufsf_slave_configure(struct ufs_hba *hba,
> > +         struct scsi_device *sdev)
> > +{
> > +  /* skip well-known LU */
> > +  if (sdev->lun >= UFS_UPIU_MAX_UNIT_NUM_ID)
> > +    return;
> > +
> > +  if (!(hba->dev_info.b_ufs_feature_sup & UFS_FEATURE_SUPPORT_HPB_BIT))
> > +    return;
> > +
> > +  atomic_inc(&hba->ufsf.slave_conf_cnt);
> > +  smp_mb__after_atomic(); /* for slave_conf_cnt */
> > +
> > +  /* waiting sdev init.*/
> > +  if (waitqueue_active(&hba->ufsf.sdev_wait))
> > +    wake_up(&hba->ufsf.sdev_wait);
> > +}

> Guarding a wake_up() call with a waitqueue_active() check is an
> anti-pattern. Please don't do that and call wake_up() directly.
> Additionally, wake_up() includes a barrier if it wakes up a kernel
> thread so the smp_mb__after_atomic() can be left out if the
> waitqueue_active() call is removed.
OK, I will change it.

> > +/**
> > + * struct ufsf_operation - UFS feature specific callbacks
> > + * @prep_fn: called after construct upiu structure
> > + * @reset: called after proving hba
                           ^^^^^^^
> Is this a typo? Should "proving" perhaps be changed into "probing"?
Yes, I will change.

> > +struct ufshpb_driver {
> > +  struct device_driver drv;
> > +  struct list_head lh_hpb_lu;
> > +
> > +  struct ufsf_operation ufshpb_ops;
> > +
> > +  /* memory management */
> > +  struct kmem_cache *ufshpb_mctx_cache;
> > +  mempool_t *ufshpb_mctx_pool;
> > +  mempool_t *ufshpb_page_pool;
> > +
> > +  struct workqueue_struct *ufshpb_wq;
> > +};

> Why is a dedicated workqueue needed? Why are the standard workqueues not
> good enough?
The map_work handles map related operations, including IO operations. So, adding
this task to the standard WQ can interfere with other jobs and degrade HPB related performance.


> > @@ -2525,6 +2525,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> >  
> >    ufshcd_comp_scsi_upiu(hba, lrbp);
> >  
> > +  ufsf_ops_prep_fn(hba, lrbp);
> > +
> >    err = ufshcd_map_sg(hba, lrbp);
> >    if (err) {
> >      lrbp->cmd = NULL;

> What happens if a SCSI command is retried and hence ufsf_ops_prep_fn()
> is called multiple times for the same SCSI command?
Developers of UFS features should implement it so that prep_fn does not have
any problems even if it processes the same SCSI command multiple times.
In HPB feature, prep_fn modifies only upiu  structure. So it is ok to call
it multiple times because the upiu is rebuilt from ufshcd_comp_scsi_upiu().

Thanks,

Daejun.

Re: [RFC PATCH 2/5] scsi: ufs: Add UFS-feature layer

[Bart Van Assche]

On 2020-06-11 19:27, Daejun Park wrote:
>>> @@ -2525,6 +2525,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>>>  
>>>    ufshcd_comp_scsi_upiu(hba, lrbp);
>>>  
>>> +  ufsf_ops_prep_fn(hba, lrbp);
>>> +
>>>    err = ufshcd_map_sg(hba, lrbp);
>>>    if (err) {
>>>      lrbp->cmd = NULL;
> 
>> What happens if a SCSI command is retried and hence ufsf_ops_prep_fn()
>> is called multiple times for the same SCSI command?
>
> Developers of UFS features should implement it so that prep_fn does not have
> any problems even if it processes the same SCSI command multiple times.
> In HPB feature, prep_fn modifies only upiu  structure. So it is ok to call
> it multiple times because the upiu is rebuilt from ufshcd_comp_scsi_upiu().

Please make sure that this expectation is documented somewhere.

Thanks,

Bart.

[RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Daejun Park]

This is a patch for managing L2P map in HPB module.

The HPB divides logical addresses into several regions. A region consists
of several sub-regions. The sub-region is a basic unit where L2P mapping is
managed. The driver loads L2P mapping data of each sub-region. The loaded
sub-region is called active-state. The HPB driver unloads L2P mapping data
as region unit. The unloaded region is called inactive-state.

Sub-region/region candidates to be loaded and unloaded are delivered from
the UFS device. The UFS device delivers the recommended active sub-region
and inactivate region to the driver using sensedata.
The HPB module performs L2P mapping management on the host through the
delivered information.

A pinned region is a pre-set regions on the UFS device that is always
activate-state and

The data structure for map data request and L2P map uses mempool API,
minimizing allocation overhead while avoiding static allocation.

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/scsi/ufs/ufshpb.c | 1005 ++++++++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshpb.h |   72 +++
 2 files changed, 1073 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index cb0ad4d16d0f..f1aa8e7b5ce0 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -46,6 +46,63 @@ static struct ufshpb_driver ufshpb_drv;
 
 static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb);
 
+static inline bool ufshpb_is_general_lun(int lun)
+{
+	return lun < UFS_UPIU_MAX_UNIT_NUM_ID;
+}
+
+static inline bool
+ufshpb_is_pinned_region(struct ufshpb_lu *hpb, int rgn_idx)
+{
+	if (hpb->lu_pinned_end != PINNED_NOT_SET &&
+	    rgn_idx >= hpb->lu_pinned_start &&
+	    rgn_idx <= hpb->lu_pinned_end)
+		return true;
+
+	return false;
+}
+
+static bool ufshpb_is_empty_rsp_lists(struct ufshpb_lu *hpb)
+{
+	bool ret = true;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+	if (!list_empty(&hpb->lh_inact_rgn) || !list_empty(&hpb->lh_act_srgn))
+		ret = false;
+	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+
+	return ret;
+}
+
+static inline int ufshpb_may_field_valid(struct ufs_hba *hba,
+					 struct ufshcd_lrb *lrbp,
+					 struct ufshpb_rsp_field *rsp_field)
+{
+	if (be16_to_cpu(rsp_field->sense_data_len) != DEV_SENSE_SEG_LEN ||
+	    rsp_field->desc_type != DEV_DES_TYPE ||
+	    rsp_field->additional_len != DEV_ADDITIONAL_LEN ||
+	    rsp_field->hpb_type == HPB_RSP_NONE ||
+	    rsp_field->active_rgn_cnt > MAX_ACTIVE_NUM ||
+	    rsp_field->inactive_rgn_cnt > MAX_INACTIVE_NUM ||
+	    (!rsp_field->active_rgn_cnt && !rsp_field->inactive_rgn_cnt))
+		return -EINVAL;
+
+	if (!ufshpb_is_general_lun(lrbp->lun)) {
+		dev_warn(hba->dev, "ufshpb: lun(%d) not supported\n",
+			 lrbp->lun);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+static inline struct ufshpb_lu *ufshpb_get_hpb_data(struct scsi_cmnd *cmd)
+{
+	return cmd->device->hostdata;
+}
+
 static inline int ufshpb_get_state(struct ufshpb_lu *hpb)
 {
 	return atomic_read(&hpb->hpb_state);
@@ -80,6 +137,789 @@ static inline void ufshpb_lu_put(struct ufshpb_lu *hpb)
 	put_device(&hpb->hpb_lu_dev);
 }
 
+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);
+	if (!map_req)
+		return NULL;
+
+	req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
+			      REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
+	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;
+	}
+
+	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;
+	map_req->lun = hpb->lun;
+
+	return map_req;
+free_map_req:
+	kmem_cache_free(hpb->map_req_cache, map_req);
+	return NULL;
+}
+
+static inline void ufshpb_put_map_req(struct ufshpb_lu *hpb,
+				      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);
+}
+
+
+static inline int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
+				     struct ufshpb_subregion *srgn)
+{
+	WARN_ON(!srgn->mctx);
+	bitmap_zero(srgn->mctx->ppn_dirty, hpb->entries_per_srgn);
+	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;
+	struct ufshpb_subregion *srgn;
+	int srgn_idx;
+
+	rgn = hpb->rgn_tbl + rgn_idx;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn = rgn->srgn_tbl + srgn_idx;
+
+		list_del_init(&srgn->list_act_srgn);
+	}
+
+	if (list_empty(&rgn->list_inact_rgn))
+		list_add_tail(&rgn->list_inact_rgn, &hpb->lh_inact_rgn);
+}
+
+static void ufshpb_clean_active_subregion(struct ufshpb_lu *hpb,
+					  struct ufshpb_subregion *srgn)
+{
+	struct ufshpb_region *rgn;
+
+	/*
+	 * If there is no mctx in subregion
+	 * after I/O progress for HPB_READ_BUFFER, the region to which the
+	 * subregion belongs was evicted.
+	 * Mask sure the the region must not evict in I/O progress
+	 */
+	WARN_ON(!srgn->mctx);
+
+	rgn = hpb->rgn_tbl + srgn->rgn_idx;
+
+	if (unlikely(rgn->rgn_state == HPB_RGN_INACTIVE)) {
+		dev_err(&hpb->hpb_lu_dev,
+			"region %d subregion %d evicted\n",
+			srgn->rgn_idx, srgn->srgn_idx);
+		return;
+	}
+	srgn->srgn_state = HPB_SRGN_CLEAN;
+}
+
+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;
+	struct ufshpb_lu *hpb = map_req->hpb;
+	struct ufshpb_subregion *srgn;
+	unsigned long flags;
+
+	srgn = hpb->rgn_tbl[map_req->rgn_idx].srgn_tbl +
+		map_req->srgn_idx;
+
+	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+	ufshpb_clean_active_subregion(hpb, srgn);
+	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+
+	ufshpb_put_map_req(map_req->hpb, map_req);
+	ufshpb_lu_put(hpb);
+}
+
+static inline void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
+					   int srgn_idx, int srgn_mem_size)
+{
+	cdb[0] = UFSHPB_READ_BUFFER;
+	cdb[1] = UFSHPB_READ_BUFFER_ID;
+
+	put_unaligned_be32(srgn_mem_size, &cdb[5]);
+	/* cdb[5] = 0x00; */
+	put_unaligned_be16(rgn_idx, &cdb[2]);
+	put_unaligned_be16(srgn_idx, &cdb[4]);
+
+	cdb[9] = 0x00;
+}
+
+static int ufshpb_map_req_add_bio_page(struct ufshpb_lu *hpb,
+				       struct request_queue *q, struct bio *bio,
+				       struct ufshpb_map_ctx *mctx)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < hpb->pages_per_srgn; i++) {
+		ret = bio_add_pc_page(q, bio, mctx->m_page[i], PAGE_SIZE, 0);
+		if (ret != PAGE_SIZE) {
+			dev_notice(&hpb->hpb_lu_dev,
+				   "bio_add_pc_page fail %d\n", ret);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
+				  struct ufshpb_req *map_req)
+{
+	struct request_queue *q;
+	struct request *req;
+	struct scsi_request *rq;
+	int ret = 0;
+
+	q = hpb->sdev_ufs_lu->request_queue;
+	ret = ufshpb_map_req_add_bio_page(hpb, q, map_req->bio,
+					  map_req->mctx);
+	if (ret) {
+		dev_notice(&hpb->hpb_lu_dev,
+			   "map_req_add_bio_page fail %d - %d\n",
+			   map_req->rgn_idx, map_req->srgn_idx);
+		return ret;
+	}
+
+	req = map_req->req;
+
+	blk_rq_append_bio(req, &map_req->bio);
+	req->rq_flags |= RQF_QUIET;
+	req->timeout = MAP_REQ_TIMEOUT;
+	req->end_io_data = (void *)map_req;
+
+	rq = scsi_req(req);
+	ufshpb_set_read_buf_cmd(rq->cmd, map_req->rgn_idx,
+				map_req->srgn_idx, hpb->srgn_mem_size);
+	rq->cmd_len = HPB_READ_BUFFER_CMD_LENGTH;
+
+	blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_map_req_compl_fn);
+
+	atomic_inc(&hpb->stats.map_req_cnt);
+	return 0;
+}
+
+static struct ufshpb_map_ctx *ufshpb_get_map_ctx(struct ufshpb_lu *hpb)
+{
+	struct ufshpb_map_ctx *mctx;
+	int i, j;
+
+	mctx = mempool_alloc(ufshpb_drv.ufshpb_mctx_pool, GFP_KERNEL);
+	if (!mctx)
+		return NULL;
+
+	mctx->m_page = kmem_cache_alloc(hpb->m_page_cache, GFP_KERNEL);
+	if (!mctx->m_page)
+		goto release_mctx;
+
+	mctx->ppn_dirty = bitmap_zalloc(hpb->entries_per_srgn, GFP_KERNEL);
+	if (!mctx->ppn_dirty)
+		goto release_m_page;
+
+	for (i = 0; i < hpb->pages_per_srgn; i++) {
+		mctx->m_page[i] = mempool_alloc(ufshpb_drv.ufshpb_page_pool,
+						GFP_KERNEL);
+		memset(page_address(mctx->m_page[i]), 0, PAGE_SIZE);
+		if (!mctx->m_page[i]) {
+			for (j = 0; j < i; j++)
+				mempool_free(mctx->m_page[j],
+					     ufshpb_drv.ufshpb_page_pool);
+			goto release_ppn_dirty;
+		}
+	}
+
+	return mctx;
+release_ppn_dirty:
+	bitmap_free(mctx->ppn_dirty);
+release_m_page:
+	kmem_cache_free(hpb->m_page_cache, mctx->m_page);
+release_mctx:
+	mempool_free(mctx, ufshpb_drv.ufshpb_mctx_pool);
+	return NULL;
+}
+
+static inline void ufshpb_put_map_ctx(struct ufshpb_lu *hpb,
+				      struct ufshpb_map_ctx *mctx)
+{
+	int i;
+
+	for (i = 0; i < hpb->pages_per_srgn; i++)
+		mempool_free(mctx->m_page[i],
+			     ufshpb_drv.ufshpb_page_pool);
+
+	bitmap_free(mctx->ppn_dirty);
+	kmem_cache_free(hpb->m_page_cache, mctx->m_page);
+	mempool_free(mctx, ufshpb_drv.ufshpb_mctx_pool);
+}
+
+static int ufshpb_check_issue_state_srgns(struct ufshpb_lu *hpb,
+					  struct ufshpb_region *rgn)
+{
+	struct ufshpb_subregion *srgn;
+	int srgn_idx;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn  = rgn->srgn_tbl + srgn_idx;
+
+		if (srgn->srgn_state == HPB_SRGN_ISSUED)
+			return -EPERM;
+	}
+	return 0;
+}
+
+static inline void ufshpb_add_lru_info(struct victim_select_info *lru_info,
+				       struct ufshpb_region *rgn)
+{
+	struct ufshpb_subregion *srgn;
+	int srgn_idx;
+
+	rgn->rgn_state = HPB_RGN_ACTIVE;
+	list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
+	atomic_inc(&lru_info->active_cnt);
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn = rgn->srgn_tbl + srgn_idx;
+		srgn->srgn_state = HPB_SRGN_DIRTY;
+	}
+}
+
+static inline void ufshpb_hit_lru_info(struct victim_select_info *lru_info,
+				       struct ufshpb_region *rgn)
+{
+	list_move_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
+}
+
+static struct ufshpb_region *ufshpb_victim_lru_info(struct ufshpb_lu *hpb)
+{
+	struct victim_select_info *lru_info = &hpb->lru_info;
+	struct ufshpb_region *rgn, *victim_rgn = NULL;
+
+	list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
+		WARN_ON(!rgn);
+		if (ufshpb_check_issue_state_srgns(hpb, rgn))
+			continue;
+
+		victim_rgn = rgn;
+		break;
+	}
+
+	return victim_rgn;
+}
+
+static inline void ufshpb_cleanup_lru_info(struct victim_select_info *lru_info,
+					   struct ufshpb_region *rgn)
+{
+	list_del_init(&rgn->list_lru_rgn);
+	rgn->rgn_state = HPB_RGN_INACTIVE;
+	atomic_dec(&lru_info->active_cnt);
+}
+
+
+static inline int ufshpb_add_region(struct ufshpb_lu *hpb,
+				    struct ufshpb_region *rgn)
+{
+	struct ufshpb_subregion *srgn;
+	int srgn_idx, j;
+	int err = 0;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn = rgn->srgn_tbl + srgn_idx;
+
+		srgn->mctx = ufshpb_get_map_ctx(hpb);
+		if (!srgn->mctx) {
+			dev_info(&hpb->hpb_lu_dev,
+				 "alloc mctx failed regions %d - %d",
+				 rgn->rgn_idx, srgn_idx);
+			err = -ENOMEM;
+			goto release_mctx;
+		}
+	}
+
+	return 0;
+release_mctx:
+	for (j = 0; j < srgn_idx; j++) {
+		srgn = rgn->srgn_tbl + j;
+		ufshpb_put_map_ctx(hpb, srgn->mctx);
+	}
+	return err;
+}
+
+static inline void ufshpb_purge_active_subregion(struct ufshpb_lu *hpb,
+						 struct ufshpb_subregion *srgn,
+						 int state)
+{
+	if (state == HPB_SRGN_UNUSED) {
+		ufshpb_put_map_ctx(hpb, srgn->mctx);
+		srgn->mctx = NULL;
+	}
+
+	srgn->srgn_state = state;
+}
+
+static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
+				  struct ufshpb_region *rgn)
+{
+	struct victim_select_info *lru_info;
+	struct ufshpb_subregion *srgn;
+	int srgn_idx;
+
+	lru_info = &hpb->lru_info;
+
+	dev_dbg(&hpb->hpb_lu_dev, "evict region %d\n", rgn->rgn_idx);
+
+	ufshpb_cleanup_lru_info(lru_info, rgn);
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn = rgn->srgn_tbl + srgn_idx;
+
+		ufshpb_purge_active_subregion(hpb, srgn, HPB_SRGN_UNUSED);
+	}
+}
+
+static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+	if (rgn->rgn_state == HPB_RGN_PINNED) {
+		dev_warn(&hpb->hpb_lu_dev,
+			 "pinned region cannot drop-out. region %d\n",
+			 rgn->rgn_idx);
+		goto out;
+	}
+
+	if (!list_empty(&rgn->list_lru_rgn)) {
+		if (ufshpb_check_issue_state_srgns(hpb, rgn)) {
+			ret = -EBUSY;
+			goto out;
+		}
+
+		__ufshpb_evict_region(hpb, rgn);
+	}
+out:
+	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+	return ret;
+}
+
+static inline struct
+ufshpb_rsp_field *ufshpb_get_hpb_rsp(struct ufshcd_lrb *lrbp)
+{
+	return (struct ufshpb_rsp_field *)&lrbp->ucd_rsp_ptr->sr.sense_data_len;
+}
+
+static int ufshpb_issue_map_req(struct ufshpb_lu *hpb,
+				struct ufshpb_region *rgn,
+				struct ufshpb_subregion *srgn)
+{
+	struct ufshpb_req *map_req;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+	/*
+	 * Since the region state change occurs only in the hpb task-work,
+	 * the state of the region cannot HPB_RGN_INACTIVE at this point.
+	 * The region state must be changed in the hpb task-work
+	 */
+	WARN_ON(rgn->rgn_state == HPB_RGN_INACTIVE);
+
+	/*
+	 * If the subregion is already ISSUED state,
+	 * a specific event (e.g., GC or wear-leveling, etc.) occurs in
+	 * the device and HPB response for map loading is received.
+	 * In this case, after finishing the HPB_READ_BUFFER,
+	 * the next HPB_READ_BUFFER is performed again to obtain the latest
+	 * map data
+	 */
+	if (srgn->srgn_state == HPB_SRGN_ISSUED) {
+		ret = -EAGAIN;
+		goto unlock_out;
+	}
+
+	ufshpb_clear_dirty_bitmap(hpb, srgn);
+	srgn->srgn_state = HPB_SRGN_ISSUED;
+	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+
+	map_req = ufshpb_get_map_req(hpb, srgn);
+	if (!map_req) {
+		ret = -EAGAIN;
+		goto out;
+	}
+
+	ret = ufshpb_lu_get(hpb);
+	if (unlikely(ret)) {
+		dev_notice(&hpb->hpb_lu_dev,
+			   "%s: ufshpb_lu_get failed: %d", __func__, ret);
+		ret = -EAGAIN;
+		goto free_map_req;
+	}
+
+	ret = ufshpb_execute_map_req(hpb, map_req);
+	if (ret) {
+		dev_notice(&hpb->hpb_lu_dev,
+			   "%s: issue map_req failed: %d, region %d - %d\n",
+			   __func__, ret, srgn->rgn_idx, srgn->srgn_idx);
+		ufshpb_lu_put(hpb);
+		goto free_map_req;
+	}
+	return ret;
+free_map_req:
+	ufshpb_put_map_req(hpb, map_req);
+unlock_out:
+	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+out:
+	return ret;
+}
+
+static int ufshpb_load_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
+{
+	struct ufshpb_region *victim_rgn;
+	struct victim_select_info *lru_info = &hpb->lru_info;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+	/*
+	 * If region belongs to lru_list, just move the region
+	 * to the front of lru list. because the state of the region
+	 * is already active-state
+	 */
+	if (!list_empty(&rgn->list_lru_rgn)) {
+		ufshpb_hit_lru_info(lru_info, rgn);
+		goto out;
+	}
+
+	if (rgn->rgn_state == HPB_RGN_INACTIVE) {
+		if (atomic_read(&lru_info->active_cnt)
+		    == lru_info->max_lru_active_cnt) {
+			/*
+			 * If the maximum number of active regions
+			 * is exceeded, evict the least recently used region.
+			 * This case may occur when the device responds
+			 * to the eviction information late.
+			 * It is okay to evict the least recently used region,
+			 * because the device could detect this region
+			 * by not issuing HPB_READ
+			 */
+			victim_rgn = ufshpb_victim_lru_info(hpb);
+			if (!victim_rgn) {
+				dev_warn(&hpb->hpb_lu_dev,
+				    "cannot get victim region error\n");
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			dev_dbg(&hpb->hpb_lu_dev,
+				"LRU full (%d), choost victim %d\n",
+				atomic_read(&lru_info->active_cnt),
+				victim_rgn->rgn_idx);
+			__ufshpb_evict_region(hpb, victim_rgn);
+		}
+
+		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+		ret = ufshpb_add_region(hpb, rgn);
+		if (ret) {
+			dev_info(&hpb->hpb_lu_dev,
+				 "ufshpb_add_region %d add failed\n",
+				 rgn->rgn_idx);
+			goto out;
+		}
+		spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+		/*
+		 * When a region is added to lru_info list_head,
+		 * it is guaranteed that the subregion has been
+		 * assigned all mctx. If failed, try to receive mctx again
+		 * without being added to lru_info list_head
+		 */
+		ufshpb_add_lru_info(lru_info, rgn);
+	}
+out:
+	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+	return ret;
+}
+
+static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
+					 struct ufshpb_rsp_field *rsp_field)
+{
+	int i, rgn_idx, srgn_idx;
+
+	/*
+	 * If the active region and the inactive region are the same,
+	 * we will inactivate this region.
+	 * The device could check this (region inactivated) and
+	 * will response the proper active region information
+	 */
+	spin_lock(&hpb->rsp_list_lock);
+	for (i = 0; i < rsp_field->active_rgn_cnt; i++) {
+		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);
+
+		dev_dbg(&hpb->hpb_lu_dev, "activate(%d) region %d - %d\n",
+			i, rgn_idx, srgn_idx);
+		ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+		atomic_inc(&hpb->stats.rb_active_cnt);
+	}
+
+	for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
+		rgn_idx = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
+		dev_dbg(&hpb->hpb_lu_dev, "inactivate(%d) region %d\n",
+			i, rgn_idx);
+		ufshpb_update_inactive_info(hpb, rgn_idx);
+		atomic_inc(&hpb->stats.rb_inactive_cnt);
+	}
+	spin_unlock(&hpb->rsp_list_lock);
+
+	dev_dbg(&hpb->hpb_lu_dev, "Noti: #ACT %u #INACT %u\n",
+		rsp_field->active_rgn_cnt, rsp_field->inactive_rgn_cnt);
+
+	queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
+}
+
+/* routine : isr (ufs) */
+static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+	struct ufshpb_lu *hpb;
+	struct ufshpb_rsp_field *rsp_field;
+	int data_seg_len, ret;
+
+	data_seg_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2)
+		& MASK_RSP_UPIU_DATA_SEG_LEN;
+
+	if (!data_seg_len) {
+		if (!ufshpb_is_general_lun(lrbp->lun))
+			return;
+
+		hpb = ufshpb_get_hpb_data(lrbp->cmd);
+		ret = ufshpb_lu_get(hpb);
+		if (ret)
+			return;
+
+		if (!ufshpb_is_empty_rsp_lists(hpb))
+			queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
+
+		goto put_hpb;
+	}
+
+	rsp_field = ufshpb_get_hpb_rsp(lrbp);
+	if (ufshpb_may_field_valid(hba, lrbp, rsp_field))
+		return;
+
+	hpb = ufshpb_get_hpb_data(lrbp->cmd);
+	ret = ufshpb_lu_get(hpb);
+	if (ret)
+		return;
+
+	atomic_inc(&hpb->stats.rb_noti_cnt);
+
+	switch (rsp_field->hpb_type) {
+	case HPB_RSP_REQ_REGION_UPDATE:
+		WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
+		ufshpb_rsp_req_region_update(hpb, rsp_field);
+		break;
+	default:
+		dev_notice(&hpb->hpb_lu_dev, "hpb_type is not available: %d\n",
+			   rsp_field->hpb_type);
+		break;
+	}
+put_hpb:
+	ufshpb_lu_put(hpb);
+}
+
+static void ufshpb_add_active_list(struct ufshpb_lu *hpb,
+				   struct ufshpb_region *rgn,
+				   struct ufshpb_subregion *srgn)
+{
+	if (!list_empty(&rgn->list_inact_rgn))
+		return;
+
+	if (!list_empty(&srgn->list_act_srgn)) {
+		list_move(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+		return;
+	}
+
+	list_add(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+}
+
+static void ufshpb_add_starved_list(struct ufshpb_lu *hpb,
+				    struct ufshpb_region *rgn,
+				    struct list_head *starved_list)
+{
+	struct ufshpb_subregion *srgn;
+	int srgn_idx;
+
+	if (!list_empty(&rgn->list_inact_rgn))
+		return;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn = rgn->srgn_tbl + srgn_idx;
+
+		if (!list_empty(&srgn->list_act_srgn))
+			return;
+	}
+
+	list_add_tail(&rgn->list_inact_rgn, starved_list);
+}
+
+static void ufshpb_run_active_subregion_list(struct ufshpb_lu *hpb)
+{
+	struct ufshpb_region *rgn;
+	struct ufshpb_subregion *srgn;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+	while ((srgn = list_first_entry_or_null(&hpb->lh_act_srgn,
+						struct ufshpb_subregion,
+						list_act_srgn))) {
+		list_del_init(&srgn->list_act_srgn);
+		spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+
+		rgn = hpb->rgn_tbl + srgn->rgn_idx;
+		ret = ufshpb_load_region(hpb, rgn);
+		if (ret)
+			break;
+
+		ret = ufshpb_issue_map_req(hpb, rgn, srgn);
+		if (ret) {
+			dev_notice(&hpb->hpb_lu_dev,
+			    "issue map_req failed. ret %d, region %d - %d\n",
+			    ret, rgn->rgn_idx, srgn->srgn_idx);
+			break;
+		}
+		spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+	}
+
+	if (ret) {
+		dev_notice(&hpb->hpb_lu_dev, "region %d - %d, will retry\n",
+			   rgn->rgn_idx, srgn->srgn_idx);
+		spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+		srgn->srgn_state = HPB_SRGN_DIRTY;
+		ufshpb_add_active_list(hpb, rgn, srgn);
+	}
+	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+}
+
+static void ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb)
+{
+	struct ufshpb_region *rgn;
+	unsigned long flags;
+	int ret;
+	LIST_HEAD(starved_list);
+
+	spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+	while ((rgn = list_first_entry_or_null(&hpb->lh_inact_rgn,
+					       struct ufshpb_region,
+					       list_inact_rgn))) {
+		list_del_init(&rgn->list_inact_rgn);
+		spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+
+		ret = ufshpb_evict_region(hpb, rgn);
+		if (ret) {
+			spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+			ufshpb_add_starved_list(hpb, rgn, &starved_list);
+			spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+		}
+
+		spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+	}
+
+	list_splice(&starved_list, &hpb->lh_inact_rgn);
+	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+}
+
+static void ufshpb_map_work_handler(struct work_struct *work)
+{
+	struct ufshpb_lu *hpb;
+	int ret;
+
+	hpb = container_of(work, struct ufshpb_lu, map_work);
+	ret = ufshpb_lu_get(hpb);
+	if (ret) {
+		dev_info(&hpb->hpb_lu_dev, "%s: exit, state %d\n",
+			 __func__, ufshpb_get_state(hpb));
+		return;
+	}
+
+	ufshpb_run_inactive_region_list(hpb);
+	ufshpb_run_active_subregion_list(hpb);
+
+	ufshpb_lu_put(hpb);
+}
+
+/*
+ * this function doesn't need to hold lock due to be called in init.
+ * (hpb_state_lock, rsp_list_lock, etc..)
+ */
+static int ufshpb_init_pinned_active_region(struct ufs_hba *hba,
+					    struct ufshpb_lu *hpb,
+					    struct ufshpb_region *rgn)
+{
+	struct ufshpb_subregion *srgn;
+	int srgn_idx, j;
+	int err = 0;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		srgn = rgn->srgn_tbl + srgn_idx;
+
+		srgn->mctx = ufshpb_get_map_ctx(hpb);
+		if (!srgn->mctx) {
+			dev_err(hba->dev,
+				"alloc mctx for pinned region failed\n");
+			goto release;
+		}
+
+		list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+	}
+
+	rgn->rgn_state = HPB_RGN_PINNED;
+	return 0;
+
+release:
+	for (j = 0; j < srgn_idx; j++) {
+		srgn = rgn->srgn_tbl + j;
+		ufshpb_put_map_ctx(hpb, srgn->mctx);
+	}
+	return err;
+}
+
 static void ufshpb_init_subregion_tbl(struct ufshpb_lu *hpb,
 				      struct ufshpb_region *rgn)
 {
@@ -88,6 +928,8 @@ static void ufshpb_init_subregion_tbl(struct ufshpb_lu *hpb,
 	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
 		struct ufshpb_subregion *srgn = rgn->srgn_tbl + srgn_idx;
 
+		INIT_LIST_HEAD(&srgn->list_act_srgn);
+
 		srgn->rgn_idx = rgn->rgn_idx;
 		srgn->srgn_idx = srgn_idx;
 		srgn->srgn_state = HPB_SRGN_UNUSED;
@@ -120,6 +962,8 @@ static void ufshpb_init_lu_parameter(struct ufs_hba *hba,
 	hpb->lu_pinned_end = hpb_lu_info->num_pinned ?
 		(hpb_lu_info->pinned_start + hpb_lu_info->num_pinned - 1)
 		: PINNED_NOT_SET;
+	hpb->lru_info.max_lru_active_cnt =
+		hpb_lu_info->max_active_rgns - hpb_lu_info->num_pinned;
 
 	rgn_mem_size = (1ULL << hpb_dev_info->rgn_size) * HPB_RGN_SIZE_UNIT
 		/ HPB_ENTRY_BLOCK_SIZE * HPB_ENTRY_SIZE;
@@ -174,6 +1018,9 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 		rgn = rgn_table + rgn_idx;
 		rgn->rgn_idx = rgn_idx;
 
+		INIT_LIST_HEAD(&rgn->list_inact_rgn);
+		INIT_LIST_HEAD(&rgn->list_lru_rgn);
+
 		srgn_cnt = min(total_srgn_cnt, hpb->srgns_per_rgn);
 
 		ret = ufshpb_alloc_subregion_tbl(hpb, rgn, srgn_cnt);
@@ -181,7 +1028,12 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 			goto release_srgn_table;
 		ufshpb_init_subregion_tbl(hpb, rgn);
 
-		rgn->rgn_state = HPB_RGN_INACTIVE;
+		if (ufshpb_is_pinned_region(hpb, rgn_idx)) {
+			ret = ufshpb_init_pinned_active_region(hba, hpb, rgn);
+			if (ret)
+				goto release_srgn_table;
+		} else {
+			rgn->rgn_state = HPB_RGN_INACTIVE;
 		}
 	}
 
@@ -195,8 +1047,15 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
 release_srgn_table:
 	for (i = 0; i < rgn_idx; i++) {
 		rgn = rgn_table + i;
-		if (rgn->srgn_tbl)
+		if (rgn->srgn_tbl) {
+			for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt;
+			     srgn_idx++) {
+				srgn = rgn->srgn_tbl + srgn_idx;
+				if (srgn->mctx)
+					ufshpb_put_map_ctx(hpb, srgn->mctx);
+			}
 			kvfree(rgn->srgn_tbl);
+		}
 	}
 	kvfree(rgn_table);
 	return ret;
@@ -212,6 +1071,8 @@ static void ufshpb_destroy_subregion_tbl(struct ufshpb_lu *hpb,
 
 		srgn = rgn->srgn_tbl + srgn_idx;
 		srgn->srgn_state = HPB_SRGN_UNUSED;
+
+		ufshpb_put_map_ctx(hpb, srgn->mctx);
 	}
 }
 
@@ -241,10 +1102,37 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb,
 	int ret;
 
 	spin_lock_init(&hpb->hpb_state_lock);
+	spin_lock_init(&hpb->rsp_list_lock);
+
+	INIT_LIST_HEAD(&hpb->lru_info.lh_lru_rgn);
+	INIT_LIST_HEAD(&hpb->lh_act_srgn);
+	INIT_LIST_HEAD(&hpb->lh_inact_rgn);
+	INIT_LIST_HEAD(&hpb->list_hpb_lu);
+
+	INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
+
+	hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
+			  sizeof(struct ufshpb_req), 0, 0, NULL);
+	if (!hpb->map_req_cache) {
+		dev_err(hba->dev, "ufshpb(%d) ufshpb_req_cache create fail",
+			hpb->lun);
+		return -ENOMEM;
+	}
+
+	hpb->m_page_cache = kmem_cache_create("ufshpb_m_page_cache",
+			  sizeof(struct page *) * hpb->pages_per_srgn,
+			  0, 0, NULL);
+	if (!hpb->m_page_cache) {
+		dev_err(hba->dev, "ufshpb(%d) ufshpb_m_page_cache create fail",
+			hpb->lun);
+		ret = -ENOMEM;
+		goto release_req_cache;
+	}
+
 
 	ret = ufshpb_alloc_region_tbl(hba, hpb);
 	if (ret)
-		return ret;
+		goto release_m_page_cache;
 
 	ret = ufshpb_create_sysfs(hba, hpb);
 	if (ret)
@@ -254,6 +1142,10 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb,
 
 release_rgn_table:
 	ufshpb_destroy_region_tbl(hpb);
+release_m_page_cache:
+	kmem_cache_destroy(hpb->m_page_cache);
+release_req_cache:
+	kmem_cache_destroy(hpb->map_req_cache);
 	return ret;
 }
 
@@ -285,10 +1177,42 @@ static struct ufshpb_lu *ufshpb_alloc_hpb_lu(struct ufs_hba *hba, int lun,
 	return NULL;
 }
 
+static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
+{
+	struct ufshpb_region *rgn, *next_rgn;
+	struct ufshpb_subregion *srgn, *next_srgn;
+	unsigned long flags;
+
+	/*
+	 * If the device reset occurred, the remained HPB region information
+	 * may be stale. Therefore, by dicarding the lists of HPB response
+	 * that remained after reset, it prevents unnecessary work.
+	 */
+	spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+	list_for_each_entry_safe(rgn, next_rgn, &hpb->lh_inact_rgn,
+				 list_inact_rgn)
+		list_del_init(&rgn->list_inact_rgn);
+
+	list_for_each_entry_safe(srgn, next_srgn, &hpb->lh_act_srgn,
+				 list_act_srgn)
+		list_del_init(&srgn->list_act_srgn);
+	spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+}
+
+static inline void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
+{
+	cancel_work_sync(&hpb->map_work);
+}
+
 static void ufshpb_lu_release(struct ufshpb_lu *hpb)
 {
+	ufshpb_cancel_jobs(hpb);
+
 	ufshpb_destroy_region_tbl(hpb);
 
+	kmem_cache_destroy(hpb->map_req_cache);
+	kmem_cache_destroy(hpb->m_page_cache);
+
 	list_del_init(&hpb->list_hpb_lu);
 }
 
@@ -369,6 +1293,9 @@ static void ufshpb_reset_host(struct ufs_hba *hba)
 		dev_info(&hpb->hpb_lu_dev, "ufshpb run reset_host");
 
 		ufshpb_set_state(hpb, HPB_RESET);
+		ufshpb_cancel_jobs(hpb);
+		ufshpb_discard_rsp_lists(hpb);
+
 		ufshpb_lu_put(hpb);
 	}
 }
@@ -383,6 +1310,7 @@ static void ufshpb_suspend(struct ufs_hba *hba)
 
 		dev_info(&hpb->hpb_lu_dev, "ufshpb goto suspend");
 		ufshpb_set_state(hpb, HPB_SUSPEND);
+		ufshpb_cancel_jobs(hpb);
 
 		ufshpb_lu_put(hpb);
 	}
@@ -398,6 +1326,9 @@ static void ufshpb_resume(struct ufs_hba *hba)
 
 		dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
 		ufshpb_set_state(hpb, HPB_PRESENT);
+		if (!ufshpb_is_empty_rsp_lists(hpb))
+			queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
+
 		ufshpb_lu_put(hpb);
 	}
 }
@@ -656,6 +1587,14 @@ static void ufshpb_scan_hpb_lu(struct ufs_hba *hba,
 		list_for_each_entry(hpb, &ufshpb_drv.lh_hpb_lu, list_hpb_lu) {
 			dev_info(&hpb->hpb_lu_dev, "set state to present\n");
 			ufshpb_set_state(hpb, HPB_PRESENT);
+
+			if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0) {
+				dev_info(&hpb->hpb_lu_dev,
+				    "loading pinned regions %d - %d\n",
+				    hpb->lu_pinned_start, hpb->lu_pinned_end);
+				queue_work(ufshpb_drv.ufshpb_wq,
+					&hpb->map_work);
+			}
 		}
 	}
 }
@@ -727,6 +1666,8 @@ static int ufshpb_remove(struct device *dev)
 		sdev = hpb->sdev_ufs_lu;
 		sdev->hostdata = NULL;
 
+		ufshpb_cancel_jobs(hpb);
+
 		device_del(&hpb->hpb_lu_dev);
 
 		dev_info(&hpb->hpb_lu_dev, "hpb_lu_dev refcnt %d\n",
@@ -751,6 +1692,7 @@ static struct ufshpb_driver ufshpb_drv = {
 		.reset_host = ufshpb_reset_host,
 		.suspend = ufshpb_suspend,
 		.resume = ufshpb_resume,
+		.rsp_upiu = ufshpb_rsp_upiu,
 	},
 };
 
@@ -761,17 +1703,72 @@ MODULE_PARM_DESC(ufshpb_host_map_kbytes,
 
 static int __init ufshpb_init(void)
 {
+	unsigned int pool_size;
 	int ret;
 
+
+	ufshpb_drv.ufshpb_mctx_cache = kmem_cache_create("ufshpb_mctx_cache",
+					sizeof(struct ufshpb_map_ctx),
+					0, 0, NULL);
+	if (!ufshpb_drv.ufshpb_mctx_cache) {
+		pr_err("ufshpb: cannot init mctx cache\n");
+		return -ENOMEM;
+	}
+
+	pool_size = DIV_ROUND_UP(ufshpb_host_map_kbytes * 1024, 4096);
+	pr_info("%s:%d ufshpb_host_map_kbytes %u pool_size %u\n",
+	       __func__, __LINE__, ufshpb_host_map_kbytes, pool_size);
+
+	ufshpb_drv.ufshpb_mctx_pool = mempool_create_slab_pool(
+				     pool_size, ufshpb_drv.ufshpb_mctx_cache);
+	if (!ufshpb_drv.ufshpb_mctx_pool) {
+		pr_err("ufshpb: cannot init mctx pool\n");
+		ret = -ENOMEM;
+		goto release_mctx_cache;
+	}
+
+	ufshpb_drv.ufshpb_page_pool = mempool_create_page_pool(pool_size, 0);
+	if (!ufshpb_drv.ufshpb_page_pool) {
+		pr_err("ufshpb: cannot init page pool\n");
+		ret = -ENOMEM;
+		goto release_mctx_pool;
+	}
+
+	ufshpb_drv.ufshpb_wq = alloc_workqueue("ufshpb-wq",
+					WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
+	if (!ufshpb_drv.ufshpb_wq) {
+		pr_err("ufshpb: alloc workqueue failed\n");
+		ret = -ENOMEM;
+		goto release_page_pool;
+	}
+
 	ret = driver_register(&ufshpb_drv.drv);
-	if (ret)
+	if (ret) {
 		pr_err("ufshpb: driver register failed\n");
+		goto release_wq;
+	}
+
+	return 0;
+release_wq:
+	destroy_workqueue(ufshpb_drv.ufshpb_wq);
+release_page_pool:
+	mempool_destroy(ufshpb_drv.ufshpb_page_pool);
+release_mctx_pool:
+	mempool_destroy(ufshpb_drv.ufshpb_mctx_pool);
+release_mctx_cache:
+	kmem_cache_destroy(ufshpb_drv.ufshpb_mctx_cache);
 	return ret;
 }
 
 static void __exit ufshpb_exit(void)
 {
 	driver_unregister(&ufshpb_drv.drv);
+
+	mempool_destroy(ufshpb_drv.ufshpb_page_pool);
+	mempool_destroy(ufshpb_drv.ufshpb_mctx_pool);
+	kmem_cache_destroy(ufshpb_drv.ufshpb_mctx_cache);
+
+	destroy_workqueue(ufshpb_drv.ufshpb_wq);
 }
 
 MODULE_AUTHOR("Yongmyong Lee <ymhungry.lee@samsung.com>");
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index c6dd88e00849..8f2c73f585cc 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -124,10 +124,36 @@ struct ufshpb_lu_info {
 	int max_active_rgns;
 };
 
+struct ufshpb_active_field {
+	__be16 active_rgn;
+	__be16 active_srgn;
+} __packed;
+
+struct ufshpb_rsp_field {
+	__be16 sense_data_len;
+	u8 desc_type;
+	u8 additional_len;
+	u8 hpb_type;
+	u8 reserved;
+	u8 active_rgn_cnt;
+	u8 inactive_rgn_cnt;
+	struct ufshpb_active_field hpb_active_field[2];
+	__be16 hpb_inactive_field[2];
+} __packed;
+
+struct ufshpb_map_ctx {
+	struct page **m_page;
+	unsigned long *ppn_dirty;
+};
+
 struct ufshpb_subregion {
+	struct ufshpb_map_ctx *mctx;
 	enum HPB_SRGN_STATE srgn_state;
 	int rgn_idx;
 	int srgn_idx;
+
+	/* below information is used by rsp_list */
+	struct list_head list_act_srgn;
 };
 
 struct ufshpb_region {
@@ -135,6 +161,39 @@ struct ufshpb_region {
 	enum HPB_RGN_STATE rgn_state;
 	int rgn_idx;
 	int srgn_cnt;
+
+	/* below information is used by rsp_list */
+	struct list_head list_inact_rgn;
+
+	/* below information is used by lru */
+	struct list_head list_lru_rgn;
+};
+
+/**
+ * struct ufshpb_req - UFSHPB READ BUFFER (for caching map) request structure
+ * @req: block layer request for READ BUFFER
+ * @bio: bio for holding map page
+ * @hpb: ufshpb_lu structure that related to the L2P map
+ * @mctx: L2P map information
+ * @rgn_idx: target region index
+ * @srgn_idx: target sub-region index
+ * @lun: target logical unit number
+ */
+struct ufshpb_req {
+	struct request *req;
+	struct bio *bio;
+	struct ufshpb_lu *hpb;
+	struct ufshpb_map_ctx *mctx;
+
+	unsigned int rgn_idx;
+	unsigned int srgn_idx;
+	unsigned int lun;
+};
+
+struct victim_select_info {
+	struct list_head lh_lru_rgn;
+	int max_lru_active_cnt; /* supported hpb #region - pinned #region */
+	atomic_t active_cnt;
 };
 
 struct ufshpb_stats {
@@ -157,6 +216,16 @@ struct ufshpb_lu {
 	spinlock_t hpb_state_lock;
 	atomic_t hpb_state; /* hpb_state_lock */
 
+	spinlock_t rsp_list_lock;
+	struct list_head lh_act_srgn; /* rsp_list_lock */
+	struct list_head lh_inact_rgn; /* rsp_list_lock */
+
+	/* cached L2P map management worker */
+	struct work_struct map_work;
+
+	/* for selecting victim */
+	struct victim_select_info lru_info;
+
 	/* pinned region information */
 	u32 lu_pinned_start;
 	u32 lu_pinned_end;
@@ -175,6 +244,9 @@ struct ufshpb_lu {
 
 	struct ufshpb_stats stats;
 
+	struct kmem_cache *map_req_cache;
+	struct kmem_cache *m_page_cache;
+
 	struct ufsf_feature_info *ufsf;
 	struct list_head list_hpb_lu;
 };
-- 
2.17.1

RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Avri Altman]

> 
> A pinned region is a pre-set regions on the UFS device that is always
> activate-state and
This sentence got cut off

> 
> The data structure for map data request and L2P map uses mempool API,
> minimizing allocation overhead while avoiding static allocation.

Maybe one or two more sentences to explain the L2P framework:
Each hpb lun maintains 2 "to-do" lists: 
 - hpb->lh_inact_rgn - regions to be inactivated, and 
 - hpb->lh_act_srgn - subregions to be activated
Those lists are being checked on every resume and completion interrupt.

> 
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
> +       for (i = 0; i < hpb->pages_per_srgn; i++) {
> +               mctx->m_page[i] = mempool_alloc(ufshpb_drv.ufshpb_page_pool,
> +                                               GFP_KERNEL);
> +               memset(page_address(mctx->m_page[i]), 0, PAGE_SIZE);
Better move this memset after if (!mctx->m_page[i]).
And maybe use clear_page instead?

> +               if (!mctx->m_page[i]) {
> +                       for (j = 0; j < i; j++)
> +                               mempool_free(mctx->m_page[j],
> +                                            ufshpb_drv.ufshpb_page_pool);
> +                       goto release_ppn_dirty;
> +               }


> +static inline int ufshpb_add_region(struct ufshpb_lu *hpb,
> +                                   struct ufshpb_region *rgn)
> +{
Maybe better describe what this function does - ufshpb_get_rgn_map_ctx ?

> +
> +static int ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region
> *rgn)
> +{
> +       unsigned long flags;
> +       int ret = 0;
> +
> +       spin_lock_irqsave(&hpb->hpb_state_lock, flags);
> +       if (rgn->rgn_state == HPB_RGN_PINNED) {
> +               dev_warn(&hpb->hpb_lu_dev,
> +                        "pinned region cannot drop-out. region %d\n",
> +                        rgn->rgn_idx);
> +               goto out;
> +       }
> +
> +       if (!list_empty(&rgn->list_lru_rgn)) {
> +               if (ufshpb_check_issue_state_srgns(hpb, rgn)) {
So if one of its subregions has inflight map request - you add it to the "starved" list?
Why call it starved?


> +static int ufshpb_issue_map_req(struct ufshpb_lu *hpb,
> +                               struct ufshpb_region *rgn,
> +                               struct ufshpb_subregion *srgn)
> +{
> +       struct ufshpb_req *map_req;
> +       unsigned long flags;
> +       int ret = 0;
> +
> +       spin_lock_irqsave(&hpb->hpb_state_lock, flags);
> +       /*
> +        * Since the region state change occurs only in the hpb task-work,
> +        * the state of the region cannot HPB_RGN_INACTIVE at this point.
> +        * The region state must be changed in the hpb task-work
I think that you called this worker map_work?


> +               spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +               ret = ufshpb_add_region(hpb, rgn);
If this is not an active region,
Although the device indicated to activate a specific subregion, 
You are activating all the subregions of that region.
You should elaborate on that in your commit log,
and explain why this is the correct activation course.

> +       /*
> +        * If the active region and the inactive region are the same,
> +        * we will inactivate this region.
> +        * The device could check this (region inactivated) and
> +        * will response the proper active region information
> +        */
> +       spin_lock(&hpb->rsp_list_lock);
> +       for (i = 0; i < rsp_field->active_rgn_cnt; i++) {
> +               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);
get_unaligned instead of be16_to_cpu ?

> +
> +               dev_dbg(&hpb->hpb_lu_dev, "activate(%d) region %d - %d\n",
> +                       i, rgn_idx, srgn_idx);
> +               ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
> +               atomic_inc(&hpb->stats.rb_active_cnt);
> +       }
> +
> +       for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
> +               rgn_idx = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
> +               dev_dbg(&hpb->hpb_lu_dev, "inactivate(%d) region %d\n",
> +                       i, rgn_idx);
> +               ufshpb_update_inactive_info(hpb, rgn_idx);
> +               atomic_inc(&hpb->stats.rb_inactive_cnt);
> +       }
> +       spin_unlock(&hpb->rsp_list_lock);
> +
> +       dev_dbg(&hpb->hpb_lu_dev, "Noti: #ACT %u #INACT %u\n",
> +               rsp_field->active_rgn_cnt, rsp_field->inactive_rgn_cnt);
> +
> +       queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
> +}
> +
> +/* routine : isr (ufs) */
> +static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> +{
> +       struct ufshpb_lu *hpb;
> +       struct ufshpb_rsp_field *rsp_field;
> +       int data_seg_len, ret;
> +
> +       data_seg_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2)
> +               & MASK_RSP_UPIU_DATA_SEG_LEN;
get_unaligned instead of be32_to_cpu ?

> +
> +       if (!data_seg_len) {
data_seg_len should be DEV_DATA_SEG_LEN, and you should also check HPB_UPDATE_ALERT,
which you might want to do here and not in ufshpb_may_field_valid

> +               if (!ufshpb_is_general_lun(lrbp->lun))
> +                       return;
> +
> +               hpb = ufshpb_get_hpb_data(lrbp->cmd);
> +               ret = ufshpb_lu_get(hpb);
> +               if (ret)
> +                       return;
> +
> +               if (!ufshpb_is_empty_rsp_lists(hpb))
> +                       queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
> +
> +               goto put_hpb;
> +       }
> +
> +       rsp_field = ufshpb_get_hpb_rsp(lrbp);
> +       if (ufshpb_may_field_valid(hba, lrbp, rsp_field))
> +               return;
> +
> +       hpb = ufshpb_get_hpb_data(lrbp->cmd);
> +       ret = ufshpb_lu_get(hpb);
> +       if (ret)
> +               return;
> +
> +       atomic_inc(&hpb->stats.rb_noti_cnt);
> +
> +       switch (rsp_field->hpb_type) {
> +       case HPB_RSP_REQ_REGION_UPDATE:
> +               WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
> +               ufshpb_rsp_req_region_update(hpb, rsp_field);
> +               break;
What about hpb dev reset - oper 0x2?


> +       default:
> +               dev_notice(&hpb->hpb_lu_dev, "hpb_type is not available: %d\n",
> +                          rsp_field->hpb_type);
> +               break;
> +       }
> +put_hpb:
> +       ufshpb_lu_put(hpb);
> +}
> +


> +static void ufshpb_add_active_list(struct ufshpb_lu *hpb,
> +                                  struct ufshpb_region *rgn,
> +                                  struct ufshpb_subregion *srgn)
> +{
> +       if (!list_empty(&rgn->list_inact_rgn))
> +               return;
> +
> +       if (!list_empty(&srgn->list_act_srgn)) {
> +               list_move(&srgn->list_act_srgn, &hpb->lh_act_srgn);
Why is this needed?
Why updating this subregion position?

> +               return;
> +       }
> +
> +       list_add(&srgn->list_act_srgn, &hpb->lh_act_srgn);
> +}


> @@ -195,8 +1047,15 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba
> *hba, struct ufshpb_lu *hpb)
>  release_srgn_table:
>         for (i = 0; i < rgn_idx; i++) {
>                 rgn = rgn_table + i;
> -               if (rgn->srgn_tbl)
> +               if (rgn->srgn_tbl) {
> +                       for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt;
> +                            srgn_idx++) {
> +                               srgn = rgn->srgn_tbl + srgn_idx;
> +                               if (srgn->mctx)
How is it even possible that on init there is an active subregion?
ufshpb_init_pinned_active_region does its own cleanup.

> +       hpb->m_page_cache = kmem_cache_create("ufshpb_m_page_cache",
> +                         sizeof(struct page *) * hpb->pages_per_srgn,
> +                         0, 0, NULL);
What is the advantage in using an array of page pointers,
Instead of a single pointer to pages_per_srgn?

 

> @@ -398,6 +1326,9 @@ static void ufshpb_resume(struct ufs_hba *hba)
> 
>                 dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
>                 ufshpb_set_state(hpb, HPB_PRESENT);
> +               if (!ufshpb_is_empty_rsp_lists(hpb))
> +                       queue_work(ufshpb_drv.ufshpb_wq, &hpb->map_work);
Ahha - so you are using the ufs driver pm flows to poll your work queue.
Why device recommendations isn't enough?

> +
>                 ufshpb_lu_put(hpb);
>         }
>  }

Thanks,
Avri

Re: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Bart Van Assche]

On 2020-06-06 11:26, Avri Altman wrote:
>> +       data_seg_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2)
>> +               & MASK_RSP_UPIU_DATA_SEG_LEN;
> get_unaligned instead of be32_to_cpu ?

Since sparse checks that the argument of be32_to_cpu() has type __be32
and since no such check is performed for get_unaligned_*(), please keep
the be32_to_cpu().

Thanks,

Bart.

Re: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Bart Van Assche]

On 2020-06-04 18:56, Daejun Park wrote:
> +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);
> +	if (!map_req)
> +		return NULL;
> +
> +	req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
> +			      REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
> +	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;
> +	}
> +
> +	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;
> +	map_req->lun = hpb->lun;
> +
> +	return map_req;
> +free_map_req:
> +	kmem_cache_free(hpb->map_req_cache, map_req);
> +	return NULL;
> +}

Will blk_get_request() fail if all tags have been allocated? Can that
cause a deadlock or infinite loop?

> +static inline void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
> +					   int srgn_idx, int srgn_mem_size)
> +{
> +	cdb[0] = UFSHPB_READ_BUFFER;
> +	cdb[1] = UFSHPB_READ_BUFFER_ID;
> +
> +	put_unaligned_be32(srgn_mem_size, &cdb[5]);
> +	/* cdb[5] = 0x00; */
> +	put_unaligned_be16(rgn_idx, &cdb[2]);
> +	put_unaligned_be16(srgn_idx, &cdb[4]);
> +
> +	cdb[9] = 0x00;
> +}

So the put_unaligned_be32(srgn_mem_size, &cdb[5]) comes first because
the put_unaligned_be16(srgn_idx, &cdb[4]) overwrites byte cdb[5]? That
is really ugly. Please use put_unaligned_be24() instead if that is what
you meant and keep the put_*() calls in increasing cdb offset order.

> +static int ufshpb_map_req_add_bio_page(struct ufshpb_lu *hpb,
> +				       struct request_queue *q, struct bio *bio,
> +				       struct ufshpb_map_ctx *mctx)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < hpb->pages_per_srgn; i++) {
> +		ret = bio_add_pc_page(q, bio, mctx->m_page[i], PAGE_SIZE, 0);
> +		if (ret != PAGE_SIZE) {
> +			dev_notice(&hpb->hpb_lu_dev,
> +				   "bio_add_pc_page fail %d\n", ret);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}

Why bio_add_pc_page() instead of bio_add_page()?

> +static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
> +				  struct ufshpb_req *map_req)
> +{
> +	struct request_queue *q;
> +	struct request *req;
> +	struct scsi_request *rq;
> +	int ret = 0;
> +
> +	q = hpb->sdev_ufs_lu->request_queue;
> +	ret = ufshpb_map_req_add_bio_page(hpb, q, map_req->bio,
> +					  map_req->mctx);
> +	if (ret) {
> +		dev_notice(&hpb->hpb_lu_dev,
> +			   "map_req_add_bio_page fail %d - %d\n",
> +			   map_req->rgn_idx, map_req->srgn_idx);
> +		return ret;
> +	}
> +
> +	req = map_req->req;
> +
> +	blk_rq_append_bio(req, &map_req->bio);
> +	req->rq_flags |= RQF_QUIET;
> +	req->timeout = MAP_REQ_TIMEOUT;
> +	req->end_io_data = (void *)map_req;
> +
> +	rq = scsi_req(req);
> +	ufshpb_set_read_buf_cmd(rq->cmd, map_req->rgn_idx,
> +				map_req->srgn_idx, hpb->srgn_mem_size);
> +	rq->cmd_len = HPB_READ_BUFFER_CMD_LENGTH;
> +
> +	blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_map_req_compl_fn);
> +
> +	atomic_inc(&hpb->stats.map_req_cnt);
> +	return 0;
> +}

Why RQF_QUIET?

Why a custom timeout instead of the SCSI LUN timeout?

Can this function be made asynchronous such that it does not have to be
executed on the context of a workqueue?

Thanks,

Bart.

[RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Daejun Park]

This patch changes the read I/O to the HPB read I/O.

If the logical address of the read I/O belongs to active sub-region, the
HPB driver modifies the read I/O command to HPB read. It modifies the upiu
command of UFS instead of modifying the existing SCSI command.

In the HPB version 1.0, the maximum read I/O size that can be converted to
HPB read is 4KB.

The dirty map of the active sub-region prevents an incorrect HPB read that
has stale physical page number which is updated by previous write I/O.

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/scsi/ufs/ufshpb.c | 249 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 249 insertions(+)

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index f1aa8e7b5ce0..b3e488ef8675 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -46,6 +46,35 @@ static struct ufshpb_driver ufshpb_drv;
 
 static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb);
 
+static inline int ufshpb_is_valid_srgn(struct ufshpb_region *rgn,
+			     struct ufshpb_subregion *srgn)
+{
+	return rgn->rgn_state != HPB_RGN_INACTIVE &&
+		srgn->srgn_state == HPB_SRGN_CLEAN;
+}
+
+static inline bool ufshpb_is_read_cmd(struct scsi_cmnd *cmd)
+{
+	if (cmd->cmnd[0] == READ_10 || cmd->cmnd[0] == READ_16)
+		return true;
+
+	return false;
+}
+
+static inline bool ufshpb_is_write_discard_cmd(struct scsi_cmnd *cmd)
+{
+	if (cmd->cmnd[0] == WRITE_10 || cmd->cmnd[0] == WRITE_16 ||
+	    cmd->cmnd[0] == UNMAP)
+		return true;
+
+	return false;
+}
+
+static inline bool ufshpb_is_support_chunk(int transfer_len)
+{
+	return transfer_len <= HPB_MULTI_CHUNK_HIGH;
+}
+
 static inline bool ufshpb_is_general_lun(int lun)
 {
 	return lun < UFS_UPIU_MAX_UNIT_NUM_ID;
@@ -137,6 +166,225 @@ static inline void ufshpb_lu_put(struct ufshpb_lu *hpb)
 	put_device(&hpb->hpb_lu_dev);
 }
 
+static inline u32 ufshpb_get_lpn(struct scsi_cmnd *cmnd)
+{
+	return blk_rq_pos(cmnd->request) >>
+		(ilog2(cmnd->device->sector_size) - 9);
+}
+
+static inline unsigned int ufshpb_get_len(struct scsi_cmnd *cmnd)
+{
+	return blk_rq_sectors(cmnd->request) >>
+		(ilog2(cmnd->device->sector_size) - 9);
+}
+
+static void ufshpb_set_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
+			     int srgn_idx, int srgn_offset, int cnt)
+{
+	struct ufshpb_region *rgn;
+	struct ufshpb_subregion *srgn;
+	int set_bit_len;
+	int bitmap_len = hpb->entries_per_srgn;
+
+next_srgn:
+	rgn = hpb->rgn_tbl + rgn_idx;
+	srgn = rgn->srgn_tbl + srgn_idx;
+
+	if ((srgn_offset + cnt) > bitmap_len)
+		set_bit_len = bitmap_len - srgn_offset;
+	else
+		set_bit_len = cnt;
+
+	if (rgn->rgn_state != HPB_RGN_INACTIVE)
+		bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
+
+	srgn_offset = 0;
+	if (++srgn_idx == hpb->srgns_per_rgn) {
+		srgn_idx = 0;
+		rgn_idx++;
+	}
+
+	cnt -= set_bit_len;
+	if (cnt > 0)
+		goto next_srgn;
+
+	WARN_ON(cnt < 0);
+}
+
+static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
+				   int srgn_idx, int srgn_offset, int cnt)
+{
+	struct ufshpb_region *rgn;
+	struct ufshpb_subregion *srgn;
+	int bitmap_len = hpb->entries_per_srgn;
+	int i, bit_len;
+
+next_srgn:
+	rgn = hpb->rgn_tbl + rgn_idx;
+	srgn = rgn->srgn_tbl + srgn_idx;
+
+	if (!ufshpb_is_valid_srgn(rgn, srgn))
+		return true;
+
+	/*
+	 * If the region state is active, mctx must be allocated.
+	 * In this case, check whether the region is evicted or
+	 * mctx allcation fail.
+	 */
+	WARN_ON(!srgn->mctx);
+
+	if ((srgn_offset + cnt) > bitmap_len)
+		bit_len = bitmap_len - srgn_offset;
+	else
+		bit_len = cnt;
+
+	for (i = 0; i < bit_len; i++) {
+		if (test_bit(srgn_offset + i, srgn->mctx->ppn_dirty))
+			return true;
+	}
+
+	srgn_offset = 0;
+	if (++srgn_idx == hpb->srgns_per_rgn) {
+		srgn_idx = 0;
+		rgn_idx++;
+	}
+
+	cnt -= bit_len;
+	if (cnt > 0)
+		goto next_srgn;
+
+	return false;
+}
+
+static u64 ufshpb_get_ppn(struct ufshpb_lu *hpb,
+			  struct ufshpb_map_ctx *mctx, int pos, int *error)
+{
+	u64 *ppn_table;
+	struct page *page;
+	int index, offset;
+
+	index = pos / (PAGE_SIZE / HPB_ENTRY_SIZE);
+	offset = pos % (PAGE_SIZE / HPB_ENTRY_SIZE);
+
+	page = mctx->m_page[index];
+	if (unlikely(!page)) {
+		*error = -ENOMEM;
+		dev_err(&hpb->hpb_lu_dev,
+			"error. cannot find page in mctx\n");
+		return 0;
+	}
+
+	ppn_table = page_address(page);
+	if (unlikely(!ppn_table)) {
+		*error = -ENOMEM;
+		dev_err(&hpb->hpb_lu_dev, "error. cannot get ppn_table\n");
+		return 0;
+	}
+
+	return ppn_table[offset];
+}
+
+static inline void
+ufshpb_get_pos_from_lpn(struct ufshpb_lu *hpb, unsigned long lpn, int *rgn_idx,
+			int *srgn_idx, int *offset)
+{
+	int rgn_offset;
+
+	*rgn_idx = lpn >> hpb->entries_per_rgn_shift;
+	rgn_offset = lpn & hpb->entries_per_rgn_mask;
+	*srgn_idx = rgn_offset >> hpb->entries_per_srgn_shift;
+	*offset = rgn_offset & hpb->entries_per_srgn_mask;
+}
+
+static void
+ufshpb_set_hpb_read_to_upiu(struct ufshpb_lu *hpb, struct ufshcd_lrb *lrbp,
+				  u32 lpn, u64 ppn,  unsigned int transfer_len)
+{
+	unsigned char *cdb = lrbp->ucd_req_ptr->sc.cdb;
+
+	cdb[0] = UFSHPB_READ;
+
+	put_unaligned_be32(lpn, &cdb[2]);
+	put_unaligned_be64(ppn, &cdb[6]);
+	cdb[14] = transfer_len;
+}
+
+/* routine : READ10 -> HPB_READ  */
+static void ufshpb_prep_fn(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+	struct ufshpb_lu *hpb;
+	struct ufshpb_region *rgn;
+	struct ufshpb_subregion *srgn;
+	struct scsi_cmnd *cmd = lrbp->cmd;
+	u32 lpn;
+	u64 ppn;
+	unsigned long flags;
+	int transfer_len, rgn_idx, srgn_idx, srgn_offset;
+	int err = 0;
+
+	hpb = ufshpb_get_hpb_data(cmd);
+	err = ufshpb_lu_get(hpb);
+	if (unlikely(err))
+		return;
+
+	WARN_ON(hpb->lun != cmd->device->lun);
+	if (!ufshpb_is_write_discard_cmd(cmd) &&
+	    !ufshpb_is_read_cmd(cmd))
+		goto put_hpb;
+
+	transfer_len = ufshpb_get_len(cmd);
+	if (unlikely(!transfer_len))
+		goto put_hpb;
+
+	lpn = ufshpb_get_lpn(cmd);
+	ufshpb_get_pos_from_lpn(hpb, lpn, &rgn_idx, &srgn_idx, &srgn_offset);
+	rgn = hpb->rgn_tbl + rgn_idx;
+	srgn = rgn->srgn_tbl + srgn_idx;
+
+	/* If commnad type is WRITE and DISCARD, set bitmap as drity */
+	if (ufshpb_is_write_discard_cmd(cmd)) {
+		spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+		ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
+				 transfer_len);
+		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+		goto put_hpb;
+	}
+
+	WARN_ON(!ufshpb_is_read_cmd(cmd));
+
+	if (!ufshpb_is_support_chunk(transfer_len))
+		goto put_hpb;
+
+	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
+	if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
+				   transfer_len)) {
+		atomic_inc(&hpb->stats.miss_cnt);
+		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+		goto put_hpb;
+	}
+
+	ppn = ufshpb_get_ppn(hpb, srgn->mctx, srgn_offset, &err);
+	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
+	if (unlikely(err)) {
+		/*
+		 * In this case, the region state is active,
+		 * but the ppn table is not allocated.
+		 * Make sure that ppn tabie must be allocated on
+		 * active state
+		 */
+		WARN_ON(true);
+		dev_err(&hpb->hpb_lu_dev,
+			"ufshpb_get_ppn failed. err %d\n", err);
+		goto put_hpb;
+	}
+
+	ufshpb_set_hpb_read_to_upiu(hpb, lrbp, lpn, ppn, transfer_len);
+
+	atomic_inc(&hpb->stats.hit_cnt);
+put_hpb:
+	ufshpb_lu_put(hpb);
+}
+
 static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
 					     struct ufshpb_subregion *srgn)
 {
@@ -1688,6 +1936,7 @@ static struct ufshpb_driver ufshpb_drv = {
 		.bus = &ufsf_bus_type,
 	},
 	.ufshpb_ops = {
+		.prep_fn = ufshpb_prep_fn,
 		.reset = ufshpb_reset,
 		.reset_host = ufshpb_reset_host,
 		.suspend = ufshpb_suspend,
-- 
2.17.1

RE: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Avri Altman]

> +static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
> +                                  int srgn_idx, int srgn_offset, int cnt)


> +
> +       for (i = 0; i < bit_len; i++) {
> +               if (test_bit(srgn_offset + i, srgn->mctx->ppn_dirty))
Maybe use a mask or hweight instead of testing bit by bit?

> +static void
> +ufshpb_set_hpb_read_to_upiu(struct ufshpb_lu *hpb, struct ufshcd_lrb
> *lrbp,
> +                                 u32 lpn, u64 ppn,  unsigned int transfer_len)
> +{
> +       unsigned char *cdb = lrbp->ucd_req_ptr->sc.cdb;
> +
> +       cdb[0] = UFSHPB_READ;
> +
> +       put_unaligned_be32(lpn, &cdb[2]);
Is this needed? The lba is already occupying bytes 2..5

> +       put_unaligned_be64(ppn, &cdb[6]);
> +       cdb[14] = transfer_len;
> +}
> +

Thanks,
Avri

Re: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Bart Van Assche]

On 2020-06-06 11:38, Avri Altman wrote:
>> +       for (i = 0; i < bit_len; i++) {
>> +               if (test_bit(srgn_offset + i, srgn->mctx->ppn_dirty))
>
> Maybe use a mask or hweight instead of testing bit by bit?

How about using find_next_bit() from include/linux/bitmap.h?

/*
 *  find_next_bit(addr, nbits, bit) Position next set bit in *addr
 *                                  >= bit
 */

Thanks,

Bart.

Re: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Bart Van Assche]

On 2020-06-04 19:12, Daejun Park wrote:
> +static inline bool ufshpb_is_read_cmd(struct scsi_cmnd *cmd)
> +{
> +	if (cmd->cmnd[0] == READ_10 || cmd->cmnd[0] == READ_16)
> +		return true;
> +
> +	return false;
> +}

Has it been considered to check req_op(cmd->request) instead of checking
the SCSI CDB?

> +static inline bool ufshpb_is_write_discard_cmd(struct scsi_cmnd *cmd)
> +{
> +	if (cmd->cmnd[0] == WRITE_10 || cmd->cmnd[0] == WRITE_16 ||
> +	    cmd->cmnd[0] == UNMAP)
> +		return true;
> +
> +	return false;
> +}

Does the above code depend on how the sd driver translates write and
discard requests? Do UFS devices support the WRITE SAME SCSI command?
Has it been considered to check req_op(cmd->request) instead of checking
the SCSI CDB?

> +static inline bool ufshpb_is_support_chunk(int transfer_len)
> +{
> +	return transfer_len <= HPB_MULTI_CHUNK_HIGH;
> +}

The names used in the above function are mysterious. What is a support
chunk? What does "multi chunk high" mean? Please add a comment.

> +static inline u32 ufshpb_get_lpn(struct scsi_cmnd *cmnd)
> +{
> +	return blk_rq_pos(cmnd->request) >>
> +		(ilog2(cmnd->device->sector_size) - 9);
> +}
>
> +static inline unsigned int ufshpb_get_len(struct scsi_cmnd *cmnd)
> +{
> +	return blk_rq_sectors(cmnd->request) >>
> +		(ilog2(cmnd->device->sector_size) - 9);
> +}

Do the above two functions perhaps each include a duplicate of
sectors_to_logical()?

> +/* routine : READ10 -> HPB_READ  */
> +static void ufshpb_prep_fn(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> +{
> +	struct ufshpb_lu *hpb;
> +	struct ufshpb_region *rgn;
> +	struct ufshpb_subregion *srgn;
> +	struct scsi_cmnd *cmd = lrbp->cmd;
> +	u32 lpn;
> +	u64 ppn;
> +	unsigned long flags;
> +	int transfer_len, rgn_idx, srgn_idx, srgn_offset;
> +	int err = 0;
> +
> +	hpb = ufshpb_get_hpb_data(cmd);
> +	err = ufshpb_lu_get(hpb);
> +	if (unlikely(err))
> +		return;
> +
> +	WARN_ON(hpb->lun != cmd->device->lun);
        ^^^^^^^
        WARN_ON_ONCE()?

> +	if (!ufshpb_is_write_discard_cmd(cmd) &&
> +	    !ufshpb_is_read_cmd(cmd))
> +		goto put_hpb;
> +
> +	transfer_len = ufshpb_get_len(cmd);
> +	if (unlikely(!transfer_len))
> +		goto put_hpb;
> +
> +	lpn = ufshpb_get_lpn(cmd);
> +	ufshpb_get_pos_from_lpn(hpb, lpn, &rgn_idx, &srgn_idx, &srgn_offset);
> +	rgn = hpb->rgn_tbl + rgn_idx;
> +	srgn = rgn->srgn_tbl + srgn_idx;
> +
> +	/* If commnad type is WRITE and DISCARD, set bitmap as drity */
              ^^^^^^^               ^^^                        ^^^^^
              command?              or?                        dirty?
> +	if (ufshpb_is_write_discard_cmd(cmd)) {
> +		spin_lock_irqsave(&hpb->hpb_state_lock, flags);
> +		ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
> +				 transfer_len);
> +		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +		goto put_hpb;
> +	}
> +
> +	WARN_ON(!ufshpb_is_read_cmd(cmd));
        ^^^^^^^
        WARN_ON_ONCE()?

> +	if (!ufshpb_is_support_chunk(transfer_len))
> +		goto put_hpb;
> +
> +	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
> +	if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
> +				   transfer_len)) {
> +		atomic_inc(&hpb->stats.miss_cnt);
> +		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +		goto put_hpb;
> +	}
> +
> +	ppn = ufshpb_get_ppn(hpb, srgn->mctx, srgn_offset, &err);
> +	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +	if (unlikely(err)) {
> +		/*
> +		 * In this case, the region state is active,
> +		 * but the ppn table is not allocated.
> +		 * Make sure that ppn tabie must be allocated on
                                      ^^^^^
                                      table?
> +		 * active state
> +		 */
> +		WARN_ON(true);
> +		dev_err(&hpb->hpb_lu_dev,
> +			"ufshpb_get_ppn failed. err %d\n", err);
> +		goto put_hpb;
> +	}
> +
> +	ufshpb_set_hpb_read_to_upiu(hpb, lrbp, lpn, ppn, transfer_len);
> +
> +	atomic_inc(&hpb->stats.hit_cnt);
> +put_hpb:
> +	ufshpb_lu_put(hpb);
> +}

Thanks,

Bart.

RE: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Avri Altman]

> > +static inline bool ufshpb_is_support_chunk(int transfer_len)
> > +{
> > +     return transfer_len <= HPB_MULTI_CHUNK_HIGH;
> > +}
> 
> The names used in the above function are mysterious. What is a support
> chunk? What does "multi chunk high" mean? Please add a comment.
HPB1.0 limits transfer_len to be at most 1.
HPB2.0, which is in its final draft, allows transfer_len to be at most 128,
But introduce some new behavior depends on transfer_len.
This is just preparing for that.

Thanks,
Avri

RE: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Daejun Park]

> > +static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
> > +                                  int srgn_idx, int srgn_offset, int cnt)

> > +
> > +       for (i = 0; i < bit_len; i++) {
> > +               if (test_bit(srgn_offset + i, srgn->mctx->ppn_dirty))
> Maybe use a mask or hweight instead of testing bit by bit?
There is no problem in this HPB vesion because it only supports 4KB sized read IO.
However, this code is not as efficient as you pointed out. So I will change this in HPB version 2.0.

> > +static void
> > +ufshpb_set_hpb_read_to_upiu(struct ufshpb_lu *hpb, struct ufshcd_lrb
> > *lrbp,
> > +                                 u32 lpn, u64 ppn,  unsigned int transfer_len)
> > +{
> > +       unsigned char *cdb = lrbp->ucd_req_ptr->sc.cdb;
> > +
> > +       cdb[0] = UFSHPB_READ;
> > +
> > +       put_unaligned_be32(lpn, &cdb[2]);
> Is this needed? The lba is already occupying bytes 2..5
The needless code will be deleted on next patch.

Thanks,
Daejun

RE: RE: [RFC PATCH 4/5] scsi: ufs: L2P map management for HPB read

[Daejun Park]

> This is not a concern, just a question.
> If a map request started while runtime/system suspend, can you share its flow?
When suspended, the worker is cancled. And it can just 
process pending active/inactive list after resume.

Thanks,
Daejun

Re: [RFC PATCH 0/5] scsi: ufs: Add Host Performance Booster Support

[Bean Huo]

Hi Daejun

Nice to see your patch, I just run it on my testing workspace, work.
and in the next days, I can help you review your patch.

Thanks,
Bean
 

On Fri, 2020-06-05 at 10:16 +0900, Daejun Park wrote:
> NAND flash memory-based storage devices use Flash Translation Layer
> (FTL)
> to translate logical addresses of I/O requests to corresponding flash
> memory addresses. Mobile storage devices typically have RAM with
> constrained size, thus lack in memory to keep the whole mapping
> table.
> Therefore, mapping tables are partially retrieved from NAND flash on
> demand, causing random-read performance degradation.
> 
> To improve random read performance, we propose HPB (Host Performance
> Booster) which uses host system memory as a cache for the FTL mapping
> table. By using HPB, FTL data can be read from host memory faster
> than from
> NAND flash memory. 
> 
> The current version only supports the DCM (device control mode).
> This patch consists of 4 parts to support HPB feature.
> 
> 1) UFS-feature layer
> 2) HPB probe and initialization process
> 3) READ -> HPB READ using cached map information
> 4) L2P (logical to physical) map management
> 
> The UFS-feature is an additional layer to avoid the structure in
> which the
> UFS-core driver and the UFS-feature are entangled with each other in
> a 
> single module.
> By adding the layer, UFS-features composed of various combinations
> can be
> supported. Also, even if a new feature is added, modification of the 
> UFS-core driver can be minimized.
> 
> In the HPB probe and init process, the device information of the UFS
> is
> queried. After checking supported features, the data structure for
> the HPB
> is initialized according to the device information.
> 
> A read I/O in the active sub-region where the map is cached is
> changed to
> HPB READ by the HPB module.
> 
> The HPB module manages the L2P map using information received from
> the
> device. For active sub-region, the HPB module caches through
> ufshpb_map
> request. For the in-active region, the HPB module discards the L2P
> map.
> When a write I/O occurs in an active sub-region area, associated
> dirty
> bitmap checked as dirty for preventing stale read.
> 
> HPB is shown to have a performance improvement of 58 - 67% for random
> read
> workload. [1]
> 
> This series patches are based on the "5.8/scsi-queue" branch.
> 
> [1]:
> 
https://www.usenix.org/conference/hotstorage17/program/presentation/jeong
> 
> Daejun park (5):
>  scsi: ufs: Add UFS feature related parameter
>  scsi: ufs: Add UFS feature layer
>  scsi: ufs: Introduce HPB module
>  scsi: ufs: L2P map management for HPB read
>  scsi: ufs: Prepare HPB read for cached sub-region
>  
>  drivers/scsi/ufs/Kconfig      |    8 +
>  drivers/scsi/ufs/Makefile     |    3 +-
>  drivers/scsi/ufs/ufs.h        |   11 +
>  drivers/scsi/ufs/ufsfeature.c |  178 ++++
>  drivers/scsi/ufs/ufsfeature.h |   95 ++
>  drivers/scsi/ufs/ufshcd.c     |   19 +
>  drivers/scsi/ufs/ufshcd.h     |    3 +
>  drivers/scsi/ufs/ufshpb.c     | 2029
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshpb.h     |  257 ++++++
>  9 files changed, 2602 insertions(+), 1 deletion(-)
>  created mode 100644 drivers/scsi/ufs/ufsfeature.c
>  created mode 100644 drivers/scsi/ufs/ufsfeature.h
>  created mode 100644 drivers/scsi/ufs/ufshpb.c
>  created mode 100644 drivers/scsi/ufs/ufshpb.h

[RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

This is a patch for the HPB module.
The HPB module queries UFS for device information during initialization.
We added the export symbol to two functions in ufshcd.c to initialize
the HPB module.

The HPB module can be loaded or built-in as needed.
The memory pool size used in the HPB module is implemented as a module
parameter, so that it can be configurable by the user.

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/scsi/ufs/Kconfig  |   9 +
 drivers/scsi/ufs/Makefile |   1 +
 drivers/scsi/ufs/ufshcd.c |   2 +
 drivers/scsi/ufs/ufshpb.c | 783 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshpb.h | 185 +++++++++
 5 files changed, 980 insertions(+)
 create mode 100644 drivers/scsi/ufs/ufshpb.c
 create mode 100644 drivers/scsi/ufs/ufshpb.h

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index e2005aeddc2d..f7c79c369f1d 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -160,3 +160,12 @@ config SCSI_UFS_BSG
 
 	  Select this if you need a bsg device node for your UFS controller.
 	  If unsure, say N.
+
+config UFSHPB
+	tristate "Support UFS Host Performance Booster"
+	depends on SCSI_UFSHCD
+	help
+	  A UFS HPB Feature improves random read performance. It caches
+	  L2P map of UFS to host DRAM. The driver uses HPB read command
+	  by piggybacking physical page number for bypassing FTL's L2P address
+	  translation.
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index fe3a92b06c87..0baf28d674c6 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
 ufshcd-core-y				+= ufshcd.o ufs-sysfs.o ufsfeature.o
 ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
+obj-$(CONFIG_UFSHPB) += ufshpb.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
 obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index de57ba2a0b03..37416ead337b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2855,6 +2855,7 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 	ufshcd_release(hba);
 	return err;
 }
+EXPORT_SYMBOL(ufshcd_query_flag);
 
 /**
  * ufshcd_query_attr - API function for sending attribute requests
@@ -3053,6 +3054,7 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
 
 	return err;
 }
+EXPORT_SYMBOL(ufshcd_query_descriptor_retry);
 
 /**
  * ufshcd_read_desc_length - read the specified descriptor length from header
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
new file mode 100644
index 000000000000..cb0ad4d16d0f
--- /dev/null
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -0,0 +1,783 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Universal Flash Storage Host Performance Booster
+ *
+ * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *	Yongmyung Lee <ymhungry.lee@samsung.com>
+ *	Jinyoung Choi <j-young.choi@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * See the COPYING file in the top-level directory or visit
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This program is provided "AS IS" and "WITH ALL FAULTS" and
+ * without warranty of any kind. You are solely responsible for
+ * determining the appropriateness of using and distributing
+ * the program and assume all risks associated with your exercise
+ * of rights with respect to the program, including but not limited
+ * to infringement of third party rights, the risks and costs of
+ * program errors, damage to or loss of data, programs or equipment,
+ * and unavailability or interruption of operations. Under no
+ * circumstances will the contributor of this Program be liable for
+ * any damages of any kind arising from your use or distribution of
+ * this program.
+ *
+ * The Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/async.h>
+
+#include "ufshcd.h"
+#include "ufshpb.h"
+
+static struct ufshpb_driver ufshpb_drv;
+
+static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb);
+
+static inline int ufshpb_get_state(struct ufshpb_lu *hpb)
+{
+	return atomic_read(&hpb->hpb_state);
+}
+
+static inline void ufshpb_set_state(struct ufshpb_lu *hpb, int state)
+{
+	atomic_set(&hpb->hpb_state, state);
+}
+
+static inline int ufshpb_lu_get_dev(struct ufshpb_lu *hpb)
+{
+	if (get_device(&hpb->hpb_lu_dev))
+		return 0;
+
+	return -ENODEV;
+}
+
+static inline int ufshpb_lu_get(struct ufshpb_lu *hpb)
+{
+	if (!hpb || (ufshpb_get_state(hpb) != HPB_PRESENT))
+		return -ENODEV;
+
+	if (ufshpb_lu_get_dev(hpb))
+		return -ENODEV;
+
+	return 0;
+}
+
+static inline void ufshpb_lu_put(struct ufshpb_lu *hpb)
+{
+	put_device(&hpb->hpb_lu_dev);
+}
+
+static void ufshpb_init_subregion_tbl(struct ufshpb_lu *hpb,
+				      struct ufshpb_region *rgn)
+{
+	int srgn_idx;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		struct ufshpb_subregion *srgn = rgn->srgn_tbl + srgn_idx;
+
+		srgn->rgn_idx = rgn->rgn_idx;
+		srgn->srgn_idx = srgn_idx;
+		srgn->srgn_state = HPB_SRGN_UNUSED;
+	}
+}
+
+static inline int ufshpb_alloc_subregion_tbl(struct ufshpb_lu *hpb,
+					     struct ufshpb_region *rgn,
+					     int srgn_cnt)
+{
+	rgn->srgn_tbl = kvcalloc(srgn_cnt, sizeof(struct ufshpb_subregion),
+				 GFP_KERNEL);
+	if (!rgn->srgn_tbl)
+		return -ENOMEM;
+
+	rgn->srgn_cnt = srgn_cnt;
+	return 0;
+}
+
+static void ufshpb_init_lu_parameter(struct ufs_hba *hba,
+				     struct ufshpb_lu *hpb,
+				     struct ufshpb_dev_info *hpb_dev_info,
+				     struct ufshpb_lu_info *hpb_lu_info)
+{
+	u32 entries_per_rgn;
+	u64 rgn_mem_size;
+
+
+	hpb->lu_pinned_start = hpb_lu_info->pinned_start;
+	hpb->lu_pinned_end = hpb_lu_info->num_pinned ?
+		(hpb_lu_info->pinned_start + hpb_lu_info->num_pinned - 1)
+		: PINNED_NOT_SET;
+
+	rgn_mem_size = (1ULL << hpb_dev_info->rgn_size) * HPB_RGN_SIZE_UNIT
+		/ HPB_ENTRY_BLOCK_SIZE * HPB_ENTRY_SIZE;
+	hpb->srgn_mem_size = (1ULL << hpb_dev_info->srgn_size)
+		* HPB_RGN_SIZE_UNIT / HPB_ENTRY_BLOCK_SIZE * HPB_ENTRY_SIZE;
+
+	entries_per_rgn = rgn_mem_size / HPB_ENTRY_SIZE;
+	hpb->entries_per_rgn_shift = ilog2(entries_per_rgn);
+	hpb->entries_per_rgn_mask = entries_per_rgn - 1;
+
+	hpb->entries_per_srgn = hpb->srgn_mem_size /  HPB_ENTRY_SIZE;
+	hpb->entries_per_srgn_shift = ilog2(hpb->entries_per_srgn);
+	hpb->entries_per_srgn_mask = hpb->entries_per_srgn - 1;
+
+	hpb->srgns_per_rgn = rgn_mem_size / hpb->srgn_mem_size;
+
+	hpb->rgns_per_lu = DIV_ROUND_UP(hpb_lu_info->num_blocks,
+				(rgn_mem_size / HPB_ENTRY_SIZE));
+	hpb->srgns_per_lu = DIV_ROUND_UP(hpb_lu_info->num_blocks,
+				(hpb->srgn_mem_size / HPB_ENTRY_SIZE));
+
+	hpb->pages_per_srgn = hpb->srgn_mem_size / PAGE_SIZE;
+
+	dev_info(hba->dev, "ufshpb(%d): region memory size - %llu (bytes)\n",
+		 hpb->lun, rgn_mem_size);
+	dev_info(hba->dev, "ufshpb(%d): subregion memory size - %u (bytes)\n",
+		 hpb->lun, hpb->srgn_mem_size);
+	dev_info(hba->dev, "ufshpb(%d): total blocks per lu - %d\n",
+		 hpb->lun, hpb_lu_info->num_blocks);
+	dev_info(hba->dev, "ufshpb(%d): subregions per region - %d, regions per lu - %u",
+		 hpb->lun, hpb->srgns_per_rgn, hpb->rgns_per_lu);
+}
+
+
+static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
+{
+	struct ufshpb_region *rgn_table, *rgn;
+	struct ufshpb_subregion *srgn;
+	int rgn_idx, srgn_idx, total_srgn_cnt, srgn_cnt, i;
+	int ret = 0;
+
+	rgn_table = kvcalloc(hpb->rgns_per_lu, sizeof(struct ufshpb_region),
+			    GFP_KERNEL);
+	if (!rgn_table)
+		return -ENOMEM;
+
+	hpb->rgn_tbl = rgn_table;
+
+	total_srgn_cnt = hpb->srgns_per_lu;
+	for (rgn_idx = 0, srgn_cnt = 0; rgn_idx < hpb->rgns_per_lu;
+	     rgn_idx++, total_srgn_cnt -= srgn_cnt) {
+		rgn = rgn_table + rgn_idx;
+		rgn->rgn_idx = rgn_idx;
+
+		srgn_cnt = min(total_srgn_cnt, hpb->srgns_per_rgn);
+
+		ret = ufshpb_alloc_subregion_tbl(hpb, rgn, srgn_cnt);
+		if (ret)
+			goto release_srgn_table;
+		ufshpb_init_subregion_tbl(hpb, rgn);
+
+		rgn->rgn_state = HPB_RGN_INACTIVE;
+		}
+	}
+
+	if (total_srgn_cnt != 0) {
+		dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
+			hpb->lun, total_srgn_cnt);
+		goto release_srgn_table;
+	}
+
+	return 0;
+release_srgn_table:
+	for (i = 0; i < rgn_idx; i++) {
+		rgn = rgn_table + i;
+		if (rgn->srgn_tbl)
+			kvfree(rgn->srgn_tbl);
+	}
+	kvfree(rgn_table);
+	return ret;
+}
+
+static void ufshpb_destroy_subregion_tbl(struct ufshpb_lu *hpb,
+					 struct ufshpb_region *rgn)
+{
+	int srgn_idx;
+
+	for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
+		struct ufshpb_subregion *srgn;
+
+		srgn = rgn->srgn_tbl + srgn_idx;
+		srgn->srgn_state = HPB_SRGN_UNUSED;
+	}
+}
+
+static void ufshpb_destroy_region_tbl(struct ufshpb_lu *hpb)
+{
+	int rgn_idx;
+
+	for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
+		struct ufshpb_region *rgn;
+
+		rgn = hpb->rgn_tbl + rgn_idx;
+		if (rgn->rgn_state != HPB_RGN_INACTIVE) {
+			rgn->rgn_state = HPB_RGN_INACTIVE;
+
+			ufshpb_destroy_subregion_tbl(hpb, rgn);
+		}
+
+		kvfree(rgn->srgn_tbl);
+	}
+
+	kvfree(hpb->rgn_tbl);
+}
+
+static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb,
+			      struct ufshpb_dev_info *hpb_dev_info)
+{
+	int ret;
+
+	spin_lock_init(&hpb->hpb_state_lock);
+
+	ret = ufshpb_alloc_region_tbl(hba, hpb);
+	if (ret)
+		return ret;
+
+	ret = ufshpb_create_sysfs(hba, hpb);
+	if (ret)
+		goto release_rgn_table;
+
+	return 0;
+
+release_rgn_table:
+	ufshpb_destroy_region_tbl(hpb);
+	return ret;
+}
+
+static struct ufshpb_lu *ufshpb_alloc_hpb_lu(struct ufs_hba *hba, int lun,
+				     struct ufshpb_dev_info *hpb_dev_info,
+				     struct ufshpb_lu_info *hpb_lu_info)
+{
+	struct ufshpb_lu *hpb;
+	int ret;
+
+	hpb = kzalloc(sizeof(struct ufshpb_lu), GFP_KERNEL);
+	if (!hpb)
+		return NULL;
+
+	hpb->ufsf = &hba->ufsf;
+	hpb->lun = lun;
+
+	ufshpb_init_lu_parameter(hba, hpb, hpb_dev_info, hpb_lu_info);
+
+	ret = ufshpb_lu_hpb_init(hba, hpb, hpb_dev_info);
+	if (ret) {
+		dev_err(hba->dev, "hpb lu init failed. ret %d", ret);
+		goto release_hpb;
+	}
+
+	return hpb;
+release_hpb:
+	kfree(hpb);
+	return NULL;
+}
+
+static void ufshpb_lu_release(struct ufshpb_lu *hpb)
+{
+	ufshpb_destroy_region_tbl(hpb);
+
+	list_del_init(&hpb->list_hpb_lu);
+}
+
+static void ufshpb_issue_hpb_reset_query(struct ufs_hba *hba)
+{
+	int err;
+	bool flag_res = true;
+	int try = 0;
+	int retries;
+
+	for (retries = 0; retries < HPB_RESET_REQ_RETRIES; retries++) {
+		err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
+				QUERY_FLAG_IDN_HPB_RESET, 0, NULL);
+		if (err)
+			dev_dbg(hba->dev,
+				"%s: failed with error %d, retries %d\n",
+				__func__, err, retries);
+		else
+			break;
+	}
+
+	if (err) {
+		dev_err(hba->dev,
+			"%s setting fHpbReset flag failed with error %d\n",
+			__func__, err);
+		return;
+	}
+	/* wait for the device to complete HPB reset query */
+	do {
+		if (++try == HPB_RESET_REQ_RETRIES)
+			break;
+
+		dev_info(hba->dev,
+			"%s start flag reset polling %d times\n",
+			__func__, try);
+
+		/* Poll fHpbReset flag to be cleared */
+		err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
+				QUERY_FLAG_IDN_HPB_RESET, 0, &flag_res);
+		usleep_range(1000, 1100);
+	} while (flag_res);
+
+	if (err) {
+		dev_err(hba->dev,
+			"%s reading fHpbReset flag failed with error %d\n",
+			__func__, err);
+		return;
+	}
+
+	if (flag_res) {
+		dev_err(hba->dev,
+			"%s fHpbReset was not cleared by the device\n",
+			__func__);
+	}
+}
+
+static void ufshpb_reset(struct ufs_hba *hba)
+{
+	struct ufshpb_lu *hpb;
+
+	list_for_each_entry(hpb, &ufshpb_drv.lh_hpb_lu, list_hpb_lu) {
+		if (ufshpb_lu_get_dev(hpb))
+			continue;
+
+		ufshpb_set_state(hpb, HPB_PRESENT);
+		ufshpb_lu_put(hpb);
+	}
+}
+
+static void ufshpb_reset_host(struct ufs_hba *hba)
+{
+	struct ufshpb_lu *hpb;
+
+	list_for_each_entry(hpb, &ufshpb_drv.lh_hpb_lu, list_hpb_lu) {
+		if (ufshpb_lu_get(hpb))
+			continue;
+
+		dev_info(&hpb->hpb_lu_dev, "ufshpb run reset_host");
+
+		ufshpb_set_state(hpb, HPB_RESET);
+		ufshpb_lu_put(hpb);
+	}
+}
+
+static void ufshpb_suspend(struct ufs_hba *hba)
+{
+	struct ufshpb_lu *hpb;
+
+	list_for_each_entry(hpb, &ufshpb_drv.lh_hpb_lu, list_hpb_lu) {
+		if (ufshpb_lu_get(hpb))
+			continue;
+
+		dev_info(&hpb->hpb_lu_dev, "ufshpb goto suspend");
+		ufshpb_set_state(hpb, HPB_SUSPEND);
+
+		ufshpb_lu_put(hpb);
+	}
+}
+
+static void ufshpb_resume(struct ufs_hba *hba)
+{
+	struct ufshpb_lu *hpb;
+
+	list_for_each_entry(hpb, &ufshpb_drv.lh_hpb_lu, list_hpb_lu) {
+		if (ufshpb_lu_get_dev(hpb))
+			continue;
+
+		dev_info(&hpb->hpb_lu_dev, "ufshpb resume");
+		ufshpb_set_state(hpb, HPB_PRESENT);
+		ufshpb_lu_put(hpb);
+	}
+}
+
+static void ufshpb_stat_init(struct ufshpb_lu *hpb)
+{
+	atomic_set(&hpb->stats.hit_cnt, 0);
+	atomic_set(&hpb->stats.miss_cnt, 0);
+	atomic_set(&hpb->stats.rb_noti_cnt, 0);
+	atomic_set(&hpb->stats.rb_active_cnt, 0);
+	atomic_set(&hpb->stats.rb_inactive_cnt, 0);
+	atomic_set(&hpb->stats.map_req_cnt, 0);
+}
+
+/* SYSFS functions */
+#define ufshpb_sysfs_attr_show_func(__name)				\
+static ssize_t ufshpb_sysfs_show_##__name(struct device *dev,		\
+					 struct device_attribute *attr,	\
+					 char *buf)			\
+{									\
+	struct ufshpb_lu *hpb;						\
+	hpb = container_of(dev, struct ufshpb_lu, hpb_lu_dev);		\
+	return snprintf(buf, PAGE_SIZE, "%d\n",			\
+			atomic_read(&hpb->stats.__name));		\
+}
+
+ufshpb_sysfs_attr_show_func(hit_cnt);
+ufshpb_sysfs_attr_show_func(miss_cnt);
+ufshpb_sysfs_attr_show_func(rb_noti_cnt);
+ufshpb_sysfs_attr_show_func(rb_active_cnt);
+ufshpb_sysfs_attr_show_func(rb_inactive_cnt);
+ufshpb_sysfs_attr_show_func(map_req_cnt);
+
+static struct device_attribute ufshpb_sysfs_entries[] = {
+	__ATTR(hit_count, 0444, ufshpb_sysfs_show_hit_cnt, NULL),
+	__ATTR(miss_count, 0444, ufshpb_sysfs_show_miss_cnt, NULL),
+	__ATTR(rb_noti_count, 0444, ufshpb_sysfs_show_rb_noti_cnt, NULL),
+	__ATTR(rb_active_count, 0444, ufshpb_sysfs_show_rb_active_cnt, NULL),
+	__ATTR(rb_inactive_count, 0444, ufshpb_sysfs_show_rb_inactive_cnt,
+	       NULL),
+	__ATTR(map_req_count, 0444, ufshpb_sysfs_show_map_req_cnt, NULL),
+	__ATTR_NULL
+};
+
+static inline void ufshpb_dev_release(struct device *dev)
+{
+	struct ufs_hba *hba;
+	struct ufsf_feature_info *ufsf;
+	struct ufshpb_lu *hpb;
+
+	hpb = container_of(dev, struct ufshpb_lu, hpb_lu_dev);
+	ufsf = hpb->ufsf;
+	hba = container_of(ufsf, struct ufs_hba, ufsf);
+
+	ufshpb_lu_release(hpb);
+	dev_info(dev, "%s: release success\n", __func__);
+	put_device(dev->parent);
+
+	kfree(hpb);
+}
+
+static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
+{
+	struct device_attribute *attr;
+	int ret;
+
+	device_initialize(&hpb->hpb_lu_dev);
+
+	ufshpb_stat_init(hpb);
+
+	hpb->hpb_lu_dev.parent = get_device(&hba->ufsf.hpb_dev);
+	hpb->hpb_lu_dev.release = ufshpb_dev_release;
+	dev_set_name(&hpb->hpb_lu_dev, "ufshpb_lu%d", hpb->lun);
+
+	ret = device_add(&hpb->hpb_lu_dev);
+	if (ret) {
+		dev_err(hba->dev, "ufshpb(%d) device_add failed",
+			hpb->lun);
+		return -ENODEV;
+	}
+
+	for (attr = ufshpb_sysfs_entries; attr->attr.name != NULL; attr++) {
+		if (device_create_file(&hpb->hpb_lu_dev, attr))
+			dev_err(hba->dev, "ufshpb(%d) %s create file error\n",
+				hpb->lun, attr->attr.name);
+	}
+
+	return 0;
+}
+
+static int ufshpb_read_desc(struct ufs_hba *hba, u8 desc_id, u8 desc_index,
+			  u8 selector, u8 *desc_buf, u32 size)
+{
+	int err = 0;
+
+	pm_runtime_get_sync(hba->dev);
+
+	err = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
+					    desc_id, desc_index,
+					    selector,
+					    desc_buf, &size);
+	if (err)
+		dev_err(hba->dev, "read desc failed: %d, id %d, idx %d\n",
+			err, desc_id, desc_index);
+
+	pm_runtime_put_sync(hba->dev);
+
+	return err;
+}
+
+static int ufshpb_get_geo_info(struct ufs_hba *hba, u8 *geo_buf,
+			       struct ufshpb_dev_info *hpb_dev_info)
+{
+	int hpb_device_max_active_rgns = 0;
+	int hpb_num_lu;
+
+	hpb_dev_info->max_num_lun =
+		geo_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0x00 ? 8 : 32;
+
+	hpb_num_lu = geo_buf[GEOMETRY_DESC_HPB_NUMBER_LU];
+	if (hpb_num_lu == 0) {
+		dev_err(hba->dev, "No HPB LU supported\n");
+		return -ENODEV;
+	}
+
+	hpb_dev_info->rgn_size = geo_buf[GEOMETRY_DESC_HPB_REGION_SIZE];
+	hpb_dev_info->srgn_size = geo_buf[GEOMETRY_DESC_HPB_SUBREGION_SIZE];
+	hpb_device_max_active_rgns =
+		get_unaligned_be16(geo_buf +
+			GEOMETRY_DESC_HPB_DEVICE_MAX_ACTIVE_REGIONS);
+
+	if (hpb_dev_info->rgn_size == 0 || hpb_dev_info->srgn_size == 0 ||
+	    hpb_device_max_active_rgns == 0) {
+		dev_err(hba->dev, "No HPB supported device\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int ufshpb_get_dev_info(struct ufs_hba *hba,
+			       struct ufshpb_dev_info *hpb_dev_info,
+			       u8 *desc_buf)
+{
+	int ret;
+
+	ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, SELECTOR,
+			     desc_buf, hba->desc_size.dev_desc);
+	if (ret) {
+		dev_err(hba->dev, "%s: idn: %d query request failed\n",
+			__func__, QUERY_DESC_IDN_DEVICE);
+		return -ENODEV;
+	}
+
+	/*
+	 * Get the number of user logical unit to check whether all
+	 * scsi_device finish initialization
+	 */
+	hpb_dev_info->num_lu = desc_buf[DEVICE_DESC_PARAM_NUM_LU];
+
+	ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, SELECTOR,
+			       desc_buf, hba->desc_size.geom_desc);
+	if (ret) {
+		dev_err(hba->dev, "%s: idn: %d query request failed\n",
+			__func__, QUERY_DESC_IDN_DEVICE);
+		return ret;
+	}
+
+	ret = ufshpb_get_geo_info(hba, desc_buf, hpb_dev_info);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ufshpb_get_lu_info(struct ufs_hba *hba, int lun,
+				    struct ufshpb_lu_info *hpb_lu_info,
+				    u8 *desc_buf)
+{
+	u16 max_active_rgns;
+	u8 lu_enable;
+	int ret;
+
+	ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_UNIT, lun,
+			       SELECTOR, desc_buf,
+				   hba->desc_size.unit_desc);
+	if (ret) {
+		dev_err(hba->dev,
+			"%s: idn: %d lun: %d  query request failed",
+			__func__, QUERY_DESC_IDN_UNIT, lun);
+		return ret;
+	}
+
+	lu_enable = desc_buf[UNIT_DESC_PARAM_LU_ENABLE];
+	if (lu_enable != LU_ENABLED_HPB_FUNC)
+		return -ENODEV;
+
+	max_active_rgns = get_unaligned_be16(
+			desc_buf + UNIT_DESC_HPB_LU_MAX_ACTIVE_REGIONS);
+	if (!max_active_rgns) {
+		dev_err(hba->dev,
+			"lun %d wrong number of max active regions\n", lun);
+		return -ENODEV;
+	}
+
+	hpb_lu_info->num_blocks = get_unaligned_be64(
+			desc_buf + UNIT_DESC_PARAM_LOGICAL_BLK_COUNT);
+	hpb_lu_info->pinned_start = get_unaligned_be16(
+			desc_buf + UNIT_DESC_HPB_LU_PIN_REGION_START_OFFSET);
+	hpb_lu_info->num_pinned = get_unaligned_be16(
+			desc_buf + UNIT_DESC_HPB_LU_NUM_PIN_REGIONS);
+	hpb_lu_info->max_active_rgns = get_unaligned_be16(
+			desc_buf + UNIT_DESC_HPB_LU_MAX_ACTIVE_REGIONS);
+
+	return 0;
+}
+
+static void ufshpb_scan_hpb_lu(struct ufs_hba *hba,
+			       struct ufshpb_dev_info *hpb_dev_info,
+			       u8 *desc_buf)
+{
+	struct scsi_device *sdev;
+	struct ufshpb_lu *hpb;
+	int find_hpb_lu = 0;
+	int ret;
+
+	INIT_LIST_HEAD(&ufshpb_drv.lh_hpb_lu);
+
+	shost_for_each_device(sdev, hba->host) {
+		struct ufshpb_lu_info hpb_lu_info = { 0 };
+		int lun = sdev->lun;
+
+		if (lun >= hpb_dev_info->max_num_lun)
+			continue;
+
+		ret = ufshpb_get_lu_info(hba, lun, &hpb_lu_info, desc_buf);
+		if (ret)
+			continue;
+
+		hpb = ufshpb_alloc_hpb_lu(hba, lun, hpb_dev_info,
+					  &hpb_lu_info);
+		if (!hpb)
+			continue;
+
+		hpb->sdev_ufs_lu = sdev;
+		sdev->hostdata = hpb;
+
+		list_add_tail(&hpb->list_hpb_lu, &ufshpb_drv.lh_hpb_lu);
+		find_hpb_lu++;
+	}
+
+	if (find_hpb_lu) {
+		ufshpb_issue_hpb_reset_query(hba);
+		dev_set_drvdata(&hba->ufsf.hpb_dev, &ufshpb_drv);
+
+		list_for_each_entry(hpb, &ufshpb_drv.lh_hpb_lu, list_hpb_lu) {
+			dev_info(&hpb->hpb_lu_dev, "set state to present\n");
+			ufshpb_set_state(hpb, HPB_PRESENT);
+		}
+	}
+}
+
+static void ufshpb_probe_async(void *data, async_cookie_t cookie)
+{
+	struct ufshpb_dev_info hpb_dev_info = { 0 };
+	struct ufs_hba *hba = data;
+	char *desc_buf;
+	int ret;
+
+	desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
+	if (!desc_buf)
+		goto release_desc_buf;
+
+	ret = ufshpb_get_dev_info(hba, &hpb_dev_info, desc_buf);
+	if (ret)
+		goto release_desc_buf;
+
+	/*
+	 * Because HPB driver uses scsi_device data structure,
+	 * we should wait at this point until finishing initialization of all
+	 * scsi devices. Even if timeout occurs, HPB driver will search
+	 * the scsi_device list on struct scsi_host (shost->__host list_head)
+	 * and can find out HPB logical units in all scsi_devices
+	 */
+	wait_event_timeout(hba->ufsf.sdev_wait,
+			   (atomic_read(&hba->ufsf.slave_conf_cnt)
+				== hpb_dev_info.num_lu),
+			   SDEV_WAIT_TIMEOUT);
+
+	dev_dbg(hba->dev, "ufshpb: slave count %d, lu count %d\n",
+		atomic_read(&hba->ufsf.slave_conf_cnt), hpb_dev_info.num_lu);
+
+	ufshpb_scan_hpb_lu(hba, &hpb_dev_info, desc_buf);
+release_desc_buf:
+	kfree(desc_buf);
+}
+
+static int ufshpb_probe(struct device *dev)
+{
+	struct ufs_hba *hba;
+	struct ufsf_feature_info *ufsf;
+
+	if (dev->type != &ufshpb_dev_type)
+		return -ENODEV;
+
+	ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
+	hba = container_of(ufsf, struct ufs_hba, ufsf);
+
+	async_schedule(ufshpb_probe_async, hba);
+	return 0;
+}
+
+static int ufshpb_remove(struct device *dev)
+{
+	struct ufshpb_lu *hpb, *n_hpb;
+	struct ufsf_feature_info *ufsf;
+	struct scsi_device *sdev;
+
+	ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
+
+	dev_set_drvdata(&ufsf->hpb_dev, NULL);
+
+	list_for_each_entry_safe(hpb, n_hpb, &ufshpb_drv.lh_hpb_lu,
+				 list_hpb_lu) {
+		ufshpb_set_state(hpb, HPB_FAILED);
+
+		sdev = hpb->sdev_ufs_lu;
+		sdev->hostdata = NULL;
+
+		device_del(&hpb->hpb_lu_dev);
+
+		dev_info(&hpb->hpb_lu_dev, "hpb_lu_dev refcnt %d\n",
+			 kref_read(&hpb->hpb_lu_dev.kobj.kref));
+		put_device(&hpb->hpb_lu_dev);
+	}
+	dev_info(dev, "ufshpb: remove success\n");
+
+	return 0;
+}
+
+static struct ufshpb_driver ufshpb_drv = {
+	.drv = {
+		.name = "ufshpb_driver",
+		.owner = THIS_MODULE,
+		.probe = ufshpb_probe,
+		.remove = ufshpb_remove,
+		.bus = &ufsf_bus_type,
+	},
+	.ufshpb_ops = {
+		.reset = ufshpb_reset,
+		.reset_host = ufshpb_reset_host,
+		.suspend = ufshpb_suspend,
+		.resume = ufshpb_resume,
+	},
+};
+
+unsigned int ufshpb_host_map_kbytes = 1 * 1024;
+module_param(ufshpb_host_map_kbytes, uint, 0644);
+MODULE_PARM_DESC(ufshpb_host_map_kbytes,
+	 "ufshpb host mapping memory kilo-bytes for ufshpb memory-pool");
+
+static int __init ufshpb_init(void)
+{
+	int ret;
+
+	ret = driver_register(&ufshpb_drv.drv);
+	if (ret)
+		pr_err("ufshpb: driver register failed\n");
+	return ret;
+}
+
+static void __exit ufshpb_exit(void)
+{
+	driver_unregister(&ufshpb_drv.drv);
+}
+
+MODULE_AUTHOR("Yongmyong Lee <ymhungry.lee@samsung.com>");
+MODULE_AUTHOR("Jinyoung Choi <j-young.choi@samsung.com>");
+MODULE_DESCRIPTION("UFS Host Performance Booster Driver");
+
+module_init(ufshpb_init);
+module_exit(ufshpb_exit);
+MODULE_LICENSE("GPL");
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
new file mode 100644
index 000000000000..c6dd88e00849
--- /dev/null
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -0,0 +1,185 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Universal Flash Storage Host Performance Booster
+ *
+ * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *	Yongmyung Lee <ymhungry.lee@samsung.com>
+ *	Jinyoung Choi <j-young.choi@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * See the COPYING file in the top-level directory or visit
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This program is provided "AS IS" and "WITH ALL FAULTS" and
+ * without warranty of any kind. You are solely responsible for
+ * determining the appropriateness of using and distributing
+ * the program and assume all risks associated with your exercise
+ * of rights with respect to the program, including but not limited
+ * to infringement of third party rights, the risks and costs of
+ * program errors, damage to or loss of data, programs or equipment,
+ * and unavailability or interruption of operations. Under no
+ * circumstances will the contributor of this Program be liable for
+ * any damages of any kind arising from your use or distribution of
+ * this program.
+ *
+ * The Linux Foundation chooses to take subject only to the GPLv2
+ * license terms, and distributes only under these terms.
+ */
+
+#ifndef _UFSHPB_H_
+#define _UFSHPB_H_
+
+/* hpb response UPIU macro */
+#define MAX_ACTIVE_NUM				2
+#define MAX_INACTIVE_NUM			2
+#define HPB_RSP_NONE				0x00
+#define HPB_RSP_REQ_REGION_UPDATE		0x01
+#define DEV_DATA_SEG_LEN			0x14
+#define DEV_SENSE_SEG_LEN			0x12
+#define DEV_DES_TYPE				0x80
+#define DEV_ADDITIONAL_LEN			0x10
+
+/* hpb map & entries macro */
+#define HPB_RGN_SIZE_UNIT			512
+#define HPB_ENTRY_BLOCK_SIZE			4096
+#define HPB_ENTRY_SIZE				0x8
+#define PINNED_NOT_SET				U32_MAX
+
+/* hpb support chunk size */
+#define HPB_MULTI_CHUNK_HIGH			1
+
+/* 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			0x02
+#define HPB_READ_BUFFER_CMD_LENGTH		10
+#define LU_ENABLED_HPB_FUNC			0x02
+
+#define SDEV_WAIT_TIMEOUT			(10 * HZ)
+#define MAP_REQ_TIMEOUT				(30 * HZ)
+#define HPB_RESET_REQ_RETRIES			10
+#define HPB_RESET_REQ_MSLEEP			2
+
+#define SELECTOR 0
+
+enum UFSHPB_STATE {
+	HPB_PRESENT = 1,
+	HPB_SUSPEND,
+	HPB_FAILED,
+	HPB_RESET,
+};
+
+enum HPB_RGN_STATE {
+	HPB_RGN_INACTIVE,
+	HPB_RGN_ACTIVE,
+	/* pinned regions are always active */
+	HPB_RGN_PINNED,
+};
+
+enum HPB_SRGN_STATE {
+	HPB_SRGN_UNUSED,
+	HPB_SRGN_DIRTY,
+	HPB_SRGN_CLEAN,
+	HPB_SRGN_ISSUED,
+};
+
+/**
+ * struct ufshpb_dev_info - UFSHPB device related info
+ * @max_num_lun: maximum number of logical unit that HPB is supported
+ * @num_ln: the number of user logical unit to check whether all lu finished
+ *          initialization
+ * @rgn_size: device reported HPB region size
+ * @srgn_size: device reported HPB sub-region size
+ */
+struct ufshpb_dev_info {
+	int max_num_lun;
+	int num_lu;
+	int rgn_size;
+	int srgn_size;
+};
+
+/**
+ * struct ufshpb_lu_info - UFSHPB logical unit related info
+ * @num_blocks: the number of logical block
+ * @pinned_start: the start region number of pinned region
+ * @num_pinned: the number of pinned regions
+ * @max_active_rgns: maximum number of active regions
+ */
+struct ufshpb_lu_info {
+	int num_blocks;
+	int pinned_start;
+	int num_pinned;
+	int max_active_rgns;
+};
+
+struct ufshpb_subregion {
+	enum HPB_SRGN_STATE srgn_state;
+	int rgn_idx;
+	int srgn_idx;
+};
+
+struct ufshpb_region {
+	struct ufshpb_subregion *srgn_tbl;
+	enum HPB_RGN_STATE rgn_state;
+	int rgn_idx;
+	int srgn_cnt;
+};
+
+struct ufshpb_stats {
+	atomic_t hit_cnt;
+	atomic_t miss_cnt;
+	atomic_t rb_noti_cnt;
+	atomic_t rb_active_cnt;
+	atomic_t rb_inactive_cnt;
+	atomic_t map_req_cnt;
+};
+
+struct ufshpb_lu {
+	int lun;
+
+	struct device hpb_lu_dev;
+	struct scsi_device *sdev_ufs_lu;
+
+	struct ufshpb_region *rgn_tbl;
+
+	spinlock_t hpb_state_lock;
+	atomic_t hpb_state; /* hpb_state_lock */
+
+	/* pinned region information */
+	u32 lu_pinned_start;
+	u32 lu_pinned_end;
+
+	/* HPB related configuration */
+	u32 rgns_per_lu;
+	u32 srgns_per_lu;
+	int srgns_per_rgn;
+	u32 srgn_mem_size;
+	u32 entries_per_rgn_mask;
+	u32 entries_per_rgn_shift;
+	u32 entries_per_srgn;
+	u32 entries_per_srgn_mask;
+	u32 entries_per_srgn_shift;
+	u32 pages_per_srgn;
+
+	struct ufshpb_stats stats;
+
+	struct ufsf_feature_info *ufsf;
+	struct list_head list_hpb_lu;
+};
+
+extern struct device_type ufshpb_dev_type;
+extern struct bus_type ufsf_bus_type;
+
+#endif /* End of Header */
-- 
2.17.1

RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Avri Altman]

 
> 
> This is a patch for the HPB module.
> The HPB module queries UFS for device information during initialization.
> We added the export symbol to two functions in ufshcd.c to initialize
> the HPB module.
> 
> The HPB module can be loaded or built-in as needed.
> The memory pool size used in the HPB module is implemented as a module
> parameter, so that it can be configurable by the user.
Why not just allow for max-active-regions per the device's configuration?
The platform vendor can provision it per its need.


> +
> +static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu
> *hpb)
> +{
> +       struct ufshpb_region *rgn_table, *rgn;
> +       struct ufshpb_subregion *srgn;
> +       int rgn_idx, srgn_idx, total_srgn_cnt, srgn_cnt, i;
> +       int ret = 0;
> +
> +       rgn_table = kvcalloc(hpb->rgns_per_lu, sizeof(struct ufshpb_region),
> +                           GFP_KERNEL);
> +       if (!rgn_table)
> +               return -ENOMEM;
> +
> +       hpb->rgn_tbl = rgn_table;
> +
> +       total_srgn_cnt = hpb->srgns_per_lu;
> +       for (rgn_idx = 0, srgn_cnt = 0; rgn_idx < hpb->rgns_per_lu;
> +            rgn_idx++, total_srgn_cnt -= srgn_cnt) {
Maybe simplify and improve readability by moving the srgn_cnt into the for clause:
	int srgn_cnt = hpb->srgns_per_rgn;

> +               rgn = rgn_table + rgn_idx;
> +               rgn->rgn_idx = rgn_idx;
> +
> +               srgn_cnt = min(total_srgn_cnt, hpb->srgns_per_rgn);
I guess you are carefully counting the sbregions because the spec allows the lun not to be subregion aligned.
So for any region but the last its hpb->srgns_per_rgn, and for the last one its:
	If (rgn_idx == hpb->rgns_per_lu - 1)
		srgn_cnt = ((hpb->srgns_per_lu - 1) % hpb->srgns_per_rgn) + 1;

> +
> +               ret = ufshpb_alloc_subregion_tbl(hpb, rgn, srgn_cnt);
> +               if (ret)
> +                       goto release_srgn_table;
> +               ufshpb_init_subregion_tbl(hpb, rgn);
> +
> +               rgn->rgn_state = HPB_RGN_INACTIVE;
> +               }
> +       }
> +
> +       if (total_srgn_cnt != 0) {
And you won't be needing this anymore

> +               dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
> +                       hpb->lun, total_srgn_cnt);
> +               goto release_srgn_table;
> +       }
> +
> +       return 0;
> +release_srgn_table:
> +       for (i = 0; i < rgn_idx; i++) {
> +               rgn = rgn_table + i;
> +               if (rgn->srgn_tbl)
> +                       kvfree(rgn->srgn_tbl);
> +       }
> +       kvfree(rgn_table);
> +       return ret;
> +}
> +
> +static void ufshpb_destroy_subregion_tbl(struct ufshpb_lu *hpb,
> +                                        struct ufshpb_region *rgn)
> +{
> +       int srgn_idx;
> +
> +       for (srgn_idx = 0; srgn_idx < rgn->srgn_cnt; srgn_idx++) {
> +               struct ufshpb_subregion *srgn;
> +
> +               srgn = rgn->srgn_tbl + srgn_idx;
> +               srgn->srgn_state = HPB_SRGN_UNUSED;
> +       }
> +}
> +
> +static void ufshpb_destroy_region_tbl(struct ufshpb_lu *hpb)
> +{
> +       int rgn_idx;
> +
> +       for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
> +               struct ufshpb_region *rgn;
> +
> +               rgn = hpb->rgn_tbl + rgn_idx;
> +               if (rgn->rgn_state != HPB_RGN_INACTIVE) {
> +                       rgn->rgn_state = HPB_RGN_INACTIVE;
> +
> +                       ufshpb_destroy_subregion_tbl(hpb, rgn);
> +               }
> +
> +               kvfree(rgn->srgn_tbl);
This looks like it belongs to ufshpb_destroy_subregion_tbl?

> +       }
> +
> +       kvfree(hpb->rgn_tbl);
> +}
> +


> +static void ufshpb_issue_hpb_reset_query(struct ufs_hba *hba)

> +               return;
> +       }
> +       /* wait for the device to complete HPB reset query */
How about calling ufshpb_issue_hpb_reset_query right after ufshpb_get_dev_info?
This way waiting for the device to complete its reset can be done while scsi is scanning the luns?

> +
> +static void ufshpb_reset(struct ufs_hba *hba)
> +static void ufshpb_reset_host(struct ufs_hba *hba)
> +static void ufshpb_suspend(struct ufs_hba *hba)
> +static void ufshpb_resume(struct ufs_hba *hba)
The above 4 functions essentially runs the same code but set a different state.
Maybe call a helper?

> +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
Why separate from ufs-sysfs?
Also you might want to introduce all the stats not as part of the functional patch.
> +
> +static int ufshpb_get_geo_info(struct ufs_hba *hba, u8 *geo_buf,
> +                              struct ufshpb_dev_info *hpb_dev_info)
> +{
> +       int hpb_device_max_active_rgns = 0;
> +       int hpb_num_lu;
> +
> +       hpb_dev_info->max_num_lun =
> +               geo_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0x00 ? 8 :
> 32;
You already have this in hba->dev_info.max_lu_supported
> +
> +       hpb_num_lu = geo_buf[GEOMETRY_DESC_HPB_NUMBER_LU];
You should capture hpb_dev_info->max_num_lun = hpb_num_lu

> +       if (hpb_num_lu == 0) {
> +               dev_err(hba->dev, "No HPB LU supported\n");
> +               return -ENODEV;
> +       }
> +
> +       hpb_dev_info->rgn_size =
> geo_buf[GEOMETRY_DESC_HPB_REGION_SIZE];
> +       hpb_dev_info->srgn_size =
> geo_buf[GEOMETRY_DESC_HPB_SUBREGION_SIZE];
> +       hpb_device_max_active_rgns =
> +               get_unaligned_be16(geo_buf +
> +                       GEOMETRY_DESC_HPB_DEVICE_MAX_ACTIVE_REGIONS);
> +
> +       if (hpb_dev_info->rgn_size == 0 || hpb_dev_info->srgn_size == 0 ||
> +           hpb_device_max_active_rgns == 0) {
> +               dev_err(hba->dev, "No HPB supported device\n");
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ufshpb_get_dev_info(struct ufs_hba *hba,
> +                              struct ufshpb_dev_info *hpb_dev_info,
> +                              u8 *desc_buf)
> +{
> +       int ret;
> +
> +       ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, SELECTOR,
> +                            desc_buf, hba->desc_size.dev_desc);
What with this SELECTOR stuff?
Why not the default 0?

> +       if (ret) {
> +               dev_err(hba->dev, "%s: idn: %d query request failed\n",
> +                       __func__, QUERY_DESC_IDN_DEVICE);
> +               return -ENODEV;
> +       }
> +
> +       /*
> +        * Get the number of user logical unit to check whether all
> +        * scsi_device finish initialization
> +        */
> +       hpb_dev_info->num_lu = desc_buf[DEVICE_DESC_PARAM_NUM_LU];
What about the other hpb fields in the device descriptor:
DEVICE_DESC_PARAM_HPB_VER and DEVICE_DESC_PARAM_HPB_CONTROL ?

> +
> +       ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0,
> SELECTOR,
> +                              desc_buf, hba->desc_size.geom_desc);
> +       if (ret) {
> +               dev_err(hba->dev, "%s: idn: %d query request failed\n",
> +                       __func__, QUERY_DESC_IDN_DEVICE);
> +               return ret;
> +       }
> +
> +       ret = ufshpb_get_geo_info(hba, desc_buf, hpb_dev_info);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +       hpb_lu_info->num_blocks = get_unaligned_be64(
> +                       desc_buf + UNIT_DESC_PARAM_LOGICAL_BLK_COUNT);
> +       hpb_lu_info->pinned_start = get_unaligned_be16(
> +                       desc_buf + UNIT_DESC_HPB_LU_PIN_REGION_START_OFFSET);
> +       hpb_lu_info->num_pinned = get_unaligned_be16(
> +                       desc_buf + UNIT_DESC_HPB_LU_NUM_PIN_REGIONS);
> +       hpb_lu_info->max_active_rgns = get_unaligned_be16(
> +                       desc_buf + UNIT_DESC_HPB_LU_MAX_ACTIVE_REGIONS);
You already have it, its max_active_rgns

> +
> +       return 0;
> +}
> +

> +unsigned int ufshpb_host_map_kbytes = 1 * 1024;
> +module_param(ufshpb_host_map_kbytes, uint, 0644);
> +MODULE_PARM_DESC(ufshpb_host_map_kbytes,
> +        "ufshpb host mapping memory kilo-bytes for ufshpb memory-pool");
You should introduce this module parameter in the patch that uses it.

> +
> +/**
> + * struct ufshpb_dev_info - UFSHPB device related info
> + * @max_num_lun: maximum number of logical unit that HPB is supported
> + * @num_ln: the number of user logical unit to check whether all lu finished
Typo num_lu


Thanks,
Avri

RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Avri Altman]

> +static int ufshpb_get_dev_info(struct ufs_hba *hba,
> +                              struct ufshpb_dev_info *hpb_dev_info,
> +                              u8 *desc_buf)
> +{
> +       int ret;
How about here, before doing anything, check that the descriptors are in proper size?

Thanks,
Avri

Re: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Bart Van Assche]

On 2020-06-04 18:38, Daejun Park wrote:
> +	if (total_srgn_cnt != 0) {
> +		dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
> +			hpb->lun, total_srgn_cnt);
> +		goto release_srgn_table;
> +	}
> +
> +	return 0;
> +release_srgn_table:
> +	for (i = 0; i < rgn_idx; i++) {
> +		rgn = rgn_table + i;
> +		if (rgn->srgn_tbl)
> +			kvfree(rgn->srgn_tbl);
> +	}

Please insert a blank line above goto labels as is done in most of the
kernel code.

> +static struct device_attribute ufshpb_sysfs_entries[] = {
> +	__ATTR(hit_count, 0444, ufshpb_sysfs_show_hit_cnt, NULL),
> +	__ATTR(miss_count, 0444, ufshpb_sysfs_show_miss_cnt, NULL),
> +	__ATTR(rb_noti_count, 0444, ufshpb_sysfs_show_rb_noti_cnt, NULL),
> +	__ATTR(rb_active_count, 0444, ufshpb_sysfs_show_rb_active_cnt, NULL),
> +	__ATTR(rb_inactive_count, 0444, ufshpb_sysfs_show_rb_inactive_cnt,
> +	       NULL),
> +	__ATTR(map_req_count, 0444, ufshpb_sysfs_show_map_req_cnt, NULL),
> +	__ATTR_NULL
> +};

Please use __ATTR_RO() where appropriate.

> +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
> +{
> +	struct device_attribute *attr;
> +	int ret;
> +
> +	device_initialize(&hpb->hpb_lu_dev);
> +
> +	ufshpb_stat_init(hpb);
> +
> +	hpb->hpb_lu_dev.parent = get_device(&hba->ufsf.hpb_dev);
> +	hpb->hpb_lu_dev.release = ufshpb_dev_release;
> +	dev_set_name(&hpb->hpb_lu_dev, "ufshpb_lu%d", hpb->lun);
> +
> +	ret = device_add(&hpb->hpb_lu_dev);
> +	if (ret) {
> +		dev_err(hba->dev, "ufshpb(%d) device_add failed",
> +			hpb->lun);
> +		return -ENODEV;
> +	}
> +
> +	for (attr = ufshpb_sysfs_entries; attr->attr.name != NULL; attr++) {
> +		if (device_create_file(&hpb->hpb_lu_dev, attr))
> +			dev_err(hba->dev, "ufshpb(%d) %s create file error\n",
> +				hpb->lun, attr->attr.name);
> +	}
> +
> +	return 0;
> +}

This is the wrong way to create sysfs attributes. Please set the
'groups' member of struct device instead of using a loop to create sysfs
attributes. The former approach is compatible with udev but the latter
approach is not.

> +static void ufshpb_probe_async(void *data, async_cookie_t cookie)
> +{
> +	struct ufshpb_dev_info hpb_dev_info = { 0 };
> +	struct ufs_hba *hba = data;
> +	char *desc_buf;
> +	int ret;
> +
> +	desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
> +	if (!desc_buf)
> +		goto release_desc_buf;
> +
> +	ret = ufshpb_get_dev_info(hba, &hpb_dev_info, desc_buf);
> +	if (ret)
> +		goto release_desc_buf;
> +
> +	/*
> +	 * Because HPB driver uses scsi_device data structure,
> +	 * we should wait at this point until finishing initialization of all
> +	 * scsi devices. Even if timeout occurs, HPB driver will search
> +	 * the scsi_device list on struct scsi_host (shost->__host list_head)
> +	 * and can find out HPB logical units in all scsi_devices
> +	 */
> +	wait_event_timeout(hba->ufsf.sdev_wait,
> +			   (atomic_read(&hba->ufsf.slave_conf_cnt)
> +				== hpb_dev_info.num_lu),
> +			   SDEV_WAIT_TIMEOUT);
> +
> +	dev_dbg(hba->dev, "ufshpb: slave count %d, lu count %d\n",
> +		atomic_read(&hba->ufsf.slave_conf_cnt), hpb_dev_info.num_lu);
> +
> +	ufshpb_scan_hpb_lu(hba, &hpb_dev_info, desc_buf);
> +release_desc_buf:
> +	kfree(desc_buf);
> +}

What happens if two LUNs are added before the above code is woken up?
Will that perhaps cause the wait_event_timeout() call to wait forever?

> +static int ufshpb_probe(struct device *dev)
> +{
> +	struct ufs_hba *hba;
> +	struct ufsf_feature_info *ufsf;
> +
> +	if (dev->type != &ufshpb_dev_type)
> +		return -ENODEV;
> +
> +	ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> +	hba = container_of(ufsf, struct ufs_hba, ufsf);
> +
> +	async_schedule(ufshpb_probe_async, hba);
> +	return 0;
> +}

So this is an asynchronous probe that is not visible to the device
driver core? Could the PROBE_PREFER_ASYNCHRONOUS flag have been used
instead to make device probing asynchronous?

> +static int ufshpb_remove(struct device *dev)
> +{
> +	struct ufshpb_lu *hpb, *n_hpb;
> +	struct ufsf_feature_info *ufsf;
> +	struct scsi_device *sdev;
> +
> +	ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> +
> +	dev_set_drvdata(&ufsf->hpb_dev, NULL);
> +
> +	list_for_each_entry_safe(hpb, n_hpb, &ufshpb_drv.lh_hpb_lu,
> +				 list_hpb_lu) {
> +		ufshpb_set_state(hpb, HPB_FAILED);
> +
> +		sdev = hpb->sdev_ufs_lu;
> +		sdev->hostdata = NULL;
> +
> +		device_del(&hpb->hpb_lu_dev);
> +
> +		dev_info(&hpb->hpb_lu_dev, "hpb_lu_dev refcnt %d\n",
> +			 kref_read(&hpb->hpb_lu_dev.kobj.kref));
> +		put_device(&hpb->hpb_lu_dev);
> +	}
> +	dev_info(dev, "ufshpb: remove success\n");
> +
> +	return 0;
> +}

Where is the code that waits for the asynchronously scheduled probe
calls to finish?

> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> new file mode 100644
> index 000000000000..c6dd88e00849
> --- /dev/null
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Universal Flash Storage Host Performance Booster
> + *
> + * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
> + *
> + * Authors:
> + *	Yongmyung Lee <ymhungry.lee@samsung.com>
> + *	Jinyoung Choi <j-young.choi@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * See the COPYING file in the top-level directory or visit
> + * <http://www.gnu.org/licenses/gpl-2.0.html>
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * This program is provided "AS IS" and "WITH ALL FAULTS" and
> + * without warranty of any kind. You are solely responsible for
> + * determining the appropriateness of using and distributing
> + * the program and assume all risks associated with your exercise
> + * of rights with respect to the program, including but not limited
> + * to infringement of third party rights, the risks and costs of
> + * program errors, damage to or loss of data, programs or equipment,
> + * and unavailability or interruption of operations. Under no
> + * circumstances will the contributor of this Program be liable for
> + * any damages of any kind arising from your use or distribution of
> + * this program.
> + *
> + * The Linux Foundation chooses to take subject only to the GPLv2
> + * license terms, and distributes only under these terms.
> + */

Please use an SPDX declaration instead of the full GPLv2 text.

Thanks,

Bart.

Re: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Bean Huo]

On Tue, 2020-06-09 at 21:29 -0700, Bart Van Assche wrote:
> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > new file mode 100644
> > index 000000000000..c6dd88e00849
> > --- /dev/null
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -0,0 +1,185 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Universal Flash Storage Host Performance Booster
> > + *
> > + * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
> > + *
> > + * Authors:
> > + *   Yongmyung Lee <ymhungry.lee@samsung.com>
> > + *   Jinyoung Choi <j-young.choi@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * See the COPYING file in the top-level directory or visit
> > + * <http://www.gnu.org/licenses/gpl-2.0.html>
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * This program is provided "AS IS" and "WITH ALL FAULTS" and
> > + * without warranty of any kind. You are solely responsible for
> > + * determining the appropriateness of using and distributing
> > + * the program and assume all risks associated with your exercise
> > + * of rights with respect to the program, including but not
> > limited
> > + * to infringement of third party rights, the risks and costs of
> > + * program errors, damage to or loss of data, programs or
> > equipment,
> > + * and unavailability or interruption of operations. Under no
> > + * circumstances will the contributor of this Program be liable
> > for
> > + * any damages of any kind arising from your use or distribution
> > of
> > + * this program.
> > + *
> > + * The Linux Foundation chooses to take subject only to the GPLv2
> > + * license terms, and distributes only under these terms.
> > + */
> 
> Please use an SPDX declaration instead of the full GPLv2 text.
> 
> Thanks,
> 
> Bart.

agree with Bart,
also, should confirm SPDX-License-Identifier: GPL-2.0-only or  SPDX-
License-Identifier: GPL-2.0-later.

I just learnt this, based on your text, shoould be "SPDX-License-
Identifier: GPL-2.0-later"

Bean

Re: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

> > +  if (total_srgn_cnt != 0) {
> > +    dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
> > +      hpb->lun, total_srgn_cnt);
> > +    goto release_srgn_table;
> > +  }
> > +
> > +  return 0;
> > +release_srgn_table:
> > +  for (i = 0; i < rgn_idx; i++) {
> > +    rgn = rgn_table + i;
> > +    if (rgn->srgn_tbl)
> > +      kvfree(rgn->srgn_tbl);
> > +  }

> Please insert a blank line above goto labels as is done in most of the
> kernel code.
OK, I will fix it.

> > +static struct device_attribute ufshpb_sysfs_entries[] = {
> > +  __ATTR(hit_count, 0444, ufshpb_sysfs_show_hit_cnt, NULL),
> > +  __ATTR(miss_count, 0444, ufshpb_sysfs_show_miss_cnt, NULL),
> > +  __ATTR(rb_noti_count, 0444, ufshpb_sysfs_show_rb_noti_cnt, NULL),
> > +  __ATTR(rb_active_count, 0444, ufshpb_sysfs_show_rb_active_cnt, NULL),
> > +  __ATTR(rb_inactive_count, 0444, ufshpb_sysfs_show_rb_inactive_cnt,
> > +         NULL),
> > +  __ATTR(map_req_count, 0444, ufshpb_sysfs_show_map_req_cnt, NULL),
> > +  __ATTR_NULL
> > +};

> Please use __ATTR_RO() where appropriate.
They are only readable attributes. So I changed the code to use __ATTR_RO.

> > +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
> > +{
> > +  struct device_attribute *attr;
> > +  int ret;
> > +
> > +  device_initialize(&hpb->hpb_lu_dev);
> > +
> > +  ufshpb_stat_init(hpb);
> > +
> > +  hpb->hpb_lu_dev.parent = get_device(&hba->ufsf.hpb_dev);
> > +  hpb->hpb_lu_dev.release = ufshpb_dev_release;
> > +  dev_set_name(&hpb->hpb_lu_dev, "ufshpb_lu%d", hpb->lun);
> > +
> > +  ret = device_add(&hpb->hpb_lu_dev);
> > +  if (ret) {
> > +    dev_err(hba->dev, "ufshpb(%d) device_add failed",
> > +      hpb->lun);
> > +    return -ENODEV;
> > +  }
> > +
> > +  for (attr = ufshpb_sysfs_entries; attr->attr.name != NULL; attr++) {
> > +    if (device_create_file(&hpb->hpb_lu_dev, attr))
> > +      dev_err(hba->dev, "ufshpb(%d) %s create file error\n",
> > +        hpb->lun, attr->attr.name);
> > +  }
> > +
> > +  return 0;
> > +}

> This is the wrong way to create sysfs attributes. Please set the
> 'groups' member of struct device instead of using a loop to create sysfs
> attributes. The former approach is compatible with udev but the latter
> approach is not.
OK, I changed to create attributes without loop.

> > +static void ufshpb_probe_async(void *data, async_cookie_t cookie)
> > +{
> > +  struct ufshpb_dev_info hpb_dev_info = { 0 };
> > +  struct ufs_hba *hba = data;
> > +  char *desc_buf;
> > +  int ret;
> > +
> > +  desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
> > +  if (!desc_buf)
> > +    goto release_desc_buf;
> > +
> > +  ret = ufshpb_get_dev_info(hba, &hpb_dev_info, desc_buf);
> > +  if (ret)
> > +    goto release_desc_buf;
> > +
> > +  /*
> > +   * Because HPB driver uses scsi_device data structure,
> > +   * we should wait at this point until finishing initialization of all
> > +   * scsi devices. Even if timeout occurs, HPB driver will search
> > +   * the scsi_device list on struct scsi_host (shost->__host list_head)
> > +   * and can find out HPB logical units in all scsi_devices
> > +   */
> > +  wait_event_timeout(hba->ufsf.sdev_wait,
> > +         (atomic_read(&hba->ufsf.slave_conf_cnt)
> > +        == hpb_dev_info.num_lu),
> > +         SDEV_WAIT_TIMEOUT);
> > +
> > +  dev_dbg(hba->dev, "ufshpb: slave count %d, lu count %d\n",
> > +    atomic_read(&hba->ufsf.slave_conf_cnt), hpb_dev_info.num_lu);
> > +
> > +  ufshpb_scan_hpb_lu(hba, &hpb_dev_info, desc_buf);
> > +release_desc_buf:
> > +  kfree(desc_buf);
> > +}

> What happens if two LUNs are added before the above code is woken up?
> Will that perhaps cause the wait_event_timeout() call to wait forever?
I don't think it is problem. I think that the wait_event_timeout() will
check the condition before waiting.

> > +static int ufshpb_probe(struct device *dev)
> > +{
> > +  struct ufs_hba *hba;
> > +  struct ufsf_feature_info *ufsf;
> > +
> > +  if (dev->type != &ufshpb_dev_type)
> > +    return -ENODEV;
> > +
> > +  ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> > +  hba = container_of(ufsf, struct ufs_hba, ufsf);
> > +
> > +  async_schedule(ufshpb_probe_async, hba);
> > +  return 0;
> > +}

> So this is an asynchronous probe that is not visible to the device
> driver core? Could the PROBE_PREFER_ASYNCHRONOUS flag have been used
> instead to make device probing asynchronous?
I added the PROBE_PREFER_ASYNCHRONOUS flag to code and changed it to 
probe synchronously.
 
> > +static int ufshpb_remove(struct device *dev)
> > +{
> > +  struct ufshpb_lu *hpb, *n_hpb;
> > +  struct ufsf_feature_info *ufsf;
> > +  struct scsi_device *sdev;
> > +
> > +  ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> > +
> > +  dev_set_drvdata(&ufsf->hpb_dev, NULL);
> > +
> > +  list_for_each_entry_safe(hpb, n_hpb, &ufshpb_drv.lh_hpb_lu,
> > +         list_hpb_lu) {
> > +    ufshpb_set_state(hpb, HPB_FAILED);
> > +
> > +    sdev = hpb->sdev_ufs_lu;
> > +    sdev->hostdata = NULL;
> > +
> > +    device_del(&hpb->hpb_lu_dev);
> > +
> > +    dev_info(&hpb->hpb_lu_dev, "hpb_lu_dev refcnt %d\n",
> > +       kref_read(&hpb->hpb_lu_dev.kobj.kref));
> > +    put_device(&hpb->hpb_lu_dev);
> > +  }
> > +  dev_info(dev, "ufshpb: remove success\n");
> > +
> > +  return 0;
> > +}

> Where is the code that waits for the asynchronously scheduled probe
> calls to finish?
I changed it to probe without async_schedule.

> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > new file mode 100644
> > index 000000000000..c6dd88e00849
> > --- /dev/null
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -0,0 +1,185 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Universal Flash Storage Host Performance Booster
> > + *
> > + * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
> > + *
> > + * Authors:
> > + *  Yongmyung Lee <ymhungry.lee@samsung.com>
> > + *  Jinyoung Choi <j-young.choi@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * See the COPYING file in the top-level directory or visit
> > + * <http://www.gnu.org/licenses/gpl-2.0.html>
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * This program is provided "AS IS" and "WITH ALL FAULTS" and
> > + * without warranty of any kind. You are solely responsible for
> > + * determining the appropriateness of using and distributing
> > + * the program and assume all risks associated with your exercise
> > + * of rights with respect to the program, including but not limited
> > + * to infringement of third party rights, the risks and costs of
> > + * program errors, damage to or loss of data, programs or equipment,
> > + * and unavailability or interruption of operations. Under no
> > + * circumstances will the contributor of this Program be liable for
> > + * any damages of any kind arising from your use or distribution of
> > + * this program.
> > + *
> > + * The Linux Foundation chooses to take subject only to the GPLv2
> > + * license terms, and distributes only under these terms.
> > + */

> Please use an SPDX declaration instead of the full GPLv2 text.
OK, I will.

Thanks,
Daejun.

RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

> Why not just allow for max-active-regions per the device's configuration?
> The platform vendor can provision it per its need.
The max-active-region is configured as device config. The module parameter which you mentioned is just minimum value of the memory pool.

> > +
> > +       total_srgn_cnt = hpb->srgns_per_lu;
> > +       for (rgn_idx = 0, srgn_cnt = 0; rgn_idx < hpb->rgns_per_lu;
> > +            rgn_idx++, total_srgn_cnt -= srgn_cnt) {
> Maybe simplify and improve readability by moving the srgn_cnt into the for clause:
> 	int srgn_cnt = hpb->srgns_per_rgn;
OK, I will apply this for patch v2.

> > +
> > +static void ufshpb_destroy_region_tbl(struct ufshpb_lu *hpb)
> > +{
> > +       int rgn_idx;
> > +
> > +       for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
> > +               struct ufshpb_region *rgn;
> > +
> > +               rgn = hpb->rgn_tbl + rgn_idx;
> > +               if (rgn->rgn_state != HPB_RGN_INACTIVE) {
> > +                       rgn->rgn_state = HPB_RGN_INACTIVE;
> > +
> > +                       ufshpb_destroy_subregion_tbl(hpb, rgn);
> > +               }
> > +
> > +               kvfree(rgn->srgn_tbl);
> This looks like it belongs to ufshpb_destroy_subregion_tbl?
Yes, it will be changed.

> How about calling ufshpb_issue_hpb_reset_query right after ufshpb_get_dev_info?
> This way waiting for the device to complete its reset can be done while scsi is scanning the luns?
I will change the call path as follows:
- ufshpb_probe_async
  - ufshpb_get_dev_info
  - ufshpb_issue_hpb_reset_query 1/2 (query part)
  - ufshpb_scan_hpb_lu
  - ufshpb_issue_hpb_reset_query 2/2 (wait part)

> > +
> > +static void ufshpb_reset(struct ufs_hba *hba)
> > +static void ufshpb_reset_host(struct ufs_hba *hba)
> > +static void ufshpb_suspend(struct ufs_hba *hba)
> > +static void ufshpb_resume(struct ufs_hba *hba)
> The above 4 functions essentially runs the same code but set a different state.
> Maybe call a helper?
OK, I will.

> > +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
> Why separate from ufs-sysfs?
> Also you might want to introduce all the stats not as part of the functional patch.
The HPB feature is implemented as a device. So, We added the hpb-sysfs separated from ufs-sysfs.

> > +
> > +static int ufshpb_get_geo_info(struct ufs_hba *hba, u8 *geo_buf,
> > +                              struct ufshpb_dev_info *hpb_dev_info)
> > +{
> > +       int hpb_device_max_active_rgns = 0;
> > +       int hpb_num_lu;
> > +
> > +       hpb_dev_info->max_num_lun =
> > +               geo_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0x00 ? 8 :
> > 32;
> You already have this in hba->dev_info.max_lu_supported
> > +
> > +       hpb_num_lu = geo_buf[GEOMETRY_DESC_HPB_NUMBER_LU];
> You should capture hpb_dev_info->max_num_lun = hpb_num_lu
You are right. And hpb_dev_info->max_num_lun will be deleted.

> > +
> > +       ret = ufshpb_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, SELECTOR,
> > +                            desc_buf, hba->desc_size.dev_desc);
> What with this SELECTOR stuff?
> Why not the default 0?
Right, SELECTOR should be 0. I will fix it. 

> What about the other hpb fields in the device descriptor:
> DEVICE_DESC_PARAM_HPB_VER and DEVICE_DESC_PARAM_HPB_CONTROL ?
I will add codes that checks these fields on initialization.

> > +unsigned int ufshpb_host_map_kbytes = 1 * 1024;
> > +module_param(ufshpb_host_map_kbytes, uint, 0644);
> > +MODULE_PARM_DESC(ufshpb_host_map_kbytes,
> > +        "ufshpb host mapping memory kilo-bytes for ufshpb memory-pool");
> You should introduce this module parameter in the patch that uses it.
OK, could you recommend good location of introducing message? At the patch letter or in the codes?

Thanks,
Daejun

RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Avri Altman]

> > > +unsigned int ufshpb_host_map_kbytes = 1 * 1024;
> > > +module_param(ufshpb_host_map_kbytes, uint, 0644);
> > > +MODULE_PARM_DESC(ufshpb_host_map_kbytes,
> > > +        "ufshpb host mapping memory kilo-bytes for ufshpb memory-pool");
> > You should introduce this module parameter in the patch that uses it.
> OK, could you recommend good location of introducing message? At the
> patch letter or in the codes?
I think this module parameter makes its first appearance in patch 4/5 - so maybe there?

> 
> Thanks,
> Daejun

RE: [RFC PATCH 3/5] scsi: ufs: Introduce HPB module

[Daejun Park]

> > +static int ufshpb_get_dev_info(struct ufs_hba *hba,
> > +                              struct ufshpb_dev_info *hpb_dev_info,
> > +                              u8 *desc_buf)
> > +{
> > +       int ret;
> How about here, before doing anything, check that the descriptors are in proper size?
OK, I will add a size check for the descriptor.

Thanks,
Daejun

Re: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Daejun Park]

On 2020-06-04 18:38, Daejun Park wrote:
> > +  if (total_srgn_cnt != 0) {
> > +    dev_err(hba->dev, "ufshpb(%d) error total_subregion_count %d",
> > +      hpb->lun, total_srgn_cnt);
> > +    goto release_srgn_table;
> > +  }
> > +
> > +  return 0;
> > +release_srgn_table:
> > +  for (i = 0; i < rgn_idx; i++) {
> > +    rgn = rgn_table + i;
> > +    if (rgn->srgn_tbl)
> > +      kvfree(rgn->srgn_tbl);
> > +  }

> Please insert a blank line above goto labels as is done in most of the
> kernel code.
OK, I will fix it.

> > +static struct device_attribute ufshpb_sysfs_entries[] = {
> > +  __ATTR(hit_count, 0444, ufshpb_sysfs_show_hit_cnt, NULL),
> > +  __ATTR(miss_count, 0444, ufshpb_sysfs_show_miss_cnt, NULL),
> > +  __ATTR(rb_noti_count, 0444, ufshpb_sysfs_show_rb_noti_cnt, NULL),
> > +  __ATTR(rb_active_count, 0444, ufshpb_sysfs_show_rb_active_cnt, NULL),
> > +  __ATTR(rb_inactive_count, 0444, ufshpb_sysfs_show_rb_inactive_cnt,
> > +         NULL),
> > +  __ATTR(map_req_count, 0444, ufshpb_sysfs_show_map_req_cnt, NULL),
> > +  __ATTR_NULL
> > +};

> Please use __ATTR_RO() where appropriate.
They are only readable attributes. So I changed the code to use __ATTR_RO.

> > +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb)
> > +{
> > +  struct device_attribute *attr;
> > +  int ret;
> > +
> > +  device_initialize(&hpb->hpb_lu_dev);
> > +
> > +  ufshpb_stat_init(hpb);
> > +
> > +  hpb->hpb_lu_dev.parent = get_device(&hba->ufsf.hpb_dev);
> > +  hpb->hpb_lu_dev.release = ufshpb_dev_release;
> > +  dev_set_name(&hpb->hpb_lu_dev, "ufshpb_lu%d", hpb->lun);
> > +
> > +  ret = device_add(&hpb->hpb_lu_dev);
> > +  if (ret) {
> > +    dev_err(hba->dev, "ufshpb(%d) device_add failed",
> > +      hpb->lun);
> > +    return -ENODEV;
> > +  }
> > +
> > +  for (attr = ufshpb_sysfs_entries; attr->attr.name != NULL; attr++) {
> > +    if (device_create_file(&hpb->hpb_lu_dev, attr))
> > +      dev_err(hba->dev, "ufshpb(%d) %s create file error\n",
> > +        hpb->lun, attr->attr.name);
> > +  }
> > +
> > +  return 0;
> > +}

> This is the wrong way to create sysfs attributes. Please set the
> 'groups' member of struct device instead of using a loop to create sysfs
> attributes. The former approach is compatible with udev but the latter
> approach is not.
OK, I changed to create attributes without loop.

> > +static void ufshpb_probe_async(void *data, async_cookie_t cookie)
> > +{
> > +  struct ufshpb_dev_info hpb_dev_info = { 0 };
> > +  struct ufs_hba *hba = data;
> > +  char *desc_buf;
> > +  int ret;
> > +
> > +  desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
> > +  if (!desc_buf)
> > +    goto release_desc_buf;
> > +
> > +  ret = ufshpb_get_dev_info(hba, &hpb_dev_info, desc_buf);
> > +  if (ret)
> > +    goto release_desc_buf;
> > +
> > +  /*
> > +   * Because HPB driver uses scsi_device data structure,
> > +   * we should wait at this point until finishing initialization of all
> > +   * scsi devices. Even if timeout occurs, HPB driver will search
> > +   * the scsi_device list on struct scsi_host (shost->__host list_head)
> > +   * and can find out HPB logical units in all scsi_devices
> > +   */
> > +  wait_event_timeout(hba->ufsf.sdev_wait,
> > +         (atomic_read(&hba->ufsf.slave_conf_cnt)
> > +        == hpb_dev_info.num_lu),
> > +         SDEV_WAIT_TIMEOUT);
> > +
> > +  dev_dbg(hba->dev, "ufshpb: slave count %d, lu count %d\n",
> > +    atomic_read(&hba->ufsf.slave_conf_cnt), hpb_dev_info.num_lu);
> > +
> > +  ufshpb_scan_hpb_lu(hba, &hpb_dev_info, desc_buf);
> > +release_desc_buf:
> > +  kfree(desc_buf);
> > +}

> What happens if two LUNs are added before the above code is woken up?
> Will that perhaps cause the wait_event_timeout() call to wait forever?
I don't think it is problem. I think that the wait_event_timeout() will
check the condition before waiting.

> > +static int ufshpb_probe(struct device *dev)
> > +{
> > +  struct ufs_hba *hba;
> > +  struct ufsf_feature_info *ufsf;
> > +
> > +  if (dev->type != &ufshpb_dev_type)
> > +    return -ENODEV;
> > +
> > +  ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> > +  hba = container_of(ufsf, struct ufs_hba, ufsf);
> > +
> > +  async_schedule(ufshpb_probe_async, hba);
> > +  return 0;
> > +}

> So this is an asynchronous probe that is not visible to the device
> driver core? Could the PROBE_PREFER_ASYNCHRONOUS flag have been used
> instead to make device probing asynchronous?
I added the PROBE_PREFER_ASYNCHRONOUS flag to code and changed it to probe
synchronously.
 
> > +static int ufshpb_remove(struct device *dev)
> > +{
> > +  struct ufshpb_lu *hpb, *n_hpb;
> > +  struct ufsf_feature_info *ufsf;
> > +  struct scsi_device *sdev;
> > +
> > +  ufsf = container_of(dev, struct ufsf_feature_info, hpb_dev);
> > +
> > +  dev_set_drvdata(&ufsf->hpb_dev, NULL);
> > +
> > +  list_for_each_entry_safe(hpb, n_hpb, &ufshpb_drv.lh_hpb_lu,
> > +         list_hpb_lu) {
> > +    ufshpb_set_state(hpb, HPB_FAILED);
> > +
> > +    sdev = hpb->sdev_ufs_lu;
> > +    sdev->hostdata = NULL;
> > +
> > +    device_del(&hpb->hpb_lu_dev);
> > +
> > +    dev_info(&hpb->hpb_lu_dev, "hpb_lu_dev refcnt %d\n",
> > +       kref_read(&hpb->hpb_lu_dev.kobj.kref));
> > +    put_device(&hpb->hpb_lu_dev);
> > +  }
> > +  dev_info(dev, "ufshpb: remove success\n");
> > +
> > +  return 0;
> > +}

> Where is the code that waits for the asynchronously scheduled probe
> calls to finish?
I changed it to probe without async_schedule.

> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > new file mode 100644
> > index 000000000000..c6dd88e00849
> > --- /dev/null
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -0,0 +1,185 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Universal Flash Storage Host Performance Booster
> > + *
> > + * Copyright (C) 2017-2018 Samsung Electronics Co., Ltd.
> > + *
> > + * Authors:
> > + *  Yongmyung Lee <ymhungry.lee@samsung.com>
> > + *  Jinyoung Choi <j-young.choi@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * See the COPYING file in the top-level directory or visit
> > + * <http://www.gnu.org/licenses/gpl-2.0.html>
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * This program is provided "AS IS" and "WITH ALL FAULTS" and
> > + * without warranty of any kind. You are solely responsible for
> > + * determining the appropriateness of using and distributing
> > + * the program and assume all risks associated with your exercise
> > + * of rights with respect to the program, including but not limited
> > + * to infringement of third party rights, the risks and costs of
> > + * program errors, damage to or loss of data, programs or equipment,
> > + * and unavailability or interruption of operations. Under no
> > + * circumstances will the contributor of this Program be liable for
> > + * any damages of any kind arising from your use or distribution of
> > + * this program.
> > + *
> > + * The Linux Foundation chooses to take subject only to the GPLv2
> > + * license terms, and distributes only under these terms.
> > + */

> Please use an SPDX declaration instead of the full GPLv2 text.
OK, I will.

Thanks,

Daejun.