FUJITA Tomonori wrote:
On Wed, 26 Mar 2008 17:21:30 -0400
Douglas Gilbert <dougg@xxxxxxxxxx> wrote:
FUJITA Tomonori wrote:
This fixes lba calculation bugs in scsi_debug (it might happen only
with over 1TB virtual devices).
This is against scsi-misc. scsi-fixes, 2.6.24, 2.6.23, ... probably
all the versions of scsi_debug have the same bugs too but I guess that
there are few people who play scsi_debug with over virtual 1TB devices
(though this can be cleanly applied to scsi-fixes).
Several LLDs have the own code to do the same calculation. It might be
nice to sweep up them?
=
From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx>
For example, `modprobe scsi_debug virtual_gb=1100` gives:
scsi7 : scsi_debug, version 1.81 [20070104], dev_size_mb=8, opts=0x0
scsi 7:0:0:0: Direct-Access Linux scsi_debug 0004 PQ: 0 ANSI: 5
sd 7:0:0:0: [sdc] 2306867200 512-byte hardware sectors (1181116 MB)
sd 7:0:0:0: [sdc] Write Protect is off
sd 7:0:0:0: [sdc] Mode Sense: 73 00 10 08
sd 7:0:0:0: [sdc] Write cache: enabled, read cache: enabled, supports DPO and FUA
sd 7:0:0:0: [sdc] 2306867200 512-byte hardware sectors (1181116 MB)
sd 7:0:0:0: [sdc] Write Protect is off
sd 7:0:0:0: [sdc] Mode Sense: 73 00 10 08
sd 7:0:0:0: [sdc] Write cache: enabled, read cache: enabled, supports DPO and FUA
sdc: unknown partition table
sd 7:0:0:0: [sdc] Attached SCSI disk
sd 7:0:0:0: Attached scsi generic sg6 type 0
end_request: I/O error, dev sdc, sector 2306867072
Buffer I/O error on device sdc, logical block 288358384
end_request: I/O error, dev sdc, sector 2306867072
Buffer I/O error on device sdc, logical block 288358384
end_request: I/O error, dev sdc, sector 2306867192
(snip)
Note that this converts all the calculations (including the correct
calculations) for unification.
Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx>
Cc: Douglas Gilbert <dougg@xxxxxxxxxx>
Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/scsi/scsi_debug.c | 32 ++++++++++++++++++--------------
1 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 4f4c5b7..46a136a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -251,33 +251,37 @@ static struct bus_type pseudo_lld_bus;
static void get_data_transfer_info(unsigned char *cmd,
unsigned long long *lba, unsigned int *num)
{
- int i;
-
switch (*cmd) {
case WRITE_16:
case READ_16:
- for (*lba = 0, i = 0; i < 8; ++i) {
- if (i > 0)
- *lba <<= 8;
- *lba += cmd[2 + i];
- }
- *num = cmd[13] + (cmd[12] << 8) +
- (cmd[11] << 16) + (cmd[10] << 24);
+ *lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
+ (u64)cmd[7] << 16 | (u64)cmd[6] << 24 |
+ (u64)cmd[5] << 32 | (u64)cmd[4] << 40 |
+ (u64)cmd[3] << 48 | (u64)cmd[2] << 56;
+
+ *num = (u32)cmd[13] | (u32)cmd[12] << 8 | (u32)cmd[11] << 16 |
+ (u32)cmd[10] << 24;
break;
case WRITE_12:
case READ_12:
- *lba = cmd[5] + (cmd[4] << 8) + (cmd[3] << 16) + (cmd[2] << 24);
- *num = cmd[9] + (cmd[8] << 8) + (cmd[7] << 16) + (cmd[6] << 24);
+ *lba = (u32)cmd[5] | (u32)cmd[4] << 8 | (u32)cmd[3] << 16 |
+ (u32)cmd[2] << 24;
+
+ *num = (u32)cmd[9] | (u32)cmd[8] << 8 | (u32)cmd[7] << 16 |
+ (u32)cmd[6] << 24;
break;
case WRITE_10:
case READ_10:
case XDWRITEREAD_10:
- *lba = cmd[5] + (cmd[4] << 8) + (cmd[3] << 16) + (cmd[2] << 24);
- *num = cmd[8] + (cmd[7] << 8);
+ *lba = (u32)cmd[5] | (u32)cmd[4] << 8 | (u32)cmd[3] << 16 |
+ (u32)cmd[2] << 24;
+
+ *num = (u32)cmd[8] | (u32)cmd[7] << 8;
break;
case WRITE_6:
case READ_6:
- *lba = cmd[3] + (cmd[2] << 8) + ((cmd[1] & 0x1f) << 16);
+ *lba = (u32)cmd[3] | (u32)cmd[2] << 8 |
+ (u32)(cmd[1] & 0x1f) << 16;
*num = (0 == cmd[4]) ? 256 : cmd[4];
break;
default:
Signed-off-by: Douglas Gilbert <dougg@xxxxxxxxxx>
Can't actually see the "lba calculation bugs" but I'm happy
to let Tomo do it his way.
Let me make things clear though we've already agreed offline.
Something like the following code doesn't work.
unsigned char *cmd;
unsigned long long *lba;
*lba = cmd[2] << 24;
So surely there are lba calculation bugs.
It gives a surprising result when cmd[2] >= 0x80 .
And it gets worse. On a 64 bit architecture using the LP64
data model, Tomo's above example will fail in a similar
fashion with a 'unsigned long *lba;' declaration.
I just did a small audit of sg3_utils with respect to this
bug and the 'unsigned long' case is more prevalent.
Doug Gilbert
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
