[PATCH v2 0/3] Add OP-TEE based RPMB driver for UFS devices

[PATCH v2 0/3] Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

This patch series introduces OP-TEE based RPMB (Replay Protected Memory Block)
support for UFS devices, extending the kernel-level secure storage capabilities
that are currently available for eMMC devices.

Previously, OP-TEE required a userspace supplicant to access RPMB partitions,
which created complex dependencies and reliability issues, especially during
early boot scenarios. Recent work by Linaro has moved core supplicant
functionality directly into the Linux kernel for eMMC devices, eliminating
userspace dependencies and enabling immediate secure storage access. This series
extends the same approach to UFS devices, which are used in enterprise and mobile
applications that require secure storage capabilities.

Benefits:
- Eliminates dependency on userspace supplicant for UFS RPMB access
- Enables early boot secure storage access (e.g., fTPM, secure UEFI variables)
- Provides kernel-level RPMB access as soon as UFS driver is initialized
- Removes complex initramfs dependencies and boot ordering requirements
- Ensures reliable and deterministic secure storage operations
- Supports both built-in and modular fTPM configurations.

v1 -- v2:
	1. Added fix tag for patch [2/3]
	2. Incorporated feedback and suggestions from Bart

RFC v1 -- v1:
        1. Added support for all UFS RPMB regions based on https://github.com/OP-TEE/optee_os/issues/7532
        2. Incorporated feedback and suggestions from Bart


Bean Huo (3):
  rpmb: move rpmb_frame struct and constants to common header
  scsi: ufs: core: fix incorrect buffer duplication in
    ufshcd_read_string_desc()
  scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

 drivers/misc/Kconfig           |   2 +-
 drivers/mmc/core/block.c       |  42 ------
 drivers/ufs/core/Makefile      |   1 +
 drivers/ufs/core/ufs-rpmb.c    | 253 +++++++++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd-priv.h |  13 ++
 drivers/ufs/core/ufshcd.c      |  32 ++++-
 include/linux/rpmb.h           |  44 ++++++
 include/ufs/ufs.h              |   4 +
 include/ufs/ufshcd.h           |   3 +
 9 files changed, 346 insertions(+), 48 deletions(-)
 create mode 100644 drivers/ufs/core/ufs-rpmb.c

-- 
2.34.1

[PATCH v2 1/3] rpmb: move rpmb_frame struct and constants to common header

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Move struct rpmb_frame and RPMB operation constants from MMC block
driver to include/linux/rpmb.h for reuse across different RPMB
implementations (UFS, NVMe, etc.).

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/mmc/core/block.c | 42 --------------------------------------
 include/linux/rpmb.h     | 44 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b32eefcca4b7..bd5f6fcb03af 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -79,48 +79,6 @@ MODULE_ALIAS("mmc:block");
 #define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16)
 #define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0x0000FF00) >> 8)
 
-/**
- * struct rpmb_frame - rpmb frame as defined by eMMC 5.1 (JESD84-B51)
- *
- * @stuff        : stuff bytes
- * @key_mac      : The authentication key or the message authentication
- *                 code (MAC) depending on the request/response type.
- *                 The MAC will be delivered in the last (or the only)
- *                 block of data.
- * @data         : Data to be written or read by signed access.
- * @nonce        : Random number generated by the host for the requests
- *                 and copied to the response by the RPMB engine.
- * @write_counter: Counter value for the total amount of the successful
- *                 authenticated data write requests made by the host.
- * @addr         : Address of the data to be programmed to or read
- *                 from the RPMB. Address is the serial number of
- *                 the accessed block (half sector 256B).
- * @block_count  : Number of blocks (half sectors, 256B) requested to be
- *                 read/programmed.
- * @result       : Includes information about the status of the write counter
- *                 (valid, expired) and result of the access made to the RPMB.
- * @req_resp     : Defines the type of request and response to/from the memory.
- *
- * The stuff bytes and big-endian properties are modeled to fit to the spec.
- */
-struct rpmb_frame {
-	u8     stuff[196];
-	u8     key_mac[32];
-	u8     data[256];
-	u8     nonce[16];
-	__be32 write_counter;
-	__be16 addr;
-	__be16 block_count;
-	__be16 result;
-	__be16 req_resp;
-} __packed;
-
-#define RPMB_PROGRAM_KEY       0x1    /* Program RPMB Authentication Key */
-#define RPMB_GET_WRITE_COUNTER 0x2    /* Read RPMB write counter */
-#define RPMB_WRITE_DATA        0x3    /* Write data to RPMB partition */
-#define RPMB_READ_DATA         0x4    /* Read data from RPMB partition */
-#define RPMB_RESULT_READ       0x5    /* Read result request  (Internal) */
-
 #define RPMB_FRAME_SIZE        sizeof(struct rpmb_frame)
 #define CHECK_SIZE_NEQ(val) ((val) != sizeof(struct rpmb_frame))
 #define CHECK_SIZE_ALIGNED(val) IS_ALIGNED((val), sizeof(struct rpmb_frame))
diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
index cccda73eea4d..ed3f8e431eff 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -61,6 +61,50 @@ struct rpmb_dev {
 
 #define to_rpmb_dev(x)		container_of((x), struct rpmb_dev, dev)
 
+/**
+ * struct rpmb_frame - RPMB frame structure for authenticated access
+ *
+ * @stuff        : stuff bytes, a padding/reserved area of 196 bytes at the
+ *                 beginning of the RPMB frame. They don’t carry meaningful
+ *                 data but are required to make the frame exactly 512 bytes.
+ * @key_mac      : The authentication key or the message authentication
+ *                 code (MAC) depending on the request/response type.
+ *                 The MAC will be delivered in the last (or the only)
+ *                 block of data.
+ * @data         : Data to be written or read by signed access.
+ * @nonce        : Random number generated by the host for the requests
+ *                 and copied to the response by the RPMB engine.
+ * @write_counter: Counter value for the total amount of the successful
+ *                 authenticated data write requests made by the host.
+ * @addr         : Address of the data to be programmed to or read
+ *                 from the RPMB. Address is the serial number of
+ *                 the accessed block (half sector 256B).
+ * @block_count  : Number of blocks (half sectors, 256B) requested to be
+ *                 read/programmed.
+ * @result       : Includes information about the status of the write counter
+ *                 (valid, expired) and result of the access made to the RPMB.
+ * @req_resp     : Defines the type of request and response to/from the memory.
+ *
+ * The stuff bytes and big-endian properties are modeled to fit to the spec.
+ */
+struct rpmb_frame {
+	u8     stuff[196];
+	u8     key_mac[32];
+	u8     data[256];
+	u8     nonce[16];
+	__be32 write_counter;
+	__be16 addr;
+	__be16 block_count;
+	__be16 result;
+	__be16 req_resp;
+};
+
+#define RPMB_PROGRAM_KEY       0x1    /* Program RPMB Authentication Key */
+#define RPMB_GET_WRITE_COUNTER 0x2    /* Read RPMB write counter */
+#define RPMB_WRITE_DATA        0x3    /* Write data to RPMB partition */
+#define RPMB_READ_DATA         0x4    /* Read data from RPMB partition */
+#define RPMB_RESULT_READ       0x5    /* Read result request  (Internal) */
+
 #if IS_ENABLED(CONFIG_RPMB)
 struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
 void rpmb_dev_put(struct rpmb_dev *rdev);
-- 
2.34.1

[PATCH v2 2/3] scsi: ufs: core: fix incorrect buffer duplication in ufshcd_read_string_desc()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

The function ufshcd_read_string_desc() was duplicating memory starting
from the beginning of struct uc_string_id, which included the length
and type fields. As a result, the allocated buffer contained unwanted
metadata in addition to the string itself.

The correct behavior is to duplicate only the Unicode character array in
the structure. Update the code so that only the actual string content is
copied into the new buffer.

Fixes: 5f57704dbcfe ("scsi: ufs: Use kmemdup in ufshcd_read_string_desc()")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2e1fa8cf83f5..79c7588be28a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3823,7 +3823,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 		str[ret++] = '\0';
 
 	} else {
-		str = kmemdup(uc_str, uc_str->len, GFP_KERNEL);
+		str = kmemdup(uc_str->uc, uc_str->len, GFP_KERNEL);
 		if (!str) {
 			ret = -ENOMEM;
 			goto out;
-- 
2.34.1

[PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

This patch adds OP-TEE based RPMB support for UFS devices. This enables secure RPMB operations on
UFS devices through OP-TEE, providing the same functionality available for eMMC devices and
extending kernel-based secure storage support to UFS-based systems.

Benefits of OP-TEE based RPMB implementation:
- Eliminates dependency on userspace supplicant for RPMB access
- Enables early boot secure storage access (e.g., fTPM, secure UEFI variables)
- Provides kernel-level RPMB access as soon as UFS driver is initialized
- Removes complex initramfs dependencies and boot ordering requirements
- Ensures reliable and deterministic secure storage operations
- Supports both built-in and modular fTPM configurations

Co-developed-by: Can Guo <can.guo@oss.qualcomm.com>
Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/misc/Kconfig           |   2 +-
 drivers/ufs/core/Makefile      |   1 +
 drivers/ufs/core/ufs-rpmb.c    | 253 +++++++++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd-priv.h |  13 ++
 drivers/ufs/core/ufshcd.c      |  30 +++-
 include/ufs/ufs.h              |   4 +
 include/ufs/ufshcd.h           |   3 +
 7 files changed, 301 insertions(+), 5 deletions(-)
 create mode 100644 drivers/ufs/core/ufs-rpmb.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b9ca56930003..46ffa62eac6e 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -106,7 +106,7 @@ config PHANTOM
 
 config RPMB
 	tristate "RPMB partition interface"
-	depends on MMC
+	depends on MMC || SCSI_UFSHCD
 	help
 	  Unified RPMB unit interface for RPMB capable devices such as eMMC and
 	  UFS. Provides interface for in-kernel security controllers to access
diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
index cf820fa09a04..51e1867e524e 100644
--- a/drivers/ufs/core/Makefile
+++ b/drivers/ufs/core/Makefile
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_SCSI_UFSHCD)		+= ufshcd-core.o
 ufshcd-core-y				+= ufshcd.o ufs-sysfs.o ufs-mcq.o
+ufshcd-core-$(CONFIG_RPMB)		+= ufs-rpmb.o
 ufshcd-core-$(CONFIG_DEBUG_FS)		+= ufs-debugfs.o
 ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
 ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO)	+= ufshcd-crypto.o
diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
new file mode 100644
index 000000000000..84cdcf8efeb3
--- /dev/null
+++ b/drivers/ufs/core/ufs-rpmb.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UFS OP-TEE based RPMB Driver
+ *
+ * Copyright (C) 2025 Micron Technology, Inc.
+ * Copyright (C) 2025 Qualcomm Technologies, Inc.
+ *
+ * Authors:
+ *	Bean Huo <beanhuo@micron.com>
+ *	Can Guo <can.guo@oss.qualcomm.com>
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/rpmb.h>
+#include <linux/string.h>
+#include <linux/list.h>
+#include <ufs/ufshcd.h>
+#include <linux/unaligned.h>
+#include "ufshcd-priv.h"
+
+#define UFS_RPMB_SEC_PROTOCOL		0xEC	/* JEDEC UFS application */
+#define UFS_RPMB_SEC_PROTOCOL_ID	0x01	/* JEDEC UFS RPMB protocol ID, CDB byte3 */
+
+static const struct bus_type ufs_rpmb_bus_type = {
+	.name = "ufs_rpmb",
+};
+
+/* UFS RPMB device structure */
+struct ufs_rpmb_dev {
+	u8 region_id;
+	struct device dev;
+	struct rpmb_dev *rdev;
+	struct ufs_hba *hba;
+	struct list_head node;
+};
+
+static int ufs_sec_submit(struct ufs_hba *hba, u16 spsp, void *buffer, size_t len, bool send)
+{
+	struct scsi_device *sdev = hba->ufs_rpmb_wlun;
+	u8 cdb[12] = { };
+
+	cdb[0] = send ? SECURITY_PROTOCOL_OUT : SECURITY_PROTOCOL_IN;
+	cdb[1] = UFS_RPMB_SEC_PROTOCOL;
+	put_unaligned_be16(spsp, &cdb[2]);
+	put_unaligned_be32(len, &cdb[6]);
+
+	return scsi_execute_cmd(sdev, cdb, send ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
+				buffer, len, /*timeout=*/30 * HZ, 0, NULL);
+}
+
+/* UFS RPMB route frames implementation */
+static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_len, u8 *resp,
+					unsigned int resp_len)
+{
+	struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
+	struct rpmb_frame *frm_out = (struct rpmb_frame *)req;
+	bool need_result_read = true;
+	u16 req_type, protocol_id;
+	struct ufs_hba *hba;
+	int ret;
+
+	if (!ufs_rpmb) {
+		dev_err(dev, "Missing driver data\n");
+		return -ENODEV;
+	}
+
+	if (!req || !req_len || !resp || !resp_len)
+		return -EINVAL;
+
+	hba = ufs_rpmb->hba;
+
+	req_type = be16_to_cpu(frm_out->req_resp);
+
+	switch (req_type) {
+	case RPMB_PROGRAM_KEY:
+		if (req_len != sizeof(struct rpmb_frame) || resp_len != sizeof(struct rpmb_frame))
+			return -EINVAL;
+		break;
+	case RPMB_GET_WRITE_COUNTER:
+		if (req_len != sizeof(struct rpmb_frame) || resp_len != sizeof(struct rpmb_frame))
+			return -EINVAL;
+		need_result_read = false;
+		break;
+	case RPMB_WRITE_DATA:
+		if (req_len % sizeof(struct rpmb_frame) || resp_len != sizeof(struct rpmb_frame))
+			return -EINVAL;
+		break;
+	case RPMB_READ_DATA:
+		if (req_len != sizeof(struct rpmb_frame) || resp_len % sizeof(struct rpmb_frame))
+			return -EINVAL;
+		need_result_read = false;
+		break;
+	default:
+		dev_err(dev, "Unknown request type=0x%04x\n", req_type);
+		return -EINVAL;
+	}
+
+	protocol_id = ufs_rpmb->region_id << 8 | UFS_RPMB_SEC_PROTOCOL_ID;
+
+	ret = ufs_sec_submit(hba, protocol_id, req, req_len, true);
+	if (ret) {
+		dev_err(dev, "Command failed with ret=%d\n", ret);
+		return ret;
+	}
+
+	if (need_result_read) {
+		struct rpmb_frame *frm_resp = (struct rpmb_frame *)resp;
+
+		memset(frm_resp, 0, sizeof(*frm_resp));
+		frm_resp->req_resp = cpu_to_be16(RPMB_RESULT_READ);
+		ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, true);
+		if (ret) {
+			dev_err(dev, "Result read request failed with ret=%d\n", ret);
+			return ret;
+		}
+	}
+
+	if (!ret) {
+		ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, false);
+		if (ret)
+			dev_err(dev, "Response read failed with ret=%d\n", ret);
+	}
+
+	return ret;
+}
+
+static void ufs_rpmb_device_release(struct device *dev)
+{
+	struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
+
+	if (ufs_rpmb->rdev)
+		rpmb_dev_unregister(ufs_rpmb->rdev);
+}
+
+/* UFS RPMB device registration */
+int ufs_rpmb_probe(struct ufs_hba *hba)
+{
+	struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;
+	struct rpmb_dev *rdev;
+	u8 cid[16] = { };
+	int region;
+	u8 *sn;
+	u32 cap;
+	int ret;
+
+	if (!hba->ufs_rpmb_wlun) {
+		dev_info(hba->dev, "No RPMB LUN, skip RPMB registration\n");
+		return -ENODEV;
+	}
+
+	/* Get the UNICODE serial number data */
+	sn = hba->dev_info.serial_number;
+	if (!sn) {
+		dev_err(hba->dev, "Serial number not available\n");
+		return -EINVAL;
+	}
+
+	INIT_LIST_HEAD(&hba->rpmbs);
+
+	/* Copy serial number into device ID (max 15 chars + NUL). */
+	strscpy(cid, sn, sizeof(cid));
+
+	struct rpmb_descr descr = {
+		.type = RPMB_TYPE_UFS,
+		.route_frames = ufs_rpmb_route_frames,
+		.dev_id_len = sizeof(cid),
+		.reliable_wr_count = hba->dev_info.rpmb_io_size,
+	};
+
+	for (region = 0; region < 4; region++) {
+		cap = hba->dev_info.rpmb_region_size[region];
+		if (!cap)
+			continue;
+
+		ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
+		if (!ufs_rpmb) {
+			ret = -ENOMEM;
+			goto err_out;
+		}
+
+		ufs_rpmb->hba = hba;
+		ufs_rpmb->dev.parent = &hba->ufs_rpmb_wlun->sdev_gendev;
+		ufs_rpmb->dev.bus = &ufs_rpmb_bus_type;
+		ufs_rpmb->dev.release = ufs_rpmb_device_release;
+		dev_set_name(&ufs_rpmb->dev, "ufs_rpmb%d", region);
+
+		/* Set driver data BEFORE device_register */
+		dev_set_drvdata(&ufs_rpmb->dev, ufs_rpmb);
+
+		ret = device_register(&ufs_rpmb->dev);
+		if (ret) {
+			dev_err(hba->dev, "Failed to register UFS RPMB device %d\n", region);
+			put_device(&ufs_rpmb->dev);
+			goto err_out;
+		}
+
+		/* Make CID unique for this region by appending region numbe */
+		cid[sizeof(cid) - 1] = region;
+		descr.dev_id = (void *)cid;
+		descr.capacity = cap;
+
+		/* Register RPMB device */
+		rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);
+		if (IS_ERR(rdev)) {
+			dev_err(hba->dev, "Failed to register UFS RPMB device.\n");
+			device_unregister(&ufs_rpmb->dev);
+			ret = PTR_ERR(rdev);
+			goto err_out;
+		}
+
+		ufs_rpmb->rdev = rdev;
+		ufs_rpmb->region_id = region;
+
+		list_add_tail(&ufs_rpmb->node, &hba->rpmbs);
+
+		dev_info(hba->dev, "UFS RPMB region %d registered (capacity=%u)\n", region, cap);
+	}
+
+	return 0;
+err_out:
+	list_for_each_entry_safe(it, tmp, &hba->rpmbs, node) {
+		list_del(&it->node);
+		device_unregister(&it->dev);
+	}
+
+	return ret;
+}
+
+/* UFS RPMB remove handler */
+void ufs_rpmb_remove(struct ufs_hba *hba)
+{
+	struct ufs_rpmb_dev *ufs_rpmb, *tmp;
+
+	if (list_empty(&hba->rpmbs))
+		return;
+
+	/* Remove all registered RPMB devices */
+	list_for_each_entry_safe(ufs_rpmb, tmp, &hba->rpmbs, node) {
+		dev_info(hba->dev, "Removing UFS RPMB region %d\n", ufs_rpmb->region_id);
+		/* Remove from list first */
+		list_del(&ufs_rpmb->node);
+		/* Unregister device */
+		device_unregister(&ufs_rpmb->dev);
+	}
+
+	dev_info(hba->dev, "All UFS RPMB devices unregistered\n");
+}
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("OP-TEE RPMB subsystem based UFS RPMB driver");
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index d0a2c963a27d..523828d6b1d5 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -411,4 +411,17 @@ static inline u32 ufshcd_mcq_get_sq_head_slot(struct ufs_hw_queue *q)
 	return val / sizeof(struct utp_transfer_req_desc);
 }
 
+#ifdef CONFIG_RPMB
+int ufs_rpmb_probe(struct ufs_hba *hba);
+void ufs_rpmb_remove(struct ufs_hba *hba);
+#else
+static inline int ufs_rpmb_probe(struct ufs_hba *hba)
+{
+	return 0;
+}
+static inline void ufs_rpmb_remove(struct ufs_hba *hba)
+{
+}
+#endif
+
 #endif /* _UFSHCD_PRIV_H_ */
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 79c7588be28a..ec1670d94946 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5240,10 +5240,15 @@ 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;
+	/* In case of RPMB LU, check if advanced RPMB mode is enabled, and get region size */
+	if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_UPIU_RPMB_WLUN) {
+		if (desc_buf[RPMB_UNIT_DESC_PARAM_REGION_EN] & BIT(4))
+			hba->dev_info.b_advanced_rpmb_en = true;
+		hba->dev_info.rpmb_region_size[0] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION0_SIZE];
+		hba->dev_info.rpmb_region_size[1] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION1_SIZE];
+		hba->dev_info.rpmb_region_size[2] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION2_SIZE];
+		hba->dev_info.rpmb_region_size[3] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION3_SIZE];
+	}
 
 
 	kfree(desc_buf);
@@ -8151,8 +8156,11 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
 	if (IS_ERR(sdev_rpmb)) {
 		ret = PTR_ERR(sdev_rpmb);
+		hba->ufs_rpmb_wlun = NULL;
+		dev_err(hba->dev, "%s: RPMB WLUN not found\n", __func__);
 		goto remove_ufs_device_wlun;
 	}
+	hba->ufs_rpmb_wlun = sdev_rpmb;
 	ufshcd_blk_pm_runtime_init(sdev_rpmb);
 	scsi_device_put(sdev_rpmb);
 
@@ -8425,6 +8433,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	int err;
 	u8 model_index;
 	u8 *desc_buf;
+	u8 serial_index;
 	struct ufs_dev_info *dev_info = &hba->dev_info;
 
 	desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
@@ -8460,6 +8469,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 				UFS_DEV_HID_SUPPORT;
 
 	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
+	serial_index = desc_buf[DEVICE_DESC_PARAM_SN];
 
 	err = ufshcd_read_string_desc(hba, model_index,
 				      &dev_info->model, SD_ASCII_STD);
@@ -8469,6 +8479,12 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 		goto out;
 	}
 
+	err = ufshcd_read_string_desc(hba, serial_index, &dev_info->serial_number, SD_RAW);
+	if (err < 0) {
+		dev_err(hba->dev, "%s: Failed reading Serial Number. err = %d\n", __func__, err);
+		goto out;
+	}
+
 	hba->luns_avail = desc_buf[DEVICE_DESC_PARAM_NUM_LU] +
 		desc_buf[DEVICE_DESC_PARAM_NUM_WLU];
 
@@ -8504,6 +8520,8 @@ static void ufs_put_device_desc(struct ufs_hba *hba)
 
 	kfree(dev_info->model);
 	dev_info->model = NULL;
+	kfree(dev_info->serial_number);
+	dev_info->serial_number = NULL;
 }
 
 /**
@@ -8647,6 +8665,8 @@ static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
 	else if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0)
 		hba->dev_info.max_lu_supported = 8;
 
+	hba->dev_info.rpmb_io_size = desc_buf[GEOMETRY_DESC_PARAM_RPMB_RW_SIZE];
+
 out:
 	kfree(desc_buf);
 	return err;
@@ -8832,6 +8852,7 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 
 	ufs_bsg_probe(hba);
 	scsi_scan_host(hba->host);
+	ufs_rpmb_probe(hba);
 
 out:
 	return ret;
@@ -10391,6 +10412,7 @@ void ufshcd_remove(struct ufs_hba *hba)
 		ufshcd_rpm_get_sync(hba);
 	ufs_hwmon_remove(hba);
 	ufs_bsg_remove(hba);
+	ufs_rpmb_remove(hba);
 	ufs_sysfs_remove_nodes(hba->dev);
 	cancel_delayed_work_sync(&hba->ufs_rtc_update_work);
 	blk_mq_destroy_queue(hba->tmf_queue);
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 72fd385037a6..1d44e2b32253 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -651,6 +651,10 @@ struct ufs_dev_info {
 	u8 rtt_cap; /* bDeviceRTTCap */
 
 	bool hid_sup;
