Skip to contentSkip to conversation

scsi: Fix incorrect reporting of host protection capabilities

PATCH In review Martin K. Petersen 2 participants 3 messages Oct 15, 2009 – Oct 30, 2009 SCSI

lore
Share & tools
lore

Martin K. Petersen

author
#0 lore raw

PATCH 1/1 scsi: Fix incorrect reporting of host protection capabilities

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

Signed-off-by Martin K. Petersen <[email protected]>

---

include/scsi/scsi_host.h +3 -0
index 6e728b1..7220125 100644
@@ -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;
#1 lore raw
Quoted 2 lines · Martin K. Petersen

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;
}
Signature · 4 lines
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."
author
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Matthew" == Matthew Wilcox <[email protected]> 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 <[email protected]>
include/scsi/scsi_host.h +11 -18
index 6e728b1..47941fc 100644
@@ -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;
 }