[PATCH v2 0/6] UFS Advanced RPMB

[PATCH v2 0/6] UFS Advanced RPMB

[Bean Huo]

Changelog:

V1 -- V2:
    1. Added RPMB request completion handling in patch 6/6
RFC -- V1:
    1. Split the patch and Remove RFC
    2. Add all 8 types of rpmb operations
    3. Fix one EHS copy error in ufshcd_advanced_rpmb_req_handler()
    4. Fix several issues raised by Avri in the RFC patch:
    https://patchwork.kernel.org/project/linux-scsi/patch/20221107131038.201724-3-beanhuo@iokpp.de/#25081912

Bean Huo (6):
  ufs: ufs_bsg: Remove unnecessary length checkup
  ufs: ufs_bsg: Cleanup ufs_bsg_request
  ufs: core: Split ufshcd_map_sg
  ufs: core: Advanced RPMB detection
  ufs: core: Pass EHS length into ufshcd_prepare_req_desc_hdr()
  ufs: core: Add advanced RPMB support in ufs_bsg

 drivers/ufs/core/ufs_bsg.c       | 137 +++++++++++++++---------
 drivers/ufs/core/ufshcd.c        | 176 +++++++++++++++++++++++++------
 include/uapi/scsi/scsi_bsg_ufs.h |  46 +++++++-
 include/ufs/ufs.h                |  29 +++++
 include/ufs/ufshcd.h             |   6 +-
 include/ufs/ufshci.h             |   1 +
 6 files changed, 315 insertions(+), 80 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] ufs: ufs_bsg: Remove unnecessary length checkup

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Remove checks on job->request_len and job->reply_len because
The following msgcode checks will rule out malicious requests.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufs_bsg.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
index b99e3f3dc4ef..9ac8204f1ee6 100644
--- a/drivers/ufs/core/ufs_bsg.c
+++ b/drivers/ufs/core/ufs_bsg.c
@@ -30,21 +30,6 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 	return 0;
 }
 
-static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
-				     unsigned int request_len,
-				     unsigned int reply_len)
-{
-	int min_req_len = sizeof(struct ufs_bsg_request);
-	int min_rsp_len = sizeof(struct ufs_bsg_reply);
-
-	if (min_req_len > request_len || min_rsp_len > reply_len) {
-		dev_err(hba->dev, "not enough space assigned\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
 				     uint8_t **desc_buff, int *desc_len,
 				     enum query_opcode desc_op)
@@ -88,8 +73,6 @@ static int ufs_bsg_request(struct bsg_job *job)
 	struct ufs_bsg_request *bsg_request = job->request;
 	struct ufs_bsg_reply *bsg_reply = job->reply;
 	struct ufs_hba *hba = shost_priv(dev_to_shost(job->dev->parent));
-	unsigned int req_len = job->request_len;
-	unsigned int reply_len = job->reply_len;
 	struct uic_command uc = {};
 	int msgcode;
 	uint8_t *desc_buff = NULL;
@@ -97,10 +80,6 @@ static int ufs_bsg_request(struct bsg_job *job)
 	enum query_opcode desc_op = UPIU_QUERY_OPCODE_NOP;
 	int ret;
 
-	ret = ufs_bsg_verify_query_size(hba, req_len, reply_len);
-	if (ret)
-		goto out;
-
 	bsg_reply->reply_payload_rcv_len = 0;
 
 	ufshcd_rpm_get_sync(hba);
-- 
2.25.1

[PATCH v2 2/6] ufs: ufs_bsg: Cleanup ufs_bsg_request

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Move sg_copy_from_buffer() below its associated case statement.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufs_bsg.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
index 9ac8204f1ee6..850a0d798f63 100644
--- a/drivers/ufs/core/ufs_bsg.c
+++ b/drivers/ufs/core/ufs_bsg.c
@@ -90,11 +90,8 @@ static int ufs_bsg_request(struct bsg_job *job)
 		desc_op = bsg_request->upiu_req.qr.opcode;
 		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
 						&desc_len, desc_op);
-		if (ret) {
-			ufshcd_rpm_put_sync(hba);
+		if (ret)
 			goto out;
-		}
-
 		fallthrough;
 	case UPIU_TRANSACTION_NOP_OUT:
 	case UPIU_TRANSACTION_TASK_REQ:
@@ -102,9 +99,12 @@ static int ufs_bsg_request(struct bsg_job *job)
 					       &bsg_reply->upiu_rsp, msgcode,
 					       desc_buff, &desc_len, desc_op);
 		if (ret)
-			dev_err(hba->dev,
-				"exe raw upiu: error code %d\n", ret);
-
+			dev_err(hba->dev, "exe raw upiu: error code %d\n", ret);
+		else if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len)
+			bsg_reply->reply_payload_rcv_len =
+				sg_copy_from_buffer(job->request_payload.sg_list,
+						    job->request_payload.sg_cnt,
+						    desc_buff, desc_len);
 		break;
 	case UPIU_TRANSACTION_UIC_CMD:
 		memcpy(&uc, &bsg_request->upiu_req.uc, UIC_CMD_SIZE);
@@ -123,20 +123,9 @@ static int ufs_bsg_request(struct bsg_job *job)
 		break;
 	}
 
+out:
 	ufshcd_rpm_put_sync(hba);
-
-	if (!desc_buff)
-		goto out;
-
-	if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len)
-		bsg_reply->reply_payload_rcv_len =
-			sg_copy_from_buffer(job->request_payload.sg_list,
-					    job->request_payload.sg_cnt,
-					    desc_buff, desc_len);
-
 	kfree(desc_buff);
-
-out:
 	bsg_reply->result = ret;
 	job->reply_len = sizeof(struct ufs_bsg_reply);
 	/* complete the job here only if no error */
-- 
2.25.1

[PATCH v2 3/6] ufs: core: Split ufshcd_map_sg

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Take out the "map scatter-gather list to prdt" part of the code in
ufshcd_map_sg and split it into a new function ufshcd_sgl_to_prdt.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 50 ++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 768cb49d269c..1b252e6cf93f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2399,38 +2399,30 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 }
 
 /**
- * ufshcd_map_sg - Map scatter-gather list to prdt
- * @hba: per adapter instance
- * @lrbp: pointer to local reference block
- *
- * Returns 0 in case of success, non-zero value in case of failure
+ * ufshcd_sgl_to_prdt - SG list to PRTD (Physical Region Description Table, 4DW format)
+ * @hba:	per-adapter instance
+ * @lrbp:	pointer to local reference block
+ * @sg_entries:	The number of sg lists actually used
+ * @sg_list:	Pointer to SG list
  */
-static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+static void ufshcd_sgl_to_prdt(struct ufs_hba *hba, struct ufshcd_lrb *lrbp, int sg_entries,
+			       struct scatterlist *sg_list)
 {
 	struct ufshcd_sg_entry *prd_table;
 	struct scatterlist *sg;
-	struct scsi_cmnd *cmd;
-	int sg_segments;
 	int i;
 
-	cmd = lrbp->cmd;
-	sg_segments = scsi_dma_map(cmd);
-	if (sg_segments < 0)
-		return sg_segments;
-
-	if (sg_segments) {
+	if (sg_entries) {
 
 		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
 			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16((sg_segments *
-					sizeof(struct ufshcd_sg_entry)));
+				cpu_to_le16((sg_entries * sizeof(struct ufshcd_sg_entry)));
 		else
-			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16(sg_segments);
+			lrbp->utr_descriptor_ptr->prd_table_length = cpu_to_le16(sg_entries);
 
 		prd_table = lrbp->ucd_prdt_ptr;
 
-		scsi_for_each_sg(cmd, sg, sg_segments, i) {
+		for_each_sg(sg_list, sg, sg_entries, i) {
 			const unsigned int len = sg_dma_len(sg);
 
 			/*
@@ -2449,6 +2441,26 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	} else {
 		lrbp->utr_descriptor_ptr->prd_table_length = 0;
 	}
+}
+
+/**
+ * ufshcd_map_sg - Map scatter-gather list to prdt
+ * @hba: per adapter instance
+ * @lrbp: pointer to local reference block
+ *
+ * Returns 0 in case of success, non-zero value in case of failure
+ */
+static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+	struct scsi_cmnd *cmd;
+	int sg_segments;
+
+	cmd = lrbp->cmd;
+	sg_segments = scsi_dma_map(cmd);
+	if (sg_segments < 0)
+		return sg_segments;
+
+	ufshcd_sgl_to_prdt(hba, lrbp, sg_segments, scsi_sglist(cmd));
 
 	return 0;
 }
-- 
2.25.1

[PATCH v2 4/6] ufs: core: Advanced RPMB detection

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Check UFS Advanced RPMB LU enablement during ufshcd_lu_init().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c |  6 ++++++
 include/ufs/ufs.h         | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1b252e6cf93f..311172578fd8 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4956,6 +4956,12 @@ static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device *sdev)
 	    desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] == UFS_LU_POWER_ON_WP)
 		hba->dev_info.is_lu_power_on_wp = true;
 
+	/* In case of RPMB LU, check if advanced RPMB mode is enabled */
+	if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_UPIU_RPMB_WLUN &&
+	    desc_buf[RPMB_UNIT_DESC_PARAM_REGION_EN] & BIT(4))
+		hba->dev_info.b_advanced_rpmb_en = true;
+
+
 	kfree(desc_buf);
 set_qdepth:
 	/*
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 1bba3fead2ce..17e401df674c 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -212,6 +212,28 @@ enum unit_desc_param {
 	UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS	= 0x29,
 };
 
+/* RPMB Unit descriptor parameters offsets in bytes*/
+enum rpmb_unit_desc_param {
+	RPMB_UNIT_DESC_PARAM_LEN		= 0x0,
+	RPMB_UNIT_DESC_PARAM_TYPE		= 0x1,
+	RPMB_UNIT_DESC_PARAM_UNIT_INDEX		= 0x2,
+	RPMB_UNIT_DESC_PARAM_LU_ENABLE		= 0x3,
+	RPMB_UNIT_DESC_PARAM_BOOT_LUN_ID	= 0x4,
+	RPMB_UNIT_DESC_PARAM_LU_WR_PROTECT	= 0x5,
+	RPMB_UNIT_DESC_PARAM_LU_Q_DEPTH		= 0x6,
+	RPMB_UNIT_DESC_PARAM_PSA_SENSITIVE	= 0x7,
+	RPMB_UNIT_DESC_PARAM_MEM_TYPE		= 0x8,
+	RPMB_UNIT_DESC_PARAM_REGION_EN		= 0x9,
+	RPMB_UNIT_DESC_PARAM_LOGICAL_BLK_SIZE	= 0xA,
+	RPMB_UNIT_DESC_PARAM_LOGICAL_BLK_COUNT	= 0xB,
+	RPMB_UNIT_DESC_PARAM_REGION0_SIZE	= 0x13,
+	RPMB_UNIT_DESC_PARAM_REGION1_SIZE	= 0x14,
+	RPMB_UNIT_DESC_PARAM_REGION2_SIZE	= 0x15,
+	RPMB_UNIT_DESC_PARAM_REGION3_SIZE	= 0x16,
+	RPMB_UNIT_DESC_PARAM_PROVISIONING_TYPE	= 0x17,
+	RPMB_UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT	= 0x18,
+};
+
 /* Device descriptor parameters offsets in bytes*/
 enum device_desc_param {
 	DEVICE_DESC_PARAM_LEN			= 0x0,
@@ -601,6 +623,8 @@ struct ufs_dev_info {
 
 	bool	b_rpm_dev_flush_capable;
 	u8	b_presrv_uspc_en;
+
+	bool    b_advanced_rpmb_en;
 };
 
 /*
-- 
2.25.1

[PATCH v2 5/6] ufs: core: Pass EHS length into ufshcd_prepare_req_desc_hdr()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

We need to fill in the total EHS length in UTP Transfer Request Descriptor,
add this functionality to ufshcd_prepare_req_desc_hdr().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 311172578fd8..2936e1e583c3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2508,14 +2508,15 @@ static void ufshcd_disable_intr(struct ufs_hba *hba, u32 intrs)
 }
 
 /**
- * ufshcd_prepare_req_desc_hdr() - Fills the requests header
+ * ufshcd_prepare_req_desc_hdr - Fill UTP Transfer request descriptor header according to request
  * descriptor according to request
  * @lrbp: pointer to local reference block
  * @upiu_flags: flags required in the header
  * @cmd_dir: requests data direction
+ * @ehs_length: Total EHS Length (in 32‐bytes units of all Extra Header Segments)
  */
-static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
-			u8 *upiu_flags, enum dma_data_direction cmd_dir)
+static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp, u8 *upiu_flags,
+					enum dma_data_direction cmd_dir, int ehs_length)
 {
 	struct utp_transfer_req_desc *req_desc = lrbp->utr_descriptor_ptr;
 	u32 data_direction;
@@ -2534,8 +2535,8 @@ static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
 		*upiu_flags = UPIU_CMD_FLAGS_NONE;
 	}
 
-	dword_0 = data_direction | (lrbp->command_type
-				<< UPIU_COMMAND_TYPE_OFFSET);
+	dword_0 = data_direction | (lrbp->command_type << UPIU_COMMAND_TYPE_OFFSET) |
+		ehs_length << 8;
 	if (lrbp->intr_cmd)
 		dword_0 |= UTP_REQ_DESC_INT_CMD;
 
