Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-11 Thread Nicholas A. Bellinger
On Wed, 2014-06-11 at 22:32 +, Quinn Tran wrote:
> 
> On 6/11/14 2:30 PM, "Nicholas A. Bellinger"  wrote:
> 
> >On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote:
> >> On 6/11/2014 12:17 AM, Quinn Tran wrote:
> >>
> >> 
> >>
> >> > QT> Instead of using existing value within cmd->data_length, can we
> >> > calculated data_length based on secstors & blocksize.
> >> >
> >> > cmd->data_length = sectors * dev->dev_attrib.block_size;
> >>
> >> We can do that I suppose...
> >> Although it seems weird that the core discards the transport
> >>byte-count...
> >> Just wandering if we should recalc on the DIF case only or always.
> >>
> >
> >Yeah, same concern here wrt to discarding the transport length.
> >
> >This effectively makes the residual handling further down in
> >sbc_parse_cdb() -> target_cmd_size_check() always match size ==
> >cmd->data_length, because the latter as been recalculated by the former.
> >
> >Honestly, I don't know if this is a problem for normal READ + WRITE with
> >prot=1 as I've never seen size != cmd->data_length for I/O related
> >commands, but it does seem potentially dangerous.
> >
> >> >
> >> >  From the QLogic perfective, the cmd->data_length is pull directly
> >>from the
> >> > wire (i.e. FCP header) when cmd is received.  In essence, it's
> >>whatever
> >> > the Initiator is set it to.  We does not have any indicator to spot
> >>DIF vs
> >> > none-DIF cmd when 1st received, unless the code do a peek.
> >>
> >> Same for all transports I assume...
> >>
> >
> >So just to confirm Quinn, the Qlogic the initiator includes the PI bytes
> >into the FCP header data_length for the target-side *_PASS and
> >DOUT_STRIP / DIN_INSERT, that is currently passed into
> >qla_tgt_ops->handle_cmd(), right..?
> 
> QT>
> Initiator:
> DOUT_STRIP/DIN_Insert:  FCP_DL = data length only (no dif length)
> Dif PASS: FCP_DL = data length + Dif length.
> 
> Target:
> DOUT_strip/DIN_Insert:  qla_tgt_ops->handle_cmd(data length only)
> 
> sbc_check_prot()
> If (protect)
>cmd->data_length -= cmd->prot_length;  << truncation
> 
> 



> ---
> DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length)
> 
> 

Ok, so Sagi's patches for legacy fabric <-> PI backend and
PI fabric <-> PI backend cases look OK, but not for the
PI fabric <-> legacy backend case as noted above..

Given DOUT_STRIP + DIN_INSERT have not been enabled in sbc_check_prot()
just yet, I'll go ahead and merge his v2 series to address the two
existing cases in -rc1 + v3.15.y stable code.

>From there, enabling PI fabric <-> legacy backend support in >= rc2 with
target-core will be easy, as it's just a matter of updating
sbc_set_prot_op_checks() to assign prot_ops, avoid sbc_check_prot()
cmd->data_length recalculation, and making sure PROTECTION control
feature bits are still exposed for legacy -> unprotected backends.

Thanks!

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-11 Thread Martin K. Petersen
> "nab" == Nicholas A Bellinger  writes:

nab> Hard to say.  Discarding the transport length in v2 doesn't seem
nab> like a good idea, but subtracting from cmd->prot_length in v1 is
nab> using the sector count from the CDB anyways, so it's essentially
nab> the same tradeoff of recalculating the transport's cmd->data_length
nab> from values within the CDB w/ prot=1.

nab> MKP, WDYT..?

My general feeling is that once sbc on the target sees {rd,wr}protect >
0 we're in the territory where you should start separating data and
PI internally.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-11 Thread Quinn Tran


On 6/11/14 2:30 PM, "Nicholas A. Bellinger"  wrote:

>On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote:
>> On 6/11/2014 12:17 AM, Quinn Tran wrote:
>>
>> 
>>
>> > QT> Instead of using existing value within cmd->data_length, can we
>> > calculated data_length based on secstors & blocksize.
>> >
>> > cmd->data_length = sectors * dev->dev_attrib.block_size;
>>
>> We can do that I suppose...
>> Although it seems weird that the core discards the transport
>>byte-count...
>> Just wandering if we should recalc on the DIF case only or always.
>>
>
>Yeah, same concern here wrt to discarding the transport length.
>
>This effectively makes the residual handling further down in
>sbc_parse_cdb() -> target_cmd_size_check() always match size ==
>cmd->data_length, because the latter as been recalculated by the former.
>
>Honestly, I don't know if this is a problem for normal READ + WRITE with
>prot=1 as I've never seen size != cmd->data_length for I/O related
>commands, but it does seem potentially dangerous.
>
>> >
>> >  From the QLogic perfective, the cmd->data_length is pull directly
>>from the
>> > wire (i.e. FCP header) when cmd is received.  In essence, it's
>>whatever
>> > the Initiator is set it to.  We does not have any indicator to spot
>>DIF vs
>> > none-DIF cmd when 1st received, unless the code do a peek.
>>
>> Same for all transports I assume...
>>
>
>So just to confirm Quinn, the Qlogic the initiator includes the PI bytes
>into the FCP header data_length for the target-side *_PASS and
>DOUT_STRIP / DIN_INSERT, that is currently passed into
>qla_tgt_ops->handle_cmd(), right..?

QT>
Initiator:
DOUT_STRIP/DIN_Insert:  FCP_DL = data length only (no dif length)
Dif PASS: FCP_DL = data length + Dif length.

Target:
DOUT_strip/DIN_Insert:  qla_tgt_ops->handle_cmd(data length only)

sbc_check_prot()
If (protect)
   cmd->data_length -= cmd->prot_length;  << truncation


---
DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length)


>
>If that is the case, Sagi's v1 to cmd->data_length -= cmd->prot_length
>seems like it would still do right thing for *_PASS and DOUT_STRIP /
>DIN_INSERT, given that cmd->prot_length is calculated in
>sbc_check_prot() based upon dev->prot_length * sectors..
>
>> >
>> > With that said, the cmd->data_length does not guarantee to contain
>>both
>> > "data length" & "protection length" when cmd is submit to
>> > TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
>> > contain the actual data(no Dif).
>>
>> No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the
>>core
>> will take the data length as is.
>> This case is covered.
>>
>
>

QT> agree.

>
>> > It's best that the SBC code re-calculate the actual data length and
>>dif
>> > data length based on the number of sectors derived from the cmd.
>>
>> Nic, what's your take on this?
>>
>
>Hard to say.  Discarding the transport length in v2 doesn't seem like a
>good idea, but subtracting from cmd->prot_length in v1 is using the
>sector count from the CDB anyways, so it's essentially the same tradeoff
>of recalculating the transport's cmd->data_length from values within the
>CDB w/ prot=1.
>
>MKP, WDYT..?
>
>--nab
>
>
>
>




This message and any attached documents contain information from QLogic 
Corporation or its wholly-owned subsidiaries that may be confidential. If you 
are not the intended recipient, you may not read, copy, distribute, or use this 
information. If you have received this transmission in error, please notify the 
sender immediately by reply e-mail and then delete this message.
<>

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-11 Thread Nicholas A. Bellinger
On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote:
> On 6/11/2014 12:17 AM, Quinn Tran wrote:
> 
> 
> 
> > QT> Instead of using existing value within cmd->data_length, can we
> > calculated data_length based on secstors & blocksize.
> >
> > cmd->data_length = sectors * dev->dev_attrib.block_size;
> 
> We can do that I suppose...
> Although it seems weird that the core discards the transport byte-count...
> Just wandering if we should recalc on the DIF case only or always.
> 

Yeah, same concern here wrt to discarding the transport length.

This effectively makes the residual handling further down in
sbc_parse_cdb() -> target_cmd_size_check() always match size ==
cmd->data_length, because the latter as been recalculated by the former.

Honestly, I don't know if this is a problem for normal READ + WRITE with
prot=1 as I've never seen size != cmd->data_length for I/O related
commands, but it does seem potentially dangerous.

> >
> >  From the QLogic perfective, the cmd->data_length is pull directly from the
> > wire (i.e. FCP header) when cmd is received.  In essence, it's whatever
> > the Initiator is set it to.  We does not have any indicator to spot DIF vs
> > none-DIF cmd when 1st received, unless the code do a peek.
> 
> Same for all transports I assume...
> 

So just to confirm Quinn, the Qlogic the initiator includes the PI bytes
into the FCP header data_length for the target-side *_PASS and
DOUT_STRIP / DIN_INSERT, that is currently passed into
qla_tgt_ops->handle_cmd(), right..?

If that is the case, Sagi's v1 to cmd->data_length -= cmd->prot_length
seems like it would still do right thing for *_PASS and DOUT_STRIP /
DIN_INSERT, given that cmd->prot_length is calculated in
sbc_check_prot() based upon dev->prot_length * sectors..

> >
> > With that said, the cmd->data_length does not guarantee to contain both
> > "data length" & "protection length" when cmd is submit to
> > TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
> > contain the actual data(no Dif).
> 
> No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core 
> will take the data length as is.
> This case is covered.
> 



> > It's best that the SBC code re-calculate the actual data length and dif
> > data length based on the number of sectors derived from the cmd.
> 
> Nic, what's your take on this?
> 

Hard to say.  Discarding the transport length in v2 doesn't seem like a
good idea, but subtracting from cmd->prot_length in v1 is using the
sector count from the CDB anyways, so it's essentially the same tradeoff
of recalculating the transport's cmd->data_length from values within the
CDB w/ prot=1.

MKP, WDYT..?

--nab




--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-11 Thread Sagi Grimberg

On 6/11/2014 12:17 AM, Quinn Tran wrote:




QT> Instead of using existing value within cmd->data_length, can we
calculated data_length based on secstors & blocksize.

cmd->data_length = sectors * dev->dev_attrib.block_size;


We can do that I suppose...
Although it seems weird that the core discards the transport byte-count...
Just wandering if we should recalc on the DIF case only or always.



 From the QLogic perfective, the cmd->data_length is pull directly from the
wire (i.e. FCP header) when cmd is received.  In essence, it's whatever
the Initiator is set it to.  We does not have any indicator to spot DIF vs
none-DIF cmd when 1st received, unless the code do a peek.


Same for all transports I assume...



With that said, the cmd->data_length does not guarantee to contain both
"data length" & "protection length" when cmd is submit to
TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
contain the actual data(no Dif).


No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core 
will take the data length as is.

This case is covered.


It's best that the SBC code re-calculate the actual data length and dif
data length based on the number of sectors derived from the cmd.


Nic, what's your take on this?

Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-10 Thread Quinn Tran
All,

Comments inline.

Regards,
Quinn Tran




On 6/10/14 1:04 AM, "Nicholas A. Bellinger"  wrote:

>Hi Sagi & Co,
>
>On Sun, 2014-06-08 at 13:27 +0300, Sagi Grimberg wrote:
>> In various areas of the code, it is assumed that
>> se_cmd->data_length describes pure data. In case
>> that protection information exists over the wire
>> (protect bits is are on) the target core decrease
>> the protection length from the data length (instead
>> of each transport peeking in the cdb).
>>
>> Modify loopback device to include protection information
>> in the transferred data length (like other scsi transports).
>>
>> Signed-off-by: Sagi Grimberg 
>> ---
>>  drivers/target/loopback/tcm_loop.c |   15 ---
>>  drivers/target/target_core_sbc.c   |   15 +--
>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/target/loopback/tcm_loop.c
>>b/drivers/target/loopback/tcm_loop.c
>> index c886ad1..1f4c015 100644
>> --- a/drivers/target/loopback/tcm_loop.c
>> +++ b/drivers/target/loopback/tcm_loop.c
>> @@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct
>>work_struct *work)
>>  struct tcm_loop_hba *tl_hba;
>>  struct tcm_loop_tpg *tl_tpg;
>>  struct scatterlist *sgl_bidi = NULL;
>> -u32 sgl_bidi_count = 0;
>> +u32 sgl_bidi_count = 0, transfer_length;
>>  int rc;
>>
>>  tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
>> @@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct
>>work_struct *work)
>>
>>  }
>>
>> -if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) !=
>>SCSI_PROT_NORMAL)
>> +transfer_length = scsi_transfer_length(sc);
>> +if (!scsi_prot_sg_count(sc) &&
>> +scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
>>  se_cmd->prot_pto = true;
>> +/*
>> + * loopback transport doesn't support
>> + * WRITE_GENERATE, READ_STRIP protection
>> + * information operations, go ahead unprotected.
>> + */
>> +transfer_length = scsi_bufflen(sc);
>> +}
>>
>>  rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
>>  &tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
>> -scsi_bufflen(sc), tcm_loop_sam_attr(sc),
>> +transfer_length, tcm_loop_sam_attr(sc),
>>  sc->sc_data_direction, 0,
>>  scsi_sglist(sc), scsi_sg_count(sc),
>>  sgl_bidi, sgl_bidi_count,
>> diff --git a/drivers/target/target_core_sbc.c
>>b/drivers/target/target_core_sbc.c
>> index e022959..06f8ecd 100644
>> --- a/drivers/target/target_core_sbc.c
>> +++ b/drivers/target/target_core_sbc.c
>> @@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct
>>se_cmd *cmd, unsigned char *cdb,
>>
>>  cmd->prot_type = dev->dev_attrib.pi_prot_type;
>>  cmd->prot_length = dev->prot_length * sectors;
>> -pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d
>>prot_checks=%d\n",
>> - __func__, cmd->prot_type, cmd->prot_length,
>> +
>> +/**
>> + * In case protection information exists over the wire
>> + * we modify command data length to describe pure data.
>> + * The actual transfer length is data length + protection
>> + * length
>> + **/
>> +if (protect)
>> +cmd->data_length -= cmd->prot_length;
>> +

