Re: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

Re: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Eric Moore]

On Wednesday, April 15, 2009 8:55 PM, Martin K. Petersen wrote:
> For Type 1 I expect you to check guard + match ref tag to CDB LBA +
> offset.
>
> For Type 2 I expect you to check guard + match ref tag to E.I.LBA in
> 32-byte CDB + offset.
>
> For Type 3 I expect you to check guard only.


Does this patch look good?

(1) I modified _scsih_setup_eedp so for TYPE1, its sets ref and gaurd checking,
and for TYPE2 it sets gauard checking.

(2) I removed the calling of scsi_host_set_guard.


I'll repost the entire patch set if this is good to go.


Eric Moore


diff -uaprN a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h	2009-04-13 15:45:09.000000000 -0600
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h	2009-04-13 16:31:27.000000000 -0600
@@ -61,6 +61,7 @@
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsi_transport_sas.h>
 #include <scsi/scsi_dbg.h>
+#include <scsi/scsi_eh.h>
 
 #include "mpt2sas_debug.h"
 
diff -uaprN a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c	2009-04-13 15:58:57.000000000 -0600
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c	2009-04-16 09:55:42.000000000 -0600
@@ -2390,6 +2390,106 @@ mpt2sas_scsih_reset_handler(struct MPT2S
 }
 
 /**
+ * _scsih_setup_eedp - setup MPI request for EEDP transfer
+ * @scmd: pointer to scsi command object
+ * @mpi_request: pointer to the SCSI_IO reqest message frame
+ *
+ * Supporting protection 1 and 3.
+ *
+ * Returns nothing
+ */
+static void
+_scsih_setup_eedp(struct scsi_cmnd *scmd, Mpi2SCSIIORequest_t *mpi_request)
+{
+	u16 eedp_flags;
+	unsigned char prot_op = scsi_get_prot_op(scmd);
+	unsigned char prot_type = scsi_get_prot_type(scmd);
+
+	if (prot_type == SCSI_PROT_DIF_TYPE0 ||
+	   prot_type == SCSI_PROT_DIF_TYPE2 ||
+	   prot_op == SCSI_PROT_NORMAL)
+		return;
+
+	if (prot_op ==  SCSI_PROT_READ_STRIP)
+		eedp_flags = MPI2_SCSIIO_EEDPFLAGS_CHECK_REMOVE_OP;
+	else if (prot_op ==  SCSI_PROT_WRITE_INSERT)
+		eedp_flags = MPI2_SCSIIO_EEDPFLAGS_INSERT_OP;
+	else
+		return;
+
+	switch (prot_type) {
+	case SCSI_PROT_DIF_TYPE1:
+
+		/*
+		* enable ref/guard checking
+		* auto increment ref tag
+		*/
+		mpi_request->EEDPFlags = eedp_flags |
+		    MPI2_SCSIIO_EEDPFLAGS_INC_PRI_REFTAG |
+		    MPI2_SCSIIO_EEDPFLAGS_CHECK_REFTAG |
+		    MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
+		mpi_request->EEDPBlockSize = scmd->device->sector_size;
+		mpi_request->CDB.EEDP32.PrimaryReferenceTag =
+		    cpu_to_be32(scsi_get_lba(scmd));
+
+		break;
+
+	case SCSI_PROT_DIF_TYPE3:
+
+		/*
+		* enable guard checking
+		*/
+		mpi_request->EEDPFlags = eedp_flags |
+		    MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
+		mpi_request->EEDPBlockSize = scmd->device->sector_size;
+
+		break;
+	}
+}
+
+/**
+ * _scsih_eedp_error_handling - return sense code for EEDP errors
+ * @scmd: pointer to scsi command object
+ * @ioc_status: ioc status
+ *
+ * Returns nothing
+ */
+static void
+_scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
+{
+	u8 ascq;
+	u8 sk;
+	u8 host_byte;
+
+	switch (ioc_status) {
+	case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
+		ascq = 0x01;
+		break;
+	case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
+		ascq = 0x02;
+		break;
+	case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
+		ascq = 0x03;
+		break;
+	default:
+		ascq = 0x00;
+		break;
+	}
+
+	if (scmd->sc_data_direction == DMA_TO_DEVICE) {
+		sk = ILLEGAL_REQUEST;
+		host_byte = DID_ABORT;
+	} else {
+		sk = ABORTED_COMMAND;
+		host_byte = DID_OK;
+	}
+
+	scsi_build_sense_buffer(0, scmd->sense_buffer, sk, 0x10, ascq);
+	scmd->result = DRIVER_SENSE << 24 | (host_byte << 16) |
+	    SAM_STAT_CHECK_CONDITION;
+}
+
+/**
  * scsih_qcmd - main scsi request entry point
  * @scmd: pointer to scsi command object
  * @done: function pointer to be invoked on completion
@@ -2470,6 +2570,7 @@ scsih_qcmd(struct scsi_cmnd *scmd, void
 	}
 	mpi_request = mpt2sas_base_get_msg_frame(ioc, smid);
 	memset(mpi_request, 0, sizeof(Mpi2SCSIIORequest_t));
+	_scsih_setup_eedp(scmd, mpi_request);
 	mpi_request->Function = MPI2_FUNCTION_SCSI_IO_REQUEST;
 	if (sas_device_priv_data->sas_target->flags &
 	    MPT_TARGET_FLAGS_RAID_COMPONENT)
@@ -2604,6 +2705,15 @@ _scsih_scsi_ioc_info(struct MPT2SAS_ADAP
 	case MPI2_IOCSTATUS_SCSI_EXT_TERMINATED:
 		desc_ioc_state = "scsi ext terminated";
 		break;
+	case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
+		desc_ioc_state = "eedp guard error";
+		break;
+	case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
+		desc_ioc_state = "eedp ref tag error";
+		break;
+	case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
+		desc_ioc_state = "eedp app tag error";
+		break;
 	default:
 		desc_ioc_state = "unknown";
 		break;
@@ -2939,6 +3049,11 @@ scsih_io_done(struct MPT2SAS_ADAPTER *io
 			scmd->result = DID_RESET << 16;
 		break;
 
+	case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
+	case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
+	case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
+		_scsih_eedp_error_handling(scmd, ioc_status);
+		break;
 	case MPI2_IOCSTATUS_SCSI_PROTOCOL_ERROR:
 	case MPI2_IOCSTATUS_INVALID_FUNCTION:
 	case MPI2_IOCSTATUS_INVALID_SGL:
@@ -5503,6 +5618,9 @@ scsih_probe(struct pci_dev *pdev, const
 		goto out_add_shost_fail;
 	}
 
+	scsi_host_set_prot(shost, SHOST_DIF_TYPE1_PROTECTION
+	    | SHOST_DIF_TYPE3_PROTECTION);
+
 	/* event thread */
 	snprintf(ioc->firmware_event_name, sizeof(ioc->firmware_event_name),
 	    "fw_event%d", ioc->id);

