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

[Bart Van Assche <bvanassche@acm.org>]

On 7/23/26 12:47 AM, Hyeoncheol Jeong wrote:
> Thanks a lot for the detailed review. I've addressed the comments and
> will send a v3. Replies inline below.

Please post v3 as a new email thread instead of a reply to this thread.
Replies are overlooked more easily than a new thread.

> On 7/23/26 2:09 AM, Bart Van Assche wrote:

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

BSG uses struct sg_io_v4 and that data structure supports I/O vectors.
How about changing SG_ALL into 2, which should be sufficient for 4096
bytes spread over two discontiguous physical pages? I think it's fine
not to support the I/O vectors that can be generated by user space code
like this program:
https://github.com/linux-blktests/blktests/blob/master/src/discontiguous-io.cpp

Thanks,

Bart.