QT> Instead of using existing value within cmd->data_length, can we
calculated data_length based on secstors & blocksize.

cmd->data_length = sectors * dev->dev_attrib.block_size;


>From the QLogic perfective, the cmd->data_length is pull directly from the
wire (i.e. FCP header) when cmd is received.  In essence, it's whatever
the Initiator is set it to.  We does not have any indicator to spot DIF vs
none-DIF cmd when 1st received, unless the code do a peek.

With that said, the cmd->data_length does not guarantee to contain both
"data length" & "protection length" when cmd is submit to
TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
contain the actual data(no Dif).

It's best that the SBC code re-calculate the actual data length and dif
data length based on the number of sectors derived from the cmd.

Thanks.

>
>This fabric wide assumption breaks the vhost-scsi + tcm_qla2xxx PI code
>already queued for v3.16-rc1..
>
>A vhost-scsi side conversion to combine exp_data_len + prot_bytes is
>easy enough in target-pending/for-next (see below), given that
>vhost-scsi's exp_data_len is coming from virtio buffer iovecs, so
>changing virtio-scsi LLD to use scsi_transfer_length() doesn't really
>make any sense. (Adding CC's for MST + Paolo)
>
>The qla2xxx target T10 PI patch in scsi/for-next (adding Quinn CC) will
>also need a similar change to update qlt_do_work() to include PI bytes
>for data_length being passed into tgt_ops->handle_cmd(), following what
>qlt_build_ctio_crc2_pkt() is already doing for calculating
>transfer_length for HW offload.
>
>That 

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-10 Thread Paolo Bonzini

Il 10/06/2014 10:04, Nicholas A. Bellinger ha scritto:


That said, there is one other small qla2xxx change to enable per-session
PI that is currently missing in Quinn's patch in scsi/for-next code.

Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi
change below if there are no objections from MKP and/or driver
maintainers, and post an -rc2 series after the merge window closes to
address the two remaining qla2xxx specific items.


Indeed the patch looks like a tcm bugfix.  There is no reason to modify 
the LLD.


Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-10 Thread Nicholas A. Bellinger
Hi Sagi & Co,

On Sun, 2014-06-08 at 13:27 +0300, Sagi Grimberg wrote:
> In various areas of the code, it is assumed that
> se_cmd->data_length describes pure data. In case
> that protection information exists over the wire
> (protect bits is are on) the target core decrease
> the protection length from the data length (instead
> of each transport peeking in the cdb).
> 
> Modify loopback device to include protection information
> in the transferred data length (like other scsi transports).
> 
> Signed-off-by: Sagi Grimberg 
> ---
>  drivers/target/loopback/tcm_loop.c |   15 ---
>  drivers/target/target_core_sbc.c   |   15 +--
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/loopback/tcm_loop.c 
> b/drivers/target/loopback/tcm_loop.c
> index c886ad1..1f4c015 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct work_struct 
> *work)
>   struct tcm_loop_hba *tl_hba;
>   struct tcm_loop_tpg *tl_tpg;
>   struct scatterlist *sgl_bidi = NULL;
> - u32 sgl_bidi_count = 0;
> + u32 sgl_bidi_count = 0, transfer_length;
>   int rc;
>  
>   tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
> @@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct work_struct 
> *work)
>  
>   }
>  
> - if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
> + transfer_length = scsi_transfer_length(sc);
> + if (!scsi_prot_sg_count(sc) &&
> + scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
>   se_cmd->prot_pto = true;
> + /*
> +  * loopback transport doesn't support
> +  * WRITE_GENERATE, READ_STRIP protection
> +  * information operations, go ahead unprotected.
> +  */
> + transfer_length = scsi_bufflen(sc);
> + }
>  
>   rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
>   &tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
> - scsi_bufflen(sc), tcm_loop_sam_attr(sc),
> + transfer_length, tcm_loop_sam_attr(sc),
>   sc->sc_data_direction, 0,
>   scsi_sglist(sc), scsi_sg_count(sc),
>   sgl_bidi, sgl_bidi_count,
> diff --git a/drivers/target/target_core_sbc.c 
> b/drivers/target/target_core_sbc.c
> index e022959..06f8ecd 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct se_cmd 
> *cmd, unsigned char *cdb,
>  
>   cmd->prot_type = dev->dev_attrib.pi_prot_type;
>   cmd->prot_length = dev->prot_length * sectors;
> - pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d prot_checks=%d\n",
> -  __func__, cmd->prot_type, cmd->prot_length,
> +
> + /**
> +  * In case protection information exists over the wire
> +  * we modify command data length to describe pure data.
> +  * The actual transfer length is data length + protection
> +  * length
> +  **/
> + if (protect)
> + cmd->data_length -= cmd->prot_length;
> +

