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

From: Bart Van Assche
Date: Thu Mar 27 2025 - 07:38:43 EST


On 3/26/25 7:47 PM, Bao D. Nguyen wrote:
On 3/26/2025 3:49 AM, Bart Van Assche wrote:
On 3/25/25 6:15 PM, Bao D. Nguyen wrote:
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.

Adding __attribute__((__packed__)) or __packed to data structures that
don't need it is not an improvement but is a change that makes
processing slower on architectures that do not support unaligned
accesses. Instead of adding __packed to data structures in their
entirety, only add it to those members that need it and check the
structure size as follows:

static_assert(sizeof(...) == ...);

Thank you for the info on this, Bart.
IMO, this response upiu data should be __packed because the data coming from the hardware follows a strict format as defined by the spec. If we support __pack each individual field which data may be read by the driver (the attribute read commands) and check the validity of their sizes, it may add some complexity?

Hi Bao,

As explained in my previous email, adding __packed to data structures in
their entirety is a bad practice. Please don't do this.

Regarding your question: I have not yet seen any data structure that
represents an on-the-wire data format where every single data member
has to be annotated with __packed. Only data members that are not
aligned to a natural boundary need this annotation. Examples are
available in this header file: include/scsi/srp.h.

Thanks,

Bart.