On Thu, Jun 12, 2008 at 07:50:23PM +0300, Boaz Harrosh wrote:
> 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.
To use the above example, i this would be changed to:
struct ct_hdr {
u8 revision;
u8 in_id[3];
u8 gs_type;
u8 gs_subtype;
u8 options;
u8 reserved0;
__be16 cmd_rsp_code;
__be16 max_res_size;
u8 reserved1;
u8 reason_code;
u8 reason_code_expl;
u8 vendor_unique;
} __packed;
> > (same comment applies to other structs in the file)
> >
> >> +struct fcp_cmnd_iu {
> >> + u64 fcp_lun;
> >
> > Should be a struct scsi_lun?
It is used in the same way, i will change it to struct scsi_lun.
> > 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
This would be correct, i think:
wd_flag = 0x01,
rd_flag = 0x02,
add_fcp_cdb_len_mask = 0xFC,
#define add_fcp_cdb_len_shift 2
> };
>
> >> + 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.
> >
As i wrote before, i am currently focussing on the definitions used
for zfcp. If others require the FCP-3 fields, they can add them.
For the simple fields, the drivers can set them directly, e.g.
fcp_cmnd_iu.wd_rd_add_fcp_cdb_length |= rd_flag;
For things like the cdb_len, i would like to have a helper like
static inline set_add_fcp_cdb_len(struct fcp_cmnd_iu *fcp_cmnd_iu, u8 len)
{
fcp_cmnd_iu->wd_rd_add_fcp_cdb_length |= (len << add_fcp_cdb_len_shift);
}
I will wait with the patch rework until we have completed some pending
zfcp cleanup patches, since changing the structs will conflict with
the other patches.
> My $0.017
> Boaz
Thanks for the feedback.
Christof
--
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
