| Message ID | 1526561908.15506.5.camel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
> 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
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
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(+)
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
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
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