* [PATCH] iscsi: Use struct scsi_lun in iscsi structs instead of u8[8]
@ 2011-06-16 22:57 Andy Grover
2011-06-17 5:57 ` Ankit Jain
0 siblings, 1 reply; 4+ messages in thread
From: Andy Grover @ 2011-06-16 22:57 UTC (permalink / raw)
To: linux-scsi
struct scsi_lun is also just a struct with an array of 8 octets (64 bits)
but using it instead in iscsi structs lets us call scsilun_to_int
without a cast, and also lets us copy it using assignment, instead of
memcpy().
Signed-off-by: Andy Grover <agrover@redhat.com>
---
drivers/scsi/be2iscsi/be_main.c | 5 ++---
drivers/scsi/bnx2i/bnx2i_hwi.c | 8 ++++----
drivers/scsi/libiscsi.c | 14 +++++++-------
include/scsi/iscsi_proto.h | 18 +++++++++---------
include/scsi/libiscsi.h | 2 +-
5 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 94b9a07..96d7d07 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -3963,11 +3963,10 @@ static int beiscsi_iotask(struct iscsi_task *task, struct scatterlist *sg,
}
memcpy(&io_task->cmd_bhs->iscsi_data_pdu.
dw[offsetof(struct amap_pdu_data_out, lun) / 32],
- io_task->cmd_bhs->iscsi_hdr.lun, sizeof(struct scsi_lun));
+ &io_task->cmd_bhs->iscsi_hdr.lun, sizeof(struct scsi_lun));
AMAP_SET_BITS(struct amap_iscsi_wrb, lun, pwrb,
- cpu_to_be16((unsigned short)io_task->cmd_bhs->iscsi_hdr.
- lun[0]));
+ cpu_to_be16(*(unsigned short*)&io_task->cmd_bhs->iscsi_hdr.lun));
AMAP_SET_BITS(struct amap_iscsi_wrb, r2t_exp_dtl, pwrb, xferlen);
AMAP_SET_BITS(struct amap_iscsi_wrb, wrb_idx, pwrb,
io_task->pwrb_handle->wrb_index);
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 5c54a2d..550e6c4 100644
--- 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);
if (test_bit(BNX2I_NX2_DEV_57710, &ep->hba->cnic_dev_type)) {
u32 tmp = nopout_wqe->lun[0];
@@ -1711,7 +1711,7 @@ static int bnx2i_process_nopin_mesg(struct iscsi_session *session,
hdr->flags = ISCSI_FLAG_CMD_FINAL;
hdr->itt = task->hdr->itt;
hdr->ttt = cpu_to_be32(nop_in->ttt);
- memcpy(hdr->lun, nop_in->lun, 8);
+ memcpy(&hdr->lun, nop_in->lun, 8);
}
done:
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr, NULL, 0);
@@ -1754,7 +1754,7 @@ static void bnx2i_process_async_mesg(struct iscsi_session *session,
resp_hdr->opcode = async_cqe->op_code;
resp_hdr->flags = 0x80;
- memcpy(resp_hdr->lun, async_cqe->lun, 8);
+ memcpy(&resp_hdr->lun, async_cqe->lun, 8);
resp_hdr->exp_cmdsn = cpu_to_be32(async_cqe->exp_cmd_sn);
resp_hdr->max_cmdsn = cpu_to_be32(async_cqe->max_cmd_sn);
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 0c550d5..d7a4120 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -169,7 +169,7 @@ void iscsi_prep_data_out_pdu(struct iscsi_task *task, struct iscsi_r2t_info *r2t
hdr->datasn = cpu_to_be32(r2t->datasn);
r2t->datasn++;
hdr->opcode = ISCSI_OP_SCSI_DATA_OUT;
- memcpy(hdr->lun, task->lun, sizeof(hdr->lun));
+ hdr->lun = task->lun;
hdr->itt = task->hdr_itt;
hdr->exp_statsn = r2t->exp_statsn;
hdr->offset = cpu_to_be32(r2t->data_offset + r2t->sent);
@@ -296,7 +296,7 @@ static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
/*
* Allow PDUs for unrelated LUNs
*/
- hdr_lun = scsilun_to_int((struct scsi_lun *)tmf->lun);
+ hdr_lun = scsilun_to_int(&tmf->lun);
if (hdr_lun != task->sc->device->lun)
return 0;
/* fall through */
@@ -389,8 +389,8 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
return rc;
hdr->opcode = ISCSI_OP_SCSI_CMD;
hdr->flags = ISCSI_ATTR_SIMPLE;
- int_to_scsilun(sc->device->lun, (struct scsi_lun *)hdr->lun);
- memcpy(task->lun, hdr->lun, sizeof(task->lun));
+ int_to_scsilun(sc->device->lun, &hdr->lun);
+ task->lun = hdr->lun;
hdr->exp_statsn = cpu_to_be32(conn->exp_statsn);
cmd_len = sc->cmd_len;
if (cmd_len < ISCSI_CDB_SIZE)
@@ -968,7 +968,7 @@ static void iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
hdr.flags = ISCSI_FLAG_CMD_FINAL;
if (rhdr) {
- memcpy(hdr.lun, rhdr->lun, 8);
+ hdr.lun = rhdr->lun;
hdr.ttt = rhdr->ttt;
hdr.itt = RESERVED_ITT;
} else
@@ -2092,7 +2092,7 @@ static void iscsi_prep_abort_task_pdu(struct iscsi_task *task,
hdr->opcode = ISCSI_OP_SCSI_TMFUNC | ISCSI_OP_IMMEDIATE;
hdr->flags = ISCSI_TM_FUNC_ABORT_TASK & ISCSI_FLAG_TM_FUNC_MASK;
hdr->flags |= ISCSI_FLAG_CMD_FINAL;
- memcpy(hdr->lun, task->lun, sizeof(hdr->lun));
+ hdr->lun = task->lun;
hdr->rtt = task->hdr_itt;
hdr->refcmdsn = task->cmdsn;
}
@@ -2233,7 +2233,7 @@ static void iscsi_prep_lun_reset_pdu(struct scsi_cmnd *sc, struct iscsi_tm *hdr)
hdr->opcode = ISCSI_OP_SCSI_TMFUNC | ISCSI_OP_IMMEDIATE;
hdr->flags = ISCSI_TM_FUNC_LOGICAL_UNIT_RESET & ISCSI_FLAG_TM_FUNC_MASK;
hdr->flags |= ISCSI_FLAG_CMD_FINAL;
- int_to_scsilun(sc->device->lun, (struct scsi_lun *)hdr->lun);
+ int_to_scsilun(sc->device->lun, &hdr->lun);
hdr->rtt = RESERVED_ITT;
}
diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
index dd0a52c..ea68b3c 100644
--- a/include/scsi/iscsi_proto.h
+++ b/include/scsi/iscsi_proto.h
@@ -60,7 +60,7 @@ struct iscsi_hdr {
uint8_t rsvd2[2];
uint8_t hlength; /* AHSs total length */
uint8_t dlength[3]; /* Data length */
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt; /* Initiator Task Tag, opaque for target */
__be32 ttt; /* Target Task Tag */
__be32 statsn;
@@ -122,7 +122,7 @@ struct iscsi_cmd {
__be16 rsvd2;
uint8_t hlength;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt; /* Initiator Task Tag */
__be32 data_length;
__be32 cmdsn;
@@ -198,7 +198,7 @@ struct iscsi_async {
uint8_t rsvd2[2];
uint8_t rsvd3;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
uint8_t rsvd4[8];
__be32 statsn;
__be32 exp_cmdsn;
@@ -226,7 +226,7 @@ struct iscsi_nopout {
__be16 rsvd2;
uint8_t rsvd3;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt; /* Initiator Task Tag */
__be32 ttt; /* Target Transfer Tag */
__be32 cmdsn;
@@ -241,7 +241,7 @@ struct iscsi_nopin {
__be16 rsvd2;
uint8_t rsvd3;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt; /* Initiator Task Tag */
__be32 ttt; /* Target Transfer Tag */
__be32 statsn;
@@ -257,7 +257,7 @@ struct iscsi_tm {
uint8_t rsvd1[2];
uint8_t hlength;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt; /* Initiator Task Tag */
itt_t rtt; /* Reference Task Tag */
__be32 cmdsn;
@@ -315,7 +315,7 @@ struct iscsi_r2t_rsp {
uint8_t rsvd2[2];
uint8_t hlength;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt; /* Initiator Task Tag */
__be32 ttt; /* Target Transfer Tag */
__be32 statsn;
@@ -333,7 +333,7 @@ struct iscsi_data {
uint8_t rsvd2[2];
uint8_t rsvd3;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt;
__be32 ttt;
__be32 rsvd4;
@@ -353,7 +353,7 @@ struct iscsi_data_rsp {
uint8_t cmd_status;
uint8_t hlength;
uint8_t dlength[3];
- uint8_t lun[8];
+ struct scsi_lun lun;
itt_t itt;
__be32 ttt;
__be32 statsn;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 0f43677..cedcff3 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -115,7 +115,7 @@ struct iscsi_task {
/* copied values in case we need to send tmfs */
itt_t hdr_itt;
__be32 cmdsn;
- uint8_t lun[8];
+ struct scsi_lun lun;
int itt; /* this ITT */
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iscsi: Use struct scsi_lun in iscsi structs instead of u8[8]
2011-06-16 22:57 [PATCH] iscsi: Use struct scsi_lun in iscsi structs instead of u8[8] Andy Grover
@ 2011-06-17 5:57 ` Ankit Jain
2011-06-17 12:38 ` Stefan Richter
0 siblings, 1 reply; 4+ messages in thread
From: Ankit Jain @ 2011-06-17 5:57 UTC (permalink / raw)
To: Andy Grover; +Cc: linux-scsi
On 06/17/2011 04:27 AM, Andy Grover wrote:
> struct scsi_lun is also just a struct with an array of 8 octets (64 bits)
> but using it instead in iscsi structs lets us call scsilun_to_int
> without a cast, and also lets us copy it using assignment, instead of
> memcpy().
>
> Signed-off-by: Andy Grover <agrover@redhat.com>
> ---
> drivers/scsi/be2iscsi/be_main.c | 5 ++---
> drivers/scsi/bnx2i/bnx2i_hwi.c | 8 ++++----
> drivers/scsi/libiscsi.c | 14 +++++++-------
> include/scsi/iscsi_proto.h | 18 +++++++++---------
> include/scsi/libiscsi.h | 2 +-
> 5 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 94b9a07..96d7d07 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -3963,11 +3963,10 @@ static int beiscsi_iotask(struct iscsi_task *task, struct scatterlist *sg,
> }
> memcpy(&io_task->cmd_bhs->iscsi_data_pdu.
> dw[offsetof(struct amap_pdu_data_out, lun) / 32],
> - io_task->cmd_bhs->iscsi_hdr.lun, sizeof(struct scsi_lun));
> + &io_task->cmd_bhs->iscsi_hdr.lun, sizeof(struct scsi_lun));
>
> AMAP_SET_BITS(struct amap_iscsi_wrb, lun, pwrb,
> - cpu_to_be16((unsigned short)io_task->cmd_bhs->iscsi_hdr.
> - lun[0]));
> + cpu_to_be16(*(unsigned short*)&io_task->cmd_bhs->iscsi_hdr.lun));
> AMAP_SET_BITS(struct amap_iscsi_wrb, r2t_exp_dtl, pwrb, xferlen);
> AMAP_SET_BITS(struct amap_iscsi_wrb, wrb_idx, pwrb,
> io_task->pwrb_handle->wrb_index);
> diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
> index 5c54a2d..550e6c4 100644
> --- 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.
--
Ankit Jain
SUSE Labs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iscsi: Use struct scsi_lun in iscsi structs instead of u8[8]
2011-06-17 5:57 ` Ankit Jain
@ 2011-06-17 12:38 ` Stefan Richter
2011-06-17 16:31 ` Andy Grover
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Richter @ 2011-06-17 12:38 UTC (permalink / raw)
To: Ankit Jain; +Cc: Andy Grover, linux-scsi, Eddie Wai
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/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iscsi: Use struct scsi_lun in iscsi structs instead of u8[8]
2011-06-17 12:38 ` Stefan Richter
@ 2011-06-17 16:31 ` Andy Grover
0 siblings, 0 replies; 4+ messages in thread
From: Andy Grover @ 2011-06-17 16:31 UTC (permalink / raw)
To: Stefan Richter; +Cc: Ankit Jain, linux-scsi, Eddie Wai
On 06/17/2011 05:38 AM, Stefan Richter wrote:
> 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:
<snip>
I hear ya. The focus of this patch was really the change to
iscsi_proto.h. All the other bits are compile fixes from that, so I
tried not to get distracted by other obvious driver issues.
Regards -- Andy
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-06-17 16:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-16 22:57 [PATCH] iscsi: Use struct scsi_lun in iscsi structs instead of u8[8] Andy Grover
2011-06-17 5:57 ` Ankit Jain
2011-06-17 12:38 ` Stefan Richter
2011-06-17 16:31 ` Andy Grover
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox