[PATCH v2 0/2] scsi: ufs: batch device-init reads with UFS 5.0 aggregated read

[PATCH v2 0/2] scsi: ufs: batch device-init reads with UFS 5.0 aggregated read

[Hyeoncheol Jeong]

UFS 5.0 adds the AGGREGATED READ query (opcode 0x9): one QUERY RESPONSE
UPIU returns many descriptors, attributes and flags as a chain of typed
groups.

Patch 1 adds the opcode, the request mask, the group header and the reply
plumbing. Patch 2 uses it during device init: a single aggregated read
replaces the per-item queries for the attributes, flags and ID strings,
falling back to individual queries on older devices or a rejected opcode.

No functional change for pre-5.0 devices.

Motivation: on a Qualcomm SM8850 platform with a Samsung UFS 5.0 device,
timing raw UPIU queries over BSG (100 iterations) shows that replacing the
device-init read burst with a single aggregated read reduces the query
time by 75-78% (min 75.3%, avg 76.4%, max 78.5%).

(That setup batched 8 init reads; on current mainline this series batches
the 6 device-init reads that remain individual queries.)

Changes since v1:
- Rename UFS_AGG_TYPE_UNIT_DESC to UFS_AGG_TYPE_UNIT_RPMB_DESC (Peter Wang)
- Add UFS_AGG_GROUP_RESERVED = 0x00 (Peter Wang)
- Rename hdr_off/next_off to group_off/next_group_off (Peter Wang)
- Rebased onto v7.3-rc5

Hyeoncheol Jeong (2):
  scsi: ufs: Add the aggregated read query opcode and its reply format
  scsi: ufs: Serve the device init reads from one aggregated read

 drivers/ufs/core/ufshcd.c | 364 +++++++++++++++++++++++++++++++++++---
 include/ufs/ufs.h         |  47 +++++
 include/ufs/ufshcd.h      |  11 ++
 3 files changed, 402 insertions(+), 20 deletions(-)

--
2.25.1

[PATCH v2 1/2] scsi: ufs: Add the aggregated read query opcode and its reply format

[Hyeoncheol Jeong]

UFS 5.0 adds the AGGREGATED READ query opcode (0x9): a single QUERY
RESPONSE UPIU returns many descriptors, attributes and flags as a chain
of typed groups (JESD220H 10.7.9.14).

Add the AGGREGATION TYPE mask, the group types and the group header, and
let ufshcd_copy_query_response() copy the reply's data segment like a
descriptor read, recording its length in struct ufs_query_res.

Signed-off-by: Hyeoncheol Jeong <hyenc.jeong@samsung.com>
---
 drivers/ufs/core/ufshcd.c |  7 +++++--
 include/ufs/ufs.h         | 44 +++++++++++++++++++++++++++++++++++++++
 include/ufs/ufshcd.h      |  2 ++
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 54f4e7d7de02..d07834fc4646 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2473,10 +2473,13 @@ int ufshcd_copy_query_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	struct ufs_query_res *query_res = &hba->dev_cmd.query.response;
 
 	memcpy(&query_res->upiu_res, &lrbp->ucd_rsp_ptr->qr, QUERY_OSF_SIZE);
+	query_res->data_segment_length =
+		be16_to_cpu(lrbp->ucd_rsp_ptr->header.data_segment_length);
 
-	/* Get the descriptor */
+	/* Get the descriptor or aggregated data packet */
 	if (hba->dev_cmd.query.descriptor &&
-	    lrbp->ucd_rsp_ptr->qr.opcode == UPIU_QUERY_OPCODE_READ_DESC) {
+	    (lrbp->ucd_rsp_ptr->qr.opcode == UPIU_QUERY_OPCODE_READ_DESC ||
+	     lrbp->ucd_rsp_ptr->qr.opcode == UPIU_QUERY_OPCODE_AGGREGATED_READ)) {
 		u8 *descp = (u8 *)lrbp->ucd_rsp_ptr +
 				GENERAL_UPIU_REQUEST_SIZE;
 		u16 resp_len;
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index afbb32654fab..1e6ea87a5dc7 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -30,6 +30,7 @@ static_assert(sizeof(struct utp_upiu_query) == 20);
  * (ALIGNED_DEVMAN_RSP_SIZE) minus the fixed UPIU header it follows.
  */
 #define QUERY_AGGREGATED_MAX_SIZE (4096 - GENERAL_UPIU_REQUEST_SIZE)
+#define QUERY_AGG_GROUP_HDR_SIZE  4
 #define QUERY_DESC_MIN_SIZE       2
 #define QUERY_DESC_HDR_SIZE       2
 #define QUERY_OSF_SIZE            (GENERAL_UPIU_REQUEST_SIZE - \
@@ -569,6 +570,49 @@ enum ufs_dev_pwr_mode {
 
 #define UFS_WB_BUF_REMAIN_PERCENT(val) ((val) / 10)
 
+/* AGGREGATION TYPE field of an AGGREGATED READ query request */
+enum ufs_agg_type {
+	UFS_AGG_TYPE_ALL_FLAGS			= BIT(0),
+	UFS_AGG_TYPE_ALL_ATTRS			= BIT(1),
+	UFS_AGG_TYPE_DEVICE_DESC		= BIT(2),
+	/* Unit descriptors and the RPMB unit descriptor */
+	UFS_AGG_TYPE_UNIT_RPMB_DESC		= BIT(3),
+	UFS_AGG_TYPE_INTERCONNECT_DESC	= BIT(4),
+	UFS_AGG_TYPE_STRING_DESC		= BIT(5),
+	/* Geometry descriptor and power descriptor */
+	UFS_AGG_TYPE_GEOMETRY_POWER_DESC	= BIT(6),
+	UFS_AGG_TYPE_HEALTH_DESC		= BIT(7),
+};
+
+/* Group Type field of an aggregated data packet group header. */
+enum ufs_agg_group_type {
+	UFS_AGG_GROUP_RESERVED		= 0x00,
+	UFS_AGG_GROUP_FLAGS		= 0x01,
+	UFS_AGG_GROUP_ATTRS		= 0x02,
+	/* Any descriptor with a unique IDN, i.e. all but the string ones */
+	UFS_AGG_GROUP_DESCS		= 0x03,
+	UFS_AGG_GROUP_MANUFACTURER_STR	= 0x04,
+	UFS_AGG_GROUP_PRODUCT_NAME_STR	= 0x05,
+	UFS_AGG_GROUP_OEM_ID_STR	= 0x06,
+	UFS_AGG_GROUP_SERIAL_NUMBER_STR	= 0x07,
+	UFS_AGG_GROUP_PRODUCT_REV_STR	= 0x08,
+};
+
+/**
+ * struct utp_agg_group_header - aggregated data packet group header
+ * @group_type: type of the data carried by this group, see ufs_agg_group_type
+ * @reserved: reserved
+ * @next_group_offset: byte offset from the start of the packet to the next
+ *	group; zero ends the chain
+ */
+struct utp_agg_group_header {
+	u8 group_type;
+	u8 reserved;
+	__be16 next_group_offset;
+};
+
+static_assert(sizeof(struct utp_agg_group_header) == QUERY_AGG_GROUP_HDR_SIZE);
+
 /**
  * struct utp_cmd_rsp - RESPONSE UPIU structure
  * @residual_transfer_count: Residual transfer count DW-3
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index dfd302f2dc7c..1a5e181ff9e3 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -213,9 +213,11 @@ struct ufs_query_req {
 /**
  * struct ufs_query_res - UPIU QUERY
  * @upiu_res: query response data
+ * @data_segment_length: response data segment length
  */
 struct ufs_query_res {
 	struct utp_upiu_query upiu_res;
+	u16 data_segment_length;
 };
 
 /**
-- 
2.25.1

[PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[Hyeoncheol Jeong]

Device init reads several attributes and flags one query at a time. On
UFS 5.0, issue one AGGREGATED READ once the device descriptor has
established wSpecVersion, cache the reply, and serve the following
attribute, flag and ID-string reads from it. Anything not present, and
older devices or a rejected opcode, fall back to individual queries.

The following per-item queries are folded into the one aggregated read:

Attributes (ALL_ATTRS group)
	0x0c bMaxNumOfRTT                  - ufshcd_set_rtt()
	0x17 bRefClkGatingWaitTime         - ufshcd_get_ref_clk_gating_wait()
	0x1e bWriteBoosterBufferLifeTimeEst - ufshcd_wb_probe()

Flags (ALL_FLAGS group)
	0x03 fPowerOnWPEn                  - ufshcd_device_params_init()

Strings (STRING_DESC groups)
	Product Name string                - ufs_get_device_desc()
	Serial Number string               - ufshcd_create_device_id()

(bWriteBoosterBufferLifeTimeEst is served from the packet only in the
shared-buffer mode)

Signed-off-by: Hyeoncheol Jeong <hyenc.jeong@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 357 ++++++++++++++++++++++++++++++++++++--
 include/ufs/ufs.h         |   3 +
 include/ufs/ufshcd.h      |   9 +
 3 files changed, 351 insertions(+), 18 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d07834fc4646..e4135d4a5812 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3792,6 +3792,314 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
 	return err;
 }
 
+/* Attribute data width by IDN for aggregated read (JESD220H Table 14.28). */
+static const u8 ufs_agg_attr_width[] = {
+	[QUERY_ATTR_IDN_BOOT_LU_EN] = 1, [QUERY_ATTR_IDN_POWER_MODE] = 1,
+	[QUERY_ATTR_IDN_ACTIVE_ICC_LVL] = 1, [QUERY_ATTR_IDN_OOO_DATA_EN] = 1,
+	[QUERY_ATTR_IDN_BKOPS_STATUS] = 1, [QUERY_ATTR_IDN_PURGE_STATUS] = 1,
+	[QUERY_ATTR_IDN_MAX_DATA_IN] = 1, [QUERY_ATTR_IDN_MAX_DATA_OUT] = 1,
+	[QUERY_ATTR_IDN_DYN_CAP_NEEDED] = 4, [QUERY_ATTR_IDN_REF_CLK_FREQ] = 1,
+	[QUERY_ATTR_IDN_CONF_DESC_LOCK] = 1, [QUERY_ATTR_IDN_MAX_NUM_OF_RTT] = 1,
+	[QUERY_ATTR_IDN_EE_CONTROL] = 2, [QUERY_ATTR_IDN_EE_STATUS] = 2,
+	[QUERY_ATTR_IDN_SECONDS_PASSED] = 4, [QUERY_ATTR_IDN_CNTX_CONF] = 2,
+	[QUERY_ATTR_IDN_FFU_STATUS] = 1, [QUERY_ATTR_IDN_PSA_STATE] = 1,
+	[QUERY_ATTR_IDN_PSA_DATA_SIZE] = 4,
+	[QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME] = 1,
+	[QUERY_ATTR_IDN_CASE_ROUGH_TEMP] = 1, [QUERY_ATTR_IDN_HIGH_TEMP_BOUND] = 1,
+	[QUERY_ATTR_IDN_LOW_TEMP_BOUND] = 1, [0x1b] = 1,
+	[QUERY_ATTR_IDN_WB_FLUSH_STATUS] = 1,
+	[QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE] = 1,
+	[QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST] = 1,
+	[QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE] = 4,
+};
+
+/**
+ * ufshcd_agg_group - find a group in the cached aggregated data packet
+ * @hba: per-adapter instance
+ * @type: group type to find
+ * @group_len: set to the group payload length on a hit
+ *
+ * Return: the group payload, or NULL if @type is not present.
+ */
+static const u8 *ufshcd_agg_group(const struct ufs_hba *hba, u8 type,
+				  u16 *group_len)
+{
+	const u8 *packet = hba->agg_packet;
+	u16 packet_len = hba->agg_packet_len;
+	u16 group_off = 0;
+
+	while (packet && group_off + QUERY_AGG_GROUP_HDR_SIZE <= packet_len) {
+		const struct utp_agg_group_header *hdr =
+			(const void *)(packet + group_off);
+		u16 next_group_off = be16_to_cpu(hdr->next_group_offset);
+		u16 payload_off = group_off + QUERY_AGG_GROUP_HDR_SIZE;
+
+		if (next_group_off && (next_group_off < payload_off ||
+				       next_group_off + QUERY_AGG_GROUP_HDR_SIZE > packet_len))
+			return NULL;
+
+		if (hdr->group_type == type) {
+			*group_len = (next_group_off ? next_group_off : packet_len) -
+				     payload_off;
+			return packet + payload_off;
+		}
+
+		if (!next_group_off)
+			break;
+
+		group_off = next_group_off;
+	}
+
+	return NULL;
+}
+
+/**
+ * ufshcd_agg_string - find a string descriptor in the cached aggregated packet
+ * @hba: per-adapter instance
+ * @index: string descriptor index
+ * @len: set to the descriptor length on a hit
+ *
+ * Return: the string descriptor, or NULL if not present.
+ */
+static const u8 *ufshcd_agg_string(struct ufs_hba *hba, u8 index, u8 *len)
+{
+	u16 group_len;
+	const u8 *group_buf;
+	int i;
+
+	if (!index)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(hba->agg_str_idx); i++)
+		if (hba->agg_str_idx[i] == index)
+			break;
+
+	if (i == ARRAY_SIZE(hba->agg_str_idx))
+		return NULL;
+
+	group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_MANUFACTURER_STR + i,
+				     &group_len);
+	if (!group_buf || group_len < QUERY_DESC_HDR_SIZE)
+		return NULL;
+
+	*len = group_buf[QUERY_DESC_LENGTH_OFFSET];
+	if (*len < QUERY_DESC_HDR_SIZE || *len > group_len)
+		return NULL;
+
+	return group_buf;
+}
+
+/**
+ * ufshcd_agg_desc - find a descriptor in the cached aggregated data packet
+ * @hba: per-adapter instance
+ * @idn: descriptor IDN
+ * @index: unit index, for unit descriptors; string index for string ones
+ * @len: set to the descriptor length on a hit
+ *
+ * Return: the descriptor, or NULL if not present.
+ */
+static const u8 *ufshcd_agg_desc(struct ufs_hba *hba, enum desc_idn idn,
+				 u8 index, u8 *len)
+{
+	u16 group_len, off = 0;
+	const u8 *group_buf;
+
+	if (idn == QUERY_DESC_IDN_STRING)
+		return ufshcd_agg_string(hba, index, len);
+
+	group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_DESCS, &group_len);
+	while (group_buf && off + QUERY_DESC_HDR_SIZE <= group_len) {
+		const u8 *desc_buf = group_buf + off;
+		u8 desc_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
+
+		if (desc_len < QUERY_DESC_HDR_SIZE || off + desc_len > group_len)
+			break;
+
+		if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] == idn &&
+		    (idn != QUERY_DESC_IDN_UNIT ||
+		     (desc_len > UNIT_DESC_PARAM_UNIT_INDEX &&
+		      desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == index))) {
+			*len = desc_len;
+			return desc_buf;
+		}
+
+		off += desc_len;
+	}
+
+	return NULL;
+}
+
+/**
+ * ufshcd_agg_attr - read an attribute from the cached aggregated data packet
+ * @hba: per-adapter instance
+ * @idn: attribute IDN (index 0 only)
+ * @out: set to the value on a hit
+ *
+ * Return: 0 on a hit, -ENOENT otherwise.
+ */
+static int ufshcd_agg_attr(struct ufs_hba *hba, u8 idn, u32 *out)
+{
+	u16 group_len, off = 0;
+	const u8 *group_buf;
+	int i;
+
+	if (idn >= ARRAY_SIZE(ufs_agg_attr_width) || !ufs_agg_attr_width[idn])
+		return -ENOENT;
+
+	group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_ATTRS, &group_len);
+	if (!group_buf)
+		return -ENOENT;
+
+	for (i = 0; i < idn; i++)
+		off += ufs_agg_attr_width[i];
+
+	if (off + ufs_agg_attr_width[idn] > group_len)
+		return -ENOENT;
+
+	*out = 0;
+	for (i = 0; i < ufs_agg_attr_width[idn]; i++)
+		*out = (*out << 8) | group_buf[off + i];
+
+	return 0;
+}
+
+/**
+ * ufshcd_agg_flag - read a flag from the cached aggregated data packet
+ * @hba: per-adapter instance
+ * @idn: flag IDN
+ * @out: set to the value on a hit
+ *
+ * Return: 0 on a hit, -ENOENT otherwise.
+ */
+static int ufshcd_agg_flag(struct ufs_hba *hba, u8 idn, bool *out)
+{
+	u16 group_len;
+	const u8 *group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_FLAGS,
+					       &group_len);
+
+	if (!group_buf || idn >= group_len)
+		return -ENOENT;
+
+	*out = group_buf[idn];
+	return 0;
+}
+
+/**
+ * ufshcd_read_attr - read an attribute, from the cached packet if present
+ * @hba: per-adapter instance
+ * @idn: attribute IDN
+ * @index: attribute index
+ * @out: result
+ *
+ * Return: 0 on success, < 0 on error.
+ */
+static int ufshcd_read_attr(struct ufs_hba *hba, u8 idn, u8 index, u32 *out)
+{
+	if (index == 0 && !ufshcd_agg_attr(hba, idn, out))
+		return 0;
+
+	return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, idn,
+				       index, 0, out);
+}
+
+/**
+ * ufshcd_read_flag - read a flag, from the cached packet if present
+ * @hba: per-adapter instance
+ * @idn: flag IDN
+ * @out: result
+ *
+ * Return: 0 on success, < 0 on error.
+ */
+static int ufshcd_read_flag(struct ufs_hba *hba, u8 idn, bool *out)
+{
+	if (!ufshcd_agg_flag(hba, idn, out))
+		return 0;
+
+	return ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG, idn, 0,
+				       out);
+}
+
+/**
+ * ufshcd_query_aggregated_read - issue an AGGREGATED READ query
+ * @hba: per-adapter instance
+ * @agg_type: AGGREGATION TYPE mask (OSF) selecting the items to fetch
+ * @buf: buffer for the reply data segment
+ * @buf_len: requested length in, received length out
+ *
+ * Return: 0 on success; > 0 on an OCS error; < 0 otherwise.
+ */
+static int ufshcd_query_aggregated_read(struct ufs_hba *hba, u8 agg_type,
+					u8 *buf, int *buf_len)
+{
+	struct ufs_query_req *request = NULL;
+	struct ufs_query_res *response = NULL;
+	int err;
+
+	if (*buf_len <= 0 || *buf_len > QUERY_AGGREGATED_MAX_SIZE)
+		return -EINVAL;
+
+	ufshcd_dev_man_lock(hba);
+	ufshcd_init_query(hba, &request, &response,
+			  UPIU_QUERY_OPCODE_AGGREGATED_READ, agg_type, 0, 0);
+	request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
+	request->upiu_req.length = cpu_to_be16(*buf_len);
+	hba->dev_cmd.query.descriptor = buf;
+
+	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
+	if (!err)
+		*buf_len = response->data_segment_length;
+
+	hba->dev_cmd.query.descriptor = NULL;
+	ufshcd_dev_man_unlock(hba);
+	return err;
+}
+
+/**
+ * ufshcd_agg_read_begin - cache one AGGREGATED READ for the reads that follow
+ * @hba: per-adapter instance
+ * @agg_type: AGGREGATION TYPE mask to fetch
+ *
+ * Paired with ufshcd_agg_read_end(). Requires a UFS 5.0 device.
+ */
+static void ufshcd_agg_read_begin(struct ufs_hba *hba, u8 agg_type)
+{
+	int len = QUERY_AGGREGATED_MAX_SIZE;
+	int retries;
+	u8 *packet;
+
+	if (hba->dev_info.wspecversion < 0x500 ||
+	    hba->dev_info.agg_read_unsupported)
+		return;
+
+	/* Zeroed so a device over-reporting LENGTH terminates the walk safely. */
+	packet = kzalloc(QUERY_AGGREGATED_MAX_SIZE, GFP_KERNEL);
+	if (!packet)
+		return;
+
+	for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
+		if (!ufshcd_query_aggregated_read(hba, agg_type, packet, &len))
+			break;
+	}
+
+	if (!retries) {
+		dev_dbg(hba->dev, "aggregated read unsupported, using individual queries\n");
+		hba->dev_info.agg_read_unsupported = true;
+		kfree(packet);
+		return;
+	}
+
+	hba->agg_packet = packet;
+	hba->agg_packet_len = len;
+}
+
+/* ufshcd_agg_read_end - drop the packet cached by ufshcd_agg_read_begin() */
+static void ufshcd_agg_read_end(struct ufs_hba *hba)
+{
+	kfree(hba->agg_packet);
+	hba->agg_packet = NULL;
+	hba->agg_packet_len = 0;
+}
+
 /**
  * ufshcd_read_desc_param - read the specified descriptor parameter
  * @hba: Pointer to adapter instance
@@ -3813,6 +4121,8 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 {
 	int ret;
 	u8 *desc_buf;
+	const u8 *descp;
+	u8 desc_len;
 	int buff_len = QUERY_DESC_MAX_SIZE;
 	bool is_kmalloc = true;
 
@@ -3830,14 +4140,20 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		is_kmalloc = false;
 	}
 
-	/* Request for full descriptor */
-	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
-					    desc_id, desc_index, 0,
-					    desc_buf, &buff_len);
-	if (ret) {
-		dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d\n",
-			__func__, desc_id, desc_index, param_offset, ret);
-		goto out;
+	/* Serve from the cached aggregated packet, else request the full one. */
+	descp = ufshcd_agg_desc(hba, desc_id, desc_index, &desc_len);
+	if (descp) {
+		memcpy(desc_buf, descp, desc_len);
+		ret = 0;
+	} else {
+		ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
+						    desc_id, desc_index, 0,
+						    desc_buf, &buff_len);
+		if (ret) {
+			dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d\n",
+				__func__, desc_id, desc_index, param_offset, ret);
+			goto out;
+		}
 	}
 
 	/* Update descriptor length */
@@ -4019,9 +4335,8 @@ static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
 	u32 gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
 
 	if (hba->dev_info.wspecversion >= 0x300) {
-		err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
-				QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME, 0, 0,
-				&gating_wait);
+		err = ufshcd_read_attr(hba, QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME,
+				       0, &gating_wait);
 		if (err)
 			dev_err(hba->dev, "Failed reading bRefClkGatingWait. err = %d, use default %uus\n",
 					 err, gating_wait);
@@ -6531,9 +6846,8 @@ static bool ufshcd_is_wb_buf_lifetime_available(struct ufs_hba *hba)
 	u8 index;
 
 	index = ufshcd_wb_get_query_index(hba);
-	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
-				      QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST,
-				      index, 0, &lifetime);
+	ret = ufshcd_read_attr(hba, QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST,
+			       index, &lifetime);
 	if (ret) {
 		dev_err(hba->dev,
 			"%s: bWriteBoosterBufferLifeTimeEst read failed %d\n",
@@ -8677,8 +8991,7 @@ static void ufshcd_set_rtt(struct ufs_hba *hba)
 	if (dev_info->wspecversion < 0x400)
 		return;
 
-	if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
-				    QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &dev_rtt)) {
+	if (ufshcd_read_attr(hba, QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, &dev_rtt)) {
 		dev_err(hba->dev, "failed reading bMaxNumOfRTT\n");
 		return;
 	}
@@ -8899,6 +9212,14 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
 	dev_info->bqueuedepth = desc_buf[DEVICE_DESC_PARAM_Q_DPTH];
 
+	hba->agg_str_idx[0] = desc_buf[DEVICE_DESC_PARAM_MANF_NAME];
+	hba->agg_str_idx[1] = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
+	hba->agg_str_idx[2] = desc_buf[DEVICE_DESC_PARAM_OEM_ID];
+	hba->agg_str_idx[3] = desc_buf[DEVICE_DESC_PARAM_SN];
+	hba->agg_str_idx[4] = desc_buf[DEVICE_DESC_PARAM_PRDCT_REV];
+	ufshcd_agg_read_begin(hba, UFS_AGG_TYPE_ALL_ATTRS | UFS_AGG_TYPE_ALL_FLAGS |
+			      UFS_AGG_TYPE_STRING_DESC);
+
 	/*
 	 * According to the UFS standard, the UFS device queue depth
 	 * (bQueueDepth) must be in the range 1..255 if the shared queueing
@@ -9214,8 +9535,7 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
 
 	ufshcd_get_ref_clk_gating_wait(hba);
 
-	if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
-			QUERY_FLAG_IDN_PWR_ON_WPE, 0, &flag))
+	if (!ufshcd_read_flag(hba, QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
 		hba->dev_info.f_power_on_wp_en = flag;
 
 	/* Probe maximum power mode co-supported by both UFS host and device */
@@ -9226,6 +9546,7 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
 
 	ufshcd_retrieve_tx_eq_settings(hba);
 out:
+	ufshcd_agg_read_end(hba);
 	return ret;
 }
 
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 1e6ea87a5dc7..e29552d6acc2 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -707,6 +707,9 @@ struct ufs_dev_info {
 
 	bool hid_sup;
 
+	/* Set once the device has rejected an AGGREGATED READ query. */
+	bool agg_read_unsupported;
+
 	/* Unique device ID string (manufacturer+model+serial+version+date) */
 	char *device_id;
 	u8 rpmb_io_size;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 1a5e181ff9e3..53a7573890cd 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1098,6 +1098,10 @@ enum ufshcd_mcq_opr {
  * @device_deemphasis_cap: a bitfield to indicate supported DeEmphasis dBs of device's TX lanes,
  *	cache of device M-PHY TX_HS_DeEmphasis_Setting_Capability Attribute (ID 0x12)
  * @tx_eq_params: TX Equalization settings
+ * @agg_packet: aggregated data packet cached during a read batch, or NULL
+ * @agg_packet_len: length of @agg_packet
+ * @agg_str_idx: string descriptor index per string group, indexed by
+ *	group type - UFS_AGG_GROUP_MANUFACTURER_STR
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
@@ -1282,6 +1286,11 @@ struct ufs_hba {
 	u8 device_preshoot_cap;
 	u8 device_deemphasis_cap;
 	struct ufshcd_tx_eq_params tx_eq_params[UFS_HS_GEAR_MAX];
+
+	u8 *agg_packet;
+	u16 agg_packet_len;
+	u8 agg_str_idx[UFS_AGG_GROUP_PRODUCT_REV_STR -
+		       UFS_AGG_GROUP_MANUFACTURER_STR + 1];
 };
 
 /**
-- 
2.25.1

Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ufs: unaligned memory access in ufshcd_agg_group() crashes on bad device input
--

commit 8bc3a3d567bfb8009dd9c20e3fdb96ed162e1ae5
Author: Hyeoncheol Jeong <hyenc.jeong@samsung.com>

scsi: ufs: Serve the device init reads from one aggregated read

This commit introduces an aggregated read mechanism for UFS 5.0 devices to
fetch attributes, flags, and ID-strings in a single query during device
initialization.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index c18b4784a3595..028f0179ee8be 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> +static const u8 *ufshcd_agg_group(const struct ufs_hba *hba, u8 type,
> +				  u16 *group_len)
> +{
> +	const u8 *packet = hba->agg_packet;
> +	u16 packet_len = hba->agg_packet_len;
> +	u16 group_off = 0;
> +
> +	while (packet && group_off + QUERY_AGG_GROUP_HDR_SIZE <= packet_len) {
> +		const struct utp_agg_group_header *hdr =
> +			(const void *)(packet + group_off);
> +		u16 next_group_off = be16_to_cpu(hdr->next_group_offset);

[Severity: High]
Does this code safely handle unaligned memory accesses?

If a malformed or untrusted UFS device returns an odd next_group_offset, the
loop adds this odd offset to the base pointer, and the next iteration will
attempt to read the 16-bit hdr->next_group_offset from an unaligned address.

This will cause a kernel panic on architectures without unaligned access
support. Should get_unaligned_be16() be used here, or should the
utp_agg_group_header struct be marked as __packed?

> +		u16 payload_off = group_off + QUERY_AGG_GROUP_HDR_SIZE;
> +
> +		if (next_group_off && (next_group_off < payload_off ||
> +				       next_group_off + QUERY_AGG_GROUP_HDR_SIZE > packet_len))
> +			return NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1860250540.01790752381858.JavaMail.epsvc@epcpadp2new?part=2

Re: [PATCH v2 1/2] scsi: ufs: Add the aggregated read query opcode and its reply format

[Peter Wang]

On Wed, 2026-09-30 at 14:15 +0900, Hyeoncheol Jeong wrote:
> UFS 5.0 adds the AGGREGATED READ query opcode (0x9): a single QUERY
> RESPONSE UPIU returns many descriptors, attributes and flags as a
> chain
> of typed groups (JESD220H 10.7.9.14).
> 
> Add the AGGREGATION TYPE mask, the group types and the group header,
> and
> let ufshcd_copy_query_response() copy the reply's data segment like a
> descriptor read, recording its length in struct ufs_query_res.
> 
> Signed-off-by: Hyeoncheol Jeong <hyenc.jeong@samsung.com>

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[Peter Wang]

On Wed, 2026-09-30 at 14:18 +0900, Hyeoncheol Jeong wrote:
> Device init reads several attributes and flags one query at a time.
> On
> UFS 5.0, issue one AGGREGATED READ once the device descriptor has
> established wSpecVersion, cache the reply, and serve the following
> attribute, flag and ID-string reads from it. Anything not present,
> and
> older devices or a rejected opcode, fall back to individual queries.
> 
> The following per-item queries are folded into the one aggregated
> read:
> 
> Attributes (ALL_ATTRS group)
> 	0x0c bMaxNumOfRTT                  - ufshcd_set_rtt()
> 	0x17 bRefClkGatingWaitTime         -
> ufshcd_get_ref_clk_gating_wait()
> 	0x1e bWriteBoosterBufferLifeTimeEst - ufshcd_wb_probe()
> 
> Flags (ALL_FLAGS group)
> 	0x03 fPowerOnWPEn                  -
> ufshcd_device_params_init()
> 
> Strings (STRING_DESC groups)
> 	Product Name string                - ufs_get_device_desc()
> 	Serial Number string               -
> ufshcd_create_device_id()
> 
> (bWriteBoosterBufferLifeTimeEst is served from the packet only in the
> shared-buffer mode)
> 
> Signed-off-by: Hyeoncheol Jeong <hyenc.jeong@samsung.com>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[Stanley Jhu]

On Wed, 30 Sep 2026 14:18:25 +0900, Hyeoncheol Jeong wrote:
> +	for (i = 0; i < idn; i++)
> +		off += ufs_agg_attr_width[i];

This assumes the Attributes group (Group Type 02h) is serialized by
ascending IDN at fixed offsets. JESD220H Section 10.7.9.14 and
Table 10.56 define only the group header and Next Group Offset chain;
the intra-group layout and ordering are not specified.

How does this handle:

- Array attributes: wContextConf (10h) requires INDEX=LUN and
  SELECTOR=ContextID, where valid SELECTOR values are 01h..0Fh so 00h
  is invalid (Table 14.28), yet AGGREGATED READ leaves bytes 14-17
  Reserved (Table 10.42). Assuming 2 bytes in ufs_agg_attr_width[]
  breaks if a device omits it or returns all elements.

- Optional attributes: if a device omits unimplemented optional
  attributes such as PSA (15h, 16h) instead of zero-padding them,
  subsequent attributes shift (e.g. bWriteBoosterBufferLifeTimeEst at
  1Eh shifts by 5 bytes) with no per-entry framing to detect it.

- Write-only attributes: ufs_agg_attr_width[] reserves 4 bytes for
  dSecondsPassed (0Fh), which Table 14.28 marks Write only. Whether a
  device serializes write-only attributes in a read response is
  unspecified.

> +	if (index == 0 && !ufshcd_agg_attr(hba, idn, out))
> +		return 0;

The commit message states that bWriteBoosterBufferLifeTimeEst is
served from the packet only in shared-buffer mode. In LU-dedicated
mode (bWriteBoosterBufferType != 01h, Table 14.28 NOTE 15) with
wb_dedicated_lu == 0, ufshcd_wb_get_query_index() also returns 0, so
this check serves LU 0's lifetime from a packet with no LUN qualifier.

> +	if (!group_buf || idn >= group_len)
> +		return -ENOENT;
> +
> +	*out = group_buf[idn];

The Flags group (Group Type 01h) has the same issue: Table 14.26
NOTE 1 distinguishes device-level flags from array flags addressed by
INDEX and SELECTOR, and the specification defines no 1-byte-per-IDN
packed layout.

Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[Hyeoncheol Jeong]

Hi Stanley,

Thank you for taking the time to review this so carefully.

On Thu, 1 Oct 2026 13:53:07 +0800, Stanley Jhu wrote:
> On Wed, 30 Sep 2026 14:18:25 +0900, Hyeoncheol Jeong wrote:
> > +    for (i = 0; i < idn; i++)
> > +        off += ufs_agg_attr_width[i];
>
> This assumes the Attributes group (Group Type 02h) is serialized by
> ascending IDN at fixed offsets. JESD220H Section 10.7.9.14 and
> Table 10.56 define only the group header and Next Group Offset chain;
> the intra-group layout and ordering are not specified.

You are right. The layout of entries within the Attributes and Flags
groups is not defined in JESD220H.

> > +    if (index == 0 && !ufshcd_agg_attr(hba, idn, out))
> > +        return 0;
>
> The commit message states that bWriteBoosterBufferLifeTimeEst is
> served from the packet only in shared-buffer mode. In LU-dedicated
> mode (bWriteBoosterBufferType != 01h, Table 14.28 NOTE 15) with
> wb_dedicated_lu == 0, ufshcd_wb_get_query_index() also returns 0, so
> this check serves LU 0's lifetime from a packet with no LUN qualifier.

Thank you for catching this. Checking index == 0 does not limit this
to the shared-buffer mode. This code will be dropped with patch 2.

> > +    if (!group_buf || idn >= group_len)
> > +        return -ENOENT;
> > +
> > +    *out = group_buf[idn];
>
> The Flags group (Group Type 01h) has the same issue: Table 14.26
> NOTE 1 distinguishes device-level flags from array flags addressed by
> INDEX and SELECTOR, and the specification defines no 1-byte-per-IDN
> packed layout.

I agree that the same concern applies to the Flags group.

I think using these groups needs to wait until the JEDEC specification
clarifies the format. In the next version, I will keep only the
interface introduced by patch 1 and drop the users in patch 2.
The parts the specification leaves undefined will follow once
it is clarified.

Thanks,
Hyeoncheol

Re: [PATCH v2 0/2] scsi: ufs: batch device-init reads with UFS 5.0 aggregated read

[Bean Huo]

On Wed, 2026-09-30 at 14:02 +0900, Hyeoncheol Jeong wrote:
> No functional change for pre-5.0 devices.
> 
> Motivation: on a Qualcomm SM8850 platform with a Samsung UFS 5.0 device,
> timing raw UPIU queries over BSG (100 iterations) shows that replacing the
> device-init read burst with a single aggregated read reduces the query
> time by 75-78% (min 75.3%, avg 76.4%, max 78.5%).


Hi Jeong,

I don't know if you have plan to enable this feature in UEFI or any bootloaders
to eliminate the boot overhead becaused of query iterations.

Kind regards, 
Bean

Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[Bean Huo]

On Wed, 2026-09-30 at 14:18 +0900, Hyeoncheol Jeong wrote:
> Device init reads several attributes and flags one query at a time. On
> UFS 5.0, issue one AGGREGATED READ once the device descriptor has
> established wSpecVersion, cache the reply, and serve the following
> attribute, flag and ID-string reads from it. Anything not present, and
> older devices or a rejected opcode, fall back to individual queries.
> 
> The following per-item queries are folded into the one aggregated read:
> 
> Attributes (ALL_ATTRS group)
>         0x0c bMaxNumOfRTT                  - ufshcd_set_rtt()
>         0x17 bRefClkGatingWaitTime         - ufshcd_get_ref_clk_gating_wait()
>         0x1e bWriteBoosterBufferLifeTimeEst - ufshcd_wb_probe()
> 
> Flags (ALL_FLAGS group)
>         0x03 fPowerOnWPEn                  - ufshcd_device_params_init()
> 
> Strings (STRING_DESC groups)
>         Product Name string                - ufs_get_device_desc()
>         Serial Number string               - ufshcd_create_device_id()
> 
> (bWriteBoosterBufferLifeTimeEst is served from the packet only in the
> shared-buffer mode)


I suggest we should aggregate-read as many of them as possible. If we lose even
one descriptor, this feature becomes useless.

> 
> Signed-off-by: Hyeoncheol Jeong <hyenc.jeong@samsung.com>
> ---
>  drivers/ufs/core/ufshcd.c | 357 ++++++++++++++++++++++++++++++++++++--
>  include/ufs/ufs.h         |   3 +
>  include/ufs/ufshcd.h      |   9 +
>  3 files changed, 351 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d07834fc4646..e4135d4a5812 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3792,6 +3792,314 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
>         return err;
>  }
>  
> +/* Attribute data width by IDN for aggregated read (JESD220H Table 14.28). */
> +static const u8 ufs_agg_attr_width[] = {
> +       [QUERY_ATTR_IDN_BOOT_LU_EN] = 1, [QUERY_ATTR_IDN_POWER_MODE] = 1,
> +       [QUERY_ATTR_IDN_ACTIVE_ICC_LVL] = 1, [QUERY_ATTR_IDN_OOO_DATA_EN] = 1,
> +       [QUERY_ATTR_IDN_BKOPS_STATUS] = 1, [QUERY_ATTR_IDN_PURGE_STATUS] = 1,
> +       [QUERY_ATTR_IDN_MAX_DATA_IN] = 1, [QUERY_ATTR_IDN_MAX_DATA_OUT] = 1,
> +       [QUERY_ATTR_IDN_DYN_CAP_NEEDED] = 4, [QUERY_ATTR_IDN_REF_CLK_FREQ] =
> 1,
> +       [QUERY_ATTR_IDN_CONF_DESC_LOCK] = 1, [QUERY_ATTR_IDN_MAX_NUM_OF_RTT] =
> 1,
> +       [QUERY_ATTR_IDN_EE_CONTROL] = 2, [QUERY_ATTR_IDN_EE_STATUS] = 2,
> +       [QUERY_ATTR_IDN_SECONDS_PASSED] = 4, [QUERY_ATTR_IDN_CNTX_CONF] = 2,
> +       [QUERY_ATTR_IDN_FFU_STATUS] = 1, [QUERY_ATTR_IDN_PSA_STATE] = 1,
> +       [QUERY_ATTR_IDN_PSA_DATA_SIZE] = 4,
> +       [QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME] = 1,
> +       [QUERY_ATTR_IDN_CASE_ROUGH_TEMP] = 1, [QUERY_ATTR_IDN_HIGH_TEMP_BOUND]
> = 1,
> +       [QUERY_ATTR_IDN_LOW_TEMP_BOUND] = 1, [0x1b] = 1,
> +       [QUERY_ATTR_IDN_WB_FLUSH_STATUS] = 1,
> +       [QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE] = 1,
> +       [QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST] = 1,
> +       [QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE] = 4,
> +};
> +


table 14.28 gives the size of each attribute, but I could not find where Spec
defines the layout of the attributes group in the aggregated data packet,
section 10.7.9.14 only defines the group header.

dDynCapNeeded (09h) is an array attribute; its number of indexes is MaxNumberLU.
1Ch-1Fh can also be per-LU in dedicated WB mode. If a device sends one entry per
index, or keeps a slot for 01h or 11h-13h, every offset after it moves. Then
bMaxNumOfRTT, bRefClkGatingWaitTime and bWriteBoosterBufferLifeTimeEst are read
from the wrong bytes, with no error and no fallback to a single query.
ufshcd_set_rtt() then writes a value based on that wrong data.

Has this been tested on more than one vendor's UFS 5.0 device?

> +/**
> + * ufshcd_agg_group - find a group in the cached aggregated data packet
> + * @hba: per-adapter instance
> + * @type: group type to find
> + * @group_len: set to the group payload length on a hit
> + *
> + * Return: the group payload, or NULL if @type is not present.
> + */
> +static const u8 *ufshcd_agg_group(const struct ufs_hba *hba, u8 type,
> +                                 u16 *group_len)
> +{
> +       const u8 *packet = hba->agg_packet;
> +       u16 packet_len = hba->agg_packet_len;
> +       u16 group_off = 0;
> +
> +       while (packet && group_off + QUERY_AGG_GROUP_HDR_SIZE <= packet_len) {
> +               const struct utp_agg_group_header *hdr =
> +                       (const void *)(packet + group_off);
> +               u16 next_group_off = be16_to_cpu(hdr->next_group_offset);
> +               u16 payload_off = group_off + QUERY_AGG_GROUP_HDR_SIZE;
> +
> +               if (next_group_off && (next_group_off < payload_off ||
> +                                      next_group_off +
> QUERY_AGG_GROUP_HDR_SIZE > packet_len))
> +                       return NULL;
> +
> +               if (hdr->group_type == type) {

if the reply is cut short (the device may return less than requested), a group
that is complete but its next offset points past the end is rejected before its
type is checked.


> +                       *group_len = (next_group_off ? next_group_off :
> packet_len) -
> +                                    payload_off;
> +                       return packet + payload_off;
> +               }
> +
> +               if (!next_group_off)
> +                       break;
> +
> +               group_off = next_group_off;
> +       }
> +
> +       return NULL;
> +}
> +
> +/**
> + * ufshcd_agg_string - find a string descriptor in the cached aggregated
> packet
> + * @hba: per-adapter instance
> + * @index: string descriptor index
> + * @len: set to the descriptor length on a hit
> + *
> + * Return: the string descriptor, or NULL if not present.
> + */
> +static const u8 *ufshcd_agg_string(struct ufs_hba *hba, u8 index, u8 *len)
> +{
> +       u16 group_len;
> +       const u8 *group_buf;
> +       int i;
> +
> +       if (!index)
> +               return NULL;
> +
> +       for (i = 0; i < ARRAY_SIZE(hba->agg_str_idx); i++)
> +               if (hba->agg_str_idx[i] == index)
> +                       break;
> +
> +       if (i == ARRAY_SIZE(hba->agg_str_idx))
> +               return NULL;
> +
> +       group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_MANUFACTURER_STR + i,
> +                                    &group_len);
> +       if (!group_buf || group_len < QUERY_DESC_HDR_SIZE)
> +               return NULL;
> +
> +       *len = group_buf[QUERY_DESC_LENGTH_OFFSET];
> +       if (*len < QUERY_DESC_HDR_SIZE || *len > group_len)
> +               return NULL;
> +
> +       return group_buf;
> +}
> +
> +/**
> + * ufshcd_agg_desc - find a descriptor in the cached aggregated data packet
> + * @hba: per-adapter instance
> + * @idn: descriptor IDN
> + * @index: unit index, for unit descriptors; string index for string ones
> + * @len: set to the descriptor length on a hit
> + *
> + * Return: the descriptor, or NULL if not present.
> + */
> +static const u8 *ufshcd_agg_desc(struct ufs_hba *hba, enum desc_idn idn,
> +                                u8 index, u8 *len)
> +{
> +       u16 group_len, off = 0;
> +       const u8 *group_buf;
> +
> +       if (idn == QUERY_DESC_IDN_STRING)
> +               return ufshcd_agg_string(hba, index, len);
> +
> +       group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_DESCS, &group_len);
> +       while (group_buf && off + QUERY_DESC_HDR_SIZE <= group_len) {
> +               const u8 *desc_buf = group_buf + off;
> +               u8 desc_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
> +
> +               if (desc_len < QUERY_DESC_HDR_SIZE || off + desc_len >
> group_len)
> +                       break;
> +
> +               if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] == idn &&
> +                   (idn != QUERY_DESC_IDN_UNIT ||
> +                    (desc_len > UNIT_DESC_PARAM_UNIT_INDEX &&
> +                     desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == index))) {
> +                       *len = desc_len;
> +                       return desc_buf;
> +               }
> +
> +               off += desc_len;
> +       }
> +
> +       return NULL;
> +}
> +
> +/**
> + * ufshcd_agg_attr - read an attribute from the cached aggregated data packet
> + * @hba: per-adapter instance
> + * @idn: attribute IDN (index 0 only)
> + * @out: set to the value on a hit
> + *
> + * Return: 0 on a hit, -ENOENT otherwise.
> + */
> +static int ufshcd_agg_attr(struct ufs_hba *hba, u8 idn, u32 *out)
> +{
> +       u16 group_len, off = 0;
> +       const u8 *group_buf;
> +       int i;
> +
> +       if (idn >= ARRAY_SIZE(ufs_agg_attr_width) || !ufs_agg_attr_width[idn])
> +               return -ENOENT;
> +
> +       group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_ATTRS, &group_len);
> +       if (!group_buf)
> +               return -ENOENT;
> +
> +       for (i = 0; i < idn; i++)
> +               off += ufs_agg_attr_width[i];
> +
> +       if (off + ufs_agg_attr_width[idn] > group_len)
> +               return -ENOENT;
> +
> +       *out = 0;
> +       for (i = 0; i < ufs_agg_attr_width[idn]; i++)
> +               *out = (*out << 8) | group_buf[off + i];
> +
> +       return 0;
> +}
> +
> +/**
> + * ufshcd_agg_flag - read a flag from the cached aggregated data packet
> + * @hba: per-adapter instance
> + * @idn: flag IDN
> + * @out: set to the value on a hit
> + *
> + * Return: 0 on a hit, -ENOENT otherwise.
> + */
> +static int ufshcd_agg_flag(struct ufs_hba *hba, u8 idn, bool *out)
> +{
> +       u16 group_len;
> +       const u8 *group_buf = ufshcd_agg_group(hba, UFS_AGG_GROUP_FLAGS,
> +                                              &group_len);
> +
> +       if (!group_buf || idn >= group_len)
> +               return -ENOENT;
> +
> +       *out = group_buf[idn];
> +       return 0;
> +}
> +
> +/**
> + * ufshcd_read_attr - read an attribute, from the cached packet if present
> + * @hba: per-adapter instance
> + * @idn: attribute IDN
> + * @index: attribute index
> + * @out: result
> + *
> + * Return: 0 on success, < 0 on error.
> + */
> +static int ufshcd_read_attr(struct ufs_hba *hba, u8 idn, u8 index, u32 *out)
> +{
> +       if (index == 0 && !ufshcd_agg_attr(hba, idn, out))
> +               return 0;
> +
> +       return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, idn,
> +                                      index, 0, out);
> +}
> +
> +/**
> + * ufshcd_read_flag - read a flag, from the cached packet if present
> + * @hba: per-adapter instance
> + * @idn: flag IDN
> + * @out: result
> + *
> + * Return: 0 on success, < 0 on error.
> + */
> +static int ufshcd_read_flag(struct ufs_hba *hba, u8 idn, bool *out)
> +{
> +       if (!ufshcd_agg_flag(hba, idn, out))
> +               return 0;
> +
> +       return ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG, idn,
> 0,
> +                                      out);
> +}
> +
> +/**
> + * ufshcd_query_aggregated_read - issue an AGGREGATED READ query
> + * @hba: per-adapter instance
> + * @agg_type: AGGREGATION TYPE mask (OSF) selecting the items to fetch
> + * @buf: buffer for the reply data segment
> + * @buf_len: requested length in, received length out
> + *
> + * Return: 0 on success; > 0 on an OCS error; < 0 otherwise.
> + */
> +static int ufshcd_query_aggregated_read(struct ufs_hba *hba, u8 agg_type,
> +                                       u8 *buf, int *buf_len)
> +{
> +       struct ufs_query_req *request = NULL;
> +       struct ufs_query_res *response = NULL;
> +       int err;
> +
> +       if (*buf_len <= 0 || *buf_len > QUERY_AGGREGATED_MAX_SIZE)
> +               return -EINVAL;
> +
> +       ufshcd_dev_man_lock(hba);
> +       ufshcd_init_query(hba, &request, &response,
> +                         UPIU_QUERY_OPCODE_AGGREGATED_READ, agg_type, 0, 0);
> +       request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
> +       request->upiu_req.length = cpu_to_be16(*buf_len);
> +       hba->dev_cmd.query.descriptor = buf;
> +
> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
> +       if (!err)
> +               *buf_len = response->data_segment_length;
> +
> +       hba->dev_cmd.query.descriptor = NULL;
> +       ufshcd_dev_man_unlock(hba);
> +       return err;
> +}
> +
> +/**
> + * ufshcd_agg_read_begin - cache one AGGREGATED READ for the reads that
> follow
> + * @hba: per-adapter instance
> + * @agg_type: AGGREGATION TYPE mask to fetch
> + *
> + * Paired with ufshcd_agg_read_end(). Requires a UFS 5.0 device.
> + */
> +static void ufshcd_agg_read_begin(struct ufs_hba *hba, u8 agg_type)
> +{
> +       int len = QUERY_AGGREGATED_MAX_SIZE;
> +       int retries;
> +       u8 *packet;
> +
> +       if (hba->dev_info.wspecversion < 0x500 ||
> +           hba->dev_info.agg_read_unsupported)
> +               return;

is this feature mandatory? or you assume this feature should be supported by
defualt?

  
> +
> +       /* Zeroed so a device over-reporting LENGTH terminates the walk
> safely. */
> +       packet = kzalloc(QUERY_AGGREGATED_MAX_SIZE, GFP_KERNEL);
> +       if (!packet)
> +               return;
> +
> +       for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> +               if (!ufshcd_query_aggregated_read(hba, agg_type, packet,
> &len))
> +                       break;
> +       }
> +
If the device rejects the opcode with a query response error, is there a reason
to retry? Retrying makes sense for a transient error, but not for "invalid
opcode"


