Patch "sd: Disable support for 256 byte/sector disks" has been added to the 4.0-stable tree

PATCH [email protected] 5 participants 5 messages Jun 3, 2015

Subsystems (2)

Stable branch SCSI

lore

Share & tools

lore

[email protected]

author maintainer Applied

2015-06-03 06:35 UTC #0 lore raw

This is a note to let you know that I've just added the patch titled

sd: Disable support for 256 byte/sector disks

to the 4.0-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
sd-disable-support-for-256-byte-sector-disks.patch
and it can be found in the queue-4.0 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From 74856fbf441929918c49ff262ace9835048e4e6a Mon Sep 17 00:00:00 2001

From: Mark Hounschell <[email protected]>
Date: Wed, 13 May 2015 10:49:09 +0200
Subject: sd: Disable support for 256 byte/sector disks

From: Mark Hounschell <[email protected]>

commit 74856fbf441929918c49ff262ace9835048e4e6a upstream.

256 bytes per sector support has been broken since 2.6.X, and no-one stepped up to fix this.
So disable support for it.

Signed-off-by Mark Hounschell <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

Signed-off-by James Bottomley <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/scsi/sd.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

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

@@ -1624,6 +1624,7 @@ static unsigned int sd_completed_bytes(s
 {
 	u64 start_lba = blk_rq_pos(scmd->request);
 	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
+	u64 factor = scmd->device->sector_size / 512;
 	u64 bad_lba;
 	int info_valid;
 	/*

@@ -1645,16 +1646,9 @@ static unsigned int sd_completed_bytes(s
 	if (scsi_bufflen(scmd) <= scmd->device->sector_size)
 		return 0;
 
-	if (scmd->device->sector_size < 512) {
-		/* only legitimate sector_size here is 256 */
-		start_lba <<= 1;
-		end_lba <<= 1;
-	} else {
-		/* be careful ... don't want any overflows */
-		unsigned int factor = scmd->device->sector_size / 512;
-		do_div(start_lba, factor);
-		do_div(end_lba, factor);
-	}
+	/* be careful ... don't want any overflows */
+	do_div(start_lba, factor);
+	do_div(end_lba, factor);
 
 	/* The bad lba was reported incorrectly, we have no idea where
 	 * the error is.

@@ -2212,8 +2206,7 @@ got_data:
 	if (sector_size != 512 &&
 	    sector_size != 1024 &&
 	    sector_size != 2048 &&
-	    sector_size != 4096 &&
-	    sector_size != 256) {
+	    sector_size != 4096) {
 		sd_printk(KERN_NOTICE, sdkp, "Unsupported sector size %d.\n",
 			  sector_size);
 		/*

@@ -2268,8 +2261,6 @@ got_data:
 		sdkp->capacity <<= 2;
 	else if (sector_size == 1024)
 		sdkp->capacity <<= 1;
-	else if (sector_size == 256)
-		sdkp->capacity >>= 1;
 
 	blk_queue_physical_block_size(sdp->request_queue,
 				      sdkp->physical_block_size);

Patches currently in stable-queue which might be from [email protected] are

queue-4.0/sd-disable-support-for-256-byte-sector-disks.patch

Jiri Slaby

2015-06-03 09:29 UTC #1 lore raw

Quoted 36 lines · On 06/03/2015, 08:35 AM, [email protected] wrote:

From 74856fbf441929918c49ff262ace9835048e4e6a Mon Sep 17 00:00:00 2001
From: Mark Hounschell <[email protected]>
Date: Wed, 13 May 2015 10:49:09 +0200
Subject: sd: Disable support for 256 byte/sector disks
From: Mark Hounschell <[email protected]>
commit 74856fbf441929918c49ff262ace9835048e4e6a upstream.
256 bytes per sector support has been broken since 2.6.X, and no-one stepped up to fix this.
So disable support for it.
Signed-off-by Mark Hounschell <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>
Signed-off-by James Bottomley <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/scsi/sd.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/scsi/sd.c +1 -0
@@ -1624,6 +1624,7 @@ static unsigned int sd_completed_bytes(s
 {
 	u64 start_lba = blk_rq_pos(scmd->request);
 	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
+	u64 factor = scmd->device->sector_size / 512;
 	u64 bad_lba;
 	int info_valid;
 	/*
@@ -1645,16 +1646,9 @@ static unsigned int sd_completed_bytes(s
 	if (scsi_bufflen(scmd) <= scmd->device->sector_size)
 		return 0;
 

drivers/scsi/sd.c static unsigned int sd_completed_bytes(s · line 1657

Quoted 10 lines

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

-	if (scmd->device->sector_size < 512) {
-		/* only legitimate sector_size here is 256 */
-		start_lba <<= 1;
-		end_lba <<= 1;
-	} else {
-		/* be careful ... don't want any overflows */
-		unsigned int factor = scmd->device->sector_size / 512;
-		do_div(start_lba, factor);
-		do_div(end_lba, factor);
-	}

Hmm, you do 'unsigned int' -> 'u64' switch of factor type here. But this commit:
commit ef80d1e18b014af08741cf688e3fdda1fb71363f

Author: Geert Uytterhoeven <[email protected]>
Date:   Mon Nov 4 10:21:05 2013 +0100

[SCSI] sd: Do not call do_div() with a 64-bit divisor

did the switch in the opposite direction deliberately.

So why did you do the change, given sector_size is uint?

thanks,

Signature · 2 lines

js
suse labs

James Bottomley

2015-06-03 13:37 UTC #2 lore raw

Quoted 51 lines · Jiri Slaby

Quoted 36 lines · On 06/03/2015, 08:35 AM, [email protected] wrote:
From 74856fbf441929918c49ff262ace9835048e4e6a Mon Sep 17 00:00:00 2001
From: Mark Hounschell <[email protected]>
Date: Wed, 13 May 2015 10:49:09 +0200
Subject: sd: Disable support for 256 byte/sector disks
From: Mark Hounschell <[email protected]>
commit 74856fbf441929918c49ff262ace9835048e4e6a upstream.
256 bytes per sector support has been broken since 2.6.X, and no-one stepped up to fix this.
So disable support for it.
Signed-off-by Mark Hounschell <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>
Signed-off-by James Bottomley <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/scsi/sd.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/scsi/sd.c +1 -0
@@ -1624,6 +1624,7 @@ static unsigned int sd_completed_bytes(s
 {
 	u64 start_lba = blk_rq_pos(scmd->request);
 	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
+	u64 factor = scmd->device->sector_size / 512;
 	u64 bad_lba;
 	int info_valid;
 	/*
@@ -1645,16 +1646,9 @@ static unsigned int sd_completed_bytes(s
 	if (scsi_bufflen(scmd) <= scmd->device->sector_size)
 		return 0;
 
drivers/scsi/sd.c static unsigned int sd_completed_bytes(s · line 1657
Quoted 10 lines
drivers/scsi/sd.c +0 ~~-10~~
-	if (scmd->device->sector_size < 512) {
-		/* only legitimate sector_size here is 256 */
-		start_lba <<= 1;
-		end_lba <<= 1;
-	} else {
-		/* be careful ... don't want any overflows */
-		unsigned int factor = scmd->device->sector_size / 512;
-		do_div(start_lba, factor);
-		do_div(end_lba, factor);
-	}
Hmm, you do 'unsigned int' -> 'u64' switch of factor type here. But this commit:
commit ef80d1e18b014af08741cf688e3fdda1fb71363f

Quoted 8 lines

Author: Geert Uytterhoeven <[email protected]>
Date:   Mon Nov 4 10:21:05 2013 +0100
[SCSI] sd: Do not call do_div() with a 64-bit divisor
did the switch in the opposite direction deliberately.
So why did you do the change, given sector_size is uint?

Primarily because no-one spotted the reversal and none of the static checkers warns about it.

This is the trivial fix, but we should do something about the checkers.

James

---

drivers/scsi/sd.c +1 -1

index 3b2fcb4..3e137dd 100644
@@ -1600,7 +1600,7 @@ static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
 {
 	u64 start_lba = blk_rq_pos(scmd->request);
 	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
-	u64 factor = scmd->device->sector_size / 512;
+	unsigned int factor = scmd->device->sector_size / 512;
 	u64 bad_lba;
 	int info_valid;
 	/*

James Bottomley

maintainer

2015-06-03 13:54 UTC #3 lore raw

Quoted 57 lines · James Bottomley

Quoted 56 lines · Jiri Slaby
Quoted 36 lines · On 06/03/2015, 08:35 AM, [email protected] wrote:
From 74856fbf441929918c49ff262ace9835048e4e6a Mon Sep 17 00:00:00 2001
From: Mark Hounschell <[email protected]>
Date: Wed, 13 May 2015 10:49:09 +0200
Subject: sd: Disable support for 256 byte/sector disks
From: Mark Hounschell <[email protected]>
commit 74856fbf441929918c49ff262ace9835048e4e6a upstream.
256 bytes per sector support has been broken since 2.6.X, and no-one stepped up to fix this.
So disable support for it.
Signed-off-by Mark Hounschell <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>
Signed-off-by James Bottomley <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/scsi/sd.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/scsi/sd.c +1 -0
@@ -1624,6 +1624,7 @@ static unsigned int sd_completed_bytes(s
 {
 	u64 start_lba = blk_rq_pos(scmd->request);
 	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
+	u64 factor = scmd->device->sector_size / 512;
 	u64 bad_lba;
 	int info_valid;
 	/*
@@ -1645,16 +1646,9 @@ static unsigned int sd_completed_bytes(s
 	if (scsi_bufflen(scmd) <= scmd->device->sector_size)
 		return 0;
 
drivers/scsi/sd.c static unsigned int sd_completed_bytes(s · line 1657
Quoted 10 lines
drivers/scsi/sd.c +0 ~~-10~~
-	if (scmd->device->sector_size < 512) {
-		/* only legitimate sector_size here is 256 */
-		start_lba <<= 1;
-		end_lba <<= 1;
-	} else {
-		/* be careful ... don't want any overflows */
-		unsigned int factor = scmd->device->sector_size / 512;
-		do_div(start_lba, factor);
-		do_div(end_lba, factor);
-	}
Hmm, you do 'unsigned int' -> 'u64' switch of factor type here. But this commit:
commit ef80d1e18b014af08741cf688e3fdda1fb71363f
Author: Geert Uytterhoeven <[email protected]>
Date:   Mon Nov 4 10:21:05 2013 +0100
[SCSI] sd: Do not call do_div() with a 64-bit divisor

Quoted 8 lines

Quoted 3 lines
did the switch in the opposite direction deliberately.
So why did you do the change, given sector_size is uint?
Primarily because no-one spotted the reversal and none of the static checkers warns about it.
This is the trivial fix, but we should do something about the checkers.

Actually, I think there's no problem: all the architectures now have the correct conversion of the base argument.

Author: Andreas Schwab <[email protected]>
Date:   Fri Aug 9 15:14:08 2013 +0200

m68k: Truncate base in do_div()

Fixed this in m68k, which was the last one, so it looks like there's no issue and ef80d1e18b014af08741cf688e3fdda1fb71363f was pointless.

James

Geert Uytterhoeven

2015-06-03 14:37 UTC #4 lore raw

Quoted 70 lines · James Bottomley

Quoted 57 lines · James Bottomley
Quoted 56 lines · Jiri Slaby
Quoted 36 lines · On 06/03/2015, 08:35 AM, [email protected] wrote:
From 74856fbf441929918c49ff262ace9835048e4e6a Mon Sep 17 00:00:00 2001
From: Mark Hounschell <[email protected]>
Date: Wed, 13 May 2015 10:49:09 +0200
Subject: sd: Disable support for 256 byte/sector disks
From: Mark Hounschell <[email protected]>
commit 74856fbf441929918c49ff262ace9835048e4e6a upstream.
256 bytes per sector support has been broken since 2.6.X, and no-one stepped up to fix this.
So disable support for it.
Signed-off-by Mark Hounschell <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>
Signed-off-by James Bottomley <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/scsi/sd.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/scsi/sd.c +1 -0
@@ -1624,6 +1624,7 @@ static unsigned int sd_completed_bytes(s
 {
  u64 start_lba = blk_rq_pos(scmd->request);
  u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
+ u64 factor = scmd->device->sector_size / 512;
  u64 bad_lba;
  int info_valid;
  /*
@@ -1645,16 +1646,9 @@ static unsigned int sd_completed_bytes(s
  if (scsi_bufflen(scmd) <= scmd->device->sector_size)
          return 0;
 
drivers/scsi/sd.c static unsigned int sd_completed_bytes(s · line 1657
Quoted 10 lines
drivers/scsi/sd.c +0 ~~-10~~
- if (scmd->device->sector_size < 512) {
-         /* only legitimate sector_size here is 256 */
-         start_lba <<= 1;
-         end_lba <<= 1;
- } else {
-         /* be careful ... don't want any overflows */
-         unsigned int factor = scmd->device->sector_size / 512;
-         do_div(start_lba, factor);
-         do_div(end_lba, factor);
- }
Hmm, you do 'unsigned int' -> 'u64' switch of factor type here. But this commit:
commit ef80d1e18b014af08741cf688e3fdda1fb71363f
Author: Geert Uytterhoeven <[email protected]>
Date:   Mon Nov 4 10:21:05 2013 +0100
[SCSI] sd: Do not call do_div() with a 64-bit divisor
Quoted 8 lines
Quoted 3 lines
did the switch in the opposite direction deliberately.
So why did you do the change, given sector_size is uint?
Primarily because no-one spotted the reversal and none of the static checkers warns about it.
This is the trivial fix, but we should do something about the checkers.
Actually, I think there's no problem: all the architectures now have the correct conversion of the base argument.

Quoted 7 lines

Author: Andreas Schwab <[email protected]>
Date:   Fri Aug 9 15:14:08 2013 +0200
m68k: Truncate base in do_div()
Fixed this in m68k, which was the last one, so it looks like there's no issue and ef80d1e18b014af08741cf688e3fdda1fb71363f was pointless.

FWIW, "[PATCH] mn10300: Truncate base in do_div()"
(http://lkml.iu.edu/hypermail/linux/kernel/1308.2/03544.html)
hasn't been applied yet. But as mn10300 is little endian, it shouldn't suffer from the bug like m68k did.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds