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

[Hyeoncheol Jeong]

Hi Bart,

Thanks a lot for the detailed review. I've addressed the comments and
will send a v3. Replies inline below.

On 7/23/26 2:09 AM, Bart Van Assche 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"

Done. Now the comment is:
	/* Both UCD types must have a size that is a multiple of 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(...)".

Done. v3 will be two-patch series:
  [1/2] scsi: ufs: Use unsigned types for the BSG query descriptor length
  [2/2] scsi: ufs: Add support for the aggregated read query opcode

Patch 1 switches the descriptor length/buffer to u16/u8 and uses min()
instead of min_t(int, ...).

> 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().

Agreed. ufs_bsg_get_query_desc_size() was trivial, so I folded it into
its only caller, ufs_bsg_alloc_desc_buffer(), where desc_op is already
available.

> > +    if (unlikely(blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)))) {
> [...]
> > +    } else {
> > +        int slot = i - UFSHCD_NUM_RESERVED;
> [...]
>
> 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).

Done, added WARN_ON_ONCE(slot < 0) in the non-reserved branch (the
assumption is slot >= 0, so the WARN fires when it does not hold).

> > +    int tag;
>
> Shouldn't 'tag' have an unsigned type?

Done. I made it a u8 in struct ufs_dev_cmd.

> > +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.

This is the one comment I haven't applied yet, and I'd like to
double-check with you before doing so.

The aggregated read path indeed does not use the PRDT at all. But
the reserved tag is also used by advanced RPMB. I understood that path
builds a PRDT via ufshcd_sgl_to_prdt() from the BSG payload, whose sg_cnt
can exceed one when the user buffer spans multiple segments. Keeping
SG_ALL preserves the existing RPMB behaviour; I thought shrinking to a
single segment could truncate multi-segment transfers.

Did I miss something about how RPMB maps its payload here? If a single
segment is in fact guaranteed, I'll drop the prd_table[] member and size
it as:
/* Dedicated UCD for the devman/reserved slot */
struct utp_devman_cmd_desc {
        u8 command_upiu[ALIGNED_UPIU_SIZE];
        u8 response_upiu[ALIGNED_DEVMAN_RSP_SIZE];
};

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

Separately, I fixed the Sashiko-flagged OOB read in v3. I capped
QUERY_AGGREGATED_MAX_SIZE (4096 -> 4064) so that the fixed 32-byte UPIU
header plus a full data segment fits within the 4096-byte
response_upiu[], and added a static_assert to prevent regression. 4064
is still well above the ~3 KiB the spec allows in practice.

Thanks.