fix sign extension with 1.5TB usb-storage LBD=y

RFC PATCH Superseded Dave Hansen 5 participants 12 messages Apr 21, 2009 – Apr 22, 2009 SCSI

lore

Share & tools

lore

Dave Hansen

author

2009-04-21 20:53 UTC #0 lore raw

RFC PATCH 1/1 fix sign extension with 1.5TB usb-storage LBD=y

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

drivers/scsi/sd.c +1 -1

index 3fcb64b..db60e96 100644
@@ -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

Al Viro

2009-04-21 21:01 UTC #1 lore raw

Quoted 6 lines · 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():

Quoted 6 lines

	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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Matthew Wilcox

2009-04-21 21:18 UTC #2 lore raw

In reply to Dave Hansen

Quoted 6 lines · 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():

Quoted 8 lines

	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.

drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1406

Quoted 11 lines

drivers/scsi/sd.c +1 -1

index 3fcb64b..db60e96 100644
@@ -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 <[email protected]>

drivers/scsi/sd.c +2 -2

index 3fcb64b..c856b1b 100644
@@ -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;
 }

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

Randy Dunlap

2009-04-21 21:29 UTC #3 lore raw

Quoted 51 lines · Matthew Wilcox

Quoted 6 lines · 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():
Quoted 8 lines
	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.
drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1406
Quoted 11 lines
drivers/scsi/sd.c +1 -1
index 3fcb64b..db60e96 100644
@@ -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 <[email protected]>
drivers/scsi/sd.c +0 -0
index 3fcb64b..c856b1b 100644

drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1376

Quoted 5 lines

drivers/scsi/sd.c +1 -1

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

Quoted 13 lines

drivers/scsi/sd.c +1 -1

 	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

2009-04-21 21:31 UTC #4 lore raw

Quoted 9 lines · Randy Dunlap

Quoted 9 lines
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 <[email protected]>

Quoted 14 lines

drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1376
Quoted 10 lines
drivers/scsi/sd.c +1 -1
index 3fcb64b..c856b1b 100644
@@ -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.

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

Dave Hansen

author

2009-04-21 22:00 UTC #5 lore raw

PATCH v2 1/1 fix sign extension with 1.5TB usb-storage LBD=y

In reply to Matthew Wilcox

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 <[email protected]>

drivers/scsi/sd.c +5 ~~-10~~

index 3fcb64b..c50142b 100644
@@ -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

Matthew Wilcox

Reviewed-by on v2 1/1

2009-04-21 23:03 UTC #6 lore raw

Quoted 3 lines · 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.

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

Quoted 7 lines

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 <[email protected]>

Reviewed-by Matthew Wilcox <willy-VuQAYsv1563Yd54FQh9/[email protected]>

drivers/scsi/sd.c static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1349

Quoted 12 lines

drivers/scsi/sd.c +2 -6

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

Signature · 8 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."
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Hansen

author Reviewed-by Matthew Wilcox

2009-04-21 23:43 UTC #7 lore raw

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 <[email protected]>

Reviewed-by Matthew Wilcox <[email protected]>

drivers/scsi/sd.c +5 ~~-10~~

index 3fcb64b..c50142b 100644
@@ -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 "

Boaz Harrosh

2009-04-22 07:32 UTC #8 lore raw

In reply to Dave Hansen

Quoted 28 lines · 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 <[email protected]>
drivers/scsi/sd.c +1 -0
index 3fcb64b..c50142b 100644
@@ -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;

drivers/scsi/sd.c static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1349

Quoted 10 lines

drivers/scsi/sd.c +2 -6

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

Quoted 4 lines

drivers/scsi/sd.c +0 -0

 	sd_read_protection_type(sdkp, buffer);

drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp, · line 1402

Quoted 10 lines

drivers/scsi/sd.c +2 -4

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

drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,

Quoted 6 lines

drivers/scsi/sd.c +0 -1

 	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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Matthew Wilcox

2009-04-22 11:09 UTC #9 lore raw

Quoted 2 lines · Boaz Harrosh

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?

Signature · 8 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."
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Boaz Harrosh

2009-04-22 11:27 UTC #10 lore raw

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Boaz Harrosh
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?

Quoted 3 lines

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

Dave Hansen

author

2009-04-21 22:49 UTC #11 lore raw

In reply to Matthew Wilcox

Quoted 31 lines · Matthew Wilcox

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-VuQAYsv1563Yd54FQh9/[email protected]>
drivers/scsi/sd.c +1 -1
index 3fcb64b..c856b1b 100644
@@ -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 {

drivers/scsi/sd.c static int read_capacity_10(struct scsi_disk · line 1419

Quoted 8 lines

drivers/scsi/sd.c +1 -1

@@ -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 <[email protected]>

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html