Re: Advanced Format SAT devices show incorrect physical block size

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

On Tue, 10 Jan 2017, Pali Rohár wrote:

> Per Tom Yan suggestion I'm forwarding bug from bugzilla to this ML: 
> https://bugzilla.kernel.org/show_bug.cgi?id=102271

This really should be sent to the linux-scsi mailing list (CC'ed) as 
well as to linux-usb.

> === Dainius Masiliūnas wrote: ===
> 
> When using an Advanced Format drive connected through a SCSI-to-ATA Translation device, the physical 
> block size reported by the kernel (in /sys/class/block/sdc/queue/physical_block_size) is 512 bytes. 
> Whereas hdparm -I and smartctl -a do correctly show the physical block size as 4096 bytes.
> 
> In my case, the drive in question is a HGST HTS541075A9E680 hard drive, connected to my PC through a 
> USB3 HDD enclosure (USB ID 05e3:0731).
> 
> While trying to find more information about this issue, I came across someone else's blog post about 
> the same issue I'm having, which includes some in-depth analysis:
> http://nunix.fr/index.php/linux/7-astuces/65-too-hard-to-be-above-2tb
> 
> It seems that the kernel uses less reliable means to get details about such devices than hdparm 
> does.

Quick summary: READ CAPACITY(10) does not include physical sector size 
information whereas READ CAPACITY(16) does.  But the kernel uses READ 
CAPACITY(10) by default for USB drives, because quite a few of them die 
when given a READ CAPACITY(16) command.

If you can suggest a way to fix this, we'll be glad to hear it.

Alan Stern

> I'm using the kernel that's shipped in openSUSE 13.2.
> 
> === Tom Yan replied: ===
> 
> It is probably because of this:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/scsiglue.c?h=v4.4#n217
> 
> SCSI READ CAPACITY (10) will not return "Logical blocks per physical block exponent" like READ 
> CAPACITY (16) does:
> 
> [tom@localhost ~]$ sudo sg_readcap /dev/sdc
> Read Capacity results:
>    Last logical block address=468862127 (0x1bf244af), Number of blocks=468862128
>    Logical block length=512 bytes
> Hence:
>    Device size: 240057409536 bytes, 228936.6 MiB, 240.06 GB
> 
> [tom@localhost ~]$ sudo sg_readcap -16 /dev/sdc
> Read Capacity results:
>    Protection: prot_en=0, p_type=0, p_i_exponent=0
>    Logical block provisioning: lbpme=0, lbprz=0
>    Last logical block address=468862127 (0x1bf244af), Number of logical blocks=468862128
>    Logical block length=512 bytes
>    Logical blocks per physical block exponent=3 [so physical block length=4096 bytes]
>    Lowest aligned logical block address=0
> Hence:
>    Device size: 240057409536 bytes, 228936.6 MiB, 240.06 GB

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Advanced Format SAT devices show incorrect physical block size

[Dainius Masiliūnas]

Huh, intersting to know. Why would they die on 16 and not on 10? Also,
wouldn't the right way to handle it be to use a quirk for broken
models, then? Since my disk seems to work fine in that regard.

Re: Advanced Format SAT devices show incorrect physical block size

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 660 bytes --]

On Tuesday 10 January 2017 21:02:09 Alan Stern wrote:
> Quick summary: READ CAPACITY(10) does not include physical sector
> size information whereas READ CAPACITY(16) does.  But the kernel
> uses READ CAPACITY(10) by default for USB drives, because quite a
> few of them die when given a READ CAPACITY(16) command.