@@ -2590,8 +2591,7 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags)
 }
 
 /**
- * ufshcd_prepare_utp_query_req_upiu() - fills the utp_transfer_req_desc,
- * for query requsts
+ * ufshcd_prepare_utp_query_req_upiu() - fill the utp_transfer_req_desc for query request
  * @hba: UFS hba
  * @lrbp: local reference block pointer
  * @upiu_flags: flags
@@ -2662,7 +2662,7 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba,
 	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
 
-	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE);
+	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE, 0);
 	if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY)
 		ufshcd_prepare_utp_query_req_upiu(hba, lrbp, upiu_flags);
 	else if (hba->dev_cmd.type == DEV_CMD_TYPE_NOP)
@@ -2690,8 +2690,7 @@ static int ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
 
 	if (likely(lrbp->cmd)) {
-		ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
-						lrbp->cmd->sc_data_direction);
+		ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, lrbp->cmd->sc_data_direction, 0);
 		ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
 	} else {
 		ret = -EINVAL;
@@ -6862,7 +6861,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	/* update the task tag in the request upiu */
 	req_upiu->header.dword_0 |= cpu_to_be32(tag);
 
-	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE);
+	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE, 0);
 
 	/* just copy the upiu request as it is */
 	memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
-- 
2.25.1

[PATCH v2 6/6] ufs: core: Add advanced RPMB support in ufs_bsg

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Add advanced RPMB support in ufs_bsg. For these reasons, we try to add
Advanced RPMB support in ufs_bsg:

1. According to the UFS specification, only one RPMB operation can be
performed at any time. We can ensure this by using reserved slot and
its dev_cmd sync operation protection mechanism.

2. For the Advanced RPMB, RPMB metadata is packaged in an EHS(Extra
Header Segment) of a command UPIU, and the corresponding reply EHS
(from the device) should also be returned to the user space.
bsg_job->request and bsg_job->reply allow us to pass and return EHS
from/back to userspace.

Compared to normal/legacy RPMB, the advantage of advanced RPMB are:

1. The data length in the Advanced RPBM data read/write command could
be > 4KB. For the legacy RPMB, the data length in a single RPMB data
transfer is 256 bytes.
2.  All of the advanced RPMB operations will be a single command shot.
But for the legacy  RPBM, take the read write-counter value as an example,
you need two commands(first SECURITY PROTOCOL OUT, then the second SECURITY
PROTOCOL IN).

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufs_bsg.c       | 93 ++++++++++++++++++++++++++----
 drivers/ufs/core/ufshcd.c        | 99 ++++++++++++++++++++++++++++++++
 include/uapi/scsi/scsi_bsg_ufs.h | 46 ++++++++++++++-
 include/ufs/ufs.h                |  5 ++
 include/ufs/ufshcd.h             |  6 +-
 include/ufs/ufshci.h             |  1 +
 6 files changed, 238 insertions(+), 12 deletions(-)

diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
index 850a0d798f63..a8e58faa7da2 100644
--- a/drivers/ufs/core/ufs_bsg.c
+++ b/drivers/ufs/core/ufs_bsg.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bsg-lib.h>
+#include <linux/dma-mapping.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_host.h>
 #include "ufs_bsg.h"
@@ -68,6 +69,72 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
 	return 0;
 }
 
+static int ufs_bsg_exec_advanced_rpmb_req(struct ufs_hba *hba, struct bsg_job *job)
+{
+	struct ufs_rpmb_request *rpmb_request = job->request;
+	struct ufs_rpmb_reply *rpmb_reply = job->reply;
+	struct bsg_buffer *payload = NULL;
+	enum dma_data_direction dir;
+	struct scatterlist *sg_list;
+	int rpmb_req_type;
+	int sg_cnt;
+	int ret;
+	int data_len;
+
+	if (hba->ufs_version < ufshci_version(4, 0) || !hba->dev_info.b_advanced_rpmb_en ||
+	    !(hba->capabilities & MASK_EHSLUTRD_SUPPORTED))
+		return -EINVAL;
+
+	if (rpmb_request->ehs_req.length != 2 || rpmb_request->ehs_req.ehs_type != 1)
+		return -EINVAL;
+
+	rpmb_req_type = be16_to_cpu(rpmb_request->ehs_req.meta.req_resp_type);
+
+	switch (rpmb_req_type) {
+	case UFS_RPMB_WRITE_KEY:
+	case UFS_RPMB_READ_CNT:
+	case UFS_RPMB_PURGE_ENABLE:
+		dir = DMA_NONE;
+		break;
+	case UFS_RPMB_WRITE:
+	case UFS_RPMB_SEC_CONF_WRITE:
+		dir = DMA_TO_DEVICE;
+		break;
+	case UFS_RPMB_READ:
+	case UFS_RPMB_SEC_CONF_READ:
+	case UFS_RPMB_PURGE_STATUS_READ:
+		dir = DMA_FROM_DEVICE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (dir != DMA_NONE) {
+		payload = &job->request_payload;
+		if (!payload || !payload->payload_len || !payload->sg_cnt)
+			return -EINVAL;
+
+		sg_cnt = dma_map_sg(hba->host->dma_dev, payload->sg_list, payload->sg_cnt, dir);
+		if (unlikely(!sg_cnt))
+			return -ENOMEM;
+		sg_list = payload->sg_list;
+		data_len = payload->payload_len;
+	}
+
+	ret = ufshcd_advanced_rpmb_req_handler(hba, &rpmb_request->bsg_request.upiu_req,
+				   &rpmb_reply->bsg_reply.upiu_rsp, &rpmb_request->ehs_req,
+				   &rpmb_reply->ehs_rsp, sg_cnt, sg_list, dir);
+
+	if (dir != DMA_NONE) {
+		dma_unmap_sg(hba->host->dma_dev, payload->sg_list, payload->sg_cnt, dir);
+
+		if (!ret)
+			rpmb_reply->bsg_reply.reply_payload_rcv_len = data_len;
+	}
+
+	return ret;
+}
+
 static int ufs_bsg_request(struct bsg_job *job)
 {
 	struct ufs_bsg_request *bsg_request = job->request;
@@ -75,10 +142,11 @@ static int ufs_bsg_request(struct bsg_job *job)
 	struct ufs_hba *hba = shost_priv(dev_to_shost(job->dev->parent));
 	struct uic_command uc = {};
 	int msgcode;
-	uint8_t *desc_buff = NULL;
+	uint8_t *buff = NULL;
 	int desc_len = 0;
 	enum query_opcode desc_op = UPIU_QUERY_OPCODE_NOP;
 	int ret;
+	bool rpmb = false;
 
 	bsg_reply->reply_payload_rcv_len = 0;
 
@@ -88,8 +156,7 @@ static int ufs_bsg_request(struct bsg_job *job)
 	switch (msgcode) {
 	case UPIU_TRANSACTION_QUERY_REQ:
 		desc_op = bsg_request->upiu_req.qr.opcode;
-		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
-						&desc_len, desc_op);
+		ret = ufs_bsg_alloc_desc_buffer(hba, job, &buff, &desc_len, desc_op);
 		if (ret)
 			goto out;
 		fallthrough;
@@ -97,25 +164,31 @@ static int ufs_bsg_request(struct bsg_job *job)
 	case UPIU_TRANSACTION_TASK_REQ:
 		ret = ufshcd_exec_raw_upiu_cmd(hba, &bsg_request->upiu_req,
 					       &bsg_reply->upiu_rsp, msgcode,
-					       desc_buff, &desc_len, desc_op);
+					       buff, &desc_len, desc_op);
 		if (ret)
 			dev_err(hba->dev, "exe raw upiu: error code %d\n", ret);
-		else if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len)
+		else if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len) {
 			bsg_reply->reply_payload_rcv_len =
 				sg_copy_from_buffer(job->request_payload.sg_list,
 						    job->request_payload.sg_cnt,
-						    desc_buff, desc_len);
+						    buff, desc_len);
+		}
 		break;
 	case UPIU_TRANSACTION_UIC_CMD:
 		memcpy(&uc, &bsg_request->upiu_req.uc, UIC_CMD_SIZE);
 		ret = ufshcd_send_uic_cmd(hba, &uc);
 		if (ret)
