scsi: use unaligned endian helpers rather than byteshifting

RFC In review Harvey Harrison 3 participants 5 messages Dec 3, 2008 – Dec 5, 2008

Subsystems (2)

SCSI Aacraid SCSI RAID driver

lore

Share & tools

lore

Harvey Harrison

author

2008-12-03 19:38 UTC #0 lore raw

RFC 1/1 scsi: use unaligned endian helpers rather than byteshifting

Signed-off-by Harvey Harrison <[email protected]>

---
Depends on the unaligned access work in -mm. Just so you can see what the transition would look like. See in particular the READ/WRITE6 bits as just reading the full 32 bits and masking ends up being better on lots of arches. (x86/powerpc/SH at least)

 drivers/scsi/aacraid/aachba.c |  126 +++++++++-------------------------------
 1 files changed, 29 insertions(+), 97 deletions(-)

drivers/scsi/aacraid/aachba.c +29 ~~-97~~

index 90d1d08..5cdbc7d 100644
@@ -33,6 +33,7 @@
 #include <linux/blkdev.h>
 #include <asm/uaccess.h>
 #include <linux/highmem.h> /* For flush_kernel_dcache_page */
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>

@@ -1479,29 +1480,18 @@ static void io_callback(void *context, struct fib * fibptr)
 		switch (scsicmd->cmnd[0]) {
 		case WRITE_6:
 		case READ_6:
-			lba = ((scsicmd->cmnd[1] & 0x1F) << 16) |
-			    (scsicmd->cmnd[2] << 8) | scsicmd->cmnd[3];
+			lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[0]) & 0x1fffff;
 			break;
 		case WRITE_16:
 		case READ_16:
-			lba = ((u64)scsicmd->cmnd[2] << 56) |
-			      ((u64)scsicmd->cmnd[3] << 48) |
-			      ((u64)scsicmd->cmnd[4] << 40) |
-			      ((u64)scsicmd->cmnd[5] << 32) |
-			      ((u64)scsicmd->cmnd[6] << 24) |
-			      (scsicmd->cmnd[7] << 16) |
-			      (scsicmd->cmnd[8] << 8) | scsicmd->cmnd[9];
+			lba = load_be64_noalign((__be64 *)&scsicmd->cmnd[2]);
 			break;
 		case WRITE_12:
 		case READ_12:
-			lba = ((u64)scsicmd->cmnd[2] << 24) |
-			      (scsicmd->cmnd[3] << 16) |
-			      (scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
+			lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
 			break;
 		default:
-			lba = ((u64)scsicmd->cmnd[2] << 24) |
-			       (scsicmd->cmnd[3] << 16) |
-			       (scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
+			lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
 			break;
 		}
 		printk(KERN_DEBUG

@@ -1576,34 +1566,20 @@ static int aac_read(struct scsi_cmnd * scsicmd)
 	case READ_16:
 		dprintk((KERN_DEBUG "aachba: received a read(16) command on id %d.\n", scmd_id(scsicmd)));
 
-		lba =	((u64)scsicmd->cmnd[2] << 56) |
-			((u64)scsicmd->cmnd[3] << 48) |
-			((u64)scsicmd->cmnd[4] << 40) |
-			((u64)scsicmd->cmnd[5] << 32) |
-			((u64)scsicmd->cmnd[6] << 24) |
-			(scsicmd->cmnd[7] << 16) |
-			(scsicmd->cmnd[8] << 8) | scsicmd->cmnd[9];
-		count = (scsicmd->cmnd[10] << 24) |
-			(scsicmd->cmnd[11] << 16) |
-			(scsicmd->cmnd[12] << 8) | scsicmd->cmnd[13];
+		lba = load_be64_noalign((__be64 *)&scsicmd->cmnd[2]);
+		count = load_be32_noalign((__be32 *)&scsicmd->cmnd[10]);
 		break;
 	case READ_12:
 		dprintk((KERN_DEBUG "aachba: received a read(12) command on id %d.\n", scmd_id(scsicmd)));
 
-		lba = ((u64)scsicmd->cmnd[2] << 24) |
-			(scsicmd->cmnd[3] << 16) |
-			(scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
-		count = (scsicmd->cmnd[6] << 24) |
-			(scsicmd->cmnd[7] << 16) |
-			(scsicmd->cmnd[8] << 8) | scsicmd->cmnd[9];
+		lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
+		count = load_be32_noalign((__be32 *)&scsicmd->cmnd[6]);
 		break;
 	default:
 		dprintk((KERN_DEBUG "aachba: received a read(10) command on id %d.\n", scmd_id(scsicmd)));
 
-		lba = ((u64)scsicmd->cmnd[2] << 24) |
-			(scsicmd->cmnd[3] << 16) |
-			(scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
-		count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
+		lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
+		count = load_be16_noalign((__be16 *)&scsicmd->cmnd[7]);
 		break;
 	}
 	dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",

@@ -1661,28 +1637,19 @@ static int aac_write(struct scsi_cmnd * scsicmd)
 	} else if (scsicmd->cmnd[0] == WRITE_16) { /* 16 byte command */
 		dprintk((KERN_DEBUG "aachba: received a write(16) command on id %d.\n", scmd_id(scsicmd)));
 
-		lba =	((u64)scsicmd->cmnd[2] << 56) |
-			((u64)scsicmd->cmnd[3] << 48) |
-			((u64)scsicmd->cmnd[4] << 40) |
-			((u64)scsicmd->cmnd[5] << 32) |
-			((u64)scsicmd->cmnd[6] << 24) |
-			(scsicmd->cmnd[7] << 16) |
-			(scsicmd->cmnd[8] << 8) | scsicmd->cmnd[9];
-		count = (scsicmd->cmnd[10] << 24) | (scsicmd->cmnd[11] << 16) |
-			(scsicmd->cmnd[12] << 8) | scsicmd->cmnd[13];
+		lba = load_be64_noalign((__be64 *)&scsicmd->cmnd[2]);
+		count = load_be32_noalign((__be32 *)&scsicmd->cmnd[10]);
 		fua = scsicmd->cmnd[1] & 0x8;
 	} else if (scsicmd->cmnd[0] == WRITE_12) { /* 12 byte command */
 		dprintk((KERN_DEBUG "aachba: received a write(12) command on id %d.\n", scmd_id(scsicmd)));
 
-		lba = ((u64)scsicmd->cmnd[2] << 24) | (scsicmd->cmnd[3] << 16)
-		    | (scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
-		count = (scsicmd->cmnd[6] << 24) | (scsicmd->cmnd[7] << 16)
-		      | (scsicmd->cmnd[8] << 8) | scsicmd->cmnd[9];
+		lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
+		count = load_be32_noalign((__be32 *)&scsicmd->cmnd[6]);
 		fua = scsicmd->cmnd[1] & 0x8;
 	} else {
 		dprintk((KERN_DEBUG "aachba: received a write(10) command on id %d.\n", scmd_id(scsicmd)));
-		lba = ((u64)scsicmd->cmnd[2] << 24) | (scsicmd->cmnd[3] << 16) | (scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
-		count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
+		lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
+		count = load_be16_noalign((__be16 *)&scsicmd->cmnd[7]);
 		fua = scsicmd->cmnd[1] & 0x8;
 	}
 	dprintk((KERN_DEBUG "aac_write[cpu %d]: lba = %llu, t = %ld.\n",

@@ -1770,9 +1737,8 @@ static int aac_synchronize(struct scsi_cmnd *scsicmd)
 	struct scsi_device *sdev = scsicmd->device;
 	int active = 0;
 	struct aac_dev *aac;
-	u64 lba = ((u64)scsicmd->cmnd[2] << 24) | (scsicmd->cmnd[3] << 16) |
-		(scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
-	u32 count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
+	u64 lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[2]);
+	u32 count = load_be16_noalign((__be16 *)&scsicmd->cmnd[7]);
 	unsigned long flags;
 
 	/*

@@ -1786,41 +1752,19 @@ static int aac_synchronize(struct scsi_cmnd *scsicmd)
 			u32 cmnd_count;
 
 			if (cmd->cmnd[0] == WRITE_6) {
-				cmnd_lba = ((cmd->cmnd[1] & 0x1F) << 16) |
-					(cmd->cmnd[2] << 8) |
-					cmd->cmnd[3];
+				cmnd_lba = load_be32_noalign((__be32 *)&cmd->cmnd[0]) & 0x1fffff;
 				cmnd_count = cmd->cmnd[4];
 				if (cmnd_count == 0)
 					cmnd_count = 256;
 			} else if (cmd->cmnd[0] == WRITE_16) {
-				cmnd_lba = ((u64)cmd->cmnd[2] << 56) |
-					((u64)cmd->cmnd[3] << 48) |
-					((u64)cmd->cmnd[4] << 40) |
-					((u64)cmd->cmnd[5] << 32) |
-					((u64)cmd->cmnd[6] << 24) |
-					(cmd->cmnd[7] << 16) |
-					(cmd->cmnd[8] << 8) |
-					cmd->cmnd[9];
-				cmnd_count = (cmd->cmnd[10] << 24) |
-					(cmd->cmnd[11] << 16) |
-					(cmd->cmnd[12] << 8) |
-					cmd->cmnd[13];
+				cmnd_lba = load_be64_noalign((__be64 *)&cmd->cmnd[2]);
+				cmnd_count = load_be32_noalign((__be32 *)&cmd->cmnd[10]);
 			} else if (cmd->cmnd[0] == WRITE_12) {
-				cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
-					(cmd->cmnd[3] << 16) |
-					(cmd->cmnd[4] << 8) |
-					cmd->cmnd[5];
-				cmnd_count = (cmd->cmnd[6] << 24) |
-					(cmd->cmnd[7] << 16) |
-					(cmd->cmnd[8] << 8) |
-					cmd->cmnd[9];
+				cmnd_lba = load_be32_noalign((__be32 *)&cmd->cmnd[2]);
+				cmnd_count = load_be32_noalign((__be32 *)&cmd->cmnd[6]);
 			} else if (cmd->cmnd[0] == WRITE_10) {
-				cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
-					(cmd->cmnd[3] << 16) |
-					(cmd->cmnd[4] << 8) |
-					cmd->cmnd[5];
-				cmnd_count = (cmd->cmnd[7] << 8) |
-					cmd->cmnd[8];
+				cmnd_lba = load_be32_noalign((__be32 *)&cmd->cmnd[2]);
+				cmnd_count = load_be16_noalign((__be16 *)&cmd->cmnd[7]);
 			} else
 				continue;
 			if (((cmnd_lba + cmnd_count) < lba) ||

@@ -2136,23 +2080,14 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 
 		dprintk((KERN_DEBUG "READ CAPACITY_16 command.\n"));
 		capacity = fsa_dev_ptr[cid].size - 1;
-		cp[0] = (capacity >> 56) & 0xff;
-		cp[1] = (capacity >> 48) & 0xff;
-		cp[2] = (capacity >> 40) & 0xff;
-		cp[3] = (capacity >> 32) & 0xff;
-		cp[4] = (capacity >> 24) & 0xff;
-		cp[5] = (capacity >> 16) & 0xff;
-		cp[6] = (capacity >> 8) & 0xff;
-		cp[7] = (capacity >> 0) & 0xff;
+		store_be64_noalign((__be64 *)&cp[0], capacity);
 		cp[8] = 0;
 		cp[9] = 0;
 		cp[10] = 2;
 		cp[11] = 0;
 		cp[12] = 0;
 
-		alloc_len = ((scsicmd->cmnd[10] << 24)
-			     + (scsicmd->cmnd[11] << 16)
-			     + (scsicmd->cmnd[12] << 8) + scsicmd->cmnd[13]);
+		alloc_len = load_be32_noalign((__be32 *)&scsicmd->cmnd[10]);
 
 		alloc_len = min_t(size_t, alloc_len, sizeof(cp));
 		scsi_sg_copy_from_buffer(scsicmd, cp, alloc_len);

@@ -2180,10 +2115,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		else
 			capacity = (u32)-1;
 
-		cp[0] = (capacity >> 24) & 0xff;
-		cp[1] = (capacity >> 16) & 0xff;
-		cp[2] = (capacity >> 8) & 0xff;
-		cp[3] = (capacity >> 0) & 0xff;
+		store_be32_noalign((__be32 *)&cp[0], capacity);
 		cp[4] = 0;
 		cp[5] = 0;
 		cp[6] = 2;

Signature · 1 line

1.6.1.rc1.262.gb6810

Matthew Wilcox

2008-12-03 20:15 UTC #1 lore raw

drivers/scsi/aacraid/aachba.c static void io_callback(void *context, struct fib * fibptr) · line 1484

Quoted 11 lines · Harvey Harrison

Depends on the unaligned access work in -mm. Just so you can see what the transition would look like. See in particular the READ/WRITE6 bits as just reading the full 32 bits and masking ends up being better on lots of arches. (x86/powerpc/SH at least)
drivers/scsi/aacraid/aachba.c +1 -2
 		case WRITE_6:
 		case READ_6:
-			lba = ((scsicmd->cmnd[1] & 0x1F) << 16) |
-			    (scsicmd->cmnd[2] << 8) | scsicmd->cmnd[3];
+			lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[0]) & 0x1fffff;
 			break;

That may well generate better code, but I have a hard time convincing myself that it's correct. This badly needs to be abstracted into something *THAT MAKES SENSE FOR SCSI*.

James, if I resend my patches that introduce scsi_get_u24() et al, will you apply them? I'm tired of having to nack all the crazy patches that Harvey keeps sending.

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."

Harvey Harrison

author

2008-12-03 20:27 UTC #2 lore raw

Quoted 17 lines · Matthew Wilcox

drivers/scsi/aacraid/aachba.c static void io_callback(void *context, struct fib * fibptr) · line 1484
Quoted 11 lines · Harvey Harrison
Depends on the unaligned access work in -mm. Just so you can see what the transition would look like. See in particular the READ/WRITE6 bits as just reading the full 32 bits and masking ends up being better on lots of arches. (x86/powerpc/SH at least)
drivers/scsi/aacraid/aachba.c +1 -2
 		case WRITE_6:
 		case READ_6:
-			lba = ((scsicmd->cmnd[1] & 0x1F) << 16) |
-			    (scsicmd->cmnd[2] << 8) | scsicmd->cmnd[3];
+			lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[0]) & 0x1fffff;
 			break;
That may well generate better code, but I have a hard time convincing myself that it's correct. This badly needs to be abstracted into something *THAT MAKES SENSE FOR SCSI*.

Quoted 4 lines

James, if I resend my patches that introduce scsi_get_u24() et al, will you apply them? I'm tired of having to nack all the crazy patches that Harvey keeps sending.

Pardon me? How could I have done this better:

1) I went through the work to create a common API the whole kernel can use.
2) I hooked up ~20 arches and let arches provide optimized versions where they can.
3) Mine generates better code than what is already there
4) I made it exist for the aligned cases too in case you know it's aligned

Then, when I send an RFC you call it crazy?

If anything I made it a whole lot goddamn easier on _YOU_ to get you scsi patches to generate good code as these endian helpers are now available on every arch. You're fucking welcome by the way.

I also went through the kernel and removed a bunch of private endian wrappers and got them through maintainers, who, by and large, were happy to use the common infrastructure....other than scsi for some reason I cannot for the life of me understand.

So add a private scsi helper if you want, I'm not standing in your way but if anything, I think you are crazy one here.

Harvey

James Bottomley

maintainer

2008-12-03 20:33 UTC #3 lore raw

In reply to Harvey Harrison

Quoted 6 lines · Harvey Harrison

Signed-off-by Harvey Harrison <[email protected]>
---
Depends on the unaligned access work in -mm. Just so you can see what the transition would look like. See in particular the READ/WRITE6 bits as just reading the full 32 bits and masking ends up being better on lots of arches. (x86/powerpc/SH at least)

Well, as I've said before, I'm not particularly interested in moving SCSI over to the generic accessors. It does look like the proponents of SCSI specific ones gave up as well (most driver writers even open coded their own over the u32 one we already have).

However, things like this

drivers/scsi/aacraid/aachba.c static void io_callback(void *context, struct fib * fibptr) · line 1484

Quoted 8 lines

drivers/scsi/aacraid/aachba.c +1 -2

@@ -1479,29 +1480,18 @@ static void io_callback(void *context, struct fib * fibptr)
 		switch (scsicmd->cmnd[0]) {
 		case WRITE_6:
 		case READ_6:
-			lba = ((scsicmd->cmnd[1] & 0x1F) << 16) |
-			    (scsicmd->cmnd[2] << 8) | scsicmd->cmnd[3];
+			lba = load_be32_noalign((__be32 *)&scsicmd->cmnd[0]) & 0x1fffff;
 			break;

Make me think absolutely no way. It's far less readable than the original. The first thing that leaps to the mind of any SCSI person seeing this is what on earth is the opcode doing loaded as part of that
expression. It takes a bit of thought to see it's loaded and then
discarded by the and operation.

James

Harvey Harrison

author

2008-12-05 09:07 UTC #4 lore raw

Quoted 8 lines · James Bottomley

Quoted 6 lines · Harvey Harrison
Signed-off-by Harvey Harrison <[email protected]>
---
Depends on the unaligned access work in -mm. Just so you can see what the transition would look like. See in particular the READ/WRITE6 bits as just reading the full 32 bits and masking ends up being better on lots of arches. (x86/powerpc/SH at least)

Quoted 4 lines

Well, as I've said before, I'm not particularly interested in moving SCSI over to the generic accessors. It does look like the proponents of SCSI specific ones gave up as well (most driver writers even open coded their own over the u32 one we already have).

Well, I thought this over some more, since it seems that the offset provides useful information to scsi-developers, how about an interface like:

u16 load_be16_offset(u8 *buf, unsigned int offset) u32 load_be24_offset(u8 *buf, unsigned int offset) u32 load_be32_offset(u8 *buf, unsigned int offset) u64 load_be64_offset(u8 *buf, unsigned int offset)

Which reads the given value in the given endianness from a (positive) offset? Since there don't exist any common structs in scsi-land that would allow sparse checking and you'd have to have casts everywhere, this might be a nice compromise.

Document that these guys make no alignment assumptions and use the generic accessors inside...they are simple enough that they would fit in the common code and be useful elsewhere as well.

What do you think of that James/Matthew?

Harvey