This fabric wide assumption breaks the vhost-scsi + tcm_qla2xxx PI code
already queued for v3.16-rc1..

A vhost-scsi side conversion to combine exp_data_len + prot_bytes is
easy enough in target-pending/for-next (see below), given that
vhost-scsi's exp_data_len is coming from virtio buffer iovecs, so
changing virtio-scsi LLD to use scsi_transfer_length() doesn't really
make any sense. (Adding CC's for MST + Paolo)

The qla2xxx target T10 PI patch in scsi/for-next (adding Quinn CC) will
also need a similar change to update qlt_do_work() to include PI bytes
for data_length being passed into tgt_ops->handle_cmd(), following what
qlt_build_ctio_crc2_pkt() is already doing for calculating
transfer_length for HW offload.

That said, there is one other small qla2xxx change to enable per-session
PI that is currently missing in Quinn's patch in scsi/for-next code. 

Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi
change below if there are no objections from MKP and/or driver
maintainers, and post an -rc2 series after the merge window closes to
address the two remaining qla2xxx specific items.

--nab

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 667e72d..03e484f 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1144,7 +1144,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct 
vhost_virtqueue *vq)
}
 
cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr,
-exp_data_len, data_direction);
+exp_data_len + prot_bytes,
+data_direction);
if (IS_ERR(cmd)) {

[PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

2014-06-08 Thread Sagi Grimberg
In various areas of the code, it is assumed that
se_cmd->data_length describes pure data. In case
that protection information exists over the wire
(protect bits is are on) the target core decrease
the protection length from the data length (instead
of each transport peeking in the cdb).

Modify loopback device to include protection information
in the transferred data length (like other scsi transports).

Signed-off-by: Sagi Grimberg 
---
 drivers/target/loopback/tcm_loop.c |   15 ---
 drivers/target/target_core_sbc.c   |   15 +--
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c 
b/drivers/target/loopback/tcm_loop.c
index c886ad1..1f4c015 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct work_struct 
*work)
struct tcm_loop_hba *tl_hba;
struct tcm_loop_tpg *tl_tpg;
struct scatterlist *sgl_bidi = NULL;
-   u32 sgl_bidi_count = 0;
+   u32 sgl_bidi_count = 0, transfer_length;
int rc;
 
tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
@@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct work_struct 
*work)
 
}
 
-   if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
+   transfer_length = scsi_transfer_length(sc);
+   if (!scsi_prot_sg_count(sc) &&
+   scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
se_cmd->prot_pto = true;
+   /*
+* loopback transport doesn't support
+* WRITE_GENERATE, READ_STRIP protection
+* information operations, go ahead unprotected.
+*/
+   transfer_length = scsi_bufflen(sc);
+   }
 
rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
&tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
-   scsi_bufflen(sc), tcm_loop_sam_attr(sc),
+   transfer_length, tcm_loop_sam_attr(sc),
sc->sc_data_direction, 0,
scsi_sglist(sc), scsi_sg_count(sc),
sgl_bidi, sgl_bidi_count,
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index e022959..06f8ecd 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, 
unsigned char *cdb,
 
cmd->prot_type = dev->dev_attrib.pi_prot_type;
cmd->prot_length = dev->prot_length * sectors;
-   pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d prot_checks=%d\n",
-__func__, cmd->prot_type, cmd->prot_length,
+
+   /**
+* In case protection information exists over the wire
+* we modify command data length to describe pure data.
+* The actual transfer length is data length + protection
+* length
+**/
+   if (protect)
+   cmd->data_length -= cmd->prot_length;
+
+   pr_debug("%s: prot_type=%d, data_length=%d, prot_length=%d "
+"prot_op=%d prot_checks=%d\n",
+__func__, cmd->prot_type, cmd->data_length, cmd->prot_length,
 cmd->prot_op, cmd->prot_checks);
 
return true;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html