| Message ID | 20231207134441.298131-1-tasos@tasossah.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 772685c14743ad565bb271041ad3c262298cd6fc |
| Headers | show |
| Series | usb-storage: Add quirk for incorrect WP on Kingston DT Ultimate 3.0 G3 | expand |
On Thu, Dec 07, 2023 at 03:44:41PM +0200, Tasos Sahanidis wrote: > This flash drive reports write protect during the first mode sense. > > In the past this was not an issue as the kernel called revalidate twice, > thus asking the device for its write protect status twice, with write > protect being disabled in the second mode sense. > > However, since commit 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to > avoid calling revalidate twice") that is no longer the case, thus the > device shows up read only. > > [490891.289495] sd 12:0:0:0: [sdl] Write Protect is on > [490891.289497] sd 12:0:0:0: [sdl] Mode Sense: 2b 00 80 08 > > This does not appear to be a timing issue, as enabling the usbcore quirk > USB_QUIRK_DELAY_INIT has no effect on write protect. > > Fixes: 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to avoid calling revalidate twice") > Signed-off-by: Tasos Sahanidis <tasos@tasossah.com> > --- Acked-by: Alan Stern <stern@rowland.harvard.edu> > drivers/usb/storage/unusual_devs.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h > index 20dcbccb290b..fd68204374f2 100644 > --- a/drivers/usb/storage/unusual_devs.h > +++ b/drivers/usb/storage/unusual_devs.h > @@ -1305,6 +1305,17 @@ UNUSUAL_DEV( 0x090c, 0x6000, 0x0100, 0x0100, > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_INITIAL_READ10 ), > > +/* > + * Patch by Tasos Sahanidis <tasos@tasossah.com> > + * This flash drive always shows up with write protect enabled > + * during the first mode sense. > + */ > +UNUSUAL_DEV(0x0951, 0x1697, 0x0100, 0x0100, > + "Kingston", > + "DT Ultimate G3", > + USB_SC_DEVICE, USB_PR_DEVICE, NULL, > + US_FL_NO_WP_DETECT), > + > /* > * This Pentax still camera is not conformant > * to the USB storage specification: - > -- > 2.25.1 >
Tasos, > This flash drive reports write protect during the first mode sense. > > In the past this was not an issue as the kernel called revalidate twice, > thus asking the device for its write protect status twice, with write > protect being disabled in the second mode sense. > > However, since commit 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to > avoid calling revalidate twice") that is no longer the case, thus the > device shows up read only. > > [490891.289495] sd 12:0:0:0: [sdl] Write Protect is on > [490891.289497] sd 12:0:0:0: [sdl] Mode Sense: 2b 00 80 08 > > This does not appear to be a timing issue, as enabling the usbcore quirk > USB_QUIRK_DELAY_INIT has no effect on write protect. That's very interesting. I received a couple of reports about USB devices from other vendors that had a similar problems with WP but I always assumed it to be a timing issue. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Hi Martin, > That's very interesting. I received a couple of reports about USB > devices from other vendors that had a similar problems with WP but I > always assumed it to be a timing issue. Your remark got me thinking that since you have gotten more reports about this issue and it's not a one off, then perhaps I should dig further to figure out a better solution, instead of simply adding an UNUSUAL_DEVICE entry (just to get this specific device working since it's been broken for more than a year). First, I added a 5 second sleep right at the top of sd_do_mode_sense(), just to verify that a delay isn't needed after the first SCSI command has been issued. The device still reported write protect, so that confirms it is not a timing issue. Then, I proceeded to call scsi_mode_sense() multiple times inside sd_do_mode_sense(), one right after the other, and to my surprise that also did not clear WP. I then called sd_revalidate_disk() multiple times and noticed that WP was cleared only after the call to device_add_disk(). That led me to figure out that this device needs a READ_10 command before WP is disabled. The following is enough to get the device to show up without WP: --- /tmp/linux-6.6.5/drivers/scsi/sd.c 2023-12-08 09:52:25.000000000 +0200 +++ src/linux-6.6.5/drivers/scsi/sd.c 2023-12-11 03:23:22.109604623 +0200 @@ -2772,6 +2772,11 @@ } if (sdp->use_192_bytes_for_3f) { + unsigned char cmd[10] = {READ_10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00}; + unsigned char readbuf[512]; + scsi_execute_cmd(sdkp->device, cmd, REQ_OP_DRV_IN, readbuf, sizeof(readbuf), + SD_TIMEOUT, sdkp->max_retries, NULL); + res = sd_do_mode_sense(sdkp, 0, 0x3F, buffer, 192, &data, NULL); } else { /* which results in: [ 56.050090] sd 10:0:0:0: [sdb] 61472768 512-byte logical blocks: (31.5 GB/29.3 GiB) [ 56.062517] *** thread awakened [ 56.062532] Command READ_10 (10 bytes) [ 56.062543] bytes: 28 00 00 00 00 00 00 00 01 00 [ 56.062555] Bulk Command S 0x43425355 T 0x5 L 512 F 128 Trg 0 LUN 0 CL 10 [ 56.062570] xfer 31 bytes [ 56.063137] Status code 0; transferred 31/31 [ 56.063152] -- transfer complete [ 56.063159] Bulk command transfer result=0 [ 56.063168] xfer 512 bytes, 1 entries [ 56.188566] Status code 0; transferred 512/512 [ 56.188586] -- transfer complete [ 56.188594] Bulk data transfer result 0x0 [ 56.188602] Attempting to get CSW... [ 56.188609] xfer 13 bytes [ 56.190112] Status code 0; transferred 13/13 [ 56.190120] -- transfer complete [ 56.190123] Bulk status result = 0 [ 56.190126] Bulk Status S 0x53425355 T 0x5 R 0 Stat 0x0 [ 56.190132] scsi cmd done, result=0x0 [ 56.190158] *** thread sleeping [ 56.190245] *** thread awakened [ 56.190255] Command MODE_SENSE (6 bytes) [ 56.190265] bytes: 1a 00 3f 00 c0 00 [ 56.190276] Bulk Command S 0x43425355 T 0x6 L 192 F 128 Trg 0 LUN 0 CL 6 [ 56.190291] xfer 31 bytes [ 56.190733] Status code 0; transferred 31/31 [ 56.190745] -- transfer complete [ 56.190752] Bulk command transfer result=0 [ 56.190761] xfer 192 bytes, 1 entries [ 56.191741] Status code -121; transferred 44/192 [ 56.191748] -- short read transfer [ 56.191750] Bulk data transfer result 0x1 [ 56.191753] Attempting to get CSW... [ 56.191756] xfer 13 bytes [ 56.192072] Status code 0; transferred 13/13 [ 56.192077] -- transfer complete [ 56.192079] Bulk status result = 0 [ 56.192082] Bulk Status S 0x53425355 T 0x6 R 148 Stat 0x0 [ 56.192087] scsi cmd done, result=0x0 [ 56.192111] *** thread sleeping [ 56.192207] sd 10:0:0:0: [sdb] Write Protect is off [ 56.197229] sd 10:0:0:0: [sdb] Mode Sense: 2b 00 00 08 It is worth noting that Windows 10 (2004) also performs this sequence: - READ_CAPACITY (10) - READ_10 (10) LBA: 0, Len: 1 - MODE_SENSE (6) page: 0x3f I'm not sure how to proceed from here. Since there are other devices affected, I am not sure this patch with the quirk should be applied. At the very least though, I'd need to submit a V2 to correct the commit message and comment. On the other hand, I'm worried that by adding a READ_10 before every mode sense, something else might break. If you do believe that adding a READ_10 is the right way forward, I can try cleaning up the diff above (with some advice) and submit it properly. Any suggestions are welcome. -- Tasos
Tasos, > I then called sd_revalidate_disk() multiple times and noticed that WP > was cleared only after the call to device_add_disk(). That led me to > figure out that this device needs a READ_10 command before WP is > disabled. Awesome work, thanks so much for debugging this! > On the other hand, I'm worried that by adding a READ_10 before every > mode sense, something else might break. If you do believe that adding a > READ_10 is the right way forward, I can try cleaning up the diff above > (with some advice) and submit it properly. Any suggestions are welcome. Let me mull over it until tomorrow.
diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h index 20dcbccb290b..fd68204374f2 100644 --- a/drivers/usb/storage/unusual_devs.h +++ b/drivers/usb/storage/unusual_devs.h @@ -1305,6 +1305,17 @@ UNUSUAL_DEV( 0x090c, 0x6000, 0x0100, 0x0100, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_INITIAL_READ10 ), +/* + * Patch by Tasos Sahanidis <tasos@tasossah.com> + * This flash drive always shows up with write protect enabled + * during the first mode sense. + */ +UNUSUAL_DEV(0x0951, 0x1697, 0x0100, 0x0100, + "Kingston", + "DT Ultimate G3", + USB_SC_DEVICE, USB_PR_DEVICE, NULL, + US_FL_NO_WP_DETECT), + /* * This Pentax still camera is not conformant * to the USB storage specification: -
This flash drive reports write protect during the first mode sense. In the past this was not an issue as the kernel called revalidate twice, thus asking the device for its write protect status twice, with write protect being disabled in the second mode sense. However, since commit 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to avoid calling revalidate twice") that is no longer the case, thus the device shows up read only. [490891.289495] sd 12:0:0:0: [sdl] Write Protect is on [490891.289497] sd 12:0:0:0: [sdl] Mode Sense: 2b 00 80 08 This does not appear to be a timing issue, as enabling the usbcore quirk USB_QUIRK_DELAY_INIT has no effect on write protect. Fixes: 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to avoid calling revalidate twice") Signed-off-by: Tasos Sahanidis <tasos@tasossah.com> --- drivers/usb/storage/unusual_devs.h | 11 +++++++++++ 1 file changed, 11 insertions(+)