Re: [PATCH v2] scsi: ufs: Add support for the aggregated read query opcode

[Bart Van Assche]

On 7/22/26 1:48 AM, Hyeoncheol Jeong wrote:
> -	/* sizeof(struct utp_transfer_cmd_desc) must be a multiple of 128 */
> +	/* Both UCD types must be a multiple of 128 */

"must be" -> "must have a size that is"

"128" -> "128 bytes"

> -	*desc_len = min_t(int, QUERY_DESC_MAX_SIZE, desc_size);
> +	if (desc_op == UPIU_QUERY_OPCODE_AGGREGATED_READ)
> +		*desc_len = min_t(int, QUERY_AGGREGATED_MAX_SIZE, desc_size);
> +	else
> +		*desc_len = min_t(int, QUERY_DESC_MAX_SIZE, desc_size);

Please convert this patch into a series and add a patch before this 
patch that changes the data types of desc_size and desc_len from 'int'
and 'int *' into unsigned types (u16 or unsigned int). The length field
is an unsigned 16-bits number according to the UFS standard. Hence, the
UFS driver should use an unsigned type for the UPIU query length.

This will allow to change "min_t(int, ...)" into "min(...)".

Since 'desc_op' is only used derive the maximum query size, wouldn't it
be better to move the code that limits 'desc_len' into
ufs_bsg_request()? Then 'desc_op' won't have to be passed to
ufs_bsg_get_query_desc_size().

> +	/* The reserved tag uses a dedicated UCD outside the pool. */
> +	if (unlikely(blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)))) {
> +		struct utp_devman_cmd_desc *cmd_descp = hba->devman_ucd_base_addr;
> +
> +		cmd_desc_element_addr = hba->devman_ucd_dma_addr;
> +		command_upiu = cmd_descp->command_upiu;
> +		response_upiu = cmd_descp->response_upiu;
> +		prd_table = cmd_descp->prd_table;
> +	} else {
> +		int slot = i - UFSHCD_NUM_RESERVED;
> +		struct utp_transfer_cmd_desc *cmd_descp =
> +			(void *)hba->ucdl_base_addr + slot * ufshcd_get_ucd_size(hba);
> +
> +		cmd_desc_element_addr =
> +			hba->ucdl_dma_addr + slot * ufshcd_get_ucd_size(hba);
> +		command_upiu = cmd_descp->command_upiu;
> +		response_upiu = cmd_descp->response_upiu;
> +		prd_table = cmd_descp->prd_table;
> +	}

The above code is based on the assumption that
!blk_mq_is_reserved_rq(rq) implies that i >= 1. Please add a
WARN_ON_ONCE() statement that makes this assumption explicit, e.g.
WARN_ON_ONCE(slot >= 0).

> +	int tag;

Shouldn't 'tag' have an unsigned type?

> +static inline size_t ufshcd_get_devman_ucd_size(const struct ufs_hba *hba)
> +{
> +	return sizeof(struct utp_devman_cmd_desc) + SG_ALL * ufshcd_sg_entry_size(hba);
> +}

Why SG_ALL? The data buffer for device management commands is allocated
with kmalloc() and hence is contiguous so a single segment descriptor
should be sufficient.

Otherwise this patch looks good to me.

Thanks,

Bart.