Re: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Martin K. Petersen]

>>>>> "Eric" == Eric Moore <eric.moore@lsi.com> writes:

Eric,

Eric> (1) I modified _scsih_setup_eedp so for TYPE1, its sets ref and
Eric>     gaurd checking, and for TYPE2 it sets gauard checking.

You mean Type 3 and not Type 2.  But yes, looks ok.

Feel free to add:

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Eric Moore]

This add support for type 1 and 3 DIF support per the Oracle API.

Signed-off-by: Eric Moore <eric.moore@lsi.com>

diff -uaprN a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h	2009-04-13 15:45:09.000000000 -0600
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h	2009-04-13 16:31:27.000000000 -0600
@@ -61,6 +61,7 @@
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsi_transport_sas.h>
 #include <scsi/scsi_dbg.h>
+#include <scsi/scsi_eh.h>
 
 #include "mpt2sas_debug.h"
 
diff -uaprN a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c	2009-04-13 15:58:57.000000000 -0600
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c	2009-04-13 16:30:29.000000000 -0600
@@ -2390,6 +2390,94 @@ mpt2sas_scsih_reset_handler(struct MPT2S
 }
 
 /**
+ * _scsih_setup_eedp - setup MPI request for EEDP transfer
+ * @scmd: pointer to scsi command object
+ * @mpi_request: pointer to the SCSI_IO reqest message frame
+ *
+ * Supporting protection 1 and 3.
+ *
+ * Returns nothing
+ */
+static void
+_scsih_setup_eedp(struct scsi_cmnd *scmd, Mpi2SCSIIORequest_t *mpi_request)
+{
+	u16 eedp_flags;
+	unsigned char prot_op = scsi_get_prot_op(scmd);
+	unsigned char prot_type = scsi_get_prot_type(scmd);
+
+	if (prot_type == SCSI_PROT_DIF_TYPE0 ||
+	   prot_type == SCSI_PROT_DIF_TYPE2 ||
+	   prot_op == SCSI_PROT_NORMAL)
+		return;
+
+	if (prot_op ==  SCSI_PROT_READ_STRIP)
+		eedp_flags = MPI2_SCSIIO_EEDPFLAGS_CHECK_REMOVE_OP;
+	else if (prot_op ==  SCSI_PROT_WRITE_INSERT)
+		eedp_flags = MPI2_SCSIIO_EEDPFLAGS_INSERT_OP;
+	else
+		return;
+
+	/*
+	 * enable ref/app/guard checking
+	 * auto increment ref tag
+	 */
+	mpi_request->EEDPFlags = eedp_flags |
+	    MPI2_SCSIIO_EEDPFLAGS_INC_PRI_REFTAG |
+	    MPI2_SCSIIO_EEDPFLAGS_CHECK_REFTAG |
+	    MPI2_SCSIIO_EEDPFLAGS_CHECK_APPTAG |
+	    MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
+
+	/* set block size */
+	mpi_request->EEDPBlockSize = scmd->device->sector_size;
+	mpi_request->CDB.EEDP32.PrimaryApplicationTagMask = 0xFFFF;
+	mpi_request->CDB.EEDP32.PrimaryApplicationTag = 0;
+	mpi_request->CDB.EEDP32.PrimaryReferenceTag =
+	    cpu_to_be32(scsi_get_lba(scmd));
+}
+
+/**
+ * _scsih_eedp_error_handling - return sense code for EEDP errors
+ * @scmd: pointer to scsi command object
+ * @ioc_status: ioc status
+ *
+ * Returns nothing
+ */
+static void
+_scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
+{
+	u8 ascq;
+	u8 sk;
+	u8 host_byte;
+
+	switch (ioc_status) {
+	case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
+		ascq = 0x01;
+		break;
+	case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
+		ascq = 0x02;
+		break;
+	case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
+		ascq = 0x03;
+		break;
+	default:
+		ascq = 0x00;
+		break;
+	}
+
+	if (scmd->sc_data_direction == DMA_TO_DEVICE) {
+		sk = ILLEGAL_REQUEST;
+		host_byte = DID_ABORT;
+	} else {
+		sk = ABORTED_COMMAND;
+		host_byte = DID_OK;
+	}
+
+	scsi_build_sense_buffer(0, scmd->sense_buffer, sk, 0x10, ascq);
+	scmd->result = DRIVER_SENSE << 24 | (host_byte << 16) |
+	    SAM_STAT_CHECK_CONDITION;
+}
+
+/**
  * scsih_qcmd - main scsi request entry point
  * @scmd: pointer to scsi command object
  * @done: function pointer to be invoked on completion
@@ -2470,6 +2558,7 @@ scsih_qcmd(struct scsi_cmnd *scmd, void
 	}
 	mpi_request = mpt2sas_base_get_msg_frame(ioc, smid);
 	memset(mpi_request, 0, sizeof(Mpi2SCSIIORequest_t));
+	_scsih_setup_eedp(scmd, mpi_request);
 	mpi_request->Function = MPI2_FUNCTION_SCSI_IO_REQUEST;
 	if (sas_device_priv_data->sas_target->flags &
 	    MPT_TARGET_FLAGS_RAID_COMPONENT)
@@ -2604,6 +2693,15 @@ _scsih_scsi_ioc_info(struct MPT2SAS_ADAP
 	case MPI2_IOCSTATUS_SCSI_EXT_TERMINATED:
 		desc_ioc_state = "scsi ext terminated";
 		break;
+	case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
+		desc_ioc_state = "eedp guard error";
+		break;
+	case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
+		desc_ioc_state = "eedp ref tag error";
+		break;
+	case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
+		desc_ioc_state = "eedp app tag error";
+		break;
 	default:
 		desc_ioc_state = "unknown";
 		break;
@@ -2939,6 +3037,11 @@ scsih_io_done(struct MPT2SAS_ADAPTER *io
 			scmd->result = DID_RESET << 16;
 		break;
 
+	case MPI2_IOCSTATUS_EEDP_GUARD_ERROR:
+	case MPI2_IOCSTATUS_EEDP_REF_TAG_ERROR:
+	case MPI2_IOCSTATUS_EEDP_APP_TAG_ERROR:
+		_scsih_eedp_error_handling(scmd, ioc_status);
+		break;
 	case MPI2_IOCSTATUS_SCSI_PROTOCOL_ERROR:
 	case MPI2_IOCSTATUS_INVALID_FUNCTION:
 	case MPI2_IOCSTATUS_INVALID_SGL:
@@ -5503,6 +5606,10 @@ scsih_probe(struct pci_dev *pdev, const
 		goto out_add_shost_fail;
 	}
 
+	scsi_host_set_prot(shost, SHOST_DIF_TYPE1_PROTECTION
+	    | SHOST_DIF_TYPE3_PROTECTION);
+	scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
+
 	/* event thread */
 	snprintf(ioc->firmware_event_name, sizeof(ioc->firmware_event_name),
 	    "fw_event%d", ioc->id);

