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