-			dev_err(hba->dev,
-				"send uic cmd: error code %d\n", ret);
+			dev_err(hba->dev, "send uic cmd: error code %d\n", ret);
 
 		memcpy(&bsg_reply->upiu_rsp.uc, &uc, UIC_CMD_SIZE);
 
 		break;
+	case UPIU_TRANSACTION_ARPMB_CMD:
+		rpmb = true;
+		ret = ufs_bsg_exec_advanced_rpmb_req(hba, job);
+		if (ret)
+			dev_err(hba->dev, "ARPMB OP failed: error code  %d\n", ret);
+		break;
 	default:
 		ret = -ENOTSUPP;
 		dev_err(hba->dev, "unsupported msgcode 0x%x\n", msgcode);
@@ -125,9 +198,9 @@ static int ufs_bsg_request(struct bsg_job *job)
 
 out:
 	ufshcd_rpm_put_sync(hba);
-	kfree(desc_buff);
+	kfree(buff);
 	bsg_reply->result = ret;
-	job->reply_len = sizeof(struct ufs_bsg_reply);
+	job->reply_len = !rpmb ? sizeof(struct ufs_bsg_reply) : sizeof(struct ufs_rpmb_reply);
 	/* complete the job here only if no error */
 	if (ret == 0)
 		bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2936e1e583c3..863aa9dd28bb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -56,6 +56,9 @@
 /* Query request timeout */
 #define QUERY_REQ_TIMEOUT 1500 /* 1.5 seconds */
 
+/* Advanced RPMB request timeout */
+#define ADVANCED_RPMB_REQ_TIMEOUT  3000 /* 3 seconds */
+
 /* Task management command timeout */
 #define TM_CMD_TIMEOUT	100 /* msecs */
 
@@ -2956,6 +2959,12 @@ ufshcd_dev_cmd_completion(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		dev_err(hba->dev, "%s: Reject UPIU not fully implemented\n",
 				__func__);
 		break;
+	case UPIU_TRANSACTION_RESPONSE:
+		if (hba->dev_cmd.type != DEV_CMD_TYPE_RPMB) {
+			err = -EINVAL;
+			dev_err(hba->dev, "%s: unexpected response %x\n", __func__, resp);
+		}
+		break;
 	default:
 		err = -EINVAL;
 		dev_err(hba->dev, "%s: Invalid device management cmd response: %x\n",
@@ -6984,6 +6993,96 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 	return err;
 }
 
+/**
+ * ufshcd_advanced_rpmb_req_handler - handle advanced RPMB request
+ * @hba:	per adapter instance
+ * @req_upiu:	upiu request
+ * @rsp_upiu:	upiu reply
+ * @req_ehs:	EHS field which contains Advanced RPMB Request Message
+ * @rsp_ehs:	EHS field which returns Advanced RPMB Response Message
+ * @sg_cnt:	The number of sg lists actually used
+ * @sg_list:	Pointer to SG list when DATA IN/OUT UPIU is required in ARPMB operation
+ * @dir:	DMA direction
+ *
+ * Returns zero on success, non-zero on failure
+ */
+int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *req_upiu,
+			 struct utp_upiu_req *rsp_upiu, struct ufs_ehs *req_ehs,
+			 struct ufs_ehs *rsp_ehs, int sg_cnt, struct scatterlist *sg_list,
+			 enum dma_data_direction dir)
+{
+	DECLARE_COMPLETION_ONSTACK(wait);
+	const u32 tag = hba->reserved_slot;
+	struct ufshcd_lrb *lrbp;
+	int err = 0;
+	u8 upiu_flags;
+	u8 *ehs_data;
+	u16 ehs_len;
+
+	/* Protects use of hba->reserved_slot. */
+	ufshcd_hold(hba, false);
+	mutex_lock(&hba->dev_cmd.lock);
+	down_read(&hba->clk_scaling_lock);
+
+	lrbp = &hba->lrb[tag];
+	WARN_ON(lrbp->cmd);
+	lrbp->cmd = NULL;
+	lrbp->task_tag = tag;
+	lrbp->lun = UFS_UPIU_RPMB_WLUN;
+
+	lrbp->intr_cmd = true;
+	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
+	hba->dev_cmd.type = DEV_CMD_TYPE_RPMB;
+
+	/* Advanced RPMB starts from UFS 4.0, so its command type is UTP_CMD_TYPE_UFS_STORAGE */
+	lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
+
+	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, dir, 2);
+
+	/* update the task tag and LUN in the request upiu */
+	req_upiu->header.dword_0 |= cpu_to_be32(upiu_flags << 16 | UFS_UPIU_RPMB_WLUN << 8 | tag);
+
+	/* copy the UPIU(contains CDB) request as it is */
+	memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
+	/* Copy EHS, starting with byte32, immediately after the CDB package */
+	memcpy(lrbp->ucd_req_ptr + 1, req_ehs, sizeof(*req_ehs));
+
+	if (dir != DMA_NONE && sg_list)
+		ufshcd_sgl_to_prdt(hba, lrbp, sg_cnt, sg_list);
+
+	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
+
+	hba->dev_cmd.complete = &wait;
+
+	ufshcd_send_command(hba, tag);
+
+	err = ufshcd_wait_for_dev_cmd(hba, lrbp, ADVANCED_RPMB_REQ_TIMEOUT);
+
+	if (!err) {
+		/* just copy the upiu response as it is */
+		memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, sizeof(*rsp_upiu));
+		ehs_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2) >> 24;
+		/*
+		 * Since the bLength in EHS indicates the total size of the EHS Header and EHS Data
+		 * in 32 Byte units, the value of the bLength Request/Response for Advanced RPMB
+		 * Message is 02h
+		 */
+		if (ehs_len == 2 && rsp_ehs) {
+			/*
+			 * ucd_rsp_ptr points to a buffer with a length of 512 bytes
+			 * (ALIGNED_UPIU_SIZE = 512), and the EHS data just starts from byte32
+			 */
+			ehs_data = (u8 *)lrbp->ucd_rsp_ptr + EHS_OFFSET_IN_RESPONSE;
+			memcpy(rsp_ehs, ehs_data, ehs_len * 32);
+		}
+	}
+
+	up_read(&hba->clk_scaling_lock);
+	mutex_unlock(&hba->dev_cmd.lock);
+	ufshcd_release(hba);
+	return err;
+}
+
 /**
  * ufshcd_eh_device_reset_handler() - Reset a single logical unit.
  * @cmd: SCSI command pointer
diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h
index d55f2176dfd4..1d605aaf5d6f 100644
--- a/include/uapi/scsi/scsi_bsg_ufs.h
+++ b/include/uapi/scsi/scsi_bsg_ufs.h
@@ -14,10 +14,27 @@
  */
 
 #define UFS_CDB_SIZE	16
-#define UPIU_TRANSACTION_UIC_CMD 0x1F
 /* uic commands are 4DW long, per UFSHCI V2.1 paragraph 5.6.1 */
 #define UIC_CMD_SIZE (sizeof(__u32) * 4)
 
+enum ufs_bsg_msg_code {
+	UPIU_TRANSACTION_UIC_CMD = 0x1F,
+	UPIU_TRANSACTION_ARPMB_CMD,
+};
+
+/* UFS RPMB Request Message Types */
+enum ufs_rpmb_op_type {
+	UFS_RPMB_WRITE_KEY		= 0x01,
+	UFS_RPMB_READ_CNT		= 0x02,
+	UFS_RPMB_WRITE			= 0x03,
+	UFS_RPMB_READ			= 0x04,
+	UFS_RPMB_READ_RESP		= 0x05,
+	UFS_RPMB_SEC_CONF_WRITE		= 0x06,
+	UFS_RPMB_SEC_CONF_READ		= 0x07,
+	UFS_RPMB_PURGE_ENABLE		= 0x08,
+	UFS_RPMB_PURGE_STATUS_READ	= 0x09,
+};
+
 /**
  * struct utp_upiu_header - UPIU header structure
  * @dword_0: UPIU header DW-0
@@ -79,6 +96,23 @@ struct utp_upiu_req {
 	};
 };
 
+struct ufs_arpmb_meta {
+	__u16	req_resp_type;
+	__u8	nonce[16];
+	__u32	write_counter;
+	__u16	addr_lun;
+	__u16	block_count;
+	__u16	result;
+};
+
+struct ufs_ehs {
+	__u8	length;
+	__u8	ehs_type;
+	__u16	ehssub_type;
+	struct ufs_arpmb_meta meta;
+	__u8	mac_key[32];
+};
+
 /* request (CDB) structure of the sg_io_v4 */
 struct ufs_bsg_request {
 	__u32 msgcode;
@@ -102,4 +136,14 @@ struct ufs_bsg_reply {
 
 	struct utp_upiu_req upiu_rsp;
 };
+
+struct ufs_rpmb_request {
+	struct ufs_bsg_request bsg_request;
+	struct ufs_ehs ehs_req;
+};
+
+struct ufs_rpmb_reply {
+	struct ufs_bsg_reply bsg_reply;
+	struct ufs_ehs ehs_rsp;
+};
 #endif /* UFS_BSG_H */
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 17e401df674c..0c112195b288 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -49,6 +49,11 @@
  */
 #define UFS_WB_EXCEED_LIFETIME		0x0B
 
+/*
+ * In UFS Spec, the Extra Header Segment (EHS) starts from byte 32 in UPIU request/response packet
+ */
+#define EHS_OFFSET_IN_RESPONSE 32
+
 /* Well known logical unit id in LUN field of UPIU */
 enum {
 	UFS_UPIU_REPORT_LUNS_WLUN	= 0x81,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 5cf81dff60aa..c3dfa8084b5c 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -30,6 +30,7 @@ struct ufs_hba;
 enum dev_cmd_type {
 	DEV_CMD_TYPE_NOP		= 0x0,
 	DEV_CMD_TYPE_QUERY		= 0x1,
+	DEV_CMD_TYPE_RPMB		= 0x2,
 };
 
 enum ufs_event_type {
@@ -1201,7 +1202,10 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     int msgcode,
 			     u8 *desc_buff, int *buff_len,
 			     enum query_opcode desc_op);
-
+int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *req_upiu,
+				     struct utp_upiu_req *rsp_upiu, struct ufs_ehs *ehs_req,
+				     struct ufs_ehs *ehs_rsp, int sg_cnt,
+				     struct scatterlist *sg_list, enum dma_data_direction dir);
 int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
 int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable);
 int ufshcd_suspend_prepare(struct device *dev);
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index f525566a0864..af216296b86e 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -63,6 +63,7 @@ enum {
 enum {
 	MASK_TRANSFER_REQUESTS_SLOTS		= 0x0000001F,
 	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
+	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,
 	MASK_AUTO_HIBERN8_SUPPORT		= 0x00800000,
 	MASK_64_ADDRESSING_SUPPORT		= 0x01000000,
 	MASK_OUT_OF_ORDER_DATA_DELIVERY_SUPPORT	= 0x02000000,
-- 
2.25.1

RE: [PATCH v2 2/6] ufs: ufs_bsg: Cleanup ufs_bsg_request

[Avri Altman]

> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Move sg_copy_from_buffer() below its associated case statement.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/ufs/core/ufs_bsg.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c index
> 9ac8204f1ee6..850a0d798f63 100644
> --- a/drivers/ufs/core/ufs_bsg.c
> +++ b/drivers/ufs/core/ufs_bsg.c
> @@ -90,11 +90,8 @@ static int ufs_bsg_request(struct bsg_job *job)
>                 desc_op = bsg_request->upiu_req.qr.opcode;
>                 ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
>                                                 &desc_len, desc_op);
> -               if (ret) {
> -                       ufshcd_rpm_put_sync(hba);
> +               if (ret)
>                         goto out;
> -               }
> -
>                 fallthrough;
>         case UPIU_TRANSACTION_NOP_OUT:
>         case UPIU_TRANSACTION_TASK_REQ:
> @@ -102,9 +99,12 @@ static int ufs_bsg_request(struct bsg_job *job)
>                                                &bsg_reply->upiu_rsp, msgcode,
>                                                desc_buff, &desc_len, desc_op);
>                 if (ret)
> -                       dev_err(hba->dev,
> -                               "exe raw upiu: error code %d\n", ret);
> -
> +                       dev_err(hba->dev, "exe raw upiu: error code %d\n", ret);
> +               else if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len)
> +                       bsg_reply->reply_payload_rcv_len =
> +                               sg_copy_from_buffer(job->request_payload.sg_list,
> +                                                   job->request_payload.sg_cnt,
> +                                                   desc_buff,
> + desc_len);
>                 break;
>         case UPIU_TRANSACTION_UIC_CMD:
>                 memcpy(&uc, &bsg_request->upiu_req.uc, UIC_CMD_SIZE); @@ -
> 123,20 +123,9 @@ static int ufs_bsg_request(struct bsg_job *job)
>                 break;
>         }
> 
> +out:
>         ufshcd_rpm_put_sync(hba);
> -
> -       if (!desc_buff)
> -               goto out;
> -
> -       if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len)
> -               bsg_reply->reply_payload_rcv_len =
> -                       sg_copy_from_buffer(job->request_payload.sg_list,
> -                                           job->request_payload.sg_cnt,
> -                                           desc_buff, desc_len);
> -
>         kfree(desc_buff);
> -
> -out:
>         bsg_reply->result = ret;
>         job->reply_len = sizeof(struct ufs_bsg_reply);
>         /* complete the job here only if no error */
> --
> 2.25.1

RE: [PATCH v2 3/6] ufs: core: Split ufshcd_map_sg

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> Take out the "map scatter-gather list to prdt" part of the code in
> ufshcd_map_sg and split it into a new function ufshcd_sgl_to_prdt.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
A nit below.

Reviewed-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/ufs/core/ufshcd.c | 50 ++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 768cb49d269c..1b252e6cf93f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2399,38 +2399,30 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)  }
> 
>  /**
> - * ufshcd_map_sg - Map scatter-gather list to prdt
> - * @hba: per adapter instance
> - * @lrbp: pointer to local reference block
> - *
> - * Returns 0 in case of success, non-zero value in case of failure
> + * ufshcd_sgl_to_prdt - SG list to PRTD (Physical Region Description Table,
> 4DW format)
> + * @hba:       per-adapter instance
> + * @lrbp:      pointer to local reference block
> + * @sg_entries:        The number of sg lists actually used
> + * @sg_list:   Pointer to SG list
>   */
> -static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> +static void ufshcd_sgl_to_prdt(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> int sg_entries,
> +                              struct scatterlist *sg_list)
>  {
>         struct ufshcd_sg_entry *prd_table;
>         struct scatterlist *sg;
> -       struct scsi_cmnd *cmd;
> -       int sg_segments;
>         int i;
> 
> -       cmd = lrbp->cmd;
> -       sg_segments = scsi_dma_map(cmd);
> -       if (sg_segments < 0)
> -               return sg_segments;
> -
> -       if (sg_segments) {
> +       if (sg_entries) {
> 
>                 if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
>                         lrbp->utr_descriptor_ptr->prd_table_length =
> -                               cpu_to_le16((sg_segments *
> -                                       sizeof(struct ufshcd_sg_entry)));
> +                               cpu_to_le16((sg_entries * sizeof(struct
> + ufshcd_sg_entry)));
>                 else
> -                       lrbp->utr_descriptor_ptr->prd_table_length =
> -                               cpu_to_le16(sg_segments);
> +                       lrbp->utr_descriptor_ptr->prd_table_length =
> + cpu_to_le16(sg_entries);
> 
>                 prd_table = lrbp->ucd_prdt_ptr;
> 
> -               scsi_for_each_sg(cmd, sg, sg_segments, i) {
> +               for_each_sg(sg_list, sg, sg_entries, i) {
>                         const unsigned int len = sg_dma_len(sg);
> 
>                         /*
> @@ -2449,6 +2441,26 @@ static int ufshcd_map_sg(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>         } else {
>                 lrbp->utr_descriptor_ptr->prd_table_length = 0;
>         }
> +}
> +
> +/**
> + * ufshcd_map_sg - Map scatter-gather list to prdt
> + * @hba: per adapter instance
> + * @lrbp: pointer to local reference block
> + *
> + * Returns 0 in case of success, non-zero value in case of failure  */
> +static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> +{
> +       struct scsi_cmnd *cmd;
> +       int sg_segments;
> +
> +       cmd = lrbp->cmd;
> +       sg_segments = scsi_dma_map(cmd);
Maybe initialize in declaration?

> +       if (sg_segments < 0)
> +               return sg_segments;
> +
> +       ufshcd_sgl_to_prdt(hba, lrbp, sg_segments, scsi_sglist(cmd));
> 
>         return 0;
>  }
> --
> 2.25.1

RE: [PATCH v2 4/6] ufs: core: Advanced RPMB detection

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> Check UFS Advanced RPMB LU enablement during ufshcd_lu_init().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/ufs/core/ufshcd.c |  6 ++++++
>  include/ufs/ufs.h         | 24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 1b252e6cf93f..311172578fd8 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4956,6 +4956,12 @@ static void ufshcd_lu_init(struct ufs_hba *hba,
> struct scsi_device *sdev)
>             desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] ==
> UFS_LU_POWER_ON_WP)
>                 hba->dev_info.is_lu_power_on_wp = true;
> 
> +       /* In case of RPMB LU, check if advanced RPMB mode is enabled */
> +       if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] ==
> UFS_UPIU_RPMB_WLUN &&
> +           desc_buf[RPMB_UNIT_DESC_PARAM_REGION_EN] & BIT(4))
If instead you use bit 10 in dExtendedUFSFeaturesSupport,
You wouldn't need to add the rpmb unit descriptor, just the appropriate bit.

Thanks,
Avri

RE: [PATCH v2 5/6] ufs: core: Pass EHS length into ufshcd_prepare_req_desc_hdr()

[Avri Altman]

 
> From: Bean Huo <beanhuo@micron.com>
> 
> We need to fill in the total EHS length in UTP Transfer Request Descriptor,
> add this functionality to ufshcd_prepare_req_desc_hdr().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/ufs/core/ufshcd.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 311172578fd8..2936e1e583c3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2508,14 +2508,15 @@ static void ufshcd_disable_intr(struct ufs_hba
> *hba, u32 intrs)  }
> 
>  /**
> - * ufshcd_prepare_req_desc_hdr() - Fills the requests header
> + * ufshcd_prepare_req_desc_hdr - Fill UTP Transfer request descriptor
> + header according to request
>   * descriptor according to request
>   * @lrbp: pointer to local reference block
>   * @upiu_flags: flags required in the header
>   * @cmd_dir: requests data direction
> + * @ehs_length: Total EHS Length (in 32‐bytes units of all Extra Header
> + Segments)
>   */
> -static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
> -                       u8 *upiu_flags, enum dma_data_direction cmd_dir)
> +static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp, u8
> *upiu_flags,
> +                                       enum dma_data_direction cmd_dir,
> +int ehs_length)
>  {
>         struct utp_transfer_req_desc *req_desc = lrbp->utr_descriptor_ptr;
>         u32 data_direction;
> @@ -2534,8 +2535,8 @@ static void ufshcd_prepare_req_desc_hdr(struct
> ufshcd_lrb *lrbp,
>                 *upiu_flags = UPIU_CMD_FLAGS_NONE;
>         }
> 
> -       dword_0 = data_direction | (lrbp->command_type
> -                               << UPIU_COMMAND_TYPE_OFFSET);
> +       dword_0 = data_direction | (lrbp->command_type <<
> UPIU_COMMAND_TYPE_OFFSET) |
> +               ehs_length << 8;
>         if (lrbp->intr_cmd)
>                 dword_0 |= UTP_REQ_DESC_INT_CMD;
> 
> @@ -2590,8 +2591,7 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct
> ufshcd_lrb *lrbp, u8 upiu_flags)  }
> 
>  /**
> - * ufshcd_prepare_utp_query_req_upiu() - fills the utp_transfer_req_desc,
> - * for query requsts
> + * ufshcd_prepare_utp_query_req_upiu() - fill the utp_transfer_req_desc
> + for query request
>   * @hba: UFS hba
>   * @lrbp: local reference block pointer
>   * @upiu_flags: flags
> @@ -2662,7 +2662,7 @@ static int ufshcd_compose_devman_upiu(struct
> ufs_hba *hba,
>         else
>                 lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
> 
> -       ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE);
> +       ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE, 0);
>         if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY)
>                 ufshcd_prepare_utp_query_req_upiu(hba, lrbp, upiu_flags);
>         else if (hba->dev_cmd.type == DEV_CMD_TYPE_NOP) @@ -2690,8
> +2690,7 @@ static int ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp)
>                 lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
> 
>         if (likely(lrbp->cmd)) {
> -               ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
> -                                               lrbp->cmd->sc_data_direction);
> +               ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
> + lrbp->cmd->sc_data_direction, 0);
>                 ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
>         } else {
>                 ret = -EINVAL;
> @@ -6862,7 +6861,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>         /* update the task tag in the request upiu */
>         req_upiu->header.dword_0 |= cpu_to_be32(tag);
> 
> -       ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE);
> +       ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE, 0);
> 
>         /* just copy the upiu request as it is */
>         memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
> --
> 2.25.1