Re: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Martin K. Petersen]

>>>>> "Eric" == Eric Moore <eric.moore@lsi.com> writes:

Eric,

+_scsih_setup_eedp(struct scsi_cmnd *scmd, Mpi2SCSIIORequest_t *mpi_request)
[...]
+	/*
+	 * enable ref/app/guard checking
+	 * auto increment ref tag
+	 */
+	mpi_request->EEDPFlags = eedp_flags |
+	    MPI2_SCSIIO_EEDPFLAGS_INC_PRI_REFTAG |
+	    MPI2_SCSIIO_EEDPFLAGS_CHECK_REFTAG |
+	    MPI2_SCSIIO_EEDPFLAGS_CHECK_APPTAG |
+	    MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;

How can you check the app tag?


+_scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
[...]
+	if (scmd->sc_data_direction == DMA_TO_DEVICE) {
+		sk = ILLEGAL_REQUEST;
+		host_byte = DID_ABORT;
+	} else {
+		sk = ABORTED_COMMAND;
+		host_byte = DID_OK;
+	}

ILLEGAL REQUEST is used when the HBA detects a mismatch between data and
protection information.  If you are not receiving PI from the OS (WRITE
INSERT) then the HBA can't detect an integrity error.  So in the
DMA_TO_DEVICE case only the target can catch the error (and it will
return ABORTED_COMMAND).

