Date: Wed, 7 May 2025 21:37:18 GMT From: Warner Losh <imp@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: d41600e59c3f - main - usb: Make autoquirk code optional and opt out Message-ID: <202505072137.547LbI6c092320@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=d41600e59c3f13419066e9dd771a03328c44624f commit d41600e59c3f13419066e9dd771a03328c44624f Author: Warner Losh <imp@FreeBSD.org> AuthorDate: 2025-05-07 16:08:01 +0000 Commit: Warner Losh <imp@FreeBSD.org> CommitDate: 2025-05-07 21:36:55 +0000 usb: Make autoquirk code optional and opt out There are significant problems with the current autoquirk code. This results in quite a bit of bogus over-quirking. Most commands don't do the proper "sense" dance to get the scsi sense codes to see if the failures are interesting or not. A number of 'sleeps' are used to try to get around this, but they are racy. Rather than fix these, use better hueristics just introduced to catch SYNCHRONIZE CACHE problems, etc. The test for getting max lun number was bogus. It would set this quirk both on errors and when 0 was returned. It appears to be an attempt to filter our REPORT LUNS error messages that are actually benign (we ignore the errors properly). These errors are also only filtered sometimes, so the test is unreliable. In addition, it's doing exactly the same test that the umass driver is doing and recovering in the same way. There's no value add here. The TEST UNIT READY almost always fails because the drive is becoming ready. The SENSE is usually UNIT ATTENTION 28/0 "Drive went from not ready to ready" which is a normal condition. The crazy looping to get INQUIRY data is odd. It shouldn't be needed and rarely actually fails (I've not seen any, despite using this code on some really sketchy drives). It should set a NO_INQUIRY quirk if it fails, but instead sets a whole bunch of other, mostly unrelated quirks if it fails. The INQUIRY code also doesn't recognie RBC devices as well as DIRECT devices. This means it fails on some older generations of cameras that could actually benefit from this code. The SYNCHRONIZE CACHE test is flawed. It will do the same failed test over and over again in the event the command succeeds. There are better ways to detect probelms. The START STOP test is useless. It doesn't really help on any of the devices I've tested on. It appears to be another result of the failure to properly obtain the SENSE code and do appropriate things with it. The PREVENT ALLOW test is useless. It is overwhelmingly used to prevent an error message later. However, after it was added the error message was changed to be informative and not scary. We properly probe this at runtime on all the devices I tested on. At the end of the tests, we try to clear the SENSE errors, but do so imperfectly. Only one is cleared and we use INQUIRY rather than the better TEST UNIT READY. Attempted re-write to fix this caused additional problems as the reset code was not at all robust (the same sequnce in umass / CAM worked when we disabled this code). In addition, the over-quirking and hair-triggered declaration that SYNCHRONIE CACHE is bad would mean that some working drives that have cache wouldn't flush the cache when WCE=1, leading to corruption. Thankfully, nearly all (but not all) the USB sticks I have default to WCE=0. One, however, did default to WCE=1 and some allow setting it (despite the fact this is a bad idea on removeable devices). However, for real disks attached via USB could be tripped up over this. When we do reset, some small subset of drives are now failing to probe. There are reports on the FreeBSD forums that at least one ebook reader no longer works. A different ebook reads is affected as well (one of my long-time friends has htis). in my collection, one USB memory stick, one SD card reader and one USB to generic PATA adapter no longer work. All of them are pretty obscure (you could literally say they were found in my junk drawer), but are troubling. These problems appear to disappear if we stop doing the auto-quirk code. For all these reasons, I'm turning this off and will likely remove it entirely in the future once the alternative SYNC CACHE code has provent itself. Differential Revision: https://reviews.freebsd.org/D49477 Sponsored by: Netflix --- sys/dev/usb/usb_device.c | 2 +- sys/dev/usb/usb_freebsd.h | 1 + sys/dev/usb/usb_freebsd_loader.h | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c index e478fa66b6c9..49195bd0af38 100644 --- a/sys/dev/usb/usb_device.c +++ b/sys/dev/usb/usb_device.c @@ -2061,7 +2061,7 @@ repeat_set_config: } #endif } -#if USB_HAVE_MSCTEST +#if USB_HAVE_MSCTEST_AUTOQUIRK if (set_config_failed == 0 && config_index == 0 && usb_test_quirk(&uaa, UQ_MSC_NO_START_STOP) == 0 && usb_test_quirk(&uaa, UQ_MSC_NO_PREVENT_ALLOW) == 0 && diff --git a/sys/dev/usb/usb_freebsd.h b/sys/dev/usb/usb_freebsd.h index d67e230e72dd..02ae6b245134 100644 --- a/sys/dev/usb/usb_freebsd.h +++ b/sys/dev/usb/usb_freebsd.h @@ -42,6 +42,7 @@ #define USB_HAVE_TT_SUPPORT 1 #define USB_HAVE_POWERD 1 #define USB_HAVE_MSCTEST 1 +#define USB_HAVE_MSCTEST_AUTOQUIRK 0 #define USB_HAVE_MSCTEST_DETACH 1 #define USB_HAVE_PF 1 #define USB_HAVE_ROOT_MOUNT_HOLD 1 diff --git a/sys/dev/usb/usb_freebsd_loader.h b/sys/dev/usb/usb_freebsd_loader.h index 404456f76152..edc6bcc9720c 100644 --- a/sys/dev/usb/usb_freebsd_loader.h +++ b/sys/dev/usb/usb_freebsd_loader.h @@ -42,6 +42,7 @@ #define USB_HAVE_TT_SUPPORT 1 #define USB_HAVE_POWERD 1 #define USB_HAVE_MSCTEST 1 +#define USB_HAVE_MSCTEST_AUTOQUIRK 0 #define USB_HAVE_MSCTEST_DETACH 0 #define USB_HAVE_PF 0 #define USB_HAVE_ROOT_MOUNT_HOLD 0
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202505072137.547LbI6c092320>