From nobody Wed May 07 21:37:18 2025 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Zt7rt5sxtz5vKrY; Wed, 07 May 2025 21:37:18 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Zt7rt3f04z3mPN; Wed, 07 May 2025 21:37:18 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1746653838; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=MNWHCJwiw7DJ3B/iValItA+ZO8r5mt7hGg2Jlh1t4Fo=; b=Ldi1ClPsTCL63bO+rrIamHqEFXsaJe4D9QJPxz6kVUOnNNqL5pIryt/PUM30sZgNDHszAo 2MmtYqX/LzrP6O6gpZB3Gjy4/3+b3qoJ5fY9aBvJ7UKNHF7ksTocGfnwcFNIYG4WjWMlyU ++Ye2h1bHi0ROfDfA1WvjrxMzzBLhb6nGH6azz2sRxAYI/D55MWA9SJhwcCG1QSd5WAgQZ 7iezYAbIGvAfbIZzOAVsCqWNht+Kf/mL8xlbygoI05Rkjlra3i8mtCItJ5eipll7fXOHfT TZ8nC6N5xUiArSb84Fd0dRZ97bNFI87eXY+HpL2AqztTBwTSOH3CfYaLFkT0lA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1746653838; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=MNWHCJwiw7DJ3B/iValItA+ZO8r5mt7hGg2Jlh1t4Fo=; b=D+oAHPYxRVZbokJo8PHAvr+mLkHkA7yPmH+PMTpUIseQXPUQy0IUmpCn9rxrAG6kInlzZS M+h+jbKF+yZWR35xxWMg8t2lHzLaVbuaxFzR8TKcmnQgZ4zFwxyy9ezHseDbi9v42v+aLI o4fCrYxOcwf8iTbLPYDFgHoNBx24hQTXM+0eFjBuj5K8gzQ6EJU8WxNBoix4PNe3RpYb8S 0iwP1cRTpgJrF2oQJnkFRnFO+1QQmxp3tah8PHa2KB7fkSEZaBIFFMnryGmx7iPOMGKdzq s0bZRyPZeN54tX3l1HeG7WCkg5ufHYkwFM1jhFnouz+b14ZmOGLLD+l/lAw1Rw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1746653838; a=rsa-sha256; cv=none; b=svao1FqLyzYBZjVcmps1NLWIrGXZAQZmjY6H7DCs0MKYyBl0x4GW+wMy89uhMzlH3AwaBq VEIXRCzpz3w9WuEyFb8my48lXXxXjkNyX8/qWasE51olrG3DL1uELgQsZpeifklN8J7ok7 DJtwnVlvhUZMG4UpK71M5MiljQDiaXCZbsuJMAs2MYpM2D4yZxDYlrXv4Tz3T97kL8b+rS y93crs2/fNrmSko3MEPK/kpNTbX9xc4edv3PXin9VmK1TNpOKaywlxesGQWpZsdarILMz4 ++nT0QWaZC3/k+G5jDPUX5Xs75EWpbFFAgCxBn65+A71RbPkBM/6C/WPDyZ27Q== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4Zt7rt1003zmpp; Wed, 07 May 2025 21:37:18 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 547LbIg9092323; Wed, 7 May 2025 21:37:18 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 547LbI6c092320; Wed, 7 May 2025 21:37:18 GMT (envelope-from git) Date: Wed, 7 May 2025 21:37:18 GMT Message-Id: <202505072137.547LbI6c092320@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Warner Losh Subject: git: d41600e59c3f - main - usb: Make autoquirk code optional and opt out List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: imp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: d41600e59c3f13419066e9dd771a03328c44624f Auto-Submitted: auto-generated The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=d41600e59c3f13419066e9dd771a03328c44624f commit d41600e59c3f13419066e9dd771a03328c44624f Author: Warner Losh AuthorDate: 2025-05-07 16:08:01 +0000 Commit: Warner Losh 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