[PATCH] scsi: sr.c use unaligned access helpers

[PATCH] scsi: sr.c use unaligned access helpers

[Harvey Harrison]

Preserve the cd->capacity indentation near the #if 0'd if() statement.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 drivers/scsi/sr.c |   26 ++++++++------------------
 1 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 27f5bfd..f1b650e 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -45,6 +45,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <asm/uaccess.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
@@ -280,10 +281,7 @@ static int sr_done(struct scsi_cmnd *SCpnt)
 		case ILLEGAL_REQUEST:
 			if (!(SCpnt->sense_buffer[0] & 0x90))
 				break;
-			error_sector = (SCpnt->sense_buffer[3] << 24) |
-				(SCpnt->sense_buffer[4] << 16) |
-				(SCpnt->sense_buffer[5] << 8) |
-				SCpnt->sense_buffer[6];
+			error_sector = get_unaligned_be32(SCpnt->sense_buffer + 3);
 			if (SCpnt->request->bio != NULL)
 				block_sectors =
 					bio_sectors(SCpnt->request->bio);
@@ -445,13 +443,9 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->sdb.length = this_count * s_size;
 	}
 
-	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
-	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
-	SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
-	SCpnt->cmnd[5] = (unsigned char) block & 0xff;
+	put_unaligned_be32(block, SCpnt->cmnd + 2);
 	SCpnt->cmnd[6] = SCpnt->cmnd[9] = 0;
-	SCpnt->cmnd[7] = (unsigned char) (this_count >> 8) & 0xff;
-	SCpnt->cmnd[8] = (unsigned char) this_count & 0xff;
+	put_unaligned_be16(this_count, SCpnt->cmnd + 7);
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -701,12 +695,8 @@ static void get_sectorsize(struct scsi_cd *cd)
 		if (cdrom_get_last_written(&cd->cdi,
 					   &cd->capacity))
 #endif
-			cd->capacity = 1 + ((buffer[0] << 24) |
-						    (buffer[1] << 16) |
-						    (buffer[2] << 8) |
-						    buffer[3]);
-		sector_size = (buffer[4] << 24) |
-		    (buffer[5] << 16) | (buffer[6] << 8) | buffer[7];
+			cd->capacity = 1 + get_unaligned_be32(buffer);
+		sector_size = get_unaligned_be32(buffer + 4);
 		switch (sector_size) {
 			/*
 			 * HP 4020i CD-Recorder reports 2340 byte sectors
@@ -792,12 +782,12 @@ static void get_capabilities(struct scsi_cd *cd)
 	}
 
 	n = data.header_length + data.block_descriptor_length;
-	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
+	cd->cdi.speed = get_unaligned_be16(buffer + n + 8) / 176;
 	cd->readcd_known = 1;
 	cd->readcd_cdda = buffer[n + 5] & 0x01;
 	/* print some capability bits */
 	printk("%s: scsi3-mmc drive: %dx/%dx %s%s%s%s%s%s\n", cd->cdi.name,
-	       ((buffer[n + 14] << 8) + buffer[n + 15]) / 176,
+	       get_unaligned_be16(buffer + n + 14) / 176,
 	       cd->cdi.speed,
 	       buffer[n + 3] & 0x01 ? "writer " : "", /* CD Writer */
 	       buffer[n + 3] & 0x20 ? "dvd-ram " : "",
-- 
1.6.0.2.471.g47a76

Re: [PATCH] scsi: sr.c use unaligned access helpers

[Jens Axboe]

On Fri, Oct 03 2008, Harvey Harrison wrote:
> Preserve the cd->capacity indentation near the #if 0'd if() statement.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
>  drivers/scsi/sr.c |   26 ++++++++------------------
>  1 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 27f5bfd..f1b650e 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -45,6 +45,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/mutex.h>
>  #include <asm/uaccess.h>
> +#include <asm/unaligned.h>
>  
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_dbg.h>
> @@ -280,10 +281,7 @@ static int sr_done(struct scsi_cmnd *SCpnt)
>  		case ILLEGAL_REQUEST:
>  			if (!(SCpnt->sense_buffer[0] & 0x90))
>  				break;
> -			error_sector = (SCpnt->sense_buffer[3] << 24) |
> -				(SCpnt->sense_buffer[4] << 16) |
> -				(SCpnt->sense_buffer[5] << 8) |
> -				SCpnt->sense_buffer[6];
> +			error_sector = get_unaligned_be32(SCpnt->sense_buffer + 3);
>  			if (SCpnt->request->bio != NULL)
>  				block_sectors =
>  					bio_sectors(SCpnt->request->bio);
> @@ -445,13 +443,9 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  		SCpnt->sdb.length = this_count * s_size;
>  	}
>  
> -	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> -	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
> -	SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> -	SCpnt->cmnd[5] = (unsigned char) block & 0xff;
> +	put_unaligned_be32(block, SCpnt->cmnd + 2);
>  	SCpnt->cmnd[6] = SCpnt->cmnd[9] = 0;
> -	SCpnt->cmnd[7] = (unsigned char) (this_count >> 8) & 0xff;
> -	SCpnt->cmnd[8] = (unsigned char) this_count & 0xff;
> +	put_unaligned_be16(this_count, SCpnt->cmnd + 7);
>  
>  	/*
>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb

Lets not please, it reduces readability a lot when you are used to SCSI
cdb filling.


-- 
Jens Axboe

Re: [PATCH] scsi: sr.c use unaligned access helpers

[Boaz Harrosh]

Jens Axboe wrote:
> On Fri, Oct 03 2008, Harvey Harrison wrote:
>> Preserve the cd->capacity indentation near the #if 0'd if() statement.
>>
>> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>> ---
>>  drivers/scsi/sr.c |   26 ++++++++------------------
>>  1 files changed, 8 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>> index 27f5bfd..f1b650e 100644
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>> @@ -45,6 +45,7 @@
>>  #include <linux/blkdev.h>
>>  #include <linux/mutex.h>
>>  #include <asm/uaccess.h>
>> +#include <asm/unaligned.h>
>>  
>>  #include <scsi/scsi.h>
>>  #include <scsi/scsi_dbg.h>
>> @@ -280,10 +281,7 @@ static int sr_done(struct scsi_cmnd *SCpnt)
>>  		case ILLEGAL_REQUEST:
>>  			if (!(SCpnt->sense_buffer[0] & 0x90))
>>  				break;
>> -			error_sector = (SCpnt->sense_buffer[3] << 24) |
>> -				(SCpnt->sense_buffer[4] << 16) |
>> -				(SCpnt->sense_buffer[5] << 8) |
>> -				SCpnt->sense_buffer[6];
>> +			error_sector = get_unaligned_be32(SCpnt->sense_buffer + 3);
>>  			if (SCpnt->request->bio != NULL)
>>  				block_sectors =
>>  					bio_sectors(SCpnt->request->bio);
>> @@ -445,13 +443,9 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>  		SCpnt->sdb.length = this_count * s_size;
>>  	}
>>  
>> -	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
>> -	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
>> -	SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
>> -	SCpnt->cmnd[5] = (unsigned char) block & 0xff;
>> +	put_unaligned_be32(block, SCpnt->cmnd + 2);
>>  	SCpnt->cmnd[6] = SCpnt->cmnd[9] = 0;
>> -	SCpnt->cmnd[7] = (unsigned char) (this_count >> 8) & 0xff;
>> -	SCpnt->cmnd[8] = (unsigned char) this_count & 0xff;
>> +	put_unaligned_be16(this_count, SCpnt->cmnd + 7);
>>  
>>  	/*
>>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb
> 
> Lets not please, it reduces readability a lot when you are used to SCSI
> cdb filling.
> 
> 

I feel the opposite. That is: put_unaligned_be32(block, SCpnt->cmnd + 2);
is much more readable for me. Coming from the spec, I'm looking for a __b32
at offset CDB+2 and not: "SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;"
At offset CDB+4 the 2nd-or-3rd? order byte of "block".

And for BE systems it's a gain. So please DO

My $0.017
Boaz

Re: [PATCH] scsi: sr.c use unaligned access helpers

[Andrew Morton]

On Tue, 07 Oct 2008 14:19:14 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> Jens Axboe wrote:
> > On Fri, Oct 03 2008, Harvey Harrison wrote:
> >> Preserve the cd->capacity indentation near the #if 0'd if() statement.
> >>
> >> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> >> ---
> >>  drivers/scsi/sr.c |   26 ++++++++------------------
> >>  1 files changed, 8 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> >> index 27f5bfd..f1b650e 100644
> >> --- a/drivers/scsi/sr.c
> >> +++ b/drivers/scsi/sr.c
> >> @@ -45,6 +45,7 @@
> >>  #include <linux/blkdev.h>
> >>  #include <linux/mutex.h>
> >>  #include <asm/uaccess.h>
> >> +#include <asm/unaligned.h>
> >>  
> >>  #include <scsi/scsi.h>
> >>  #include <scsi/scsi_dbg.h>
> >> @@ -280,10 +281,7 @@ static int sr_done(struct scsi_cmnd *SCpnt)
> >>  		case ILLEGAL_REQUEST:
> >>  			if (!(SCpnt->sense_buffer[0] & 0x90))
> >>  				break;
> >> -			error_sector = (SCpnt->sense_buffer[3] << 24) |
> >> -				(SCpnt->sense_buffer[4] << 16) |
> >> -				(SCpnt->sense_buffer[5] << 8) |
> >> -				SCpnt->sense_buffer[6];
> >> +			error_sector = get_unaligned_be32(SCpnt->sense_buffer + 3);
> >>  			if (SCpnt->request->bio != NULL)
> >>  				block_sectors =
> >>  					bio_sectors(SCpnt->request->bio);
> >> @@ -445,13 +443,9 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
> >>  		SCpnt->sdb.length = this_count * s_size;
> >>  	}
> >>  
> >> -	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> >> -	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
> >> -	SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> >> -	SCpnt->cmnd[5] = (unsigned char) block & 0xff;
> >> +	put_unaligned_be32(block, SCpnt->cmnd + 2);
> >>  	SCpnt->cmnd[6] = SCpnt->cmnd[9] = 0;
> >> -	SCpnt->cmnd[7] = (unsigned char) (this_count >> 8) & 0xff;
> >> -	SCpnt->cmnd[8] = (unsigned char) this_count & 0xff;
> >> +	put_unaligned_be16(this_count, SCpnt->cmnd + 7);
> >>  
> >>  	/*
> >>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb
> > 
> > Lets not please, it reduces readability a lot when you are used to SCSI
> > cdb filling.
> > 
> > 
> 
> I feel the opposite. That is: put_unaligned_be32(block, SCpnt->cmnd + 2);
> is much more readable for me. Coming from the spec, I'm looking for a __b32
> at offset CDB+2 and not: "SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;"
> At offset CDB+4 the 2nd-or-3rd? order byte of "block".
> 
> And for BE systems it's a gain. So please DO

Yeah.  For neophytes it's a good change.  This:

	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
	SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
	SCpnt->cmnd[5] = (unsigned char) block & 0xff;

is "wtf is that doing?", whereas this:

	put_unaligned_be32(block, SCpnt->cmnd + 2);

is "ah, I know what that's doing".

Re: [PATCH] scsi: sr.c use unaligned access helpers

[Matthew Wilcox]

On Tue, Oct 07, 2008 at 12:25:31PM -0700, Andrew Morton wrote:
> On Tue, 07 Oct 2008 14:19:14 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> > I feel the opposite. That is: put_unaligned_be32(block, SCpnt->cmnd + 2);
> > is much more readable for me. Coming from the spec, I'm looking for a __b32
> > at offset CDB+2 and not: "SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;"
> > At offset CDB+4 the 2nd-or-3rd? order byte of "block".
> > 
> > And for BE systems it's a gain. So please DO
> 
> Yeah.  For neophytes it's a good change.  This:
> 
> 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> 	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
> 	SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> 	SCpnt->cmnd[5] = (unsigned char) block & 0xff;
> 
> is "wtf is that doing?", whereas this:
> 
> 	put_unaligned_be32(block, SCpnt->cmnd + 2);
> 
> is "ah, I know what that's doing".

And scsi_put_u32(block, SCpnt->cmnd + 2); is even more obvious.  You
don't even need to know that SCSI is a big-endian protocol that way,
just that the u32 is being written in the appropriate way for SCSI
(see also ntohl et al).

Also, SCSI needs to be able to read/write 'u24' quantities, which is
potentially an ambiguous thing if you consider it in terms of
byteswapping.  I don't think we want a put_unaligned_be24() function,
but we do want a scsi_put_u24().

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