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

From: Alan Stern <stern@rowland.harvard.edu>
To: Alexander Kappner <agk@godking.net>
Cc: gregkh@linuxfoundation.org, <linux-usb@vger.kernel.org>,
	<usb-storage@lists.one-eyed-alien.net>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work
Date: Wed, 16 May 2018 16:55:14 -0400 (EDT)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1805161647330.1604-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <1526500880-26512-1-git-send-email-agk@godking.net>

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