scsi: sr.c use unaligned access helpers
PATCH 1/1 scsi: sr.c use unaligned access helpers
Preserve the cd->capacity indentation near the #if 0'd if() statement.
--- drivers/scsi/sr.c | 26 ++++++++------------------ 1 files changed, 8 insertions(+), 18 deletions(-)
drivers/scsi/sr.c +8 -18
@@ -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 " : "",
Signature · 1 line
1.6.0.2.471.g47a76
Quoted 37 lines · Harvey Harrison
Preserve the
cd->capacityindentation near the #if 0'dif()statement.Signed-off-by Harvey Harrison <[email protected]>--- drivers/scsi/sr.c | 26 ++++++++------------------ 1 files changed, 8 insertions(+), 18 deletions(-)drivers/scsi/sr.c +2
-6@@ -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 ;
drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 451
Quoted 10 lines
drivers/scsi/sr.c +2
-4- 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.
Signature · 1 line
Jens Axboe
Quoted 46 lines · Jens Axboe
Quoted 45 lines · Harvey Harrison
Preserve the
cd->capacityindentation near the #if 0'dif()statement.Signed-off-by Harvey Harrison <[email protected]>--- drivers/scsi/sr.c | 26 ++++++++------------------ 1 files changed, 8 insertions(+), 18 deletions(-)drivers/scsi/sr.c +4
-10@@ -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 );
Quoted 7 lines
drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 451
Quoted 2 lines
drivers/scsi/sr.c +0
-0/* * 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
Quoted 55 lines · On Tue, 07 Oct 2008 14:19:14 +0200 Boaz Harrosh <[email protected]> wrote:
Quoted 46 lines · Jens Axboe
Quoted 45 lines · Harvey Harrison
Preserve the
cd->capacityindentation near the #if 0'dif()statement.Signed-off-by Harvey Harrison <[email protected]>--- drivers/scsi/sr.c | 26 ++++++++------------------ 1 files changed, 8 insertions(+), 18 deletions(-)drivers/scsi/sr.c +4
-10@@ -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 );Quoted 7 lines
drivers/scsi/sr.c static int sr_prep_fn(struct request_queue *q, struct request *rq) · line 451
Quoted 2 lines
drivers/scsi/sr.c +0
-0/* * 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.
Quoted 6 lines
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__b32at 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".
Quoted 16 lines · Andrew Morton
Quoted 6 lines · On Tue, 07 Oct 2008 14:19:14 +0200 Boaz Harrosh <[email protected]> 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__b32at 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;
Quoted 5 lines
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().
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."