Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 3 Mar 2022 14:41:48 GMT
From:      Hans Petter Selasky <hselasky@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: cf912859d228 - stable/13 - usb(4): Automagically apply all quirks for USB mass storage devices.
Message-ID:  <202203031441.223Efmm3018346@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by hselasky:

URL: https://cgit.FreeBSD.org/src/commit/?id=cf912859d228b41ef62b4233cc198a1a9e4ece31

commit cf912859d228b41ef62b4233cc198a1a9e4ece31
Author:     Hans Petter Selasky <hselasky@FreeBSD.org>
AuthorDate: 2022-02-21 08:24:28 +0000
Commit:     Hans Petter Selasky <hselasky@FreeBSD.org>
CommitDate: 2022-03-03 14:41:12 +0000

    usb(4): Automagically apply all quirks for USB mass storage devices.
    
    Currently there are five quirks the USB stack tries to automagically detect:
    - UQ_MSC_NO_PREVENT_ALLOW
    - UQ_MSC_NO_SYNC_CACHE
    - UQ_MSC_NO_TEST_UNIT_READY
    - UQ_MSC_NO_GETMAXLUN
    - UQ_MSC_NO_START_STOP
    
    If any of the quirks above are set, no further quirks will be probed.
    
    If any of the USB mass storage tests fail, the USB device is
    re-enumerated as a last resort to clear any error states from the
    device. Then the USB stack will try to probe and attach the umass<N>
    device passing the detected quirks.
    
    While at it give more details in dmesg about what is going on.
    
    Tested by:              several
    Submitted by:           Idwer Vollering <vidwer_fbsdbugs@gmail.com>
    Differential Revision:  https://reviews.freebsd.org/D30919
    Sponsored by:           NVIDIA Networking
    
    (cherry picked from commit 7520b88860d7a79432e12ffcc47056844518bb62)
---
 sys/dev/usb/storage/umass.c |   7 +++
 sys/dev/usb/usb_device.c    |   5 +-
 sys/dev/usb/usb_msctest.c   | 133 +++++++++++++++++++++++++++-----------------
 sys/dev/usb/usb_msctest.h   |   4 +-
 4 files changed, 96 insertions(+), 53 deletions(-)

diff --git a/sys/dev/usb/storage/umass.c b/sys/dev/usb/storage/umass.c
index 8260226e5d12..b8b502494264 100644
--- a/sys/dev/usb/storage/umass.c
+++ b/sys/dev/usb/storage/umass.c
@@ -2294,6 +2294,13 @@ umass_cam_action(struct cam_sim *sim, union ccb *ccb)
 						xpt_done(ccb);
 						goto done;
 					}
+				} else if (sc->sc_transfer.cmd_data[0] == START_STOP_UNIT) {
+					if (sc->sc_quirks & NO_START_STOP) {
+						ccb->csio.scsi_status = SCSI_STATUS_OK;
+						ccb->ccb_h.status = CAM_REQ_CMP;
+						xpt_done(ccb);
+						goto done;
+					}
 				}
 				umass_command_start(sc, dir, ccb->csio.data_ptr,
 				    ccb->csio.dxfer_len,
diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c
index 322e4f5401ba..8d0e7961f675 100644
--- a/sys/dev/usb/usb_device.c
+++ b/sys/dev/usb/usb_device.c
@@ -2047,13 +2047,16 @@ repeat_set_config:
 	}
 #if USB_HAVE_MSCTEST
 	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 &&
 	    usb_test_quirk(&uaa, UQ_MSC_NO_SYNC_CACHE) == 0 &&
