[RFC][PATCH] fix sign extension with 1.5TB usb-storage LBD=y

[RFC][PATCH] fix sign extension with 1.5TB usb-storage LBD=y

[Dave Hansen]

This is with current git as of this morning, which is at v2.6.30-rc2.

I have a 1.5TB USB device which gets a bit angry when I plug it in.  It
ends up with a scsi_disk->capacity of ffffffffaea87b30.  I tracked it
down to the lba calculation in read_capacity_10():

	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
 		(buffer[2] << 8) | buffer[3];

lba is getting all 0xf's in its high 32 bits.  It seems odd that this
would happen since 'buffer' is an 'unsigned char', but that is
apparently what is going on.  Note that this isn't an issue 32-bit
kernels compiled with CONFIG_LBD=n since there's no more bits into which
the sign could be extended.

The attached one-liner seems to make my system happier.  

Before patch (with a bit of my added debugging):

sd 5:0:0:0: [sdb] read_capacity_10() sector size: 0x200/512 
sd 5:0:0:0: [sdb] capacity (in 512-byte blocks: ffffffffaea87b30
sd 5:0:0:0: [sdb] Very big device. Trying to use READ CAPACITY(16).
sd 5:0:0:0: [sdb] Using 0xffffffff as device size sector_size was: 0xffffffea/-22
sd 5:0:0:0: [sdb] 4294967296 512-byte hardware sectors: (2.19 TB/2.00 TiB)
sd 5:0:0:0: [sdb] Write Protect is off
sd 5:0:0:0: [sdb] Mode Sense: 1c 00 00 00
sd 5:0:0:0: [sdb] Assuming drive cache: write through
sd 5:0:0:0: [sdb] read_capacity_10() sector size: 0x200/512 
sd 5:0:0:0: [sdb] capacity (in 512-byte blocks: ffffffffaea87b30
sd 5:0:0:0: [sdb] Very big device. Trying to use READ CAPACITY(16).
sd 5:0:0:0: [sdb] Using 0xffffffff as device size sector_size was: 0xffffffea/-22
sd 5:0:0:0: [sdb] Assuming drive cache: write through
 sdb: sdb1 
sd 5:0:0:0: [sdb] Attached SCSI disk 
sd 5:0:0:0: [sdb] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
sd 5:0:0:0: [sdb] Sense Key : Aborted Command [current] 
sd 5:0:0:0: [sdb] Add. Sense: No additional sense information

after patch:

[ 5257.068646] usb 5-2: new high speed USB device using ehci_hcd and address 9
[ 5257.215421] usb 5-2: configuration #1 chosen from 1 choice
[ 5257.221640] scsi14 : SCSI emulation for USB Mass Storage devices
[ 5257.221937] usb-storage: device found at 9
[ 5257.221939] usb-storage: waiting for device to settle before scanning
[ 5262.225981] usb-storage: device scan complete
[ 5262.227477] scsi 14:0:0:0: Direct-Access     Seagate  FreeAgent        102D PQ: 0 ANSI: 4
[ 5262.227936] sd 14:0:0:0: Attached scsi generic sg3 type 0
[ 5273.458856] sd 14:0:0:0: [sdc] 2930277168 512-byte hardware sectors: (1.50 TB/1.36 TiB)
[ 5273.460121] sd 14:0:0:0: [sdc] Write Protect is off
[ 5273.460126] sd 14:0:0:0: [sdc] Mode Sense: 1c 00 00 00
[ 5273.460130] sd 14:0:0:0: [sdc] Assuming drive cache: write through
[ 5273.475750] sd 14:0:0:0: [sdc] Assuming drive cache: write through
[ 5273.488125]  sdc: sdc1
[ 5273.541147] sd 14:0:0:0: [sdc] Attached SCSI disk


diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..db60e96 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1402,7 +1402,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 
 	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
 			(buffer[6] << 8) | buffer[7];
-	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
+	lba =	((sector_t)buffer[0] << 24) | (buffer[1] << 16) |
 		(buffer[2] << 8) | buffer[3];
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {


-- Dave

Re: [RFC][PATCH] fix sign extension with 1.5TB usb-storage LBD=y

[Al Viro]

On Tue, Apr 21, 2009 at 01:52:54PM -0700, Dave Hansen wrote:
> This is with current git as of this morning, which is at v2.6.30-rc2.
> 
> I have a 1.5TB USB device which gets a bit angry when I plug it in.  It
> ends up with a scsi_disk->capacity of ffffffffaea87b30.  I tracked it
> down to the lba calculation in read_capacity_10():
> 
> 	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
>  		(buffer[2] << 8) | buffer[3];
> 
> lba is getting all 0xf's in its high 32 bits.  It seems odd that this
> would happen since 'buffer' is an 'unsigned char', but that is
> apparently what is going on.

Nothing odd here; you get unsigned char promoted to int (since the entire
range fits into the range of int), then calculations are carried within
int and in the end you have (overflown into negative) int sign-extended
into u64.

Solution is correct, but perhaps use of get_unaligned_be32(buffer) would be
better.

Re: [RFC][PATCH] fix sign extension with 1.5TB usb-storage LBD=y

[Matthew Wilcox]

On Tue, Apr 21, 2009 at 01:52:54PM -0700, Dave Hansen wrote:
> This is with current git as of this morning, which is at v2.6.30-rc2.
> 
> I have a 1.5TB USB device which gets a bit angry when I plug it in.  It
> ends up with a scsi_disk->capacity of ffffffffaea87b30.  I tracked it
> down to the lba calculation in read_capacity_10():
> 
> 	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
>  		(buffer[2] << 8) | buffer[3];
> 
> lba is getting all 0xf's in its high 32 bits.  It seems odd that this
> would happen since 'buffer' is an 'unsigned char', but that is
> apparently what is going on.  Note that this isn't an issue 32-bit
> kernels compiled with CONFIG_LBD=n since there's no more bits into which
> the sign could be extended.

I think I know ... unsigned char gets promoted to signed int since it will
fit.  then signed int gets cast to unsigned long long, sign-extending.  C's
promotion rules have always felt a bit wacky to me.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3fcb64b..db60e96 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1402,7 +1402,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  
>  	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
>  			(buffer[6] << 8) | buffer[7];
> -	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
> +	lba =	((sector_t)buffer[0] << 24) | (buffer[1] << 16) |
>  		(buffer[2] << 8) | buffer[3];

this certainly fixes your problem.  I'd prefer this patch instead, just
because I find the cast unaesthetic ...

----

Fix READ CAPACITY 10 with drives over 1TB

Shifting an unsigned char implicitly casts it to a signed int.  This
caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
which was not supported by at least one drive.  Making 'lba' an unsigned
int ensures that sign extension will not occur.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..c856b1b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1373,7 +1373,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	int sense_valid = 0;
 	int the_result;
 	int retries = 3;
-	sector_t lba;
+	unsigned lba;
 	unsigned sector_size;
 
 	do {
@@ -1413,7 +1413,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		return -EOVERFLOW;
 	}
 
-	sdkp->capacity = lba + 1;
+	sdkp->capacity = (sector_t)lba + 1;
 	return sector_size;
 }
 

-- 
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] fix sign extension with 1.5TB usb-storage LBD=y

[Randy Dunlap]

Matthew Wilcox wrote:
> On Tue, Apr 21, 2009 at 01:52:54PM -0700, Dave Hansen wrote:
>> This is with current git as of this morning, which is at v2.6.30-rc2.
>>
>> I have a 1.5TB USB device which gets a bit angry when I plug it in.  It
>> ends up with a scsi_disk->capacity of ffffffffaea87b30.  I tracked it
>> down to the lba calculation in read_capacity_10():
>>
>> 	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
>>  		(buffer[2] << 8) | buffer[3];
>>
>> lba is getting all 0xf's in its high 32 bits.  It seems odd that this
>> would happen since 'buffer' is an 'unsigned char', but that is
>> apparently what is going on.  Note that this isn't an issue 32-bit
>> kernels compiled with CONFIG_LBD=n since there's no more bits into which
>> the sign could be extended.
> 
> I think I know ... unsigned char gets promoted to signed int since it will
> fit.  then signed int gets cast to unsigned long long, sign-extending.  C's
> promotion rules have always felt a bit wacky to me.
> 
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 3fcb64b..db60e96 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1402,7 +1402,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>>  
>>  	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
>>  			(buffer[6] << 8) | buffer[7];
>> -	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
>> +	lba =	((sector_t)buffer[0] << 24) | (buffer[1] << 16) |
>>  		(buffer[2] << 8) | buffer[3];
> 
> this certainly fixes your problem.  I'd prefer this patch instead, just
> because I find the cast unaesthetic ...
> 
> ----
> 
> Fix READ CAPACITY 10 with drives over 1TB
> 
> Shifting an unsigned char implicitly casts it to a signed int.  This
> caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
> which was not supported by at least one drive.  Making 'lba' an unsigned
> int ensures that sign extension will not occur.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3fcb64b..c856b1b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1373,7 +1373,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	int sense_valid = 0;
>  	int the_result;
>  	int retries = 3;
> -	sector_t lba;
> +	unsigned lba;

sector_t is either unsigned long or u64, depending on CONFIG_LBD.
Are you saying (implying) that the higher-order bits of it don't matter here?
If so, I'd just like for that to be clear(er).


>  	unsigned sector_size;
>  
>  	do {
> @@ -1413,7 +1413,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		return -EOVERFLOW;
>  	}
>  
> -	sdkp->capacity = lba + 1;
> +	sdkp->capacity = (sector_t)lba + 1;
>  	return sector_size;
>  }
>  
> 

Re: [RFC][PATCH] fix sign extension with 1.5TB usb-storage LBD=y

[Matthew Wilcox]

On Tue, Apr 21, 2009 at 02:29:54PM -0700, Randy Dunlap wrote:
> > Fix READ CAPACITY 10 with drives over 1TB
> > 
> > Shifting an unsigned char implicitly casts it to a signed int.  This
> > caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
> > which was not supported by at least one drive.  Making 'lba' an unsigned
> > int ensures that sign extension will not occur.
> > 
> > Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 3fcb64b..c856b1b 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -1373,7 +1373,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >  	int sense_valid = 0;
> >  	int the_result;
> >  	int retries = 3;
> > -	sector_t lba;
> > +	unsigned lba;
> 
> sector_t is either unsigned long or u64, depending on CONFIG_LBD.
> Are you saying (implying) that the higher-order bits of it don't matter here?
> If so, I'd just like for that to be clear(er).

We're reading 32 bits of information from the drive response.  The top
32 bits are implicitly zero.

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

[PATCH v2] fix sign extension with 1.5TB usb-storage LBD=y

[Dave Hansen]

Here's a patch implementing Al's suggestion.  Not quite as trivial as
Matthew's, but even nicer aesthetically.  (Description stolen from
Matthew's patch).  I have verified that this fixes my issue.

--

Shifting an unsigned char implicitly casts it to a signed int.  This
caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
which was not supported by at least one drive.  Using the
get_unaligned_be*() helpers keeps us from having to worry about how the
extension might occur.

Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..c50142b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -50,6 +50,7 @@
 #include <linux/string_helpers.h>
 #include <linux/async.h>
 #include <asm/uaccess.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -1344,12 +1345,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		return -EINVAL;
 	}
 
-	sector_size =	(buffer[8] << 24) | (buffer[9] << 16) |
-			(buffer[10] << 8) | buffer[11];
-	lba =  (((u64)buffer[0] << 56) | ((u64)buffer[1] << 48) |
-		((u64)buffer[2] << 40) | ((u64)buffer[3] << 32) |
-		((u64)buffer[4] << 24) | ((u64)buffer[5] << 16) |
-		((u64)buffer[6] << 8) | (u64)buffer[7]);
+	sector_size = get_unaligned_be32(&buffer[8]);
+	lba =  get_unaligned_be64(&buffer[0]);
 
 	sd_read_protection_type(sdkp, buffer);
 
@@ -1400,10 +1397,8 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		return -EINVAL;
 	}
 
-	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
-			(buffer[6] << 8) | buffer[7];
-	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
-		(buffer[2] << 8) | buffer[3];
+	sector_size = get_unaligned_be32(&buffer[4]);
+	lba = get_unaligned_be32(&buffer[0]);
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "


-- Dave

Re: [PATCH v2] fix sign extension with 1.5TB usb-storage LBD=y

[Matthew Wilcox]

On Tue, Apr 21, 2009 at 03:00:10PM -0700, Dave Hansen wrote:
> Here's a patch implementing Al's suggestion.  Not quite as trivial as
> Matthew's, but even nicer aesthetically.  (Description stolen from
> Matthew's patch).  I have verified that this fixes my issue.

I prefer this approach, but I'm equally happy with whichever patch
James chooses.

> Shifting an unsigned char implicitly casts it to a signed int.  This
> caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
> which was not supported by at least one drive.  Using the
> get_unaligned_be*() helpers keeps us from having to worry about how the
> extension might occur.
> 
> Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>

Reviewed-by: Matthew Wilcox <willy@linux.intel.com>

> @@ -1344,12 +1345,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		return -EINVAL;
>  	}
>  
> -	sector_size =	(buffer[8] << 24) | (buffer[9] << 16) |
> -			(buffer[10] << 8) | buffer[11];
> -	lba =  (((u64)buffer[0] << 56) | ((u64)buffer[1] << 48) |
> -		((u64)buffer[2] << 40) | ((u64)buffer[3] << 32) |
> -		((u64)buffer[4] << 24) | ((u64)buffer[5] << 16) |
> -		((u64)buffer[6] << 8) | (u64)buffer[7]);
> +	sector_size = get_unaligned_be32(&buffer[8]);
> +	lba =  get_unaligned_be64(&buffer[0]);

The smallest of quibbles ... you have an excess space after the = ...

-- 
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: [PATCH v2] fix sign extension with 1.5TB usb-storage LBD=y

[Dave Hansen]

Here's a patch implementing Al's suggestion.  Not quite as trivial as
Matthew's, but even nicer aesthetically.  (Description stolen from
Matthew's patch).  I have verified that this fixes my issue.

v3 fixed extra whitespace.

--

Shifting an unsigned char implicitly casts it to a signed int.  This
caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
which was not supported by at least one drive.  Using the
get_unaligned_be*() helpers keeps us from having to worry about how the
extension might occur.

Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>
Reviewed-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..c50142b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -50,6 +50,7 @@
 #include <linux/string_helpers.h>
 #include <linux/async.h>
 #include <asm/uaccess.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -1344,12 +1345,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		return -EINVAL;
 	}
 
-	sector_size =	(buffer[8] << 24) | (buffer[9] << 16) |
-			(buffer[10] << 8) | buffer[11];
-	lba =  (((u64)buffer[0] << 56) | ((u64)buffer[1] << 48) |
-		((u64)buffer[2] << 40) | ((u64)buffer[3] << 32) |
-		((u64)buffer[4] << 24) | ((u64)buffer[5] << 16) |
-		((u64)buffer[6] << 8) | (u64)buffer[7]);
+	sector_size = get_unaligned_be32(&buffer[8]);
+	lba = get_unaligned_be64(&buffer[0]);
 
 	sd_read_protection_type(sdkp, buffer);
 
@@ -1400,10 +1397,8 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		return -EINVAL;
 	}
 
-	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
-			(buffer[6] << 8) | buffer[7];
-	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
-		(buffer[2] << 8) | buffer[3];
+	sector_size = get_unaligned_be32(&buffer[4]);
+	lba = get_unaligned_be32(&buffer[0]);
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "

Re: [PATCH v2] fix sign extension with 1.5TB usb-storage LBD=y

[Boaz Harrosh]

On 04/22/2009 01:00 AM, Dave Hansen wrote:
> Here's a patch implementing Al's suggestion.  Not quite as trivial as
> Matthew's, but even nicer aesthetically.  (Description stolen from
> Matthew's patch).  I have verified that this fixes my issue.
> 
> --
> 
> Shifting an unsigned char implicitly casts it to a signed int.  This
> caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
> which was not supported by at least one drive.  Using the
> get_unaligned_be*() helpers keeps us from having to worry about how the
> extension might occur.
> 
> Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3fcb64b..c50142b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -50,6 +50,7 @@
>  #include <linux/string_helpers.h>
>  #include <linux/async.h>
>  #include <asm/uaccess.h>
> +#include <asm/unaligned.h>
>  
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -1344,12 +1345,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		return -EINVAL;
>  	}
>  
> -	sector_size =	(buffer[8] << 24) | (buffer[9] << 16) |
> -			(buffer[10] << 8) | buffer[11];
> -	lba =  (((u64)buffer[0] << 56) | ((u64)buffer[1] << 48) |
> -		((u64)buffer[2] << 40) | ((u64)buffer[3] << 32) |
> -		((u64)buffer[4] << 24) | ((u64)buffer[5] << 16) |
> -		((u64)buffer[6] << 8) | (u64)buffer[7]);
> +	sector_size = get_unaligned_be32(&buffer[8]);
> +	lba =  get_unaligned_be64(&buffer[0]);

These are actually aligned access it might be worth sacrificing a cast
to be32/64 for sake of speed.

>  
>  	sd_read_protection_type(sdkp, buffer);
>  
> @@ -1400,10 +1397,8 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		return -EINVAL;
>  	}
>  
> -	sector_size =	(buffer[4] << 24) | (buffer[5] << 16) |
> -			(buffer[6] << 8) | buffer[7];
> -	lba =	(buffer[0] << 24) | (buffer[1] << 16) |
> -		(buffer[2] << 8) | buffer[3];
> +	sector_size = get_unaligned_be32(&buffer[4]);
> +	lba = get_unaligned_be32(&buffer[0]);
>  

Here too, both are actually aligned.

>  	if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {
>  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
> 
> 
> -- Dave
> 

I'm not 100% convinced just a thought.
Boaz

Re: [PATCH v2] fix sign extension with 1.5TB usb-storage LBD=y

[Matthew Wilcox]

On Wed, Apr 22, 2009 at 10:32:59AM +0300, Boaz Harrosh wrote:
> These are actually aligned access it might be worth sacrificing a cast
> to be32/64 for sake of speed.

"for sake of speed"?  How often do you think we ask a device how large
it is?  How much overhead do you think is incurred by the unaligned code
if the data happens to be aligned?

-- 
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: [PATCH v2] fix sign extension with 1.5TB usb-storage LBD=y

[Boaz Harrosh]

On 04/22/2009 02:09 PM, Matthew Wilcox wrote:
> On Wed, Apr 22, 2009 at 10:32:59AM +0300, Boaz Harrosh wrote:
>> These are actually aligned access it might be worth sacrificing a cast
>> to be32/64 for sake of speed.
> 
> "for sake of speed"?  How often do you think we ask a device how large
> it is?  

OK, that was the wrong choice of words, on my part. I meant for sake of
"nobleness". I calculated as a programmer that these are aligned do I make
the extra effort of stating that in code, or I get lazy because it does
not matter?

> How much overhead do you think is incurred by the unaligned code
> if the data happens to be aligned?
> 

Well for BE systems we are already order of magnitude faster by just
using the accessors, so I guess we are already well in the "plus" ;)

This is such a small matter, sorry to bother you about it.
Just that it's a programming style I'm constantly debating with myself,
feel free to ignore it.

The patch looks very good to me as it is.

Thanks
Boaz

Re: [RFC][PATCH] fix sign extension with 1.5TB usb-storage LBD=y

[Dave Hansen]

On Tue, 2009-04-21 at 15:18 -0600, Matthew Wilcox wrote:
> 
> this certainly fixes your problem.  I'd prefer this patch instead, just
> because I find the cast unaesthetic ...
> 
> ----
> 
> Fix READ CAPACITY 10 with drives over 1TB
> 
> Shifting an unsigned char implicitly casts it to a signed int.  This
> caused 'lba' to sign-extend and Linux would then try READ CAPACITY 16
> which was not supported by at least one drive.  Making 'lba' an
> unsigned
> int ensures that sign extension will not occur.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3fcb64b..c856b1b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1373,7 +1373,7 @@ static int read_capacity_10(struct scsi_disk
> *sdkp, struct scsi_device *sdp,
>         int sense_valid = 0;
>         int the_result;
>         int retries = 3;
> -       sector_t lba;
> +       unsigned lba;
>         unsigned sector_size;
> 
>         do {
> @@ -1413,7 +1413,7 @@ static int read_capacity_10(struct scsi_disk
> *sdkp, struct scsi_device *sdp,
>                 return -EOVERFLOW;
>         }
> 
> -       sdkp->capacity = lba + 1;
> +       sdkp->capacity = (sector_t)lba + 1;
>         return sector_size;
>  }

That's fine with me too.  This patch fixes my issue as well.

Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>

-- Dave