Ah :-( Are there lot of "broken" devices for creating blacklist?

> If you can suggest a way to fix this, we'll be glad to hear it.

Tom Yan wrote that smartctl/hdparm "works" because they use the SCSI ATA 
PASSTHROUGH command. It is not an option for kernel?

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

On Tue, 10 Jan 2017, Dainius Masiliūnas wrote:

> Huh, intersting to know. Why would they die on 16 and not on 10? Also,

Probably because they are too old to support READ CAPACITY(16) 
correctly.

> wouldn't the right way to handle it be to use a quirk for broken
> models, then? Since my disk seems to work fine in that regard.

There _is_ a quirk for broken models.  However, we don't know how
complete the set of quirk entries is, so we err on the side of caution.


On Tue, 10 Jan 2017, Pali Rohár wrote:

> On Tuesday 10 January 2017 21:02:09 Alan Stern wrote:
> > Quick summary: READ CAPACITY(10) does not include physical sector
> > size information whereas READ CAPACITY(16) does.  But the kernel
> > uses READ CAPACITY(10) by default for USB drives, because quite a
> > few of them die when given a READ CAPACITY(16) command.
>
> Ah :-( Are there lot of "broken" devices for creating blacklist?

I have no idea how many there are.

> > If you can suggest a way to fix this, we'll be glad to hear it.
>
> Tom Yan wrote that smartctl/hdparm "works" because they use the SCSI ATA
> PASSTHROUGH command. It is not an option for kernel?

No, because many devices do not implement SCSI ATA PASSTHROUGH.  
(Consider devices whose underlying technology does not use ATA or SATA,
for example.)  And some of the ones that don't implement it will die if
you try to send them an ATA PASSTHROUGH command.

You have to understand that consumer USB storage really is very
low quality in many cases.  Vendors aim for low cost rather than high 
reliability or correctness.

Alan Stern

Re: Advanced Format SAT devices show incorrect physical block size

[Dainius Masiliūnas]

(I pressed reply instead of reply to all, sorry. Resending this.)

On Tue, Jan 10, 2017 at 8:29 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> There _is_ a quirk for broken models.  However, we don't know how
> complete the set of quirk entries is, so we err on the side of caution.

Then what is it used for? There doesn't seem to be much point in a
blacklist when the functionality is disabled by default to begin with.

And if there's a whitelist, does it mean that we should submit the IDs
of working USB devices for whitelisting? Also, is there a module
parameter to toggle the functionality, to facilitate testing whether
the device works fine or not?

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

On Tue, 10 Jan 2017, Dainius Masiliūnas wrote:

> (I pressed reply instead of reply to all, sorry. Resending this.)
> 
> On Tue, Jan 10, 2017 at 8:29 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > There _is_ a quirk for broken models.  However, we don't know how
> > complete the set of quirk entries is, so we err on the side of caution.
> 
> Then what is it used for? There doesn't seem to be much point in a
> blacklist when the functionality is disabled by default to begin with.

It is used for preventing the kernel from issuing a READ CAPACITY(16) 
command to the device.  Normally the kernel would do this if the reply 
to READ CAPACITY(10) indicated there were more than 2^32 blocks (about 
2 TB).

> And if there's a whitelist, does it mean that we should submit the IDs
> of working USB devices for whitelisting? Also, is there a module
> parameter to toggle the functionality, to facilitate testing whether
> the device works fine or not?

There is no whitelist, but there is a quirk flag for devices that 
require READ CAPACITY(16) instead of READ CAPACITY(10) (because READ 
CAPACITY(10) returns incorrect information).

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.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Advanced Format SAT devices show incorrect physical block size

[Dainius Masiliūnas]

On Tue, Jan 10, 2017 at 9:00 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> It is used for preventing the kernel from issuing a READ CAPACITY(16)
> command to the device.  Normally the kernel would do this if the reply
> to READ CAPACITY(10) indicated there were more than 2^32 blocks (about
> 2 TB).

Ah, OK, that makes sense. Though that's still not a proper solution,
and if anything it's confusing for the users: if they plug in a >2 TiB
drive and it breaks, the user will assume that it's the fault of the
drive and not the USB case, or that the case is incapable of handling
such large drives, whereas the true reason is completely different.

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

Well, the problems are 1) we don't really know how many such
misbehaving drives there are, since only those with >2 TiB drives
experience it, thus it's hard to know what effect such change would
have, and 2) there is no way to make the kernel output the true value
even if you know your drive is working fine.

And when a drive breaks due to the 16 command, is there a way to
revive it, perhaps by trying to reconnect? That would allow the kernel
to try it first, see that it failed after that command was sent, then
issue a warning in the log saying that this device should have a
quirk, reset and continue with the quirk applied.

Also, is `sg_readcap -16` a proper test to determine whether the
device works as expected?

Re: Advanced Format SAT devices show incorrect physical block size

[James Bottomley]

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

James

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Advanced Format SAT devices show incorrect physical block size

[Pali Rohár]

On Tuesday 10 January 2017 14:12:25 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

So.. what does it mean? Can we enable READ CAPACITY(16) for some USB
devices?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: Advanced Format SAT devices show incorrect physical block size

[Pali Rohár]

On Tuesday 10 January 2017 15:29:23 Alan Stern wrote:
> > Tom Yan wrote that smartctl/hdparm "works" because they use the SCSI ATA
> > PASSTHROUGH command. It is not an option for kernel?
> 
> No, because many devices do not implement SCSI ATA PASSTHROUGH.  
> (Consider devices whose underlying technology does not use ATA or SATA,
> for example.)  And some of the ones that don't implement it will die if
> you try to send them an ATA PASSTHROUGH command.

It is not possible to detect if underlaying device is ATA?

> You have to understand that consumer USB storage really is very
> low quality in many cases.  Vendors aim for low cost rather than high 
> reliability or correctness.

Understood. But lot of distributions call hdparm for inserted disks and
also set some APM (or at least check it)... That means there is already
some way how to deal with these problems (in userspace).

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

On Tue, 10 Jan 2017, Dainius Masiliūnas wrote:

> On Tue, Jan 10, 2017 at 9:00 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > It is used for preventing the kernel from issuing a READ CAPACITY(16)
> > command to the device.  Normally the kernel would do this if the reply
> > to READ CAPACITY(10) indicated there were more than 2^32 blocks (about
> > 2 TB).
> 
> Ah, OK, that makes sense. Though that's still not a proper solution,
> and if anything it's confusing for the users: if they plug in a >2 TiB
> drive and it breaks, the user will assume that it's the fault of the
> drive and not the USB case, or that the case is incapable of handling
> such large drives, whereas the true reason is completely different.

I'm not sure what you mean.  If the quirk flag is set and the user 
plugs in a >2-TB drive, it won't break.  It just won't report the 
correct size, but at least it will continue to work in a degraded 
fashion.

> > 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.
> 
> Well, the problems are 1) we don't really know how many such
> misbehaving drives there are, since only those with >2 TiB drives
> experience it, thus it's hard to know what effect such change would
> have, and 2) there is no way to make the kernel output the true value
> even if you know your drive is working fine.

We could add that easily enough: a usb-storage quirk to set the 
NEEDS_CAP16 flag.  But it would be better if this wasn't necessary.

> And when a drive breaks due to the 16 command, is there a way to
> revive it, perhaps by trying to reconnect? That would allow the kernel
> to try it first, see that it failed after that command was sent, then
> issue a warning in the log saying that this device should have a
> quirk, reset and continue with the quirk applied.

I'm not sure what you mean by "trying to reconnect".  The recovery
method most likely to work is to unplug the USB cable and then
re-attach it.  Issuing a reset might work in some cases and not in 
others; we have seen examples of devices that crash extremely hard when 
given something they don't know how to handle.

> Also, is `sg_readcap -16` a proper test to determine whether the
> device works as expected?

You mean, to determine whether the device can handle a READ 
CAPACITY(16) command?  Yes, since that is the command it sends.

Alan Stern

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

On Wed, 11 Jan 2017, Pali Rohár wrote:

> On Tuesday 10 January 2017 15:29:23 Alan Stern wrote:
> > > Tom Yan wrote that smartctl/hdparm "works" because they use the SCSI ATA
> > > PASSTHROUGH command. It is not an option for kernel?
> > 
> > No, because many devices do not implement SCSI ATA PASSTHROUGH.  
> > (Consider devices whose underlying technology does not use ATA or SATA,
> > for example.)  And some of the ones that don't implement it will die if
> > you try to send them an ATA PASSTHROUGH command.
> 
> It is not possible to detect if underlaying device is ATA?

