On Wed, 2014-06-11 at 22:32 +0000, Quinn Tran wrote: > > On 6/11/14 2:30 PM, "Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx> wrote: > > >On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote: > >> On 6/11/2014 12:17 AM, Quinn Tran wrote: > >> > >> <SNIP> > >> > >> > 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 > > <nod> > --- > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
