Advanced Format SAT devices show incorrect physical block size

Commit Message

Alan Stern Jan. 11, 2017, 3:23 p.m. UTC

On Tue, 10 Jan 2017, James Bottomley wrote:

> On Tue, 2017-01-10 at 16:00 -0500, Alan Stern wrote:
> > In theory, I suppose we could change the kernel so that it would 
> > default to READ CAPACITY(16) for devices that report a SCSI level >= 
> > 3, or something along those lines.  In general we hesitate to make
> > changes of this sort, because they almost always end up breaking 
> > _some_ devices -- and if that happens then the change is reverted, 
> > with no exceptions.  Linus has a very strict rule about not breaking 
> > working systems.
> 
> You shouldn't have to change anything: it already does (otherwise how
> else would we detect physical exponent for proper SCSI devices) see
> sd.c:sd_try_rc16_first().  It always returns false for USB because you
> set sdev->try_rc_10_first

In fact, this approach probably won't work.  See Bugzilla entries
#43265 and #43391.  The devices in those reports claimed to be ANSI
level 4, but they failed anyway.

If you guys want to try the quirk flag, you can apply the patch below.  
Then set the usb-storage module parameter quirks=vvvv:pppp:k where
vvvv and pppp are the Vendor and Product ID codes for your device (as 4 
hex digits).

In the long run, however, this is not a viable approach.  We'd be 
better off with an explicit blacklist.

Alan Stern




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

Comments

Pali Rohár Jan. 29, 2017, 5:18 p.m. UTC | #1

On Wednesday 11 January 2017 16:23:29 Alan Stern wrote:
> On Tue, 10 Jan 2017, James Bottomley wrote:
> > On Tue, 2017-01-10 at 16:00 -0500, Alan Stern wrote:
> > > In theory, I suppose we could change the kernel so that it would
> > > default to READ CAPACITY(16) for devices that report a SCSI level
> > > >= 3, or something along those lines.  In general we hesitate to
> > > make changes of this sort, because they almost always end up
> > > breaking _some_ devices -- and if that happens then the change
> > > is reverted, with no exceptions.  Linus has a very strict rule
> > > about not breaking working systems.
> > 
> > You shouldn't have to change anything: it already does (otherwise
> > how else would we detect physical exponent for proper SCSI
> > devices) see sd.c:sd_try_rc16_first().  It always returns false
> > for USB because you set sdev->try_rc_10_first
> 
> In fact, this approach probably won't work.  See Bugzilla entries
> #43265 and #43391.  The devices in those reports claimed to be ANSI
> level 4, but they failed anyway.

Seems those devices return capacity 0x7F000000000001 or 0xFF000000000001
Maybe there is some error pattern?

> If you guys want to try the quirk flag, you can apply the patch
> below. Then set the usb-storage module parameter quirks=vvvv:pppp:k
> where vvvv and pppp are the Vendor and Product ID codes for your
> device (as 4 hex digits).
> 
> In the long run, however, this is not a viable approach.  We'd be
> better off with an explicit blacklist.

Ok, so what are next steps? I think that explicit blacklist would be 
needed if "bad" devices is less.

How many bug reports were there?

> Alan Stern
> 
> 
> 
> Index: usb-4.x/drivers/usb/storage/usb.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/storage/usb.c
> +++ usb-4.x/drivers/usb/storage/usb.c
> @@ -498,7 +498,7 @@ void usb_stor_adjust_quirks(struct usb_d
>  			US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
>  			US_FL_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
>  			US_FL_MAX_SECTORS_240 | US_FL_NO_REPORT_LUNS |
> -			US_FL_ALWAYS_SYNC);
> +			US_FL_ALWAYS_SYNC | US_FL_NEEDS_CAP16);
> 
>  	p = quirks;
>  	while (*p) {
> @@ -551,6 +551,9 @@ void usb_stor_adjust_quirks(struct usb_d
>  		case 'j':
>  			f |= US_FL_NO_REPORT_LUNS;
>  			break;
> +		case 'k':
> +			f |= US_FL_NEEDS_CAP16;
> +			break;
>  		case 'l':
>  			f |= US_FL_NOT_LOCKABLE;
>  			break;

Patch

Index: usb-4.x/drivers/usb/storage/usb.c
===================================================================
--- usb-4.x.orig/drivers/usb/storage/usb.c
+++ usb-4.x/drivers/usb/storage/usb.c
@@ -498,7 +498,7 @@  void usb_stor_adjust_quirks(struct usb_d
 			US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
 			US_FL_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
 			US_FL_MAX_SECTORS_240 | US_FL_NO_REPORT_LUNS |
-			US_FL_ALWAYS_SYNC);
+			US_FL_ALWAYS_SYNC | US_FL_NEEDS_CAP16);
 
 	p = quirks;
 	while (*p) {
@@ -551,6 +551,9 @@  void usb_stor_adjust_quirks(struct usb_d
 		case 'j':
 			f |= US_FL_NO_REPORT_LUNS;
 			break;
+		case 'k':
+			f |= US_FL_NEEDS_CAP16;
+			break;
 		case 'l':
 			f |= US_FL_NOT_LOCKABLE;
 			break;