Kind regards,
Bean

Re: [PATCH v2 0/2] scsi: ufs: batch device-init reads with UFS 5.0 aggregated read

[Hyeoncheol Jeong]

Hi Bean,

On Fri, 2 Oct 2026 11:24:15 +0200, Bean Huo wrote:
> On Wed, 2026-09-30 at 14:02 +0900, Hyeoncheol Jeong wrote:
> > Motivation: on a Qualcomm SM8850 platform with a Samsung UFS 5.0 device,
> > timing raw UPIU queries over BSG (100 iterations) shows that replacing the
> > device-init read burst with a single aggregated read reduces the query
> > time by 75-78% (min 75.3%, avg 76.4%, max 78.5%).
>
> I don't know if you have plan to enable this feature in UEFI or any bootloaders
> to eliminate the boot overhead becaused of query iterations.

For now, my focus is on the kernel side. I do plan to bring the same
approach to bootloaders such as EDK II as a follow-up.

Thanks,
Hyeoncheol

Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[Hyeoncheol Jeong]

Hi Bean,

Thank you for the detailed review.

On Fri, 02 Oct 2026 17:50:19 +0200, Bean Huo wrote:
> On Wed, 2026-09-30 at 14:18 +0900, Hyeoncheol Jeong wrote:
> > (bWriteBoosterBufferLifeTimeEst is served from the packet only in the
> > shared-buffer mode)
>
> I suggest we should aggregate-read as many of them as possible. If we lose even
> one descriptor, this feature becomes useless.