+	    usb_test_quirk(&uaa, UQ_MSC_NO_TEST_UNIT_READY) == 0 &&
 	    usb_test_quirk(&uaa, UQ_MSC_NO_GETMAXLUN) == 0) {
 		/*
 		 * Try to figure out if there are any MSC quirks we
 		 * should apply automatically:
 		 */
-		err = usb_msc_auto_quirk(udev, 0);
+		err = usb_msc_auto_quirk(udev, 0, &uaa);
 
 		if (err != 0) {
 			set_config_failed = 1;
diff --git a/sys/dev/usb/usb_msctest.c b/sys/dev/usb/usb_msctest.c
index 0fffd99a73c4..5dcf8d151119 100644
--- a/sys/dev/usb/usb_msctest.c
+++ b/sys/dev/usb/usb_msctest.c
@@ -2,7 +2,8 @@
 /*-
  * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
  *
- * Copyright (c) 2008,2011 Hans Petter Selasky. All rights reserved.
+ * Copyright (c) 2008-2022 Hans Petter Selasky.
+ * Copyright (c) 2021-2022 Idwer Vollering.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -29,9 +30,6 @@
 /*
  * The following file contains code that will detect USB autoinstall
  * disks.
- *
- * TODO: Potentially we could add code to automatically detect USB
- * mass storage quirks for not supported SCSI commands!
  */
 
 #ifdef USB_GLOBAL_INCLUDE_FILE
@@ -97,7 +95,8 @@ enum {
 static uint8_t scsi_test_unit_ready[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
 static uint8_t scsi_inquiry[] = { 0x12, 0x00, 0x00, 0x00, SCSI_INQ_LEN, 0x00 };
 static uint8_t scsi_rezero_init[] =     { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00 };
-static uint8_t scsi_start_stop_unit[] = { 0x1b, 0x00, 0x00, 0x00, 0x02, 0x00 };
+static uint8_t scsi_start_unit[] = { 0x1b, 0x00, 0x00, 0x00, 0x01, 0x00 };
+static uint8_t scsi_stop_unit[] = { 0x1b, 0x00, 0x00, 0x00, 0x02, 0x00 };
 static uint8_t scsi_ztestor_eject[] =   { 0x85, 0x01, 0x01, 0x01, 0x18, 0x01,
 					  0x01, 0x01, 0x01, 0x01, 0x00, 0x00 };
 static uint8_t scsi_cmotech_eject[] =   { 0xff, 0x52, 0x44, 0x45, 0x56, 0x43,
@@ -759,28 +758,49 @@ usb_msc_get_max_lun(struct usb_device *udev, uint8_t iface_index)
 	return (buf);
 }
 
+#define	USB_ADD_QUIRK(udev, any, which) do { \
+	if (usb_get_manufacturer(udev) != NULL && usb_get_product(udev) != NULL) { \
+		DPRINTFN(0, #which " set for USB mass storage device %s %s (0x%04x:0x%04x)\n", \
+			usb_get_manufacturer(udev), \
+			usb_get_product(udev), \
+			UGETW(udev->ddesc.idVendor), \
+			UGETW(udev->ddesc.idProduct)); \
+	} else { \
+		DPRINTFN(0, #which " set for USB mass storage device, 0x%04x:0x%04x\n", \
+			UGETW(udev->ddesc.idVendor), \
+			UGETW(udev->ddesc.idProduct)); \
+	} \
+	usbd_add_dynamic_quirk(udev, which); \
+	any = 1; \
+} while (0)
+
 usb_error_t
-usb_msc_auto_quirk(struct usb_device *udev, uint8_t iface_index)
+usb_msc_auto_quirk(struct usb_device *udev, uint8_t iface_index,
+    const struct usb_attach_arg *uaa)
 {
 	struct bbb_transfer *sc;
 	uint8_t timeout;
 	uint8_t is_no_direct;
 	uint8_t sid_type;
+	uint8_t any_quirk;
 	int err;
 
 	sc = bbb_attach(udev, iface_index, UICLASS_MASS);
 	if (sc == NULL)
 		return (0);
 
+	any_quirk = 0;
+
 	/*
 	 * Some devices need a delay after that the configuration
 	 * value is set to function properly:
 	 */
 	usb_pause_mtx(NULL, hz);
 
-	if (usb_msc_get_max_lun(udev, iface_index) == 0) {
+	if (usb_test_quirk(uaa, UQ_MSC_NO_GETMAXLUN) == 0 &&
+	    usb_msc_get_max_lun(udev, iface_index) == 0) {
 		DPRINTF("Device has only got one LUN.\n");
-		usbd_add_dynamic_quirk(udev, UQ_MSC_NO_GETMAXLUN);
+		USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_GETMAXLUN);
 	}
 
 	is_no_direct = 1;
@@ -807,37 +827,40 @@ usb_msc_auto_quirk(struct usb_device *udev, uint8_t iface_index)
 		goto done;
 	}
 
-	err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
-	    &scsi_test_unit_ready, sizeof(scsi_test_unit_ready),
-	    USB_MS_HZ);
+	if (usb_test_quirk(uaa, UQ_MSC_NO_TEST_UNIT_READY) == 0) {
+		err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0,
+		    &scsi_test_unit_ready, sizeof(scsi_test_unit_ready),
+		    USB_MS_HZ);
 
-	if (err != 0) {
-		if (err != ERR_CSW_FAILED)
-			goto error;
-		DPRINTF("Test unit ready failed\n");
+		if (err != 0) {
+			if (err != ERR_CSW_FAILED)
+				goto error;
+			USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_TEST_UNIT_READY);
+		}
 	}
 
-	err = bbb_command_start(sc, DIR_OUT, 0, NULL, 0,
-	    &scsi_prevent_removal, sizeof(scsi_prevent_removal),
-	    USB_MS_HZ);
-
-	if (err == 0) {
-		err = bbb_command_start(sc, DIR_OUT, 0, NULL, 0,
-		    &scsi_allow_removal, sizeof(scsi_allow_removal),
+	if (usb_test_quirk(uaa, UQ_MSC_NO_PREVENT_ALLOW) == 0) {
+		err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0,
+		    &scsi_prevent_removal, sizeof(scsi_prevent_removal),
 		    USB_MS_HZ);
-	}
 
-	if (err != 0) {
-		if (err != ERR_CSW_FAILED)
-			goto error;
-		DPRINTF("Device doesn't handle prevent and allow removal\n");
-		usbd_add_dynamic_quirk(udev, UQ_MSC_NO_PREVENT_ALLOW);
+		if (err == 0) {
+			err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0,
+			    &scsi_allow_removal, sizeof(scsi_allow_removal),
+			    USB_MS_HZ);
+		}
+
+		if (err != 0) {
+			if (err != ERR_CSW_FAILED)
+				goto error;
+			USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_PREVENT_ALLOW);
+		}
 	}
 
 	timeout = 1;
 
 retry_sync_cache:
-	err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
+	err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0,
 	    &scsi_sync_cache, sizeof(scsi_sync_cache),
 	    USB_MS_HZ);
 
@@ -845,9 +868,7 @@ retry_sync_cache:
 		if (err != ERR_CSW_FAILED)
 			goto error;
 
-		DPRINTF("Device doesn't handle synchronize cache\n");
-
-		usbd_add_dynamic_quirk(udev, UQ_MSC_NO_SYNC_CACHE);
+		USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_SYNC_CACHE);
 	} else {
 		/*
 		 * Certain Kingston memory sticks fail the first
@@ -872,11 +893,7 @@ retry_sync_cache:
 				if (timeout--)
 					goto retry_sync_cache;
 
-				DPRINTF("Device most likely doesn't "
-				    "handle synchronize cache\n");
-
-				usbd_add_dynamic_quirk(udev,
-				    UQ_MSC_NO_SYNC_CACHE);
+				USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_SYNC_CACHE);
 			} else {
 				if (err != ERR_CSW_FAILED)
 					goto error;
@@ -884,6 +901,18 @@ retry_sync_cache:
 		}
 	}
 
+	if (usb_test_quirk(uaa, UQ_MSC_NO_START_STOP) == 0) {
+		err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0,
+		    &scsi_start_unit, sizeof(scsi_start_unit),
+		    USB_MS_HZ);
+
+		if (err != 0) {
+			if (err != ERR_CSW_FAILED)
+				goto error;
+			USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_START_STOP);
+		}
+	}
+
 	/* clear sense status of any failed commands on the device */
 
 	err = bbb_command_start(sc, DIR_IN, 0, sc->buffer,
@@ -907,24 +936,28 @@ retry_sync_cache:
 		if (err != ERR_CSW_FAILED)
 			goto error;
 	}
+	goto done;
 
+error:
+	/* Apply most quirks */
+	USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_SYNC_CACHE);
+	USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_PREVENT_ALLOW);
+	USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_TEST_UNIT_READY);
+	USB_ADD_QUIRK(udev, any_quirk, UQ_MSC_NO_START_STOP);
 done:
 	bbb_detach(sc);
-	return (0);
 
-error:
- 	bbb_detach(sc);
-
-	DPRINTF("Device did not respond, enabling all quirks\n");
-
-	usbd_add_dynamic_quirk(udev, UQ_MSC_NO_SYNC_CACHE);
-	usbd_add_dynamic_quirk(udev, UQ_MSC_NO_PREVENT_ALLOW);
-	usbd_add_dynamic_quirk(udev, UQ_MSC_NO_TEST_UNIT_READY);
+	if (any_quirk) {
+		/* Unconfigure device, to clear software data toggle. */
+		usbd_set_config_index(udev, USB_UNCONFIG_INDEX);
 
-	/* Need to re-enumerate the device */
-	usbd_req_re_enumerate(udev, NULL);
+		/* Need to re-enumerate the device to clear its state. */
+		usbd_req_re_enumerate(udev, NULL);
+		return (USB_ERR_STALLED);
+	}
 
-	return (USB_ERR_STALLED);
+	/* No quirks were added, continue as usual. */
+	return (0);
 }
 
 usb_error_t
@@ -944,7 +977,7 @@ usb_msc_eject(struct usb_device *udev, uint8_t iface_index, int method)
 		    USB_MS_HZ);
 		DPRINTF("Test unit ready status: %s\n", usbd_errstr(err));
 		err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
-		    &scsi_start_stop_unit, sizeof(scsi_start_stop_unit),
+		    &scsi_stop_unit, sizeof(scsi_stop_unit),
 		    USB_MS_HZ);
 		break;
 	case MSC_EJECT_REZERO:
diff --git a/sys/dev/usb/usb_msctest.h b/sys/dev/usb/usb_msctest.h
index 6b5d3283738b..ba4e094bab60 100644
--- a/sys/dev/usb/usb_msctest.h
+++ b/sys/dev/usb/usb_msctest.h
@@ -2,7 +2,7 @@
 /*-
  * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
  *
- * Copyright (c) 2008 Hans Petter Selasky. All rights reserved.
+ * Copyright (c) 2008-2022 Hans Petter Selasky.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -44,7 +44,7 @@ int usb_iface_is_cdrom(struct usb_device *udev,
 usb_error_t usb_msc_eject(struct usb_device *udev,
 	    uint8_t iface_index, int method);
 usb_error_t usb_msc_auto_quirk(struct usb_device *udev,
-	    uint8_t iface_index);
+	    uint8_t iface_index, const struct usb_attach_arg *uaa);
 usb_error_t usb_msc_read_10(struct usb_device *udev,
 	    uint8_t iface_index, uint32_t lba, uint32_t blocks,
 	    void *buffer);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202203031441.223Efmm3018346>