+
+	u8 *serial_number;
+	u8 rpmb_io_size;
+	u8 rpmb_region_size[4];
 };
 
 /*
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 1d3943777584..17e97a45ef71 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -984,6 +984,7 @@ struct ufs_hba {
 	struct Scsi_Host *host;
 	struct device *dev;
 	struct scsi_device *ufs_device_wlun;
+	struct scsi_device *ufs_rpmb_wlun;
 
 #ifdef CONFIG_SCSI_UFS_HWMON
 	struct device *hwmon_device;
@@ -1140,6 +1141,8 @@ struct ufs_hba {
 	int critical_health_count;
 	atomic_t dev_lvl_exception_count;
 	u64 dev_lvl_exception_id;
+
+	struct list_head rpmbs;
 };
 
 /**
-- 
2.34.1

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Jens Wiklander]

Hi Bean,

On Wed, Oct 1, 2025 at 8:08 AM Bean Huo <beanhuo@iokpp.de> wrote:
>
> From: Bean Huo <beanhuo@micron.com>
>
> This patch adds OP-TEE based RPMB support for UFS devices. This enables secure RPMB operations on
> UFS devices through OP-TEE, providing the same functionality available for eMMC devices and
> extending kernel-based secure storage support to UFS-based systems.
>
> Benefits of OP-TEE based RPMB implementation:
> - Eliminates dependency on userspace supplicant for RPMB access
> - Enables early boot secure storage access (e.g., fTPM, secure UEFI variables)
> - Provides kernel-level RPMB access as soon as UFS driver is initialized
> - Removes complex initramfs dependencies and boot ordering requirements
> - Ensures reliable and deterministic secure storage operations
> - Supports both built-in and modular fTPM configurations
>
> Co-developed-by: Can Guo <can.guo@oss.qualcomm.com>
> Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/misc/Kconfig           |   2 +-
>  drivers/ufs/core/Makefile      |   1 +
>  drivers/ufs/core/ufs-rpmb.c    | 253 +++++++++++++++++++++++++++++++++
>  drivers/ufs/core/ufshcd-priv.h |  13 ++
>  drivers/ufs/core/ufshcd.c      |  30 +++-
>  include/ufs/ufs.h              |   4 +
>  include/ufs/ufshcd.h           |   3 +
>  7 files changed, 301 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/ufs/core/ufs-rpmb.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b9ca56930003..46ffa62eac6e 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -106,7 +106,7 @@ config PHANTOM
>
>  config RPMB
>         tristate "RPMB partition interface"
> -       depends on MMC
> +       depends on MMC || SCSI_UFSHCD
>         help
>           Unified RPMB unit interface for RPMB capable devices such as eMMC and
>           UFS. Provides interface for in-kernel security controllers to access
> diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
> index cf820fa09a04..51e1867e524e 100644
> --- a/drivers/ufs/core/Makefile
> +++ b/drivers/ufs/core/Makefile
> @@ -2,6 +2,7 @@
>
>  obj-$(CONFIG_SCSI_UFSHCD)              += ufshcd-core.o
>  ufshcd-core-y                          += ufshcd.o ufs-sysfs.o ufs-mcq.o
> +ufshcd-core-$(CONFIG_RPMB)             += ufs-rpmb.o

SCSI_UFSHCD might need the same trick ("depends on RPMB || !RPMB") in
Kconfig as we have for MMC_BLOCK.

>  ufshcd-core-$(CONFIG_DEBUG_FS)         += ufs-debugfs.o
>  ufshcd-core-$(CONFIG_SCSI_UFS_BSG)     += ufs_bsg.o
>  ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO)  += ufshcd-crypto.o
> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> new file mode 100644
> index 000000000000..84cdcf8efeb3
> --- /dev/null
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UFS OP-TEE based RPMB Driver
> + *
> + * Copyright (C) 2025 Micron Technology, Inc.
> + * Copyright (C) 2025 Qualcomm Technologies, Inc.
> + *
> + * Authors:
> + *     Bean Huo <beanhuo@micron.com>
> + *     Can Guo <can.guo@oss.qualcomm.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/rpmb.h>
> +#include <linux/string.h>
> +#include <linux/list.h>
> +#include <ufs/ufshcd.h>
> +#include <linux/unaligned.h>
> +#include "ufshcd-priv.h"
> +
> +#define UFS_RPMB_SEC_PROTOCOL          0xEC    /* JEDEC UFS application */
> +#define UFS_RPMB_SEC_PROTOCOL_ID       0x01    /* JEDEC UFS RPMB protocol ID, CDB byte3 */
> +
> +static const struct bus_type ufs_rpmb_bus_type = {
> +       .name = "ufs_rpmb",
> +};
> +
> +/* UFS RPMB device structure */
> +struct ufs_rpmb_dev {
> +       u8 region_id;
> +       struct device dev;
> +       struct rpmb_dev *rdev;
> +       struct ufs_hba *hba;
> +       struct list_head node;
> +};
> +
> +static int ufs_sec_submit(struct ufs_hba *hba, u16 spsp, void *buffer, size_t len, bool send)
> +{
> +       struct scsi_device *sdev = hba->ufs_rpmb_wlun;
> +       u8 cdb[12] = { };
> +
> +       cdb[0] = send ? SECURITY_PROTOCOL_OUT : SECURITY_PROTOCOL_IN;
> +       cdb[1] = UFS_RPMB_SEC_PROTOCOL;
> +       put_unaligned_be16(spsp, &cdb[2]);
> +       put_unaligned_be32(len, &cdb[6]);
> +
> +       return scsi_execute_cmd(sdev, cdb, send ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
> +                               buffer, len, /*timeout=*/30 * HZ, 0, NULL);
> +}
> +
> +/* UFS RPMB route frames implementation */
> +static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_len, u8 *resp,
> +                                       unsigned int resp_len)
> +{
> +       struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
> +       struct rpmb_frame *frm_out = (struct rpmb_frame *)req;
> +       bool need_result_read = true;
> +       u16 req_type, protocol_id;
> +       struct ufs_hba *hba;
> +       int ret;
> +
> +       if (!ufs_rpmb) {
> +               dev_err(dev, "Missing driver data\n");
> +               return -ENODEV;
> +       }
> +
> +       if (!req || !req_len || !resp || !resp_len)
> +               return -EINVAL;

This function is only called indirectly from rpmb_route_frames(),
where this is already checked.

> +
> +       hba = ufs_rpmb->hba;
> +
> +       req_type = be16_to_cpu(frm_out->req_resp);
> +
> +       switch (req_type) {
> +       case RPMB_PROGRAM_KEY:
> +               if (req_len != sizeof(struct rpmb_frame) || resp_len != sizeof(struct rpmb_frame))
> +                       return -EINVAL;
> +               break;
> +       case RPMB_GET_WRITE_COUNTER:
> +               if (req_len != sizeof(struct rpmb_frame) || resp_len != sizeof(struct rpmb_frame))
> +                       return -EINVAL;
> +               need_result_read = false;
> +               break;
> +       case RPMB_WRITE_DATA:
> +               if (req_len % sizeof(struct rpmb_frame) || resp_len != sizeof(struct rpmb_frame))
> +                       return -EINVAL;
> +               break;
> +       case RPMB_READ_DATA:
> +               if (req_len != sizeof(struct rpmb_frame) || resp_len % sizeof(struct rpmb_frame))
> +                       return -EINVAL;
> +               need_result_read = false;
> +               break;
> +       default:
> +               dev_err(dev, "Unknown request type=0x%04x\n", req_type);
> +               return -EINVAL;
> +       }
> +
> +       protocol_id = ufs_rpmb->region_id << 8 | UFS_RPMB_SEC_PROTOCOL_ID;
> +
> +       ret = ufs_sec_submit(hba, protocol_id, req, req_len, true);
> +       if (ret) {
> +               dev_err(dev, "Command failed with ret=%d\n", ret);
> +               return ret;
> +       }
> +
> +       if (need_result_read) {
> +               struct rpmb_frame *frm_resp = (struct rpmb_frame *)resp;
> +
> +               memset(frm_resp, 0, sizeof(*frm_resp));
> +               frm_resp->req_resp = cpu_to_be16(RPMB_RESULT_READ);
> +               ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, true);
> +               if (ret) {
> +                       dev_err(dev, "Result read request failed with ret=%d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (!ret) {
> +               ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, false);
> +               if (ret)
> +                       dev_err(dev, "Response read failed with ret=%d\n", ret);
> +       }
> +
> +       return ret;
> +}
> +
> +static void ufs_rpmb_device_release(struct device *dev)
> +{
> +       struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
> +
> +       if (ufs_rpmb->rdev)

rpmb_dev_unregister() already checks rdev for NULL.

> +               rpmb_dev_unregister(ufs_rpmb->rdev);
> +}
> +
> +/* UFS RPMB device registration */
> +int ufs_rpmb_probe(struct ufs_hba *hba)
> +{
> +       struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;
> +       struct rpmb_dev *rdev;
> +       u8 cid[16] = { };
> +       int region;
> +       u8 *sn;
> +       u32 cap;
> +       int ret;
> +
> +       if (!hba->ufs_rpmb_wlun) {
> +               dev_info(hba->dev, "No RPMB LUN, skip RPMB registration\n");
> +               return -ENODEV;
> +       }
> +
> +       /* Get the UNICODE serial number data */
> +       sn = hba->dev_info.serial_number;
> +       if (!sn) {
> +               dev_err(hba->dev, "Serial number not available\n");
> +               return -EINVAL;
> +       }
> +
> +       INIT_LIST_HEAD(&hba->rpmbs);
> +
> +       /* Copy serial number into device ID (max 15 chars + NUL). */
> +       strscpy(cid, sn, sizeof(cid));

strscpy(cid, sn) is enough; strscpy() can determine the size itself
since cid is an array.

> +
> +       struct rpmb_descr descr = {
> +               .type = RPMB_TYPE_UFS,

We'll need another type if the device uses the extended RPMB frame
format. How about you clarify this, where RPMB_TYPE_UFS is defined to
avoid confusion?

> +               .route_frames = ufs_rpmb_route_frames,
> +               .dev_id_len = sizeof(cid),
> +               .reliable_wr_count = hba->dev_info.rpmb_io_size,
> +       };
> +
> +       for (region = 0; region < 4; region++) {

Would it make sense to use ARRAY_SIZE(hba->dev_info.rpmb_region_size)
to avoid another magic number?

> +               cap = hba->dev_info.rpmb_region_size[region];
> +               if (!cap)
> +                       continue;
> +
> +               ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
> +               if (!ufs_rpmb) {
> +                       ret = -ENOMEM;
> +                       goto err_out;
> +               }
> +
> +               ufs_rpmb->hba = hba;
> +               ufs_rpmb->dev.parent = &hba->ufs_rpmb_wlun->sdev_gendev;
> +               ufs_rpmb->dev.bus = &ufs_rpmb_bus_type;
> +               ufs_rpmb->dev.release = ufs_rpmb_device_release;
> +               dev_set_name(&ufs_rpmb->dev, "ufs_rpmb%d", region);
> +
> +               /* Set driver data BEFORE device_register */
> +               dev_set_drvdata(&ufs_rpmb->dev, ufs_rpmb);
> +
> +               ret = device_register(&ufs_rpmb->dev);
> +               if (ret) {
> +                       dev_err(hba->dev, "Failed to register UFS RPMB device %d\n", region);
> +                       put_device(&ufs_rpmb->dev);
> +                       goto err_out;
> +               }
> +
> +               /* Make CID unique for this region by appending region numbe */
> +               cid[sizeof(cid) - 1] = region;
> +               descr.dev_id = (void *)cid;

Casting isn't needed; descr.dev_id has the same type as cid.

Cheers,
Jens

> +               descr.capacity = cap;
> +
> +               /* Register RPMB device */
> +               rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);
> +               if (IS_ERR(rdev)) {
> +                       dev_err(hba->dev, "Failed to register UFS RPMB device.\n");
> +                       device_unregister(&ufs_rpmb->dev);
> +                       ret = PTR_ERR(rdev);
> +                       goto err_out;
> +               }
> +
> +               ufs_rpmb->rdev = rdev;
> +               ufs_rpmb->region_id = region;
> +
> +               list_add_tail(&ufs_rpmb->node, &hba->rpmbs);
> +
> +               dev_info(hba->dev, "UFS RPMB region %d registered (capacity=%u)\n", region, cap);
> +       }
> +
> +       return 0;
> +err_out:
> +       list_for_each_entry_safe(it, tmp, &hba->rpmbs, node) {
> +               list_del(&it->node);
> +               device_unregister(&it->dev);
> +       }
> +
> +       return ret;
> +}
> +
> +/* UFS RPMB remove handler */
> +void ufs_rpmb_remove(struct ufs_hba *hba)
> +{
> +       struct ufs_rpmb_dev *ufs_rpmb, *tmp;
> +
> +       if (list_empty(&hba->rpmbs))
> +               return;
> +
> +       /* Remove all registered RPMB devices */
> +       list_for_each_entry_safe(ufs_rpmb, tmp, &hba->rpmbs, node) {
> +               dev_info(hba->dev, "Removing UFS RPMB region %d\n", ufs_rpmb->region_id);
> +               /* Remove from list first */
> +               list_del(&ufs_rpmb->node);
> +               /* Unregister device */
> +               device_unregister(&ufs_rpmb->dev);
> +       }
> +
> +       dev_info(hba->dev, "All UFS RPMB devices unregistered\n");
> +}
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("OP-TEE RPMB subsystem based UFS RPMB driver");
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index d0a2c963a27d..523828d6b1d5 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -411,4 +411,17 @@ static inline u32 ufshcd_mcq_get_sq_head_slot(struct ufs_hw_queue *q)
>         return val / sizeof(struct utp_transfer_req_desc);
>  }
>
> +#ifdef CONFIG_RPMB
> +int ufs_rpmb_probe(struct ufs_hba *hba);
> +void ufs_rpmb_remove(struct ufs_hba *hba);
> +#else
> +static inline int ufs_rpmb_probe(struct ufs_hba *hba)
> +{
> +       return 0;
> +}
> +static inline void ufs_rpmb_remove(struct ufs_hba *hba)
> +{
> +}
> +#endif
> +
>  #endif /* _UFSHCD_PRIV_H_ */
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 79c7588be28a..ec1670d94946 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5240,10 +5240,15 @@ 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;
> +       /* In case of RPMB LU, check if advanced RPMB mode is enabled, and get region size */
> +       if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_UPIU_RPMB_WLUN) {
> +               if (desc_buf[RPMB_UNIT_DESC_PARAM_REGION_EN] & BIT(4))
> +                       hba->dev_info.b_advanced_rpmb_en = true;

Does this indicate that the other RPMB frame format is used?

> +               hba->dev_info.rpmb_region_size[0] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION0_SIZE];
> +               hba->dev_info.rpmb_region_size[1] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION1_SIZE];
> +               hba->dev_info.rpmb_region_size[2] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION2_SIZE];
> +               hba->dev_info.rpmb_region_size[3] = desc_buf[RPMB_UNIT_DESC_PARAM_REGION3_SIZE];
> +       }
>
>
>         kfree(desc_buf);
> @@ -8151,8 +8156,11 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
>                 ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
>         if (IS_ERR(sdev_rpmb)) {
>                 ret = PTR_ERR(sdev_rpmb);
> +               hba->ufs_rpmb_wlun = NULL;
> +               dev_err(hba->dev, "%s: RPMB WLUN not found\n", __func__);
>                 goto remove_ufs_device_wlun;
>         }
> +       hba->ufs_rpmb_wlun = sdev_rpmb;
>         ufshcd_blk_pm_runtime_init(sdev_rpmb);
>         scsi_device_put(sdev_rpmb);
>
> @@ -8425,6 +8433,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>         int err;
>         u8 model_index;
>         u8 *desc_buf;
> +       u8 serial_index;
>         struct ufs_dev_info *dev_info = &hba->dev_info;
>
>         desc_buf = kzalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
> @@ -8460,6 +8469,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>                                 UFS_DEV_HID_SUPPORT;
>
>         model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
> +       serial_index = desc_buf[DEVICE_DESC_PARAM_SN];
>
>         err = ufshcd_read_string_desc(hba, model_index,
>                                       &dev_info->model, SD_ASCII_STD);
> @@ -8469,6 +8479,12 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>                 goto out;
>         }
>
> +       err = ufshcd_read_string_desc(hba, serial_index, &dev_info->serial_number, SD_RAW);
> +       if (err < 0) {
> +               dev_err(hba->dev, "%s: Failed reading Serial Number. err = %d\n", __func__, err);
> +               goto out;
> +       }
> +
>         hba->luns_avail = desc_buf[DEVICE_DESC_PARAM_NUM_LU] +
>                 desc_buf[DEVICE_DESC_PARAM_NUM_WLU];
>
> @@ -8504,6 +8520,8 @@ static void ufs_put_device_desc(struct ufs_hba *hba)
>
>         kfree(dev_info->model);
>         dev_info->model = NULL;
> +       kfree(dev_info->serial_number);
> +       dev_info->serial_number = NULL;
>  }
>
>  /**
> @@ -8647,6 +8665,8 @@ static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
>         else if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0)
>                 hba->dev_info.max_lu_supported = 8;
>
> +       hba->dev_info.rpmb_io_size = desc_buf[GEOMETRY_DESC_PARAM_RPMB_RW_SIZE];
> +
>  out:
>         kfree(desc_buf);
>         return err;
> @@ -8832,6 +8852,7 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>
>         ufs_bsg_probe(hba);
>         scsi_scan_host(hba->host);
> +       ufs_rpmb_probe(hba);
>
>  out:
>         return ret;
> @@ -10391,6 +10412,7 @@ void ufshcd_remove(struct ufs_hba *hba)
>                 ufshcd_rpm_get_sync(hba);
>         ufs_hwmon_remove(hba);
>         ufs_bsg_remove(hba);
> +       ufs_rpmb_remove(hba);
>         ufs_sysfs_remove_nodes(hba->dev);
>         cancel_delayed_work_sync(&hba->ufs_rtc_update_work);
>         blk_mq_destroy_queue(hba->tmf_queue);
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> index 72fd385037a6..1d44e2b32253 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -651,6 +651,10 @@ struct ufs_dev_info {
>         u8 rtt_cap; /* bDeviceRTTCap */
>
>         bool hid_sup;
> +
> +       u8 *serial_number;
> +       u8 rpmb_io_size;
> +       u8 rpmb_region_size[4];
>  };
>
>  /*
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 1d3943777584..17e97a45ef71 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -984,6 +984,7 @@ struct ufs_hba {
>         struct Scsi_Host *host;
>         struct device *dev;
>         struct scsi_device *ufs_device_wlun;
> +       struct scsi_device *ufs_rpmb_wlun;
>
>  #ifdef CONFIG_SCSI_UFS_HWMON
>         struct device *hwmon_device;
> @@ -1140,6 +1141,8 @@ struct ufs_hba {
>         int critical_health_count;
>         atomic_t dev_lvl_exception_count;
>         u64 dev_lvl_exception_id;
> +
> +       struct list_head rpmbs;
>  };
>
>  /**
> --
> 2.34.1
>

RE: [PATCH v2 1/3] rpmb: move rpmb_frame struct and constants to common header

[Avri Altman]

> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Move struct rpmb_frame and RPMB operation constants from MMC block
> driver to include/linux/rpmb.h for reuse across different RPMB
> implementations (UFS, NVMe, etc.).
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>

RE: [PATCH v2 2/3] scsi: ufs: core: fix incorrect buffer duplication in ufshcd_read_string_desc()

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> The function ufshcd_read_string_desc() was duplicating memory starting from
> the beginning of struct uc_string_id, which included the length and type
> fields. As a result, the allocated buffer contained unwanted metadata in
> addition to the string itself.
> 
> The correct behavior is to duplicate only the Unicode character array in the
> structure. Update the code so that only the actual string content is copied into
> the new buffer.
2 Nits - only If you'll have another spin:
Nit 1: maybe add one more sentence: This does not imply any ABI change as there are no current callers with SD_RAW
Nit 2: you might want to remove the duplicate definitions of SD_ASCII_STD & SD_RAW

> 
> Fixes: 5f57704dbcfe ("scsi: ufs: Use kmemdup in ufshcd_read_string_desc()")
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>


> ---
>  drivers/ufs/core/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 2e1fa8cf83f5..79c7588be28a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3823,7 +3823,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba,
> u8 desc_index,
>                 str[ret++] = '\0';
> 
>         } else {
> -               str = kmemdup(uc_str, uc_str->len, GFP_KERNEL);
> +               str = kmemdup(uc_str->uc, uc_str->len, GFP_KERNEL);
>                 if (!str) {
>                         ret = -ENOMEM;
>                         goto out;
> --
> 2.34.1

RE: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> This patch adds OP-TEE based RPMB support for UFS devices. This enables
> secure RPMB operations on UFS devices through OP-TEE, providing the same
> functionality available for eMMC devices and extending kernel-based secure
> storage support to UFS-based systems.
> 
> Benefits of OP-TEE based RPMB implementation:
> - Eliminates dependency on userspace supplicant for RPMB access
> - Enables early boot secure storage access (e.g., fTPM, secure UEFI variables)
> - Provides kernel-level RPMB access as soon as UFS driver is initialized
> - Removes complex initramfs dependencies and boot ordering requirements
> - Ensures reliable and deterministic secure storage operations
> - Supports both built-in and modular fTPM configurations
> 
> Co-developed-by: Can Guo <can.guo@oss.qualcomm.com>
> Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>

Nit: Would it make sense to simplify things, e.g. :
Instead of struct list_head rpmbs;
Use:
struct ufs_rpmb_dev *rpmbs[4];

Also, I don't remember if you were planning to add the additional rpmb operations (6 to 9) later or not.

Thanks,
Avri

Re: [PATCH v2 2/3] scsi: ufs: core: fix incorrect buffer duplication in ufshcd_read_string_desc()

[Bart Van Assche]

On 9/30/25 11:08 PM, Bean Huo wrote:
> The function ufshcd_read_string_desc() was duplicating memory starting
> from the beginning of struct uc_string_id, which included the length
> and type fields. As a result, the allocated buffer contained unwanted
> metadata in addition to the string itself.
> 
> The correct behavior is to duplicate only the Unicode character array in
> the structure. Update the code so that only the actual string content is
> copied into the new buffer.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 1/3] rpmb: move rpmb_frame struct and constants to common header

[Bart Van Assche]

On 9/30/25 11:08 PM, Bean Huo wrote:
> Move struct rpmb_frame and RPMB operation constants from MMC block
> driver to include/linux/rpmb.h for reuse across different RPMB
> implementations (UFS, NVMe, etc.).

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bart Van Assche]

On 9/30/25 11:08 PM, Bean Huo wrote:
> +MODULE_DESCRIPTION("OP-TEE RPMB subsystem based UFS RPMB driver");

Hmm ... "OP-TEE UFS RPMB driver" is probably more clear and still
unambiguous. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 2/3] scsi: ufs: core: fix incorrect buffer duplication in ufshcd_read_string_desc()

[Bean Huo]

On Wed, 2025-10-01 at 10:03 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > The function ufshcd_read_string_desc() was duplicating memory starting from
> > the beginning of struct uc_string_id, which included the length and type
> > fields. As a result, the allocated buffer contained unwanted metadata in
> > addition to the string itself.
> > 
> > The correct behavior is to duplicate only the Unicode character array in the
> > structure. Update the code so that only the actual string content is copied
> > into
> > the new buffer.
> 2 Nits - only If you'll have another spin:
> Nit 1: maybe add one more sentence: This does not imply any ABI change as
> there are no current callers with SD_RAW
> Nit 2: you might want to remove the duplicate definitions of SD_ASCII_STD &
> SD_RAW

yes, thanks Avri. I will address it in next version.

Kind regards,
Bean

> 
> > 
> > Fixes: 5f57704dbcfe ("scsi: ufs: Use kmemdup in ufshcd_read_string_desc()")
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Avri Altman <avri.altman@sandisk.com>

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Wed, 2025-10-01 at 10:06 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > This patch adds OP-TEE based RPMB support for UFS devices. This enables
> > secure RPMB operations on UFS devices through OP-TEE, providing the same
> > functionality available for eMMC devices and extending kernel-based secure
> > storage support to UFS-based systems.
> > 
> > Benefits of OP-TEE based RPMB implementation:
> > - Eliminates dependency on userspace supplicant for RPMB access
> > - Enables early boot secure storage access (e.g., fTPM, secure UEFI
> > variables)
> > - Provides kernel-level RPMB access as soon as UFS driver is initialized
> > - Removes complex initramfs dependencies and boot ordering requirements
> > - Ensures reliable and deterministic secure storage operations
> > - Supports both built-in and modular fTPM configurations
> > 
> > Co-developed-by: Can Guo <can.guo@oss.qualcomm.com>
> > Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Avri Altman <avri.altman@sandisk.com>
> 
> Nit: Would it make sense to simplify things, e.g. :
> Instead of struct list_head rpmbs;
> Use:
> struct ufs_rpmb_dev *rpmbs[4];

Avri, 

yes, having a fixed-size data set, choose an array over a list when needs fast,
random access to elements by index. I will address it in next version.

> Also, I don't remember if you were planning to add the additional rpmb
> operations (6 to 9) later or not.

yes, to make those usable, firstly need to enable in op-tee OS,  I will check
op-tee OS and enable in extension patch.

Kind regards,
Bean

> 
> Thanks,
> Avri

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Wed, 2025-10-01 at 09:50 +0200, Jens Wiklander wrote:
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -5240,10 +5240,15 @@ 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;
> > +       /* In case of RPMB LU, check if advanced RPMB mode is enabled, and
> > get region size */
> > +       if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_UPIU_RPMB_WLUN) {
> > +               if (desc_buf[RPMB_UNIT_DESC_PARAM_REGION_EN] & BIT(4))
> > +                       hba->dev_info.b_advanced_rpmb_en = true;
> 
> Does this indicate that the other RPMB frame format is used?

yes, if BIT4 is 1, means Advanced RPMB is enabled, from the Spec:

"There are two RPMB modes; Normal RPMB mode and Advanced RPMB mode using EHS.
The RPMB mode can be configured by setting Bit4 of bRPMBRegionEnable parameter
of the RPMB descriptor in the configuration stage. If the device receives an
RPMB operation request of a different mode than the configured RPMB mode, the
device shall respond with ILLEGAL REQUEST."


if it is in Advanced RPMB mode, and use sends normal RPMB frame, the device will
respond ILLEGAL REQUEST in response.  but it is better to mute this noise, I
will add this in next version:  if it is in advanced RPMB, we will not call RPMB
probe.

Regarding the advanced RPMB, its frame is quite different with normal RPMB,
since we need to pass EHS, not just RPMB frame as we use. It is better to have a
check you and us online, see which way is better to enalbe it in op-tee OS.


Kind regards,
Bean

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Wed, 2025-10-01 at 12:51 -0700, Bart Van Assche wrote:
> On 9/30/25 11:08 PM, Bean Huo wrote:
> > +MODULE_DESCRIPTION("OP-TEE RPMB subsystem based UFS RPMB driver");
> 
> Hmm ... "OP-TEE UFS RPMB driver" is probably more clear and still
> unambiguous. Anyway:

Bart,

ok, I will change to it in next version.

Kind regards,
Bean

Re: [PATCH v2 1/3] rpmb: move rpmb_frame struct and constants to common header

[Jens Wiklander]

On Wed, Oct 1, 2025 at 8:08 AM Bean Huo <beanhuo@iokpp.de> wrote:
>
> From: Bean Huo <beanhuo@micron.com>
>
> Move struct rpmb_frame and RPMB operation constants from MMC block
> driver to include/linux/rpmb.h for reuse across different RPMB
> implementations (UFS, NVMe, etc.).
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/mmc/core/block.c | 42 --------------------------------------
>  include/linux/rpmb.h     | 44 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 42 deletions(-)

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks,
Jens

Re: [PATCH v2 1/3] rpmb: move rpmb_frame struct and constants to common header

[Ulf Hansson]

On Wed, 1 Oct 2025 at 08:08, Bean Huo <beanhuo@iokpp.de> wrote:
>
> From: Bean Huo <beanhuo@micron.com>
>
> Move struct rpmb_frame and RPMB operation constants from MMC block
> driver to include/linux/rpmb.h for reuse across different RPMB
> implementations (UFS, NVMe, etc.).
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>

I have queued this up via my mmc tree and plan to send a late
pull-request to Linus to get this included for 6.18-rc1.

That should help to land the remaining pieces for ufs in the second
step without having to care about the mmc parts.

Kind regards
Uffe

> ---
>  drivers/mmc/core/block.c | 42 --------------------------------------
>  include/linux/rpmb.h     | 44 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index b32eefcca4b7..bd5f6fcb03af 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -79,48 +79,6 @@ MODULE_ALIAS("mmc:block");
>  #define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16)
>  #define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0x0000FF00) >> 8)
>
> -/**
> - * struct rpmb_frame - rpmb frame as defined by eMMC 5.1 (JESD84-B51)
> - *
> - * @stuff        : stuff bytes
> - * @key_mac      : The authentication key or the message authentication
> - *                 code (MAC) depending on the request/response type.
> - *                 The MAC will be delivered in the last (or the only)
> - *                 block of data.
> - * @data         : Data to be written or read by signed access.
> - * @nonce        : Random number generated by the host for the requests
> - *                 and copied to the response by the RPMB engine.
> - * @write_counter: Counter value for the total amount of the successful
> - *                 authenticated data write requests made by the host.
> - * @addr         : Address of the data to be programmed to or read
> - *                 from the RPMB. Address is the serial number of
> - *                 the accessed block (half sector 256B).
> - * @block_count  : Number of blocks (half sectors, 256B) requested to be
> - *                 read/programmed.
> - * @result       : Includes information about the status of the write counter
> - *                 (valid, expired) and result of the access made to the RPMB.
> - * @req_resp     : Defines the type of request and response to/from the memory.
> - *
> - * The stuff bytes and big-endian properties are modeled to fit to the spec.
> - */
> -struct rpmb_frame {
> -       u8     stuff[196];
> -       u8     key_mac[32];
> -       u8     data[256];
> -       u8     nonce[16];
> -       __be32 write_counter;
> -       __be16 addr;
> -       __be16 block_count;
> -       __be16 result;
> -       __be16 req_resp;
> -} __packed;
> -
> -#define RPMB_PROGRAM_KEY       0x1    /* Program RPMB Authentication Key */
> -#define RPMB_GET_WRITE_COUNTER 0x2    /* Read RPMB write counter */
> -#define RPMB_WRITE_DATA        0x3    /* Write data to RPMB partition */
> -#define RPMB_READ_DATA         0x4    /* Read data from RPMB partition */
> -#define RPMB_RESULT_READ       0x5    /* Read result request  (Internal) */
> -
>  #define RPMB_FRAME_SIZE        sizeof(struct rpmb_frame)
>  #define CHECK_SIZE_NEQ(val) ((val) != sizeof(struct rpmb_frame))
>  #define CHECK_SIZE_ALIGNED(val) IS_ALIGNED((val), sizeof(struct rpmb_frame))
> diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
> index cccda73eea4d..ed3f8e431eff 100644
> --- a/include/linux/rpmb.h
> +++ b/include/linux/rpmb.h
> @@ -61,6 +61,50 @@ struct rpmb_dev {
>
>  #define to_rpmb_dev(x)         container_of((x), struct rpmb_dev, dev)
>
> +/**
> + * struct rpmb_frame - RPMB frame structure for authenticated access
> + *
> + * @stuff        : stuff bytes, a padding/reserved area of 196 bytes at the
> + *                 beginning of the RPMB frame. They don’t carry meaningful
> + *                 data but are required to make the frame exactly 512 bytes.
> + * @key_mac      : The authentication key or the message authentication
> + *                 code (MAC) depending on the request/response type.
> + *                 The MAC will be delivered in the last (or the only)
> + *                 block of data.
> + * @data         : Data to be written or read by signed access.
> + * @nonce        : Random number generated by the host for the requests
> + *                 and copied to the response by the RPMB engine.
> + * @write_counter: Counter value for the total amount of the successful
> + *                 authenticated data write requests made by the host.
> + * @addr         : Address of the data to be programmed to or read
> + *                 from the RPMB. Address is the serial number of
> + *                 the accessed block (half sector 256B).
> + * @block_count  : Number of blocks (half sectors, 256B) requested to be
> + *                 read/programmed.
> + * @result       : Includes information about the status of the write counter
> + *                 (valid, expired) and result of the access made to the RPMB.
> + * @req_resp     : Defines the type of request and response to/from the memory.
> + *
> + * The stuff bytes and big-endian properties are modeled to fit to the spec.
> + */
> +struct rpmb_frame {
> +       u8     stuff[196];
> +       u8     key_mac[32];
> +       u8     data[256];
> +       u8     nonce[16];
> +       __be32 write_counter;
> +       __be16 addr;
> +       __be16 block_count;
> +       __be16 result;
> +       __be16 req_resp;
> +};
> +
> +#define RPMB_PROGRAM_KEY       0x1    /* Program RPMB Authentication Key */
> +#define RPMB_GET_WRITE_COUNTER 0x2    /* Read RPMB write counter */
> +#define RPMB_WRITE_DATA        0x3    /* Write data to RPMB partition */
> +#define RPMB_READ_DATA         0x4    /* Read data from RPMB partition */
> +#define RPMB_RESULT_READ       0x5    /* Read result request  (Internal) */
> +
>  #if IS_ENABLED(CONFIG_RPMB)
>  struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
>  void rpmb_dev_put(struct rpmb_dev *rdev);
> --
> 2.34.1
>

Re: [PATCH v2 1/3] rpmb: move rpmb_frame struct and constants to common header

[Bean Huo]

On Mon, 2025-10-06 at 12:21 +0200, Ulf Hansson wrote:
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> 
> I have queued this up via my mmc tree and plan to send a late
> pull-request to Linus to get this included for 6.18-rc1.
> 
> That should help to land the remaining pieces for ufs in the second
> step without having to care about the mmc parts.
> 
> Kind regards
> Uffe


thanks Uffe,

Kind regards,
Bean

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Wed, 2025-10-01 at 10:06 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > This patch adds OP-TEE based RPMB support for UFS devices. This enables
> > secure RPMB operations on UFS devices through OP-TEE, providing the same
> > functionality available for eMMC devices and extending kernel-based secure
> > storage support to UFS-based systems.
> > 
> > Benefits of OP-TEE based RPMB implementation:
> > - Eliminates dependency on userspace supplicant for RPMB access
> > - Enables early boot secure storage access (e.g., fTPM, secure UEFI
> > variables)
> > - Provides kernel-level RPMB access as soon as UFS driver is initialized
> > - Removes complex initramfs dependencies and boot ordering requirements
> > - Ensures reliable and deterministic secure storage operations
> > - Supports both built-in and modular fTPM configurations
> > 
> > Co-developed-by: Can Guo <can.guo@oss.qualcomm.com>
> > Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Avri Altman <avri.altman@sandisk.com>
> 
> Nit: Would it make sense to simplify things, e.g. :
> Instead of struct list_head rpmbs;
> Use:
> struct ufs_rpmb_dev *rpmbs[4];


Hi Avri, 

I am working on the next version, seems we should keep struct list_head rpmbs. 

On the hot path, runtime RPMB I/O operations use dev_get_drvdata(dev) to get the
device pointer directly, never searching through hba->rpmbs. Array's O(1) direct
access advantage is therefore irrelevant. and the list is only accessed during
probe/remove (one-time operations at boot/shutdown) where performance
differences are negligible. The list iterates only over active regions without
NULL checks, while an array requires checking all 4 slots.

List uses 16 bytes per active region, array uses 32 bytes (4 × 8-byte pointers)
regardless of how many regions are active, most of UFS devices only enabled 1-2
RPMB regions, making the list more memory-efficient, right?


how do you think?

Kind regards,
Bean

RE: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Avri Altman]

> 
> On Wed, 2025-10-01 at 10:06 +0000, Avri Altman wrote:
> > > From: Bean Huo <beanhuo@micron.com>
> > >
> > > This patch adds OP-TEE based RPMB support for UFS devices. This
> > > enables secure RPMB operations on UFS devices through OP-TEE,
> > > providing the same functionality available for eMMC devices and
> > > extending kernel-based secure storage support to UFS-based systems.
> > >
> > > Benefits of OP-TEE based RPMB implementation:
> > > - Eliminates dependency on userspace supplicant for RPMB access
> > > - Enables early boot secure storage access (e.g., fTPM, secure UEFI
> > > variables)
> > > - Provides kernel-level RPMB access as soon as UFS driver is
> > > initialized
> > > - Removes complex initramfs dependencies and boot ordering
> > > requirements
> > > - Ensures reliable and deterministic secure storage operations
> > > - Supports both built-in and modular fTPM configurations
> > >
> > > Co-developed-by: Can Guo <can.guo@oss.qualcomm.com>
> > > Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > Reviewed-by: Avri Altman <avri.altman@sandisk.com>
> >
> > Nit: Would it make sense to simplify things, e.g. :
> > Instead of struct list_head rpmbs;
> > Use:
> > struct ufs_rpmb_dev *rpmbs[4];
> 
> 
> Hi Avri,
> 
> I am working on the next version, seems we should keep struct list_head
> rpmbs.
> 
> On the hot path, runtime RPMB I/O operations use dev_get_drvdata(dev) to
> get the device pointer directly, never searching through hba->rpmbs. Array's
> O(1) direct access advantage is therefore irrelevant. and the list is only
> accessed during probe/remove (one-time operations at boot/shutdown)
> where performance differences are negligible. The list iterates only over active
> regions without NULL checks, while an array requires checking all 4 slots.
> 
> List uses 16 bytes per active region, array uses 32 bytes (4 × 8-byte pointers)
> regardless of how many regions are active, most of UFS devices only enabled
> 1-2 RPMB regions, making the list more memory-efficient, right?
Yes.  The code looks good as it is.

Thanks,
Avri

> 
> 
> how do you think?
> 
> Kind regards,
> Bean
> 
> 

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

Jens, 

I incorporated your suggestions in my v3 excpet these two:


On Wed, 2025-10-01 at 09:50 +0200, Jens Wiklander wrote:
> > diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
> > index cf820fa09a04..51e1867e524e 100644
> > --- a/drivers/ufs/core/Makefile
> > +++ b/drivers/ufs/core/Makefile
> > @@ -2,6 +2,7 @@
> > 
> >   obj-$(CONFIG_SCSI_UFSHCD)              += ufshcd-core.o
> >   ufshcd-core-y                          += ufshcd.o ufs-sysfs.o ufs-mcq.o
> > +ufshcd-core-$(CONFIG_RPMB)             += ufs-rpmb.o
> 
> SCSI_UFSHCD might need the same trick ("depends on RPMB || !RPMB") in
> Kconfig as we have for MMC_BLOCK.
> 
> > 
When RPMB=m and SCSI_UFSHCD=y, the ufs-rpmb.o is compiled into the built-in
ufshcd-core, ufs-rpmb.c calls functions from the OP-TEE RPMB subsystem module,
The kernel allows built-in code to reference module symbols (they become runtime
dependencies, not link-time), please check, I tested.

> > 
> > 
> 
> > +
> > +       struct rpmb_descr descr = {
> > +               .type = RPMB_TYPE_UFS,
> 
> We'll need another type if the device uses the extended RPMB frame
> format. How about you clarify this, where RPMB_TYPE_UFS is defined to
> avoid confusion?

As ufs-bsg.c, we could use ARPMB_TYPE_UFS for UFS advanced RPMB frame, if it is
RPMB, we take it as normal RPMB, the frame should be the same as MMC RPMB.


Kind regards,
Bean 

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Jens Wiklander]

Hi Bean,



On Wed, Oct 8, 2025 at 5:07 PM Bean Huo <huobean@gmail.com> wrote:
>
> Jens,
>
> I incorporated your suggestions in my v3 excpet these two:
>
>
> On Wed, 2025-10-01 at 09:50 +0200, Jens Wiklander wrote:
> > > diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
> > > index cf820fa09a04..51e1867e524e 100644
> > > --- a/drivers/ufs/core/Makefile
> > > +++ b/drivers/ufs/core/Makefile
> > > @@ -2,6 +2,7 @@
> > >
> > >   obj-$(CONFIG_SCSI_UFSHCD)              += ufshcd-core.o
> > >   ufshcd-core-y                          += ufshcd.o ufs-sysfs.o ufs-mcq.o
> > > +ufshcd-core-$(CONFIG_RPMB)             += ufs-rpmb.o
> >
> > SCSI_UFSHCD might need the same trick ("depends on RPMB || !RPMB") in
> > Kconfig as we have for MMC_BLOCK.
> >
> > >
> When RPMB=m and SCSI_UFSHCD=y, the ufs-rpmb.o is compiled into the built-in
> ufshcd-core, ufs-rpmb.c calls functions from the OP-TEE RPMB subsystem module,
> The kernel allows built-in code to reference module symbols (they become runtime
> dependencies, not link-time), please check, I tested.
>
> > >
> > >
> >
> > > +
> > > +       struct rpmb_descr descr = {
> > > +               .type = RPMB_TYPE_UFS,
> >
> > We'll need another type if the device uses the extended RPMB frame
> > format. How about you clarify this, where RPMB_TYPE_UFS is defined to
> > avoid confusion?
>
> As ufs-bsg.c, we could use ARPMB_TYPE_UFS for UFS advanced RPMB frame, if it is
> RPMB, we take it as normal RPMB, the frame should be the same as MMC RPMB.

Isn't it a bit confusing to set the type to RPMB_TYPE_EMMC when it's
actually a UFS RPMB, even if it's supposedly compatible enough?

While the frame format works, I'm concerned about the CID. It's
essentially a namespace of its own for eMMC, and at least the OP-TEE
implementation makes assumptions about the format by masking out the
PRV (Product Revision) and CRC (CRC7 checksum) fields from the CID
when deriving the RPMB key. For this to work reliably, the CID must be
guaranteed to be unique per RPMB device.

From what I understand, for UFS, the serial number is only guaranteed
to be unique if the manufacturer and the product name are taken into
account. Combined, these fields can be much larger than 16 bytes, and
we also have the partition number to consider.

By using RPMB_TYPE_UFS we can define a device ID tailored for UFS with
all the fields we need. Thoughts?

Cheers,
Jens

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Mon, 2025-10-13 at 10:21 +0200, Jens Wiklander wrote:
> Hi Bean,
> 
> 
> 
> On Wed, Oct 8, 2025 at 5:07 PM Bean Huo <huobean@gmail.com> wrote:
> > 
> > Jens,
> > 
> > I incorporated your suggestions in my v3 excpet these two:
> > 
> > 
> > On Wed, 2025-10-01 at 09:50 +0200, Jens Wiklander wrote:
> > > > diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
> > > > index cf820fa09a04..51e1867e524e 100644
> > > > --- a/drivers/ufs/core/Makefile
> > > > +++ b/drivers/ufs/core/Makefile
> > > > @@ -2,6 +2,7 @@
> > > > 
> > > >   obj-$(CONFIG_SCSI_UFSHCD)              += ufshcd-core.o
> > > >   ufshcd-core-y                          += ufshcd.o ufs-sysfs.o ufs-
> > > > mcq.o
> > > > +ufshcd-core-$(CONFIG_RPMB)             += ufs-rpmb.o
> > > 
> > > SCSI_UFSHCD might need the same trick ("depends on RPMB || !RPMB") in
> > > Kconfig as we have for MMC_BLOCK.
> > > 
> > > > 
> > When RPMB=m and SCSI_UFSHCD=y, the ufs-rpmb.o is compiled into the built-in
> > ufshcd-core, ufs-rpmb.c calls functions from the OP-TEE RPMB subsystem
> > module,
> > The kernel allows built-in code to reference module symbols (they become
> > runtime
> > dependencies, not link-time), please check, I tested.
> > 
> > > > 
> > > > 
> > > 
> > > > +
> > > > +       struct rpmb_descr descr = {
> > > > +               .type = RPMB_TYPE_UFS,
> > > 
> > > We'll need another type if the device uses the extended RPMB frame
> > > format. How about you clarify this, where RPMB_TYPE_UFS is defined to
> > > avoid confusion?
> > 
> > As ufs-bsg.c, we could use ARPMB_TYPE_UFS for UFS advanced RPMB frame, if it
> > is
> > RPMB, we take it as normal RPMB, the frame should be the same as MMC RPMB.
> 
> Isn't it a bit confusing to set the type to RPMB_TYPE_EMMC when it's
> actually a UFS RPMB, even if it's supposedly compatible enough?
> 

The RPMB data format is the same for both eMMC RPMB and standard UFS RPMB.
However, the application commands used to access RPMB differ — eMMC uses MMC
commands, while UFS uses SCSI commands.

Additionally, UFS RPMB supports more RPMB operations than eMMC RPMB. Therefore,
we need to distinguish between them:

RPMB_TYPE_EMMC for eMMC RPMB

RPMB_TYPE_UFS for standard UFS RPMB

ARPMB_TYPE_UFS for advanced UFS RPMB.


> While the frame format works, I'm concerned about the CID. It's
> essentially a namespace of its own for eMMC, and at least the OP-TEE
> implementation makes assumptions about the format by masking out the
> PRV (Product Revision) and CRC (CRC7 checksum) fields from the CID
> when deriving the RPMB key. For this to work reliably, the CID must be
> guaranteed to be unique per RPMB device.
> 
> From what I understand, for UFS, the serial number is only guaranteed
> to be unique if the manufacturer and the product name are taken into
> account. Combined, these fields can be much larger than 16 bytes, and
> we also have the partition number to consider.
> 
> By using RPMB_TYPE_UFS we can define a device ID tailored for UFS with
> all the fields we need. Thoughts?
> 

For certain memory vendors, the serial number is guaranteed to be unique among
all devices.

For partitions or regions, we have appended the region number to the end of the
CID — please check the patch for details.

Regarding improving CID uniqueness, we could include the OEM ID or product
number. However, this would make the CID longer than 16 bytes.



Kind regards,
Bean

> Cheers,
> Jens

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Jens Wiklander]

On Mon, Oct 13, 2025 at 2:04 PM Bean Huo <huobean@gmail.com> wrote:
>
> On Mon, 2025-10-13 at 10:21 +0200, Jens Wiklander wrote:
> > Hi Bean,
> >
> >
> >
> > On Wed, Oct 8, 2025 at 5:07 PM Bean Huo <huobean@gmail.com> wrote:
> > >
> > > Jens,
> > >
> > > I incorporated your suggestions in my v3 excpet these two:
> > >
> > >
> > > On Wed, 2025-10-01 at 09:50 +0200, Jens Wiklander wrote:
> > > > > diff --git a/drivers/ufs/core/Makefile b/drivers/ufs/core/Makefile
> > > > > index cf820fa09a04..51e1867e524e 100644
> > > > > --- a/drivers/ufs/core/Makefile
> > > > > +++ b/drivers/ufs/core/Makefile
> > > > > @@ -2,6 +2,7 @@
> > > > >
> > > > >   obj-$(CONFIG_SCSI_UFSHCD)              += ufshcd-core.o
> > > > >   ufshcd-core-y                          += ufshcd.o ufs-sysfs.o ufs-
> > > > > mcq.o
> > > > > +ufshcd-core-$(CONFIG_RPMB)             += ufs-rpmb.o
> > > >
> > > > SCSI_UFSHCD might need the same trick ("depends on RPMB || !RPMB") in
> > > > Kconfig as we have for MMC_BLOCK.
> > > >
> > > > >
> > > When RPMB=m and SCSI_UFSHCD=y, the ufs-rpmb.o is compiled into the built-in
> > > ufshcd-core, ufs-rpmb.c calls functions from the OP-TEE RPMB subsystem
> > > module,
> > > The kernel allows built-in code to reference module symbols (they become
> > > runtime
> > > dependencies, not link-time), please check, I tested.
> > >
> > > > >
> > > > >
> > > >
> > > > > +
> > > > > +       struct rpmb_descr descr = {
> > > > > +               .type = RPMB_TYPE_UFS,
> > > >
> > > > We'll need another type if the device uses the extended RPMB frame
> > > > format. How about you clarify this, where RPMB_TYPE_UFS is defined to
> > > > avoid confusion?
> > >
> > > As ufs-bsg.c, we could use ARPMB_TYPE_UFS for UFS advanced RPMB frame, if it
> > > is
> > > RPMB, we take it as normal RPMB, the frame should be the same as MMC RPMB.
> >
> > Isn't it a bit confusing to set the type to RPMB_TYPE_EMMC when it's
> > actually a UFS RPMB, even if it's supposedly compatible enough?
> >
>
> The RPMB data format is the same for both eMMC RPMB and standard UFS RPMB.
> However, the application commands used to access RPMB differ — eMMC uses MMC
> commands, while UFS uses SCSI commands.
>
> Additionally, UFS RPMB supports more RPMB operations than eMMC RPMB. Therefore,
> we need to distinguish between them:
>
> RPMB_TYPE_EMMC for eMMC RPMB
>
> RPMB_TYPE_UFS for standard UFS RPMB
>
> ARPMB_TYPE_UFS for advanced UFS RPMB.

Good. A distinction was what I was asking for.

>
>
> > While the frame format works, I'm concerned about the CID. It's
> > essentially a namespace of its own for eMMC, and at least the OP-TEE
> > implementation makes assumptions about the format by masking out the
> > PRV (Product Revision) and CRC (CRC7 checksum) fields from the CID
> > when deriving the RPMB key. For this to work reliably, the CID must be
> > guaranteed to be unique per RPMB device.
> >
> > From what I understand, for UFS, the serial number is only guaranteed
> > to be unique if the manufacturer and the product name are taken into
> > account. Combined, these fields can be much larger than 16 bytes, and
> > we also have the partition number to consider.
> >
> > By using RPMB_TYPE_UFS we can define a device ID tailored for UFS with
> > all the fields we need. Thoughts?
> >
>
> For certain memory vendors, the serial number is guaranteed to be unique among
> all devices.

This is supposed to be generic and not rely on the behavior of only
some vendors.

>
> For partitions or regions, we have appended the region number to the end of the
> CID — please check the patch for details.

Yes, but how do you know that you don't overwrite a part of the serial number?

>
> Regarding improving CID uniqueness, we could include the OEM ID or product
> number. However, this would make the CID longer than 16 bytes.

UFS doesn't have a CID, but there's no need for one either. struct
rpmb_descr has dev_id and dev_id_len. It can be any length, within
reason.

Cheers,
Jens

>
>
>
> Kind regards,
> Bean
>
> > Cheers,
> > Jens
>

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Mon, 2025-10-13 at 14:22 +0200, Jens Wiklander wrote:
> > 
> > For certain memory vendors, the serial number is guaranteed to be unique
> > among
> > all devices.
> 
> This is supposed to be generic and not rely on the behavior of only
> some vendors.
> 
> > 
> > For partitions or regions, we have appended the region number to the end of
> > the
> > CID — please check the patch for details.
> 
> Yes, but how do you know that you don't overwrite a part of the serial number?
> 
> > 
> > Regarding improving CID uniqueness, we could include the OEM ID or product
> > number. However, this would make the CID longer than 16 bytes.
> 
> UFS doesn't have a CID, but there's no need for one either. struct
> rpmb_descr has dev_id and dev_id_len. It can be any length, within
> reason.
> 
> Cheers,
> Jens


Hi Jens,

how about combining wManufacturerID, wManufactureDate, wDeviceVersion,  Serial
Number (in unicode), plus the region number?


Kind regards,
Bean

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Jens Wiklander]

On Mon, Oct 13, 2025 at 5:43 PM Bean Huo <beanhuo@iokpp.de> wrote:
>
> On Mon, 2025-10-13 at 14:22 +0200, Jens Wiklander wrote:
> > >
> > > For certain memory vendors, the serial number is guaranteed to be unique
> > > among
> > > all devices.
> >
> > This is supposed to be generic and not rely on the behavior of only
> > some vendors.
> >
> > >
> > > For partitions or regions, we have appended the region number to the end of
> > > the
> > > CID — please check the patch for details.
> >
> > Yes, but how do you know that you don't overwrite a part of the serial number?
> >
> > >
> > > Regarding improving CID uniqueness, we could include the OEM ID or product
> > > number. However, this would make the CID longer than 16 bytes.
> >
> > UFS doesn't have a CID, but there's no need for one either. struct
> > rpmb_descr has dev_id and dev_id_len. It can be any length, within
> > reason.
> >
> > Cheers,
> > Jens
>
>
> Hi Jens,
>
> how about combining wManufacturerID, wManufactureDate, wDeviceVersion,  Serial
> Number (in unicode), plus the region number?

Sounds good.

Cheers,
Jens

>
>
> Kind regards,
> Bean

Re: [PATCH v2 3/3] scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices

[Bean Huo]

On Mon, 2025-10-13 at 17:53 +0200, Jens Wiklander wrote:
> > Hi Jens,
> > 
> > how about combining wManufacturerID, wManufactureDate, wDeviceVersion, 
> > Serial
> > Number (in unicode), plus the region number?
> 
> Sounds good.
> 
> Cheers,
> Jens

Jens, 

please check the new version patch v5 I have sent, in which, I addressed your
concern.

Kind regards,
Bean