Matthew Wilcox wrote:
> On Thu, Jun 12, 2008 at 03:02:46PM +0200, Christof Schmitt wrote:
>> This was suggested a while ago, now i finally put together a patch
>> that moves the Fibre Channel protocol definitions from zfcp to a new
>> global header file. With the global header, the definitions can be
>> shared across all FC drives.
>
> I think this is a great step forward, thank you for doing it.
>
>> +struct ct_hdr {
>> + u8 revision;
>> + u8 in_id[3];
>> + u8 gs_type;
>> + u8 gs_subtype;
>> + u8 options;
>> + u8 reserved0;
>> + u16 cmd_rsp_code;
>> + u16 max_res_size;
>> + u8 reserved1;
>> + u8 reason_code;
>> + u8 reason_code_expl;
>> + u8 vendor_unique;
>> +} __attribute__ ((packed));
>
> I question the need for packed. Looking at <scsi/scsi.h>, none of the
> structures there are packed. Everything is naturally aligned and
> explicitly padded ('reserved1', etc). Also, those structs use __be16
> instead of u16 to allow sparse to check the correct endian conversion
> functions are used.
>
It's best to use the "__packed" macro which might be defined differently
for some tool-chains.
And it is best to *do* keep the __packed. At above example the biggest type
is be16 so for >=32 bit arches it's packed the same, by all gcc versions. But
if you start having bigger-then-natural types in a structure then different
size arches will pack things differently. I have been bitten by this, even
though I kept everything well defined.
Also I like the __packed as a warning to fellow programmers that this is
something on-the-wired defined.
And yes please use __be16 this is SCSI.
> (same comment applies to other structs in the file)
>
>> +struct fcp_cmnd_iu {
>> + u64 fcp_lun;
>
> Should be a struct scsi_lun?
>
>> + u8 crn;
>> +#if defined(__BIG_ENDIAN_BITFIELD)
>> + u8 reserved0:5;
>> + u8 task_attribute:3;
>> + u8 task_management_flags;
>> + u8 add_fcp_cdb_length:6;
>> + u8 rddata:1;
>> + u8 wddata:1;
>> +#elif defined(__LITTLE_ENDIAN_BITFIELD)
>> + u8 wddata:1;
>> + u8 rddata:1;
>> + u8 add_fcp_cdb_length:6;
>> + u8 task_management_flags;
>> + u8 task_attribute:3;
>> + u8 reserved0:5;
>> +#endif
>
> This isn't right; the endianness has you confused.
>
> +#elif defined(__LITTLE_ENDIAN_BITFIELD)
> + u8 task_attribute:3;
> + u8 reserved0:5;
> + u8 task_management_flags;
> + u8 wddata:1;
> + u8 rddata:1;
> + u8 add_fcp_cdb_length:6;
> +#endif
>
Yes, that GCC bug on some ARCHES, rrrr.
Matthew is right the T10 standard always define the exact
byte-order which does not change. best just define:
u8 task_attribute;
u8 task_management_flags;
u8 wd_rd_add_fcp_cdb_length;
and use things like:
enum {
task_attribute_mask = 0xc0,
task_attribute_shift = 5,
wd_flag = 0x80,
rd_flag = 0x40,
add_fcp_cdb_length_mask = 0x3F
};
>> + u8 fcp_cdb[FCP_CDB_LENGTH];
>> +} __attribute__((packed));
>
> I also wonder if we shouldn't define the fields that are in FCP-3.
>
> I also wonder whether we should define bitfields or whether we should
> let drivers mask and shift themselves. That's jejb's call, IMO.
>
My $0.017
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
