[PATCH] scsi: Fix incorrect reporting of host protection capabilities

[PATCH] scsi: Fix incorrect reporting of host protection capabilities

[Martin K. Petersen]

The advent of DIF Type 2 devices exposed these missing break statements.

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

---

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 6e728b1..7220125 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -801,12 +801,15 @@ static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsign
 	case 1:
 		if (shost->prot_capabilities & SHOST_DIF_TYPE1_PROTECTION)
 			return target_type;
+		break;
 	case 2:
 		if (shost->prot_capabilities & SHOST_DIF_TYPE2_PROTECTION)
 			return target_type;
+		break;
 	case 3:
 		if (shost->prot_capabilities & SHOST_DIF_TYPE3_PROTECTION)
 			return target_type;
+		break;
 	}
 
 	return 0;

Re: [PATCH] scsi: Fix incorrect reporting of host protection capabilities

[Matthew Wilcox]

On Thu, Oct 15, 2009 at 02:46:13PM -0400, Martin K. Petersen wrote:
> 
> The advent of DIF Type 2 devices exposed these missing break statements.

Is there not a similar problem in scsi_host_dix_capable()?

And are these performance path?  If not, it might be nice to move them
out of line and code them like this:

unsigned scsi_host_dif_capable(struct Scsi_Host *shost, unsigned target_type)
{
	static unsigned cap[4] = {
		0, SHOST_DIF_TYPE1_PROTECTION,
		SHOST_DIF_TYPE2_PROTECTION, SHOST_DIF_TYPE3_PROTECTION
	};
	unsigned type = cap[target_type];
	return (shost->prot_capabilities & type) ? 1 : 0;
}

unsigned scsi_host_dix_capable(struct Scsi_Host *shost, unsigned target_type)
{
	static unsigned cap[4] = {
		SHOST_DIX_TYPE0_PROTECTION, SHOST_DIX_TYPE1_PROTECTION,
		SHOST_DIF_TYPE2_PROTECTION, SHOST_DIF_TYPE3_PROTECTION
	};
	unsigned type = cap[target_type];
	return (shost->prot_capabilities & type) ? 1 : 0;
}

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] scsi: Fix incorrect reporting of host protection capabilities

[Martin K. Petersen]

>>>>> "Matthew" == Matthew Wilcox <matthew@wil.cx> writes:

Matthew> And are these performance path?  If not, it might be nice to
Matthew> move them out of line and code them like this:

I compared the object files for the two approaches and it looks like
indexing into the array is a clear win compared to the switch statement.

Also, the DIF case must return the chosen protection type and not
true/false.


scsi: Fix incorrect reporting of host protection capabilities

The advent of DIF Type 2 devices exposed some missing break statements
in the protection mask switch constructs.  However, rewriting the code
to use an index into a small static array seemed like a more elegant
solution.

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

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 6e728b1..47941fc 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -797,30 +797,23 @@ static inline unsigned int scsi_host_get_prot(struct Scsi_Host *shost)
 
 static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsigned int target_type)
 {
-	switch (target_type) {
-	case 1:
-		if (shost->prot_capabilities & SHOST_DIF_TYPE1_PROTECTION)
-			return target_type;
-	case 2:
-		if (shost->prot_capabilities & SHOST_DIF_TYPE2_PROTECTION)
-			return target_type;
-	case 3:
-		if (shost->prot_capabilities & SHOST_DIF_TYPE3_PROTECTION)
-			return target_type;
-	}
+	static unsigned char cap[] = { 0,
+				       SHOST_DIF_TYPE1_PROTECTION,
+				       SHOST_DIF_TYPE2_PROTECTION,
+				       SHOST_DIF_TYPE3_PROTECTION };
 
-	return 0;
+	return shost->prot_capabilities & cap[target_type] ? target_type : 0;
 }
 
 static inline unsigned int scsi_host_dix_capable(struct Scsi_Host *shost, unsigned int target_type)
 {
 #if defined(CONFIG_BLK_DEV_INTEGRITY)
-	switch (target_type) {
-	case 0: return shost->prot_capabilities & SHOST_DIX_TYPE0_PROTECTION;
-	case 1: return shost->prot_capabilities & SHOST_DIX_TYPE1_PROTECTION;
-	case 2: return shost->prot_capabilities & SHOST_DIX_TYPE2_PROTECTION;
-	case 3: return shost->prot_capabilities & SHOST_DIX_TYPE3_PROTECTION;
-	}
+	static unsigned char cap[] = { SHOST_DIX_TYPE0_PROTECTION,
+				       SHOST_DIX_TYPE1_PROTECTION,
+				       SHOST_DIX_TYPE2_PROTECTION,
+				       SHOST_DIX_TYPE3_PROTECTION };
+
+	return shost->prot_capabilities & cap[target_type];
 #endif
 	return 0;
 }