For READ, both the drive firmware and the HBA can ostensibly check that
the data buffers and PI match.

Are your EEDP error flags only set when the HBA firmware detects a
problem or also when the drive returns ABORTED COMMAND/0x10?

I.e. do we get double failure scenarios (both drive and HBA detect the
mismatch)?


+	scsi_host_set_prot(shost, SHOST_DIF_TYPE1_PROTECTION
+	    | SHOST_DIF_TYPE3_PROTECTION);

How do you handle Type 3?  The application tag and reference tags are
both defined as being opaque storage for Type 3.

If I understand correctly, your firmware only deals with the Type 2
usage model of the application tag.  And Type 1 + 2 usage of the
reference tag.


+	scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);

Technically speaking that is only required for DIX exchange.  But
setting it doesn't hurt.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Moore, Eric]

On Wednesday, April 15, 2009 1:32 PM,  Martin K. Petersen  wrote:
> 
> +_scsih_setup_eedp(struct scsi_cmnd *scmd, 
> Mpi2SCSIIORequest_t *mpi_request)
> [...]
> +	/*
> +	 * enable ref/app/guard checking
> +	 * auto increment ref tag
> +	 */
> +	mpi_request->EEDPFlags = eedp_flags |
> +	    MPI2_SCSIIO_EEDPFLAGS_INC_PRI_REFTAG |
> +	    MPI2_SCSIIO_EEDPFLAGS_CHECK_REFTAG |
> +	    MPI2_SCSIIO_EEDPFLAGS_CHECK_APPTAG |
> +	    MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
> 
> How can you check the app tag?