RE: [PATCH v2 6/6] ufs: core: Add advanced RPMB support in ufs_bsg

[Avri Altman]

> +               sg_cnt = dma_map_sg(hba->host->dma_dev, payload->sg_list,
> payload->sg_cnt, dir);
> +               if (unlikely(!sg_cnt))
> +                       return -ENOMEM;
> +               sg_list = payload->sg_list;
> +               data_len = payload->payload_len;
> +       }
Isn't bsg_map_buffer does that for you already?
For both request & reply?

Thanks,
Avri

RE: [PATCH v2 6/6] ufs: core: Add advanced RPMB support in ufs_bsg

[Avri Altman]

> 
> > +               sg_cnt = dma_map_sg(hba->host->dma_dev, payload->sg_list,
> > payload->sg_cnt, dir);
> > +               if (unlikely(!sg_cnt))
> > +                       return -ENOMEM;
> > +               sg_list = payload->sg_list;
> > +               data_len = payload->payload_len;
> > +       }
> Isn't bsg_map_buffer does that for you already?
> For both request & reply?
Answering my own question - no it doesn't...

Thanks,
Avri

> 
> Thanks,
> Avri

RE: [PATCH v2 6/6] ufs: core: Add advanced RPMB support in ufs_bsg

[Avri Altman]

> +static int ufs_bsg_exec_advanced_rpmb_req(struct ufs_hba *hba, struct
> +bsg_job *job) {
> +       struct ufs_rpmb_request *rpmb_request = job->request;
> +       struct ufs_rpmb_reply *rpmb_reply = job->reply;
> +       struct bsg_buffer *payload = NULL;
> +       enum dma_data_direction dir;
> +       struct scatterlist *sg_list;
> +       int rpmb_req_type;
> +       int sg_cnt;
> +       int ret;
> +       int data_len;
> +
> +       if (hba->ufs_version < ufshci_version(4, 0) || !hba-
> >dev_info.b_advanced_rpmb_en ||
> +           !(hba->capabilities & MASK_EHSLUTRD_SUPPORTED))
> +               return -EINVAL;
> +
> +       if (rpmb_request->ehs_req.length != 2 || rpmb_request-
> >ehs_req.ehs_type != 1)
> +               return -EINVAL;
Maybe you could also check:
In case of rpmb write, the request payload 4096 × Advanced RPMB Block Count,
And same goes for response payload for rpmb read.

Thanks,
Avri

Re: [PATCH v2 3/6] ufs: core: Split ufshcd_map_sg

[Bean Huo]

On Tue, 2022-11-22 at 08:15 +0000, Avri Altman wrote:
> > +
> > +       cmd = lrbp->cmd;
> > +       sg_segments = scsi_dma_map(cmd);
> 
> Maybe initialize in declaration?

yes, agree, will change it in the next version

Kind regards,
Bean

Re: [PATCH v2 6/6] ufs: core: Add advanced RPMB support in ufs_bsg

[Bean Huo]

On Tue, 2022-11-22 at 11:55 +0000, Avri Altman wrote:
> > +static int ufs_bsg_exec_advanced_rpmb_req(struct ufs_hba *hba,
> > struct
> > +bsg_job *job) {
> > +       struct ufs_rpmb_request *rpmb_request = job->request;
> > +       struct ufs_rpmb_reply *rpmb_reply = job->reply;
> > +       struct bsg_buffer *payload = NULL;
> > +       enum dma_data_direction dir;
> > +       struct scatterlist *sg_list;
> > +       int rpmb_req_type;
> > +       int sg_cnt;
> > +       int ret;
> > +       int data_len;
> > +
> > +       if (hba->ufs_version < ufshci_version(4, 0) || !hba-
> > > dev_info.b_advanced_rpmb_en ||
> > +           !(hba->capabilities & MASK_EHSLUTRD_SUPPORTED))
> > +               return -EINVAL;
> > +
> > +       if (rpmb_request->ehs_req.length != 2 || rpmb_request-
> > > ehs_req.ehs_type != 1)
> > +               return -EINVAL;
> Maybe you could also check:
> In case of rpmb write, the request payload 4096 × Advanced RPMB Block
> Count,
> And same goes for response payload for rpmb read.
> 
> Thanks,
> Avri
> 

Hi Avri, 

in Spec:

"If the Block Count indicates a value greater than bRPMB_ReadWriteSize,
then the authenticated data write/read operation fails and the Result
is set to “General failure” (0001h)."


I think this should be checked in the application in userspace because
if the application passes a wrong/incorrect payload length, it will
error out and have no effect on UFS. In order to add this check in a
driver in the kernel, we need to maintain bRPMB_ReadWriteSize in kernel
space. Sometimes this is a waste of resources because we don't know if
the client will access the RPMB.

I have enabled Advanced RPMB feature in the ufs-utils as an example,
will be refered in cover-letter in the next version.

Kind regards,
Bean

RE: [PATCH v2 6/6] ufs: core: Add advanced RPMB support in ufs_bsg

[Avri Altman]

> On Tue, 2022-11-22 at 11:55 +0000, Avri Altman wrote:
> > > +static int ufs_bsg_exec_advanced_rpmb_req(struct ufs_hba *hba,
> > > struct
> > > +bsg_job *job) {
> > > +       struct ufs_rpmb_request *rpmb_request = job->request;
> > > +       struct ufs_rpmb_reply *rpmb_reply = job->reply;
> > > +       struct bsg_buffer *payload = NULL;
> > > +       enum dma_data_direction dir;
> > > +       struct scatterlist *sg_list;
> > > +       int rpmb_req_type;
> > > +       int sg_cnt;
> > > +       int ret;
> > > +       int data_len;
> > > +
> > > +       if (hba->ufs_version < ufshci_version(4, 0) || !hba-
> > > > dev_info.b_advanced_rpmb_en ||
> > > +           !(hba->capabilities & MASK_EHSLUTRD_SUPPORTED))
> > > +               return -EINVAL;
> > > +
> > > +       if (rpmb_request->ehs_req.length != 2 || rpmb_request-
> > > > ehs_req.ehs_type != 1)
> > > +               return -EINVAL;
> > Maybe you could also check:
> > In case of rpmb write, the request payload 4096 × Advanced RPMB Block
> > Count, And same goes for response payload for rpmb read.
> >
> > Thanks,
> > Avri
> >
> 
> Hi Avri,
> 
> in Spec:
> 
> "If the Block Count indicates a value greater than bRPMB_ReadWriteSize,
> then the authenticated data write/read operation fails and the Result is set
> to “General failure” (0001h)."
> 
> 
> I think this should be checked in the application in userspace because if the
> application passes a wrong/incorrect payload length, it will error out and
> have no effect on UFS. In order to add this check in a driver in the kernel, we
> need to maintain bRPMB_ReadWriteSize in kernel space. Sometimes this is a
> waste of resources because we don't know if the client will access the RPMB.
Fair enough.
Please add my reviewed-by tag to this patch as well.

Thanks,
Avri
> 
> I have enabled Advanced RPMB feature in the ufs-utils as an example, will be
> refered in cover-letter in the next version.
> 
> Kind regards,
> Bean

RE: [PATCH v2 1/6] ufs: ufs_bsg: Remove unnecessary length checkup

[Avri Altman]

> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Remove checks on job->request_len and job->reply_len because The following
> msgcode checks will rule out malicious requests.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>


> ---
>  drivers/ufs/core/ufs_bsg.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c index
> b99e3f3dc4ef..9ac8204f1ee6 100644
> --- a/drivers/ufs/core/ufs_bsg.c
> +++ b/drivers/ufs/core/ufs_bsg.c
> @@ -30,21 +30,6 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba
> *hba, int *desc_len,
>         return 0;
>  }
> 
> -static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
> -                                    unsigned int request_len,
> -                                    unsigned int reply_len)
> -{
> -       int min_req_len = sizeof(struct ufs_bsg_request);
> -       int min_rsp_len = sizeof(struct ufs_bsg_reply);
> -
> -       if (min_req_len > request_len || min_rsp_len > reply_len) {
> -               dev_err(hba->dev, "not enough space assigned\n");
> -               return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
>  static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
>                                      uint8_t **desc_buff, int *desc_len,
>                                      enum query_opcode desc_op) @@ -88,8 +73,6 @@
> static int ufs_bsg_request(struct bsg_job *job)
>         struct ufs_bsg_request *bsg_request = job->request;
>         struct ufs_bsg_reply *bsg_reply = job->reply;
>         struct ufs_hba *hba = shost_priv(dev_to_shost(job->dev->parent));
> -       unsigned int req_len = job->request_len;
> -       unsigned int reply_len = job->reply_len;
>         struct uic_command uc = {};
>         int msgcode;
>         uint8_t *desc_buff = NULL;
> @@ -97,10 +80,6 @@ static int ufs_bsg_request(struct bsg_job *job)
>         enum query_opcode desc_op = UPIU_QUERY_OPCODE_NOP;
>         int ret;
> 
> -       ret = ufs_bsg_verify_query_size(hba, req_len, reply_len);
> -       if (ret)
> -               goto out;
> -
>         bsg_reply->reply_payload_rcv_len = 0;
> 
>         ufshcd_rpm_get_sync(hba);
> --
> 2.25.1

[PATCH v2 1/6] ufs: ufs_bsg: Remove unnecessary length checkup

[Bean Huo]

Remove checks on job->request_len and job->reply_len because
The following msgcode checks will rule out malicious requests.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufs_bsg.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
index b99e3f3dc4ef..9ac8204f1ee6 100644
--- a/drivers/ufs/core/ufs_bsg.c
+++ b/drivers/ufs/core/ufs_bsg.c
@@ -30,21 +30,6 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 	return 0;
 }
 
-static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
-				     unsigned int request_len,
-				     unsigned int reply_len)
-{
-	int min_req_len = sizeof(struct ufs_bsg_request);
-	int min_rsp_len = sizeof(struct ufs_bsg_reply);
-
-	if (min_req_len > request_len || min_rsp_len > reply_len) {
-		dev_err(hba->dev, "not enough space assigned\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
 				     uint8_t **desc_buff, int *desc_len,
 				     enum query_opcode desc_op)
@@ -88,8 +73,6 @@ static int ufs_bsg_request(struct bsg_job *job)
 	struct ufs_bsg_request *bsg_request = job->request;
 	struct ufs_bsg_reply *bsg_reply = job->reply;
 	struct ufs_hba *hba = shost_priv(dev_to_shost(job->dev->parent));
-	unsigned int req_len = job->request_len;
-	unsigned int reply_len = job->reply_len;
 	struct uic_command uc = {};
 	int msgcode;
 	uint8_t *desc_buff = NULL;
@@ -97,10 +80,6 @@ static int ufs_bsg_request(struct bsg_job *job)
 	enum query_opcode desc_op = UPIU_QUERY_OPCODE_NOP;
 	int ret;
 
-	ret = ufs_bsg_verify_query_size(hba, req_len, reply_len);
-	if (ret)
-		goto out;
-
 	bsg_reply->reply_payload_rcv_len = 0;
 
 	ufshcd_rpm_get_sync(hba);
-- 
2.25.1