[PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

The "G-Drive" (sold by G-Technology) external USB 3.0 drive
 hangs on write access under UAS:

[  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
[  136.079180] print_req_error: critical target error, dev sdi, sector 0
[  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
[  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
[  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
[  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584052] Aborting journal on device sdi-8.

The proposed patch adds compatibility quirks. Because the drive requires two
quirks (one to disable UAS, and another to work with usb-storage), adding this
under unusual_devs.h and not unusual_uas.h so kernels compiled without UAS
receive the quirk. With the patch, the drive works reliably
(tested on NEC Corporation uPD720200 USB 3.0 host controller).

Signed-off-by: Alexander Kappner <agk@godking.net>
---
 drivers/usb/storage/unusual_devs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 747d3a9..b8661a1 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
 		"Micro Mini 1GB",
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
 
+/* "G-DRIVE" external HDD hangs on write without these.
+ * Reported-by: Alexander Kappner <agk@godking.net>
+ */
+UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
+		"SimpleTech",
+		"External HDD",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_IGNORE_UAS | US_FL_NO_WP_DETECT),
+
 /*
  * Nick Bowler <nbowler@elliptictech.com>
  * SCSI stack spams (otherwise harmless) error messages.
-- 
2.1.4

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

The "G-Drive" (sold by G-Technology) external USB 3.0 drive
 hangs on write access under UAS:

[  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
[  136.079180] print_req_error: critical target error, dev sdi, sector 0
[  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
[  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
[  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
[  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584052] Aborting journal on device sdi-8.

The proposed patch adds compatibility quirks. Because the drive requires two
quirks (one to disable UAS, and another to work with usb-storage), adding this
under unusual_devs.h and not unusual_uas.h so kernels compiled without UAS
receive the quirk. With the patch, the drive works reliably
(tested on NEC Corporation uPD720200 USB 3.0 host controller).

Signed-off-by: Alexander Kappner <agk@godking.net>
---
 drivers/usb/storage/unusual_devs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 747d3a9..b8661a1 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
 		"Micro Mini 1GB",
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
 
+/* "G-DRIVE" external HDD hangs on write without these.
+ * Reported-by: Alexander Kappner <agk@godking.net>
+ */
+UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
+		"SimpleTech",
+		"External HDD",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_IGNORE_UAS | US_FL_NO_WP_DETECT),
+
 /*
  * Nick Bowler <nbowler@elliptictech.com>
  * SCSI stack spams (otherwise harmless) error messages.

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Wed, 16 May 2018, Alexander Kappner wrote:

> The "G-Drive" (sold by G-Technology) external USB 3.0 drive
>  hangs on write access under UAS:
> 
> [  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
> [  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
> [  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
> [  136.079180] print_req_error: critical target error, dev sdi, sector 0
> [  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
> [  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
> [  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
> [  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
> [  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
> [  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
> [  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
> [  140.584052] Aborting journal on device sdi-8.

That's kind of weird.  Does the drive work under Windows in UAS mode?  
If so, why are the WRITE(16) commands failing under Linux?

> The proposed patch adds compatibility quirks. Because the drive requires two
> quirks (one to disable UAS, and another to work with usb-storage), adding this
> under unusual_devs.h and not unusual_uas.h so kernels compiled without UAS
> receive the quirk.

That doesn't quite make sense.  Since you prevent the uas driver from 
binding to this device, it will end up using usb-storage no matter how 
the kernel was built.  Therefore the second quirk flag has to go into 
unusual_devs.h no matter what.

>  With the patch, the drive works reliably
> (tested on NEC Corporation uPD720200 USB 3.0 host controller).

You don't describe the second quirk flag at all.  Are you sure it is 
needed?

> Signed-off-by: Alexander Kappner <agk@godking.net>
> ---
>  drivers/usb/storage/unusual_devs.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
> index 747d3a9..b8661a1 100644
> --- a/drivers/usb/storage/unusual_devs.h
> +++ b/drivers/usb/storage/unusual_devs.h
> @@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
>  		"Micro Mini 1GB",
>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
>  
> +/* "G-DRIVE" external HDD hangs on write without these.
> + * Reported-by: Alexander Kappner <agk@godking.net>
> + */
> +UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
> +		"SimpleTech",
> +		"External HDD",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_IGNORE_UAS | US_FL_NO_WP_DETECT),
> +
>  /*
>   * Nick Bowler <nbowler@elliptictech.com>
>   * SCSI stack spams (otherwise harmless) error messages.
> 

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Wed, 16 May 2018, Alexander Kappner wrote:

> The "G-Drive" (sold by G-Technology) external USB 3.0 drive
>  hangs on write access under UAS:
> 
> [  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
> [  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
> [  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
> [  136.079180] print_req_error: critical target error, dev sdi, sector 0
> [  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
> [  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
> [  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
> [  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
> [  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
> [  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
> [  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
> [  140.584052] Aborting journal on device sdi-8.

That's kind of weird.  Does the drive work under Windows in UAS mode?  
If so, why are the WRITE(16) commands failing under Linux?

> The proposed patch adds compatibility quirks. Because the drive requires two
> quirks (one to disable UAS, and another to work with usb-storage), adding this
> under unusual_devs.h and not unusual_uas.h so kernels compiled without UAS
> receive the quirk.

That doesn't quite make sense.  Since you prevent the uas driver from 
binding to this device, it will end up using usb-storage no matter how 
the kernel was built.  Therefore the second quirk flag has to go into 
unusual_devs.h no matter what.

>  With the patch, the drive works reliably
> (tested on NEC Corporation uPD720200 USB 3.0 host controller).

You don't describe the second quirk flag at all.  Are you sure it is 
needed?

> Signed-off-by: Alexander Kappner <agk@godking.net>
> ---
>  drivers/usb/storage/unusual_devs.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
> index 747d3a9..b8661a1 100644
> --- a/drivers/usb/storage/unusual_devs.h
> +++ b/drivers/usb/storage/unusual_devs.h
> @@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
>  		"Micro Mini 1GB",
>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
>  
> +/* "G-DRIVE" external HDD hangs on write without these.
> + * Reported-by: Alexander Kappner <agk@godking.net>
> + */
> +UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
> +		"SimpleTech",
> +		"External HDD",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_IGNORE_UAS | US_FL_NO_WP_DETECT),
> +
>  /*
>   * Nick Bowler <nbowler@elliptictech.com>
>   * SCSI stack spams (otherwise harmless) error messages.
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

Hi Alan,

thanks for reviewing. (This is my first contribution that touches 
usb-storage, so please bear with me.)

> That's kind of weird.  Does the drive work under Windows in UAS mode?  

On the Windows 10 VM that I just spun up for testing this, access to the 
drive uses "usbstor.inf" (rather than "uaspstor.sys"). So I believe the 
answer is no. 

> If so, why are the WRITE(16) commands failing under Linux?

I don't know exactly why they're failing, but another entry in the 
unusual_uas.h shows another SimpleTech device (only with a slightly 
different product ID) needing the same UAS quirk (see 
https://bugzilla.redhat.com/show_bug.cgi?id=1124119). So my guess is those 
drives are just buggy. 

> That doesn't quite make sense.  Since you prevent the uas driver from 
> binding to this device, it will end up using usb-storage no matter how 
> the kernel was built.  Therefore the second quirk flag has to go into 
> unusual_devs.h no matter what.

Yes that's what I was trying to get at. So even though the UAS flag would 
conceptually belong into unusual_uas, I'm putting both into unusual_devs to 
avoid having to create two separate entries for the same device.

> You don't describe the second quirk flag at all.  Are you sure it is 
> needed?

Yes. Without this flag, the device keeps throwing similar errors on 
usb-storage. That's the same result I get on a host that doesn't have UAS 
compiled in. Here's a dmesg:

[    2.183472] usb 3-1: New USB device found, idVendor=4971, idProduct=8024, bcdDevice=24.03
[    2.184285] usb 3-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[    2.184980] usb 3-1: Product: G-DRIVE
[    2.185447] usb 3-1: Manufacturer: HGST
[    2.185829] usb 3-1: SerialNumber: AA015010004C
[    2.195509] usb-storage 3-1:1.0: USB Mass Storage device detected
[    2.198668] Floppy drive(s): fd0 is 2.88M AMI BIOS
[    2.202981] scsi host2: usb-storage 3-1:1.0
...
[    3.233085] scsi 2:0:0:0: Direct-Access     G-DRIVE                   2403 PQ: 0 ANSI: 6
[    3.234514] sd 2:0:0:0: Attached scsi generic sg1 type 0
[    3.235465] sd 2:0:0:0: [sda] Very big device. Trying to use READ CAPACITY(16).
[    3.236847] sd 2:0:0:0: [sda] 7814037168 512-byte logical blocks: (4.00 TB/3.64 TiB)
[    3.237794] sd 2:0:0:0: [sda] 4096-byte physical blocks
[    3.241255] sd 2:0:0:0: [sda] Write Protect is off
[    3.242096] sd 2:0:0:0: [sda] Mode Sense: 47 00 10 08
[    3.243595] sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports DPO and FUA
[    3.257893]  sda: sda1 sda9
[    3.261402] sd 2:0:0:0: [sda] Attached SCSI disk
...
[   92.433428] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[   92.434759] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
[   92.435637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[   92.436401] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
[   92.437493] print_req_error: critical target error, dev sda, sector 0
[   92.438211] Buffer I/O error on dev sda, logical block 0, lost sync page write
[   92.516692] EXT4-fs (sda): mounted filesystem with ordered data mode. Opts: (null)
[  101.449311] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  101.450598] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
[  101.451401] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[  101.452041] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
[  101.452906] print_req_error: critical target error, dev sda, sector 3905159192
[  101.453593] print_req_error: critical target error, dev sda, sector 3905159192
[  101.454889] Aborting journal on device sda-8.
[  101.457103] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  101.457988] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
[  101.458637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[  101.459250] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 00 00 00 00 08 00 00

Source code comments describe this as a known problem (scsiglue.c:182):

                /*
                 * Some devices don't like MODE SENSE with page=0x3f,
                 * which is the command used for checking if a device
                 * is write-protected.  Now that we tell the sd driver
                 * to do a 192-byte transfer with this command the
                 * majority of devices work fine, but a few still can't
                 * handle it.  The sd driver will simply assume those
                 * devices are write-enabled.
                 */
                if (us->fflags & US_FL_NO_WP_DETECT)
                        sdev->skip_ms_page_3f = 1;

--agk 



On 05/16/2018 01:55 PM, Alan Stern wrote:
> On Wed, 16 May 2018, Alexander Kappner wrote:
> 
>> The "G-Drive" (sold by G-Technology) external USB 3.0 drive
>>  hangs on write access under UAS:
>>
>> [  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>> [  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
>> [  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
>> [  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
>> [  136.079180] print_req_error: critical target error, dev sdi, sector 0
>> [  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
>> [  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
>> [  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>> [  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
>> [  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
>> [  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
>> [  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
>> [  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
>> [  140.584052] Aborting journal on device sdi-8.
> 
> That's kind of weird.  Does the drive work under Windows in UAS mode?  
> If so, why are the WRITE(16) commands failing under Linux?
> 
>> The proposed patch adds compatibility quirks. Because the drive requires two
>> quirks (one to disable UAS, and another to work with usb-storage), adding this
>> under unusual_devs.h and not unusual_uas.h so kernels compiled without UAS
>> receive the quirk.
> 
> That doesn't quite make sense.  Since you prevent the uas driver from 
> binding to this device, it will end up using usb-storage no matter how 
> the kernel was built.  Therefore the second quirk flag has to go into 
> unusual_devs.h no matter what.
> 
>>  With the patch, the drive works reliably
>> (tested on NEC Corporation uPD720200 USB 3.0 host controller).
> 
> You don't describe the second quirk flag at all.  Are you sure it is 
> needed?
> 
>> Signed-off-by: Alexander Kappner <agk@godking.net>
>> ---
>>  drivers/usb/storage/unusual_devs.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
>> index 747d3a9..b8661a1 100644
>> --- a/drivers/usb/storage/unusual_devs.h
>> +++ b/drivers/usb/storage/unusual_devs.h
>> @@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
>>  		"Micro Mini 1GB",
>>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
>>  
>> +/* "G-DRIVE" external HDD hangs on write without these.
>> + * Reported-by: Alexander Kappner <agk@godking.net>
>> + */
>> +UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
>> +		"SimpleTech",
>> +		"External HDD",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_IGNORE_UAS | US_FL_NO_WP_DETECT),
>> +
>>  /*
>>   * Nick Bowler <nbowler@elliptictech.com>
>>   * SCSI stack spams (otherwise harmless) error messages.
>>

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

Hi Alan,

thanks for reviewing. (This is my first contribution that touches 
usb-storage, so please bear with me.)

> That's kind of weird.  Does the drive work under Windows in UAS mode?  

On the Windows 10 VM that I just spun up for testing this, access to the 
drive uses "usbstor.inf" (rather than "uaspstor.sys"). So I believe the 
answer is no. 

> If so, why are the WRITE(16) commands failing under Linux?

I don't know exactly why they're failing, but another entry in the 
unusual_uas.h shows another SimpleTech device (only with a slightly 
different product ID) needing the same UAS quirk (see 
https://bugzilla.redhat.com/show_bug.cgi?id=1124119). So my guess is those 
drives are just buggy. 

> That doesn't quite make sense.  Since you prevent the uas driver from 
> binding to this device, it will end up using usb-storage no matter how 
> the kernel was built.  Therefore the second quirk flag has to go into 
> unusual_devs.h no matter what.

Yes that's what I was trying to get at. So even though the UAS flag would 
conceptually belong into unusual_uas, I'm putting both into unusual_devs to 
avoid having to create two separate entries for the same device.

> You don't describe the second quirk flag at all.  Are you sure it is 
> needed?

Yes. Without this flag, the device keeps throwing similar errors on 
usb-storage. That's the same result I get on a host that doesn't have UAS 
compiled in. Here's a dmesg:

[    2.183472] usb 3-1: New USB device found, idVendor=4971, idProduct=8024, bcdDevice=24.03
[    2.184285] usb 3-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[    2.184980] usb 3-1: Product: G-DRIVE
[    2.185447] usb 3-1: Manufacturer: HGST
[    2.185829] usb 3-1: SerialNumber: AA015010004C
[    2.195509] usb-storage 3-1:1.0: USB Mass Storage device detected
[    2.198668] Floppy drive(s): fd0 is 2.88M AMI BIOS
[    2.202981] scsi host2: usb-storage 3-1:1.0
...
[    3.233085] scsi 2:0:0:0: Direct-Access     G-DRIVE                   2403 PQ: 0 ANSI: 6
[    3.234514] sd 2:0:0:0: Attached scsi generic sg1 type 0
[    3.235465] sd 2:0:0:0: [sda] Very big device. Trying to use READ CAPACITY(16).
[    3.236847] sd 2:0:0:0: [sda] 7814037168 512-byte logical blocks: (4.00 TB/3.64 TiB)
[    3.237794] sd 2:0:0:0: [sda] 4096-byte physical blocks
[    3.241255] sd 2:0:0:0: [sda] Write Protect is off
[    3.242096] sd 2:0:0:0: [sda] Mode Sense: 47 00 10 08
[    3.243595] sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports DPO and FUA
[    3.257893]  sda: sda1 sda9
[    3.261402] sd 2:0:0:0: [sda] Attached SCSI disk
...
[   92.433428] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[   92.434759] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
[   92.435637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[   92.436401] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
[   92.437493] print_req_error: critical target error, dev sda, sector 0
[   92.438211] Buffer I/O error on dev sda, logical block 0, lost sync page write
[   92.516692] EXT4-fs (sda): mounted filesystem with ordered data mode. Opts: (null)
[  101.449311] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  101.450598] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
[  101.451401] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[  101.452041] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
[  101.452906] print_req_error: critical target error, dev sda, sector 3905159192
[  101.453593] print_req_error: critical target error, dev sda, sector 3905159192
[  101.454889] Aborting journal on device sda-8.
[  101.457103] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  101.457988] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
[  101.458637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[  101.459250] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 00 00 00 00 08 00 00

Source code comments describe this as a known problem (scsiglue.c:182):

                /*
                 * Some devices don't like MODE SENSE with page=0x3f,
                 * which is the command used for checking if a device
                 * is write-protected.  Now that we tell the sd driver
                 * to do a 192-byte transfer with this command the
                 * majority of devices work fine, but a few still can't
                 * handle it.  The sd driver will simply assume those
                 * devices are write-enabled.
                 */
                if (us->fflags & US_FL_NO_WP_DETECT)
                        sdev->skip_ms_page_3f = 1;

--agk 



On 05/16/2018 01:55 PM, Alan Stern wrote:
> On Wed, 16 May 2018, Alexander Kappner wrote:
> 
>> The "G-Drive" (sold by G-Technology) external USB 3.0 drive
>>  hangs on write access under UAS:
>>
>> [  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>> [  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
>> [  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
>> [  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
>> [  136.079180] print_req_error: critical target error, dev sdi, sector 0
>> [  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
>> [  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
>> [  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>> [  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
>> [  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
>> [  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
>> [  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
>> [  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
>> [  140.584052] Aborting journal on device sdi-8.
> 
> That's kind of weird.  Does the drive work under Windows in UAS mode?  
> If so, why are the WRITE(16) commands failing under Linux?
> 
>> The proposed patch adds compatibility quirks. Because the drive requires two
>> quirks (one to disable UAS, and another to work with usb-storage), adding this
>> under unusual_devs.h and not unusual_uas.h so kernels compiled without UAS
>> receive the quirk.
> 
> That doesn't quite make sense.  Since you prevent the uas driver from 
> binding to this device, it will end up using usb-storage no matter how 
> the kernel was built.  Therefore the second quirk flag has to go into 
> unusual_devs.h no matter what.
> 
>>  With the patch, the drive works reliably
>> (tested on NEC Corporation uPD720200 USB 3.0 host controller).
> 
> You don't describe the second quirk flag at all.  Are you sure it is 
> needed?
> 
>> Signed-off-by: Alexander Kappner <agk@godking.net>
>> ---
>>  drivers/usb/storage/unusual_devs.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
>> index 747d3a9..b8661a1 100644
>> --- a/drivers/usb/storage/unusual_devs.h
>> +++ b/drivers/usb/storage/unusual_devs.h
>> @@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
>>  		"Micro Mini 1GB",
>>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
>>  
>> +/* "G-DRIVE" external HDD hangs on write without these.
>> + * Reported-by: Alexander Kappner <agk@godking.net>
>> + */
>> +UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
>> +		"SimpleTech",
>> +		"External HDD",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_IGNORE_UAS | US_FL_NO_WP_DETECT),
>> +
>>  /*
>>   * Nick Bowler <nbowler@elliptictech.com>
>>   * SCSI stack spams (otherwise harmless) error messages.
>>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

Am Donnerstag, den 17.05.2018, 01:15 -0700 schrieb Alexander Kappner:
> Yes. Without this flag, the device keeps throwing similar errors on 
> usb-storage. That's the same result I get on a host that doesn't have UAS 
> compiled in. Here's a dmesg:

Hi,

this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
by itself would make the device work. Can you please test that?
You will need the attached patch for the quirk to be supported.

	Regards
		Oliver

[-- Attachment #2: 0001-UAS-add-support-for-the-quirk-US_FL_NO_WP_DETECT.patch --]
[-- Type: text/x-patch, Size: 951 bytes --]

From 1ff6c9c9cc66ddb4cf7ca95bea589d6a30c63623 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 17 May 2018 14:46:47 +0200
Subject: [PATCH] UAS: add support for the quirk US_FL_NO_WP_DETECT

The assumption that this quirk can be limited to old storage
devices was too optimistic.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/storage/uas.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39b67d1..7ee67e8c1f1e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -836,6 +836,10 @@ static int uas_slave_configure(struct scsi_device *sdev)
 	if (devinfo->flags & US_FL_BROKEN_FUA)
 		sdev->broken_fua = 1;
 
+	/* UAS also needs to support FL_NO_WP_DETECT */
+	if (devinfo->flags & US_FL_NO_WP_DETECT)
+		sdev->skip_ms_page_3f = 1;
+
 	scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
 	return 0;
 }
-- 
2.13.6

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Oliver Neukum]

Am Donnerstag, den 17.05.2018, 01:15 -0700 schrieb Alexander Kappner:
> Yes. Without this flag, the device keeps throwing similar errors on 
> usb-storage. That's the same result I get on a host that doesn't have UAS 
> compiled in. Here's a dmesg:

Hi,

this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
by itself would make the device work. Can you please test that?
You will need the attached patch for the quirk to be supported.

	Regards
		Oliver

From 1ff6c9c9cc66ddb4cf7ca95bea589d6a30c63623 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 17 May 2018 14:46:47 +0200
Subject: [PATCH] UAS: add support for the quirk US_FL_NO_WP_DETECT

The assumption that this quirk can be limited to old storage
devices was too optimistic.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/storage/uas.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39b67d1..7ee67e8c1f1e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -836,6 +836,10 @@ static int uas_slave_configure(struct scsi_device *sdev)
 	if (devinfo->flags & US_FL_BROKEN_FUA)
 		sdev->broken_fua = 1;
 
+	/* UAS also needs to support FL_NO_WP_DETECT */
+	if (devinfo->flags & US_FL_NO_WP_DETECT)
+		sdev->skip_ms_page_3f = 1;
+
 	scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
 	return 0;
 }
-- 
2.13.6

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

Oliver and Alan,

thank for investigating.

> this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
> by itself would make the device work. Can you please test that?

US_FL_NO_WP_DETECT without US_FL_IGNORE_UAS does not make a difference, 
even with the patch you included applied:

[   44.108417] JBD2: Clearing recovery information on journal
[   44.119593] sd 2:0:0:0: [sda] tag#1 FAILED Result: hostbyte=DID_OK 
driverbyte=DRIVER_SENSE
[   44.121605] sd 2:0:0:0: [sda] tag#1 Sense Key : Illegal Request 
[current] 
[   44.123254] sd 2:0:0:0: [sda] tag#1 Add. Sense: Invalid field in cdb
[   44.124798] sd 2:0:0:0: [sda] tag#1 CDB: Write(16) 8a 08 00 00 00 00 e8 
c4 00 00 00 00 00 08 00 00
[   44.126847] print_req_error: critical target error, dev sda, sector 
3905159168
[   44.128450] Buffer I/O error on dev sda, logical block 488144896, lost 
sync page write
[   44.130776] JBD2: Error -5 detected when updating journal superblock for 
sda-8.
[   44.141059] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK 
driverbyte=DRIVER_SENSE
[   44.141376] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request 
[current] 
[   44.141376] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[   44.141376] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 
00 00 00 00 00 00 08 00 00
[   44.141376] print_req_error: critical target error, dev sda, sector 0
[   44.141376] Buffer I/O error on dev sda, logical block 0, lost sync page 
write

> That's bizarre too.  Even though the only difference is a MODE SENSE 
> command, the command that actually faliled was WRITE(16).
It looks to me like the MODE SENSE simply hangs the drive, so anything 
issued after that will fail. Of course the drive says it's the "current 
command" that caused the failure, but I wouldn't give too much credence to 
that. FYI -- this device is a consumer grade rotational drive that you can 
get for less than $200, so I wouldn't be surprised if the implementations 
have issues. 

Also, I noticed that copying onto the drive with dd works fine, whereas 
trying to mount a filesystem immediately crashes it. I suspect this is 
because check_disk_change is called on mount (which eventually calls down 
to sd_read_write_protect_flag, which is where the US_FL_NO_WP_DETECT flag 
comes into play).



On 05/17/2018 05:58 AM, Oliver Neukum wrote:
> Am Donnerstag, den 17.05.2018, 01:15 -0700 schrieb Alexander Kappner:
>> Yes. Without this flag, the device keeps throwing similar errors on 
>> usb-storage. That's the same result I get on a host that doesn't have UAS 
>> compiled in. Here's a dmesg:
> 
> Hi,
> 
> this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
> by itself would make the device work. Can you please test that?
> You will need the attached patch for the quirk to be supported.
> 
> 	Regards
> 		Oliver
> 

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

Oliver and Alan,

thank for investigating.

> this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
> by itself would make the device work. Can you please test that?

US_FL_NO_WP_DETECT without US_FL_IGNORE_UAS does not make a difference, 
even with the patch you included applied:

[   44.108417] JBD2: Clearing recovery information on journal
[   44.119593] sd 2:0:0:0: [sda] tag#1 FAILED Result: hostbyte=DID_OK 
driverbyte=DRIVER_SENSE
[   44.121605] sd 2:0:0:0: [sda] tag#1 Sense Key : Illegal Request 
[current] 
[   44.123254] sd 2:0:0:0: [sda] tag#1 Add. Sense: Invalid field in cdb
[   44.124798] sd 2:0:0:0: [sda] tag#1 CDB: Write(16) 8a 08 00 00 00 00 e8 
c4 00 00 00 00 00 08 00 00
[   44.126847] print_req_error: critical target error, dev sda, sector 
3905159168
[   44.128450] Buffer I/O error on dev sda, logical block 488144896, lost 
sync page write
[   44.130776] JBD2: Error -5 detected when updating journal superblock for 
sda-8.
[   44.141059] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK 
driverbyte=DRIVER_SENSE
[   44.141376] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request 
[current] 
[   44.141376] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
[   44.141376] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 
00 00 00 00 00 00 08 00 00
[   44.141376] print_req_error: critical target error, dev sda, sector 0
[   44.141376] Buffer I/O error on dev sda, logical block 0, lost sync page 
write

> That's bizarre too.  Even though the only difference is a MODE SENSE 
> command, the command that actually faliled was WRITE(16).
It looks to me like the MODE SENSE simply hangs the drive, so anything 
issued after that will fail. Of course the drive says it's the "current 
command" that caused the failure, but I wouldn't give too much credence to 
that. FYI -- this device is a consumer grade rotational drive that you can 
get for less than $200, so I wouldn't be surprised if the implementations 
have issues. 

Also, I noticed that copying onto the drive with dd works fine, whereas 
trying to mount a filesystem immediately crashes it. I suspect this is 
because check_disk_change is called on mount (which eventually calls down 
to sd_read_write_protect_flag, which is where the US_FL_NO_WP_DETECT flag 
comes into play).



On 05/17/2018 05:58 AM, Oliver Neukum wrote:
> Am Donnerstag, den 17.05.2018, 01:15 -0700 schrieb Alexander Kappner:
>> Yes. Without this flag, the device keeps throwing similar errors on 
>> usb-storage. That's the same result I get on a host that doesn't have UAS 
>> compiled in. Here's a dmesg:
> 
> Hi,
> 
> this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
> by itself would make the device work. Can you please test that?
> You will need the attached patch for the quirk to be supported.
> 
> 	Regards
> 		Oliver
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Thu, 17 May 2018, Alexander Kappner wrote:

> Oliver and Alan,
> 
> thank for investigating.
> 
> > this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
> > by itself would make the device work. Can you please test that?
> 
> US_FL_NO_WP_DETECT without US_FL_IGNORE_UAS does not make a difference, 
> even with the patch you included applied:

Are you certain Oliver's new code was executed?  If you put 
US_FL_NO_WP_DETECT only in unusual_devs.h and not in ususual_uas.h then 
it would not affect the uas driver.

> > That's bizarre too.  Even though the only difference is a MODE SENSE 
> > command, the command that actually faliled was WRITE(16).
> It looks to me like the MODE SENSE simply hangs the drive, so anything 
> issued after that will fail. Of course the drive says it's the "current 
> command" that caused the failure, but I wouldn't give too much credence to 
> that. FYI -- this device is a consumer grade rotational drive that you can 
> get for less than $200, so I wouldn't be surprised if the implementations 
> have issues. 
> 
> Also, I noticed that copying onto the drive with dd works fine, whereas 
> trying to mount a filesystem immediately crashes it. I suspect this is 
> because check_disk_change is called on mount (which eventually calls down 
> to sd_read_write_protect_flag, which is where the US_FL_NO_WP_DETECT flag 
> comes into play).

Was this tested with uas or usb-storage?

Alan Stern

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Thu, 17 May 2018, Alexander Kappner wrote:

> Oliver and Alan,
> 
> thank for investigating.
> 
> > this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
> > by itself would make the device work. Can you please test that?
> 
> US_FL_NO_WP_DETECT without US_FL_IGNORE_UAS does not make a difference, 
> even with the patch you included applied:

Are you certain Oliver's new code was executed?  If you put 
US_FL_NO_WP_DETECT only in unusual_devs.h and not in ususual_uas.h then 
it would not affect the uas driver.

> > That's bizarre too.  Even though the only difference is a MODE SENSE 
> > command, the command that actually faliled was WRITE(16).
> It looks to me like the MODE SENSE simply hangs the drive, so anything 
> issued after that will fail. Of course the drive says it's the "current 
> command" that caused the failure, but I wouldn't give too much credence to 
> that. FYI -- this device is a consumer grade rotational drive that you can 
> get for less than $200, so I wouldn't be surprised if the implementations 
> have issues. 
> 
> Also, I noticed that copying onto the drive with dd works fine, whereas 
> trying to mount a filesystem immediately crashes it. I suspect this is 
> because check_disk_change is called on mount (which eventually calls down 
> to sd_read_write_protect_flag, which is where the US_FL_NO_WP_DETECT flag 
> comes into play).

Was this tested with uas or usb-storage?

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

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

> Was this tested with uas or usb-storage?
On both. dd works either way.  

> Are you certain Oliver's new code was executed?  If you put 
> US_FL_NO_WP_DETECT only in unusual_devs.h and not in ususual_uas.h then 
> it would not affect the uas driver.
Yes, the code ran.

Further debugging shows that the code that causes the device to hang is in 
drivers/scsi/sd.c:2698. So the reason why usb-storage works and UAS does 
not is because the device setting both skip_ms_page_3f=1 and 
skip_ms_page_8=1 is required. The US_FL_NO_WP_DETECT flag does the former, 
and usb-storage (but not UAS) by default does the latter 
(drivers/usb/storage/scsiglue.c:198):

 /*
                 * A number of devices have problems with MODE SENSE for
                 * page x08, so we will skip it.
                 */
                sdev->skip_ms_page_8 = 1;


Forcing both skip_ms_page_3f and skip_ms_page_8 to 1 results in UAS and 
usb-storage working. Oliver's code only set skip_ms_page_3f. 

Do we want a patch to introduce a quirk flag for skip_ms_page_8,  similar to 
the US_FL_NO_WP_DETECT quirk bit for skip_ms_page_3f? This may even resolve 
the issues with other devices in unusual_uas.h that currently have all UAS 
support disabled. I'd be happy to write the patch, but I'm not sure we want 
to reserve a quirk bit for what's currently only a single device known to 
have this issue.  Please advise. 


On 05/17/2018 12:13 PM, Alan Stern wrote:
> On Thu, 17 May 2018, Alexander Kappner wrote:
> 
>> Oliver and Alan,
>>
>> thank for investigating.
>>
>>> this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
>>> by itself would make the device work. Can you please test that?
>>
>> US_FL_NO_WP_DETECT without US_FL_IGNORE_UAS does not make a difference, 
>> even with the patch you included applied:
> 
> Are you certain Oliver's new code was executed?  If you put 
> US_FL_NO_WP_DETECT only in unusual_devs.h and not in ususual_uas.h then 
> it would not affect the uas driver.
> 
>>> That's bizarre too.  Even though the only difference is a MODE SENSE 
>>> command, the command that actually faliled was WRITE(16).
>> It looks to me like the MODE SENSE simply hangs the drive, so anything 
>> issued after that will fail. Of course the drive says it's the "current 
>> command" that caused the failure, but I wouldn't give too much credence to 
>> that. FYI -- this device is a consumer grade rotational drive that you can 
>> get for less than $200, so I wouldn't be surprised if the implementations 
>> have issues. 
>>
>> Also, I noticed that copying onto the drive with dd works fine, whereas 
>> trying to mount a filesystem immediately crashes it. I suspect this is 
>> because check_disk_change is called on mount (which eventually calls down 
>> to sd_read_write_protect_flag, which is where the US_FL_NO_WP_DETECT flag 
>> comes into play).
> 
> Was this tested with uas or usb-storage?
> 
> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

> Was this tested with uas or usb-storage?
On both. dd works either way.  

> Are you certain Oliver's new code was executed?  If you put 
> US_FL_NO_WP_DETECT only in unusual_devs.h and not in ususual_uas.h then 
> it would not affect the uas driver.
Yes, the code ran.

Further debugging shows that the code that causes the device to hang is in 
drivers/scsi/sd.c:2698. So the reason why usb-storage works and UAS does 
not is because the device setting both skip_ms_page_3f=1 and 
skip_ms_page_8=1 is required. The US_FL_NO_WP_DETECT flag does the former, 
and usb-storage (but not UAS) by default does the latter 
(drivers/usb/storage/scsiglue.c:198):

 /*
                 * A number of devices have problems with MODE SENSE for
                 * page x08, so we will skip it.
                 */
                sdev->skip_ms_page_8 = 1;


Forcing both skip_ms_page_3f and skip_ms_page_8 to 1 results in UAS and 
usb-storage working. Oliver's code only set skip_ms_page_3f. 

Do we want a patch to introduce a quirk flag for skip_ms_page_8,  similar to 
the US_FL_NO_WP_DETECT quirk bit for skip_ms_page_3f? This may even resolve 
the issues with other devices in unusual_uas.h that currently have all UAS 
support disabled. I'd be happy to write the patch, but I'm not sure we want 
to reserve a quirk bit for what's currently only a single device known to 
have this issue.  Please advise. 


On 05/17/2018 12:13 PM, Alan Stern wrote:
> On Thu, 17 May 2018, Alexander Kappner wrote:
> 
>> Oliver and Alan,
>>
>> thank for investigating.
>>
>>> this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
>>> by itself would make the device work. Can you please test that?
>>
>> US_FL_NO_WP_DETECT without US_FL_IGNORE_UAS does not make a difference, 
>> even with the patch you included applied:
> 
> Are you certain Oliver's new code was executed?  If you put 
> US_FL_NO_WP_DETECT only in unusual_devs.h and not in ususual_uas.h then 
> it would not affect the uas driver.
> 
>>> That's bizarre too.  Even though the only difference is a MODE SENSE 
>>> command, the command that actually faliled was WRITE(16).
>> It looks to me like the MODE SENSE simply hangs the drive, so anything 
>> issued after that will fail. Of course the drive says it's the "current 
>> command" that caused the failure, but I wouldn't give too much credence to 
>> that. FYI -- this device is a consumer grade rotational drive that you can 
>> get for less than $200, so I wouldn't be surprised if the implementations 
>> have issues. 
>>
>> Also, I noticed that copying onto the drive with dd works fine, whereas 
>> trying to mount a filesystem immediately crashes it. I suspect this is 
>> because check_disk_change is called on mount (which eventually calls down 
>> to sd_read_write_protect_flag, which is where the US_FL_NO_WP_DETECT flag 
>> comes into play).
> 
> Was this tested with uas or usb-storage?
> 
> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Fri, 18 May 2018, Alexander Kappner wrote:

> Further debugging shows that the code that causes the device to hang is in 
> drivers/scsi/sd.c:2698. So the reason why usb-storage works and UAS does 
> not is because the device setting both skip_ms_page_3f=1 and 
> skip_ms_page_8=1 is required. The US_FL_NO_WP_DETECT flag does the former, 
> and usb-storage (but not UAS) by default does the latter 
> (drivers/usb/storage/scsiglue.c:198):
> 
>  /*
>                  * A number of devices have problems with MODE SENSE for
>                  * page x08, so we will skip it.
>                  */
>                 sdev->skip_ms_page_8 = 1;
> 
> 
> Forcing both skip_ms_page_3f and skip_ms_page_8 to 1 results in UAS and 
> usb-storage working. Oliver's code only set skip_ms_page_3f. 

Good detective work!

> Do we want a patch to introduce a quirk flag for skip_ms_page_8,  similar to 
> the US_FL_NO_WP_DETECT quirk bit for skip_ms_page_3f? This may even resolve 
> the issues with other devices in unusual_uas.h that currently have all UAS 
> support disabled. I'd be happy to write the patch, but I'm not sure we want 
> to reserve a quirk bit for what's currently only a single device known to 
> have this issue.  Please advise. 

Yes, I think we want a patch.  Unless Oliver disagrees, please go ahead 
and prepare one.

Alan Stern

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Fri, 18 May 2018, Alexander Kappner wrote:

> Further debugging shows that the code that causes the device to hang is in 
> drivers/scsi/sd.c:2698. So the reason why usb-storage works and UAS does 
> not is because the device setting both skip_ms_page_3f=1 and 
> skip_ms_page_8=1 is required. The US_FL_NO_WP_DETECT flag does the former, 
> and usb-storage (but not UAS) by default does the latter 
> (drivers/usb/storage/scsiglue.c:198):
> 
>  /*
>                  * A number of devices have problems with MODE SENSE for
>                  * page x08, so we will skip it.
>                  */
>                 sdev->skip_ms_page_8 = 1;
> 
> 
> Forcing both skip_ms_page_3f and skip_ms_page_8 to 1 results in UAS and 
> usb-storage working. Oliver's code only set skip_ms_page_3f. 

Good detective work!

> Do we want a patch to introduce a quirk flag for skip_ms_page_8,  similar to 
> the US_FL_NO_WP_DETECT quirk bit for skip_ms_page_3f? This may even resolve 
> the issues with other devices in unusual_uas.h that currently have all UAS 
> support disabled. I'd be happy to write the patch, but I'm not sure we want 
> to reserve a quirk bit for what's currently only a single device known to 
> have this issue.  Please advise. 

Yes, I think we want a patch.  Unless Oliver disagrees, please go ahead 
and prepare one.

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

[PATCH v2 0/2] Re: usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and then
adds the flag as quirks for the device at issue. This allows the G-Drive to work
under both UAS and usb-storage.

Alexander Kappner (2):
  [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
  [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive

 drivers/usb/storage/uas.c          | 6 ++++++
 drivers/usb/storage/unusual_devs.h | 9 +++++++++
 drivers/usb/storage/unusual_uas.h  | 9 +++++++++
 3 files changed, 24 insertions(+)

-- 
2.1.4

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alexander Kappner]

v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and then
adds the flag as quirks for the device at issue. This allows the G-Drive to work
under both UAS and usb-storage.

Alexander Kappner (2):
  [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
  [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive

 drivers/usb/storage/uas.c          | 6 ++++++
 drivers/usb/storage/unusual_devs.h | 9 +++++++++
 drivers/usb/storage/unusual_uas.h  | 9 +++++++++
 3 files changed, 24 insertions(+)

[PATCH v2 1/2] usb-storage: Add support for FL_ALWAYS_SYNC flag in the UAS driver

[Alexander Kappner]

The ALWAYS_SYNC flag is currently honored by the usb-storage driver but not UAS
and is required to work around devices that become unstable upon being
queried for cache. This code is taken straight from:
drivers/usb/storage/scsiglue.c:284

Signed-off-by: Alexander Kappner <agk@godking.net>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Cc: stable <stable@vger.kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/storage/uas.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39..9e9de54 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -836,6 +836,12 @@ static int uas_slave_configure(struct scsi_device *sdev)
 	if (devinfo->flags & US_FL_BROKEN_FUA)
 		sdev->broken_fua = 1;
 
+	/* UAS also needs to support FL_ALWAYS_SYNC */
+	if (devinfo->flags & US_FL_ALWAYS_SYNC) {
+		sdev->skip_ms_page_3f = 1;
+		sdev->skip_ms_page_8 = 1;
+		sdev->wce_default_on = 1;
+	}
 	scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
 	return 0;
 }
-- 
2.1.4

[v2,1/2,usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver

[Alexander Kappner]

The ALWAYS_SYNC flag is currently honored by the usb-storage driver but not UAS
and is required to work around devices that become unstable upon being
queried for cache. This code is taken straight from:
drivers/usb/storage/scsiglue.c:284

Signed-off-by: Alexander Kappner <agk@godking.net>
---
 drivers/usb/storage/uas.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39..9e9de54 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -836,6 +836,12 @@ static int uas_slave_configure(struct scsi_device *sdev)
 	if (devinfo->flags & US_FL_BROKEN_FUA)
 		sdev->broken_fua = 1;
 
+	/* UAS also needs to support FL_ALWAYS_SYNC */
+	if (devinfo->flags & US_FL_ALWAYS_SYNC) {
+		sdev->skip_ms_page_3f = 1;
+		sdev->skip_ms_page_8 = 1;
+		sdev->wce_default_on = 1;
+	}
 	scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
 	return 0;
 }

Re: [PATCH v2 1/2] [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver

[Oliver Neukum]

Am Freitag, den 18.05.2018, 21:50 -0700 schrieb Alexander Kappner:
> The ALWAYS_SYNC flag is currently honored by the usb-storage driver but not UAS
> and is required to work around devices that become unstable upon being
> queried for cache. This code is taken straight from:
> drivers/usb/storage/scsiglue.c:284
> 
> Signed-off-by: Alexander Kappner <agk@godking.net>
Acked-by: Oliver Neukum <oneukum@suse.com>

[v2,1/2,usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver

[Oliver Neukum]

Am Freitag, den 18.05.2018, 21:50 -0700 schrieb Alexander Kappner:
> The ALWAYS_SYNC flag is currently honored by the usb-storage driver but not UAS
> and is required to work around devices that become unstable upon being
> queried for cache. This code is taken straight from:
> drivers/usb/storage/scsiglue.c:284
> 
> Signed-off-by: Alexander Kappner <agk@godking.net>
Acked-by: Oliver Neukum <oneukum@suse.com>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] usb-storage: Add compatibility quirk flags for G-Technologies G-Drive

[Alexander Kappner]

The "G-Drive" (sold by G-Technology) external USB 3.0 drive
 hangs on write access under UAS and usb-storage:

[  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
[  136.079180] print_req_error: critical target error, dev sdi, sector 0
[  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
[  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
[  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
[  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584052] Aborting journal on device sdi-8.

The proposed patch adds compatibility quirks. Because the drive requires two
quirks (one to work with UAS, and another to work with usb-storage), adding this
under unusual_devs.h and not just unusual_uas.h so kernels compiled without UAS
receive the quirk. With the patch, the drive works reliably on UAS and usb-
storage.
(tested on NEC Corporation uPD720200 USB 3.0 host controller).

Signed-off-by: Alexander Kappner <agk@godking.net>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Cc: stable <stable@vger.kernel.org>
---
 drivers/usb/storage/unusual_devs.h | 9 +++++++++
 drivers/usb/storage/unusual_uas.h  | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 747d3a9..22fcfcc 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
 		"Micro Mini 1GB",
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
 
+/* "G-DRIVE" external HDD hangs on write without these.
+ * Patch submitted by Alexander Kappner <agk@godking.net>
+ */
+UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
+		"SimpleTech",
+		"External HDD",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_ALWAYS_SYNC),
+
 /*
  * Nick Bowler <nbowler@elliptictech.com>
  * SCSI stack spams (otherwise harmless) error messages.
diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
index 38434d8..d0bdebd 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -107,3 +107,12 @@ UNUSUAL_DEV(0x4971, 0x8017, 0x0000, 0x9999,
 		"External HDD",
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
 		US_FL_NO_REPORT_OPCODES),
+
+/* "G-DRIVE" external HDD hangs on write without these.
+ * Patch submitted by Alexander Kappner <agk@godking.net>
+ */
+UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
+		"SimpleTech",
+		"External HDD",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_ALWAYS_SYNC),
-- 
2.1.4

[v2,2/2,usb-storage] Add compatibility quirk flags for G-Technologies G-Drive

[Alexander Kappner]

The "G-Drive" (sold by G-Technology) external USB 3.0 drive
 hangs on write access under UAS and usb-storage:

[  136.079121] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  136.079144] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  136.079152] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  136.079176] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
[  136.079180] print_req_error: critical target error, dev sdi, sector 0
[  136.079183] Buffer I/O error on dev sdi, logical block 0, lost sync page write
[  136.173148] EXT4-fs (sdi): mounted filesystem with ordered data mode. Opts: (null)
[  140.583998] sd 15:0:0:0: [sdi] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[  140.584010] sd 15:0:0:0: [sdi] tag#0 Sense Key : Illegal Request [current]
[  140.584016] sd 15:0:0:0: [sdi] tag#0 Add. Sense: Invalid field in cdb
[  140.584022] sd 15:0:0:0: [sdi] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
[  140.584025] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584044] print_req_error: critical target error, dev sdi, sector 3905159192
[  140.584052] Aborting journal on device sdi-8.

The proposed patch adds compatibility quirks. Because the drive requires two
quirks (one to work with UAS, and another to work with usb-storage), adding this
under unusual_devs.h and not just unusual_uas.h so kernels compiled without UAS
receive the quirk. With the patch, the drive works reliably on UAS and usb-
storage.
(tested on NEC Corporation uPD720200 USB 3.0 host controller).

Signed-off-by: Alexander Kappner <agk@godking.net>
---
 drivers/usb/storage/unusual_devs.h | 9 +++++++++
 drivers/usb/storage/unusual_uas.h  | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 747d3a9..22fcfcc 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -2321,6 +2321,15 @@ UNUSUAL_DEV(  0x4146, 0xba01, 0x0100, 0x0100,
 		"Micro Mini 1GB",
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NOT_LOCKABLE ),
 
+/* "G-DRIVE" external HDD hangs on write without these.
+ * Patch submitted by Alexander Kappner <agk@godking.net>
+ */
+UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
+		"SimpleTech",
+		"External HDD",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_ALWAYS_SYNC),
+
 /*
  * Nick Bowler <nbowler@elliptictech.com>
  * SCSI stack spams (otherwise harmless) error messages.
diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
index 38434d8..d0bdebd 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -107,3 +107,12 @@ UNUSUAL_DEV(0x4971, 0x8017, 0x0000, 0x9999,
 		"External HDD",
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
 		US_FL_NO_REPORT_OPCODES),
+
+/* "G-DRIVE" external HDD hangs on write without these.
+ * Patch submitted by Alexander Kappner <agk@godking.net>
+ */
+UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999,
+		"SimpleTech",
+		"External HDD",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_ALWAYS_SYNC),

Re: [PATCH v2 0/2] Re: usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Fri, 18 May 2018, Alexander Kappner wrote:

> v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and then
> adds the flag as quirks for the device at issue. This allows the G-Drive to work
> under both UAS and usb-storage.
> 
> Alexander Kappner (2):
>   [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
>   [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive
> 
>  drivers/usb/storage/uas.c          | 6 ++++++
>  drivers/usb/storage/unusual_devs.h | 9 +++++++++
>  drivers/usb/storage/unusual_uas.h  | 9 +++++++++
>  3 files changed, 24 insertions(+)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

This is okay with me.  Oliver, what do you think?

Alan Stern

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Fri, 18 May 2018, Alexander Kappner wrote:

> v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and then
> adds the flag as quirks for the device at issue. This allows the G-Drive to work
> under both UAS and usb-storage.
> 
> Alexander Kappner (2):
>   [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
>   [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive
> 
>  drivers/usb/storage/uas.c          | 6 ++++++
>  drivers/usb/storage/unusual_devs.h | 9 +++++++++
>  drivers/usb/storage/unusual_uas.h  | 9 +++++++++
>  3 files changed, 24 insertions(+)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

This is okay with me.  Oliver, what do you think?

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

Re: [usb-storage] Re: [PATCH v2 0/2] Re: usb-storage: Add quirks to make G-Technology "G-Drive" work

[Oliver Neukum]

Am Montag, den 21.05.2018, 13:47 -0400 schrieb Alan Stern:
> On Fri, 18 May 2018, Alexander Kappner wrote:
> 
> > v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and then
> > adds the flag as quirks for the device at issue. This allows the G-Drive to work
> > under both UAS and usb-storage.
> > 
> > Alexander Kappner (2):
> >   [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
> >   [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive
> > 
> >  drivers/usb/storage/uas.c          | 6 ++++++
> >  drivers/usb/storage/unusual_devs.h | 9 +++++++++
> >  drivers/usb/storage/unusual_uas.h  | 9 +++++++++
> >  3 files changed, 24 insertions(+)
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> This is okay with me.  Oliver, what do you think?

Perfect.

	Regards
		Oliver

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Oliver Neukum]

Am Montag, den 21.05.2018, 13:47 -0400 schrieb Alan Stern:
> On Fri, 18 May 2018, Alexander Kappner wrote:
> 
> > v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and then
> > adds the flag as quirks for the device at issue. This allows the G-Drive to work
> > under both UAS and usb-storage.
> > 
> > Alexander Kappner (2):
> >   [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
> >   [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive
> > 
> >  drivers/usb/storage/uas.c          | 6 ++++++
> >  drivers/usb/storage/unusual_devs.h | 9 +++++++++
> >  drivers/usb/storage/unusual_uas.h  | 9 +++++++++
> >  3 files changed, 24 insertions(+)
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> This is okay with me.  Oliver, what do you think?

Perfect.

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

Re: [usb-storage] Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Thu, 17 May 2018, Alexander Kappner wrote:

> Hi Alan,
> 
> thanks for reviewing. (This is my first contribution that touches 
> usb-storage, so please bear with me.)
> 
> > That's kind of weird.  Does the drive work under Windows in UAS mode?  
> 
> On the Windows 10 VM that I just spun up for testing this, access to the 
> drive uses "usbstor.inf" (rather than "uaspstor.sys"). So I believe the 
> answer is no. 
> 
> > If so, why are the WRITE(16) commands failing under Linux?
> 
> I don't know exactly why they're failing, but another entry in the 
> unusual_uas.h shows another SimpleTech device (only with a slightly 
> different product ID) needing the same UAS quirk (see 
> https://bugzilla.redhat.com/show_bug.cgi?id=1124119). So my guess is those 
> drives are just buggy. 

Okay, that's quite possible.

> > That doesn't quite make sense.  Since you prevent the uas driver from 
> > binding to this device, it will end up using usb-storage no matter how 
> > the kernel was built.  Therefore the second quirk flag has to go into 
> > unusual_devs.h no matter what.
> 
> Yes that's what I was trying to get at. So even though the UAS flag would 
> conceptually belong into unusual_uas, I'm putting both into unusual_devs to 
> avoid having to create two separate entries for the same device.
> 
> > You don't describe the second quirk flag at all.  Are you sure it is 
> > needed?
> 
> Yes. Without this flag, the device keeps throwing similar errors on 
> usb-storage. That's the same result I get on a host that doesn't have UAS 
> compiled in. Here's a dmesg:
> 
> [    2.183472] usb 3-1: New USB device found, idVendor=4971, idProduct=8024, bcdDevice=24.03
> [    2.184285] usb 3-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> [    2.184980] usb 3-1: Product: G-DRIVE
> [    2.185447] usb 3-1: Manufacturer: HGST
> [    2.185829] usb 3-1: SerialNumber: AA015010004C
> [    2.195509] usb-storage 3-1:1.0: USB Mass Storage device detected
> [    2.198668] Floppy drive(s): fd0 is 2.88M AMI BIOS
> [    2.202981] scsi host2: usb-storage 3-1:1.0
> ...
> [    3.233085] scsi 2:0:0:0: Direct-Access     G-DRIVE                   2403 PQ: 0 ANSI: 6
> [    3.234514] sd 2:0:0:0: Attached scsi generic sg1 type 0
> [    3.235465] sd 2:0:0:0: [sda] Very big device. Trying to use READ CAPACITY(16).
> [    3.236847] sd 2:0:0:0: [sda] 7814037168 512-byte logical blocks: (4.00 TB/3.64 TiB)
> [    3.237794] sd 2:0:0:0: [sda] 4096-byte physical blocks
> [    3.241255] sd 2:0:0:0: [sda] Write Protect is off
> [    3.242096] sd 2:0:0:0: [sda] Mode Sense: 47 00 10 08
> [    3.243595] sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports DPO and FUA
> [    3.257893]  sda: sda1 sda9
> [    3.261402] sd 2:0:0:0: [sda] Attached SCSI disk
> ...
> [   92.433428] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [   92.434759] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
> [   92.435637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
> [   92.436401] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
> [   92.437493] print_req_error: critical target error, dev sda, sector 0
> [   92.438211] Buffer I/O error on dev sda, logical block 0, lost sync page write
> [   92.516692] EXT4-fs (sda): mounted filesystem with ordered data mode. Opts: (null)
> [  101.449311] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  101.450598] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
> [  101.451401] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
> [  101.452041] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
> [  101.452906] print_req_error: critical target error, dev sda, sector 3905159192
> [  101.453593] print_req_error: critical target error, dev sda, sector 3905159192
> [  101.454889] Aborting journal on device sda-8.
> [  101.457103] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  101.457988] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
> [  101.458637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
> [  101.459250] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 00 00 00 00 08 00 00

That's bizarre too.  Even though the only difference is a MODE SENSE 
command, the command that actually failed was WRITE(16).

> Source code comments describe this as a known problem (scsiglue.c:182):
> 
>                 /*
>                  * Some devices don't like MODE SENSE with page=0x3f,
>                  * which is the command used for checking if a device
>                  * is write-protected.  Now that we tell the sd driver
>                  * to do a 192-byte transfer with this command the
>                  * majority of devices work fine, but a few still can't
>                  * handle it.  The sd driver will simply assume those
>                  * devices are write-enabled.
>                  */
>                 if (us->fflags & US_FL_NO_WP_DETECT)
>                         sdev->skip_ms_page_3f = 1;

Yeah, but that comment referred to driver that fail when they receive 
the MODE SENSE.

Have you tried using uas _and_ setting the sdev->skip_ms_page_3f flag?

Alan Stern

usb-storage: Add quirks to make G-Technology "G-Drive" work

[Alan Stern]

On Thu, 17 May 2018, Alexander Kappner wrote:

> Hi Alan,
> 
> thanks for reviewing. (This is my first contribution that touches 
> usb-storage, so please bear with me.)
> 
> > That's kind of weird.  Does the drive work under Windows in UAS mode?  
> 
> On the Windows 10 VM that I just spun up for testing this, access to the 
> drive uses "usbstor.inf" (rather than "uaspstor.sys"). So I believe the 
> answer is no. 
> 
> > If so, why are the WRITE(16) commands failing under Linux?
> 
> I don't know exactly why they're failing, but another entry in the 
> unusual_uas.h shows another SimpleTech device (only with a slightly 
> different product ID) needing the same UAS quirk (see 
> https://bugzilla.redhat.com/show_bug.cgi?id=1124119). So my guess is those 
> drives are just buggy. 

Okay, that's quite possible.

> > That doesn't quite make sense.  Since you prevent the uas driver from 
> > binding to this device, it will end up using usb-storage no matter how 
> > the kernel was built.  Therefore the second quirk flag has to go into 
> > unusual_devs.h no matter what.
> 
> Yes that's what I was trying to get at. So even though the UAS flag would 
> conceptually belong into unusual_uas, I'm putting both into unusual_devs to 
> avoid having to create two separate entries for the same device.
> 
> > You don't describe the second quirk flag at all.  Are you sure it is 
> > needed?
> 
> Yes. Without this flag, the device keeps throwing similar errors on 
> usb-storage. That's the same result I get on a host that doesn't have UAS 
> compiled in. Here's a dmesg:
> 
> [    2.183472] usb 3-1: New USB device found, idVendor=4971, idProduct=8024, bcdDevice=24.03
> [    2.184285] usb 3-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> [    2.184980] usb 3-1: Product: G-DRIVE
> [    2.185447] usb 3-1: Manufacturer: HGST
> [    2.185829] usb 3-1: SerialNumber: AA015010004C
> [    2.195509] usb-storage 3-1:1.0: USB Mass Storage device detected
> [    2.198668] Floppy drive(s): fd0 is 2.88M AMI BIOS
> [    2.202981] scsi host2: usb-storage 3-1:1.0
> ...
> [    3.233085] scsi 2:0:0:0: Direct-Access     G-DRIVE                   2403 PQ: 0 ANSI: 6
> [    3.234514] sd 2:0:0:0: Attached scsi generic sg1 type 0
> [    3.235465] sd 2:0:0:0: [sda] Very big device. Trying to use READ CAPACITY(16).
> [    3.236847] sd 2:0:0:0: [sda] 7814037168 512-byte logical blocks: (4.00 TB/3.64 TiB)
> [    3.237794] sd 2:0:0:0: [sda] 4096-byte physical blocks
> [    3.241255] sd 2:0:0:0: [sda] Write Protect is off
> [    3.242096] sd 2:0:0:0: [sda] Mode Sense: 47 00 10 08
> [    3.243595] sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports DPO and FUA
> [    3.257893]  sda: sda1 sda9
> [    3.261402] sd 2:0:0:0: [sda] Attached SCSI disk
> ...
> [   92.433428] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [   92.434759] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
> [   92.435637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
> [   92.436401] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 00 00 00 00 00 00 00 08 00 00
> [   92.437493] print_req_error: critical target error, dev sda, sector 0
> [   92.438211] Buffer I/O error on dev sda, logical block 0, lost sync page write
> [   92.516692] EXT4-fs (sda): mounted filesystem with ordered data mode. Opts: (null)
> [  101.449311] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  101.450598] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
> [  101.451401] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
> [  101.452041] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 18 00 00 00 08 00 00
> [  101.452906] print_req_error: critical target error, dev sda, sector 3905159192
> [  101.453593] print_req_error: critical target error, dev sda, sector 3905159192
> [  101.454889] Aborting journal on device sda-8.
> [  101.457103] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [  101.457988] sd 2:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] 
> [  101.458637] sd 2:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
> [  101.459250] sd 2:0:0:0: [sda] tag#0 CDB: Write(16) 8a 08 00 00 00 00 e8 c4 00 00 00 00 00 08 00 00

That's bizarre too.  Even though the only difference is a MODE SENSE 
command, the command that actually failed was WRITE(16).

> Source code comments describe this as a known problem (scsiglue.c:182):
> 
>                 /*
>                  * Some devices don't like MODE SENSE with page=0x3f,
>                  * which is the command used for checking if a device
>                  * is write-protected.  Now that we tell the sd driver
>                  * to do a 192-byte transfer with this command the
>                  * majority of devices work fine, but a few still can't
>                  * handle it.  The sd driver will simply assume those
>                  * devices are write-enabled.
>                  */
>                 if (us->fflags & US_FL_NO_WP_DETECT)
>                         sdev->skip_ms_page_3f = 1;

Yeah, but that comment referred to driver that fail when they receive 
the MODE SENSE.

Have you tried using uas _and_ setting the sdev->skip_ms_page_3f flag?

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