[RFC PATCH-mm] scsi: use unaligned endian helpers rather than byteshifting

[RFC PATCH-mm] scsi: use unaligned endian helpers rather than byteshifting

[Harvey Harrison]

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
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(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 90d1d08..5cdbc7d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -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;
-- 
1.6.1.rc1.262.gb6810

Re: [RFC PATCH-mm] scsi: use unaligned endian helpers rather than byteshifting

[Matthew Wilcox]

On Wed, Dec 03, 2008 at 11:37:23AM -0800, Harvey Harrison wrote:
> 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)

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

-- 
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: [RFC PATCH-mm] scsi: use unaligned endian helpers rather than byteshifting

[Harvey Harrison]

On Wed, 2008-12-03 at 13:15 -0700, Matthew Wilcox wrote:
> On Wed, Dec 03, 2008 at 11:37:23AM -0800, Harvey Harrison wrote:
> > 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)
> 
> >  		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.
> 

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

Re: [RFC PATCH-mm] scsi: use unaligned endian helpers rather than byteshifting

[James Bottomley]

On Wed, 2008-12-03 at 11:37 -0800, Harvey Harrison wrote:
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> 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

> @@ -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

Re: [RFC PATCH-mm] scsi: use unaligned endian helpers rather than byteshifting

[Harvey Harrison]

On Wed, 2008-12-03 at 14:33 -0600, James Bottomley wrote:
> On Wed, 2008-12-03 at 11:37 -0800, Harvey Harrison wrote:
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > ---
> > 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).

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