I don't know any reliable way to do it.  Besides, even if the device is
ATA, you're still out of luck if the USB-SATA bridge doesn't support
ATA PASSTHROUGH.

> > You have to understand that consumer USB storage really is very
> > low quality in many cases.  Vendors aim for low cost rather than high 
> > reliability or correctness.
> 
> Understood. But lot of distributions call hdparm for inserted disks and
> also set some APM (or at least check it)... That means there is already
> some way how to deal with these problems (in userspace).

Maybe.  Or maybe users simply don't use hdparm if it causes problems 
for them.  You don't have a similar option with the kernel.

Alan Stern

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

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



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;

Re: Advanced Format SAT devices show incorrect physical block size

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2533 bytes --]

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;

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Advanced Format SAT devices show incorrect physical block size

[Alan Stern]

On Sun, 29 Jan 2017, Pali Rohár wrote:

> 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?

As far as I can tell, they both reported 0xFF000000000001.  That's a 
pattern -- unless somebody really does have a storage device that 
large (18 exabytes).  For the time being, perhaps we can ignore this 
possibility.

> > 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?

I don't know.

Anyway, please try out the patch below.  I don't know if it will be 
acceptable to the SCSI maintainers, but we should at least make sure it 
fixes your problem before submitting it.

Alan Stern




Index: usb-4.x/drivers/scsi/sd.c
===================================================================
--- usb-4.x.orig/drivers/scsi/sd.c
+++ usb-4.x/drivers/scsi/sd.c
@@ -2157,6 +2157,13 @@ static int read_capacity_16(struct scsi_
 		return -ENODEV;
 	}
 
+	/* Some buggy devices report an impossibly large size */
+	if (lba >= (1ULL << 54)) {
+		sd_printk(KERN_WARNING, sdkp, "Read Capacity(16) returned excessively large value: %llu", lba);
+		sdkp->capacity = 0;
+		return -EINVAL;
+	}
+
 	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
 			"kernel compiled with support for large block "
Index: usb-4.x/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-4.x.orig/drivers/usb/storage/scsiglue.c
+++ usb-4.x/drivers/usb/storage/scsiglue.c
@@ -247,8 +247,11 @@ static int slave_configure(struct scsi_d
 		 * Tell the SCSI layer to try READ_CAPACITY_10 first.
 		 * However some USB 3.0 drive enclosures return capacity
 		 * modulo 2TB. Those must use READ_CAPACITY_16
+		 *
+		 * Assume SPC3 or later devices can handle READ_CAPACITY_16.
 		 */
-		if (!(us->fflags & US_FL_NEEDS_CAP16))
+		if (sdev->scsi_level <= SCSI_SPC_2 &&
+				!(us->fflags & US_FL_NEEDS_CAP16))
 			sdev->try_rc_10_first = 1;
 
 		/* assume SPC3 or latter devices support sense size > 18 */

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Advanced Format SAT devices show incorrect physical block size

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 4089 bytes --]

On Monday 30 January 2017 17:17:03 Alan Stern wrote:
> On Sun, 29 Jan 2017, Pali Rohár wrote:
> > 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?
> 
> As far as I can tell, they both reported 0xFF000000000001.  That's a
> pattern -- unless somebody really does have a storage device that
> large (18 exabytes).  For the time being, perhaps we can ignore this
> possibility.
> 
> > > 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?
> 
> I don't know.
> 
> Anyway, please try out the patch below.  I don't know if it will be
> acceptable to the SCSI maintainers, but we should at least make sure
> it fixes your problem before submitting it.

I'm not original reporter of this problem.

Dainius, can you test it?

> Alan Stern
> 
> 
> 
> 
> Index: usb-4.x/drivers/scsi/sd.c
> ===================================================================
> --- usb-4.x.orig/drivers/scsi/sd.c
> +++ usb-4.x/drivers/scsi/sd.c
> @@ -2157,6 +2157,13 @@ static int read_capacity_16(struct scsi_
>  		return -ENODEV;
>  	}
> 
> +	/* Some buggy devices report an impossibly large size */
> +	if (lba >= (1ULL << 54)) {
> +		sd_printk(KERN_WARNING, sdkp, "Read Capacity(16) returned
> excessively large value: %llu", lba); +		sdkp->capacity = 0;
> +		return -EINVAL;
> +	}
> +
>  	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
>  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
>  			"kernel compiled with support for large block "
> Index: usb-4.x/drivers/usb/storage/scsiglue.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/storage/scsiglue.c
> +++ usb-4.x/drivers/usb/storage/scsiglue.c
> @@ -247,8 +247,11 @@ static int slave_configure(struct scsi_d
>  		 * Tell the SCSI layer to try READ_CAPACITY_10 first.
>  		 * However some USB 3.0 drive enclosures return capacity
>  		 * modulo 2TB. Those must use READ_CAPACITY_16
> +		 *
> +		 * Assume SPC3 or later devices can handle READ_CAPACITY_16.
>  		 */
> -		if (!(us->fflags & US_FL_NEEDS_CAP16))
> +		if (sdev->scsi_level <= SCSI_SPC_2 &&
> +				!(us->fflags & US_FL_NEEDS_CAP16))
>  			sdev->try_rc_10_first = 1;
> 
>  		/* assume SPC3 or latter devices support sense size > 18 */

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Advanced Format SAT devices show incorrect physical block size

[Pali Rohár]

On Monday 30 January 2017 18:43:12 Pali Rohár wrote:
> On Monday 30 January 2017 17:17:03 Alan Stern wrote:
> > On Sun, 29 Jan 2017, Pali Rohár wrote:
> > > 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?
> > 
> > As far as I can tell, they both reported 0xFF000000000001.  That's a
> > pattern -- unless somebody really does have a storage device that
> > large (18 exabytes).  For the time being, perhaps we can ignore this
> > possibility.
> > 
> > > > 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?
> > 
> > I don't know.
> > 
> > Anyway, please try out the patch below.  I don't know if it will be
> > acceptable to the SCSI maintainers, but we should at least make sure
> > it fixes your problem before submitting it.
> 
> I'm not original reporter of this problem.
> 
> Dainius, can you test it?

Just want to remind this patch so it will not be forgotten...

> > Alan Stern
> > 
> > 
> > 
> > 
> > Index: usb-4.x/drivers/scsi/sd.c
> > ===================================================================
> > --- usb-4.x.orig/drivers/scsi/sd.c
> > +++ usb-4.x/drivers/scsi/sd.c
> > @@ -2157,6 +2157,13 @@ static int read_capacity_16(struct scsi_
> >  		return -ENODEV;
> >  	}
> > 
> > +	/* Some buggy devices report an impossibly large size */
> > +	if (lba >= (1ULL << 54)) {
> > +		sd_printk(KERN_WARNING, sdkp, "Read Capacity(16) returned excessively large value: %llu", lba);
> > +		sdkp->capacity = 0;
> > +		return -EINVAL;
> > +	}
> > +
> >  	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
> >  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
> >  			"kernel compiled with support for large block "
> > Index: usb-4.x/drivers/usb/storage/scsiglue.c
> > ===================================================================
> > --- usb-4.x.orig/drivers/usb/storage/scsiglue.c
> > +++ usb-4.x/drivers/usb/storage/scsiglue.c
> > @@ -247,8 +247,11 @@ static int slave_configure(struct scsi_d
> >  		 * Tell the SCSI layer to try READ_CAPACITY_10 first.
> >  		 * However some USB 3.0 drive enclosures return capacity
> >  		 * modulo 2TB. Those must use READ_CAPACITY_16
> > +		 *
> > +		 * Assume SPC3 or later devices can handle READ_CAPACITY_16.
> >  		 */
> > -		if (!(us->fflags & US_FL_NEEDS_CAP16))
> > +		if (sdev->scsi_level <= SCSI_SPC_2 &&
> > +				!(us->fflags & US_FL_NEEDS_CAP16))
> >  			sdev->try_rc_10_first = 1;
> > 
> >  		/* assume SPC3 or latter devices support sense size > 18 */
> 

-- 
Pali Rohár
pali.rohar@gmail.com