Re: [PATCH v5 09/17] scsi_proto: Add structures and constants related to I/O groups and streams

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

On 11/30/23 05:19, Johannes Thumshirn wrote:
> On 30.11.23 02:34, Bart Van Assche wrote:
>> +/* SBC-5 IO advice hints group descriptor */
>> +struct scsi_io_group_descriptor {
>> +#if defined(__BIG_ENDIAN)
>> +	u8 io_advice_hints_mode: 2;
>> +	u8 reserved1: 3;
>> +	u8 st_enble: 1;
>> +	u8 cs_enble: 1;
>> +	u8 ic_enable: 1;
>> +#elif defined(__LITTLE_ENDIAN)
>> +	u8 ic_enable: 1;
>> +	u8 cs_enble: 1;
>> +	u8 st_enble: 1;
>> +	u8 reserved1: 3;
>> +	u8 io_advice_hints_mode: 2;
>> +#else
>> +#error
>> +#endif
>> +	u8 reserved2[3];
>> +	/* Logical block markup descriptor */
>> +#if defined(__BIG_ENDIAN)
>> +	u8 acdlu: 1;
>> +	u8 reserved3: 1;
>> +	u8 rlbsr: 2;
>> +	u8 lbm_descriptor_type: 4;
>> +#elif defined(__LITTLE_ENDIAN)
>> +	u8 lbm_descriptor_type: 4;
>> +	u8 rlbsr: 2;
>> +	u8 reserved3: 1;
>> +	u8 acdlu: 1;
>> +#else
>> +#error
>> +#endif
>> +	u8 params[2];
>> +	u8 reserved4;
>> +	u8 reserved5[8];
>> +};
>> +
>> +static_assert(sizeof(struct scsi_io_group_descriptor) == 16);
> 
> Have you considered using GENMASK() and FILED_GET() for this? All the
> ifdefs make the header rather ugly.
Hi Johannes,

When using bitfields, there are #ifdefs in the data structure
definitions but it is easy to write code that accesses the bitfields.
When using FIELD_GET(), the data structure definitions have no ifdefs
but the code that reads bitfields becomes harder to verify. The code
for setting bitfield values would have to use FIELD_PREP() and hence
would become more complex. An example of code that sets bitfields is
the definition of the gr_m_pg data structure in patch 15/17 of this
series. It would become more complicated if FIELD_PREP() would have to
be used in the definition of that data structure.

Thanks,

Bart.