On Jun 17 Ankit Jain wrote:
> On 06/17/2011 04:27 AM, Andy Grover wrote:
> > --- a/drivers/scsi/bnx2i/bnx2i_hwi.c
> > +++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
> > @@ -430,7 +430,7 @@ int bnx2i_send_iscsi_tmf(struct bnx2i_conn *bnx2i_conn,
> > default:
> > tmfabort_wqe->ref_itt = RESERVED_ITT;
> > }
> > - memcpy(scsi_lun, tmfabort_hdr->lun, sizeof(struct scsi_lun));
> > + memcpy(scsi_lun, &tmfabort_hdr->lun, sizeof(struct scsi_lun));
> > tmfabort_wqe->lun[0] = be32_to_cpu(scsi_lun[0]);
> > tmfabort_wqe->lun[1] = be32_to_cpu(scsi_lun[1]);
> >
> > @@ -547,7 +547,7 @@ int bnx2i_send_iscsi_nopout(struct bnx2i_conn *bnx2i_conn,
> >
> > nopout_wqe->op_code = nopout_hdr->opcode;
> > nopout_wqe->op_attr = ISCSI_FLAG_CMD_FINAL;
> > - memcpy(nopout_wqe->lun, nopout_hdr->lun, 8);
> > + memcpy(nopout_wqe->lun, &nopout_hdr->lun, 8);
>
> Should you be using "sizeof (..)" here (and similar instances), rather
> than 8? It is being done that way in other instances and it would be
> better practice, IMHO.
sizeof or not sizeof is the least of the coding style issues in drivers/scsi/bnx2i/.
Exhibit one from 57xx_iscsi_hsi.h:
struct bnx2i_nop_out_request {
#if defined(__BIG_ENDIAN)
[...]
#elif defined(__LITTLE_ENDIAN)
[...]
#endif
This kind of coding leads to obfuscated CPU accessors to DMA data/ on the wire data.
If a structure for on-the-wire contains a bitfield (e.g. a 32 bits wide bitfield),
just use __be... or __le... data types (e.g. __be32 or __le32) as required by the
device or protocol. When the CPU needs to read and write certain bits and bytes in
the word, use the usual cpu_to_... and ..._to_cpu accessors on the entire bitfield,
plus CPU-side shifts and masks.
The end result should be obvious to the reader of the code, and intrinsically clean
in a CF="-D__CHECK_ENDIAN__" run of make.
Did nobody ask about that when this code was staged for merge into 2.6.31?
Hard to tell at a first glance whether the introduction of struct scsi_lun into the
mix makes this code better or worse.
--
Stefan Richter
-=====-==-== -==- =---=
http://arcgraph.de/sr/
--
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