You are right. As this is the first step, I took a conservative approach
and left out the reads that are issued asynchronously, such as the power
and unit descriptors. I agree that covering as many of the init reads as
possible is what makes this worthwhile.

> table 14.28 gives the size of each attribute, but I could not find where Spec
> defines the layout of the attributes group in the aggregated data packet,
> section 10.7.9.14 only defines the group header.
>
> dDynCapNeeded (09h) is an array attribute; its number of indexes is MaxNumberLU.
> 1Ch-1Fh can also be per-LU in dedicated WB mode. If a device sends one entry per
> index, or keeps a slot for 01h or 11h-13h, every offset after it moves. Then
> bMaxNumOfRTT, bRefClkGatingWaitTime and bWriteBoosterBufferLifeTimeEst are read
> from the wrong bytes, with no error and no fallback to a single query.
> ufshcd_set_rtt() then writes a value based on that wrong data.
>
> Has this been tested on more than one vendor's UFS 5.0 device?

You are right, and thank you for the examples. It has been tested
only on a Samsung UFS 5.0 device.

I think this layout needs to be specified in more detail in the
specification. Once it is, I plan to rework the patch and submit it
again.

> if the reply is cut short (the device may return less than requested), a group
> that is complete but its next offset points past the end is rejected before its
> type is checked.

I will fix this when I rework the patch.

> > +       if (hba->dev_info.wspecversion < 0x500 ||
> > +           hba->dev_info.agg_read_unsupported)
> > +               return;
>
> is this feature mandatory? or you assume this feature should be supported by
> defualt?

JESD220H lists AGGREGATED READ in Table 10.32 without marking it
optional, and I could not find a capability bit for it. The patch
therefore assumes a UFS 5.0 device supports it, but falls back to
individual queries if the device rejects it.

> If the device rejects the opcode with a query response error, is there a reason
> to retry? Retrying makes sense for a transient error, but not for "invalid
> opcode"

Agreed. A query response error should not be retried.

As I replied to Stanley, I think consuming these groups should wait
until the JEDEC specification clarifies the intra-group format. In the
next version I will keep only the interface from patch 1 and drop the
users in patch 2, and I will address the points above when they come
back.

Thanks,
Hyeoncheol