Re: [PATCH v4 1/1] scsi: ufs: core: add device level exception support

[Bao D. Nguyen]

On 3/26/2025 12:30 AM, Arthur Simchaev wrote:
> > Hi Arthur,
> > This is not a flags attribute. This is for a Query Read 64-bit Attribute data. In
> > the existing code, we do not have a read 64-bit attribute, so adding this new
> > code would also allow future re-use.
> >
> > The new "struct utp_upiu_query_response_v4_0" would improve readability
> > because it is formatted exactly as how the jedec standard defines for Attribute
> > Read. We won't need to use type cast to get the 64-bit value.
> > There would be no issue with efficiency because the same machine code
> > would be generated.
> >
> > The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It does
> > not use the  __attribute__((__packed__)) attribute. The compiler is free to add
> > padding in this structure, resulting in the read attribute value being incorrect. I
> > plan to provide a separate patch to fix this issue.
>
> Hi Bao
>
> Upiu_query can be used for all device management command (descriptions, attributes, flags)
> See section 10.7.9 UPIU QUERY RESPONSE in the UFS 4.1 specification.
> If "struct utp_upiu_query" was properly defined, according to the UFS specification  (by OSF's),
> we would not need to add additional "struct utp_upiu_query_v4_0" structures.
Section 10.7.9 of the device spec v4.0 and v4.1 define the QUERY 
RESPONSE mostly as OSFs (Opcode Specific Field). If the driver used OSFs 
as defined by section 10.7.9, it would not be very readable. Instead, 
the driver tries its best to map these OSFs to meaningful names 
depending on the type of the QUERY command. In my opinion, there is a 
good reason to introduce "struct utp_upiu_query_v4_0". For example, in 
the spec v4.0, Write Attribute only supports up to 32-bit attribute 
size. However, v4.1 supports up to  64-bit attribute size. The "struct 
utp_upiu_query_v4_0" renamed the "__be16 reserved_osf" to "__u8 osf3" 
and "__u8 osf4" which would be better for code readability.

> If you think the structure should be packaged, you can fix "struct utp_upiu_query" and
> "struct utp_upiu_query_v4_0".
That's a fair request. I would like to fix it in a separate patch if 
that works.

Thanks, Bao