The SAS controller firmware checks the tags, not device driver.   What this code is doing is setting flags in the SCSI_IO Request message that is being sent to controller firmware.

> 
> 
> +_scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> [...]
> +	if (scmd->sc_data_direction == DMA_TO_DEVICE) {
> +		sk = ILLEGAL_REQUEST;
> +		host_byte = DID_ABORT;
> +	} else {
> +		sk = ABORTED_COMMAND;
> +		host_byte = DID_OK;
> +	}
> 
> ILLEGAL REQUEST is used when the HBA detects a mismatch 
> between data and
> protection information.  If you are not receiving PI from the 
> OS (WRITE
> INSERT) then the HBA can't detect an integrity error.  So in the
> DMA_TO_DEVICE case only the target can catch the error (and it will
> return ABORTED_COMMAND).
> 

Your right.

> For READ, both the drive firmware and the HBA can ostensibly 
> check that
> the data buffers and PI match.
> 
> Are your EEDP error flags only set when the HBA firmware detects a
> problem or also when the drive returns ABORTED COMMAND/0x10?

I wrote is a generic function based on chapter 3.3 in your linux-hba.pdf doc found at http://oss.oracle.com/projects/data-integrity/dist/documentation/linux-hba.pdf.

Obviously the only errors the current implementation will only be flaged on READs.   

My line of thinking was I write the code so it won't need to be rewrote later when DIX support is added for SAS3.0. 

> 
> I.e. do we get double failure scenarios (both drive and HBA detect the
> mismatch)?

Yes, I was told by the LSI  software test lab, they saw drives returning check condition with these ref tag errors.  I wasn't able to repro that.   What I saw were the IOCSTATUS errors, which is what the code handles in _scsih_eedp_error_handling().   Either way, both cases are covered right?

I have a 3gb SAS drive with EEDP support, model HITACHI HUS153073VLS300.  This support type 1 and 2.

I also EEDP support for target mode driver for this 6gb part, type 1, 2, and 3.

I used both to test the driver.

> 
> 
> +	scsi_host_set_prot(shost, SHOST_DIF_TYPE1_PROTECTION
> +	    | SHOST_DIF_TYPE3_PROTECTION);
> 
> How do you handle Type 3?  The application tag and reference tags are
> both defined as being opaque storage for Type 3.

Yeah I know that.    Since its app tag, it can be set to anything you want it to be.  For our internal implementation of type 3 DIF, I'm setting the lower 32bit part of the tag to the lba.  I was able to test this with a type 3 target, and it work fine.

> 
> If I understand correctly, your firmware only deals with the Type 2
> usage model of the application tag.  And Type 1 + 2 usage of the
> reference tag.
> 

Our controller firmware support all three types of protection.. 


> 
> +	scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
> 
> Technically speaking that is only required for DIX exchange.  But
> setting it doesn't hurt.

I wasn't sure whether it needed to be set.    Our controller generates the CRC. I'm not sure whether firmware or hardware implementation.  

> 
> -- 
> Martin K. Petersen	Oracle Linux Engineering
> 
> 

Re: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Martin K. Petersen]

>>>>> "Eric" == Moore, Eric <Eric.Moore@lsi.com> writes:

Eric,

Eric> The SAS controller firmware checks the tags, not device driver.
Eric> What this code is doing is setting flags in the SCSI_IO Request
Eric> message that is being sent to controller firmware.

Yeah, I figured that much.  I guess it makes sense when you treat the
app tag as being constant (modulo the mask) for one request.


Eric> Obviously the only errors the current implementation will only be
Eric> flaged on READs.

Eric> My line of thinking was I write the code so it won't need to be
Eric> rewrote later when DIX support is added for SAS3.0.

Ok, that's fine then.


Eric> Yes, I was told by the LSI software test lab, they saw drives
Eric> returning check condition with these ref tag errors.  I wasn't
Eric> able to repro that.  What I saw were the IOCSTATUS errors, which
Eric> is what the code handles in _scsih_eedp_error_handling().  Either
Eric> way, both cases are covered right?

Yeah.  With READ the difference is mostly academic.


>> If I understand correctly, your firmware only deals with the Type 2
>> usage model of the application tag.  And Type 1 + 2 usage of the
>> reference tag.

Eric> Our controller firmware support all three types of protection..

So you can actually set the application tag (and reference tag) on a
per-sector basis for Type 1 and 3 provided the OS sends down 520-byte
sectors?


>> + scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
>> 
>> Technically speaking that is only required for DIX exchange.  But
>> setting it doesn't hurt.

Eric> I wasn't sure whether it needed to be set.  Our controller
Eric> generates the CRC. I'm not sure whether firmware or hardware
Eric> implementation.

Given that it sounds like your next chip rev. will use the same driver
it should be ok to leave it in place.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Moore, Eric]

On Wednesday, April 15, 2009 4:27 PM,  Martin K. Petersen wrote:
> Eric> Yes, I was told by the LSI software test lab, they saw drives
> Eric> returning check condition with these ref tag errors.  I wasn't
> Eric> able to repro that.  What I saw were the IOCSTATUS errors, which
> Eric> is what the code handles in 
> _scsih_eedp_error_handling().  Either
> Eric> way, both cases are covered right?
> 
> Yeah.  With READ the difference is mostly academic.

I forgot to mention how they got the drives to return EEDP errors. They were using sas protocal jammers to inject errors on the bus.

> 
> 
> >> If I understand correctly, your firmware only deals with the Type 2
> >> usage model of the application tag.  And Type 1 + 2 usage of the
> >> reference tag.
> 
> Eric> Our controller firmware support all three types of protection..
> 
> So you can actually set the application tag (and reference tag) on a
> per-sector basis for Type 1 and 3 provided the OS sends down 520-byte
> sectors?
> 

We have a choice of setting the tags on a per IO request basis.  The same tag can apply to all sectors, or you can set some bit telling controller firmware to increment the tag for each sector.    So the bottom line, the current interface I have is not setup for setting each sector from the driver unless I send the normal data having the protection bytes already interleaved in the scatter gather list from the device driver.   (its silly to do that).  

So what I did is good for type1, which is set the ref tag to the lower 32bit of the lba, and the app tag to zero.

For type 3, I asked others for suggestions from a number of co-workers and the software archetic, and they said to set the tags the same way I did for type 1.   Due to my limitations with controller firmware, what do you suggest for type 3?      


> 
> Eric> I wasn't sure whether it needed to be set.  Our controller
> Eric> generates the CRC. I'm not sure whether firmware or hardware
> Eric> implementation.
> 
> Given that it sounds like your next chip rev. will use the same driver
> it should be ok to leave it in place.
> 

Thats the plan I hope.

Re: [PATCH 9/13] mpt2sas: T10 DIF Support - EEDP

[Martin K. Petersen]

>>>>> "Eric" == Moore, Eric <Eric.Moore@lsi.com> writes:

Eric,

Eric> So what I did is good for type1, which is set the ref tag to the
Eric> lower 32bit of the lba, and the app tag to zero.

That's fine.


Eric> For type 3, I asked others for suggestions from a number of
Eric> co-workers and the software archetic, and they said to set the
Eric> tags the same way I did for type 1.  Due to my limitations with
Eric> controller firmware, what do you suggest for type 3?

Well, so technically you are not supposed to set the reference tag to
the LBA in Type 3.  The 16 bits of the app tag + the 32 bits of the ref
tag are considered opaque storage.

The storage device has no way of checking neither app, nor ref with Type
3.  It can only check the guard tag.

My main concern is if you always expect the ref to be lower 32 bits of
LBA and have the HBA firmware verify that on READ.  Because the ref tag
could conceivably contain anything and the I/O might fail.

I realize this is a bit academic given that with the current setup your
HBA will be doing both writing and reading.  But in a multi-initiator
setup or for future compatibility it might throw a wrench in the
machinery to have the HBA check the ref tag on Type 3.

For Type 1 I expect you to check guard + match ref tag to CDB LBA +
offset.

For Type 2 I expect you to check guard + match ref tag to E.I.LBA in
32-byte CDB + offset.

For Type 3 I expect you to check guard only.

-- 
Martin K. Petersen	Oracle Linux Engineering