Re: [PATCH v2 2/2] scsi: ufs: Serve the device init reads from one aggregated read
From: Bean Huo
Date: Fri Oct 02 2026 - 11:56:37 EST
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@xxxxxxxxxxx>
> ---
> 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