Re: git: 7520b88860d7 - main - usb(4): Automagically apply all quirks for USB mass storage devices.
- Reply: Hans Petter Selasky : "Re: git: 7520b88860d7 - main - usb(4): Automagically apply all quirks for USB mass storage devices."
- In reply to: Hans Petter Selasky : "git: 7520b88860d7 - main - usb(4): Automagically apply all quirks for USB mass storage devices."
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Sun, 27 Feb 2022 19:48:25 UTC
Hi,
I've an xhci issue with this change on my ARM Morello Board.
I've attached boot log. Can you look?
Thanks.
Ruslan
On Thu, Feb 24, 2022 at 09:30:15AM +0000, Hans Petter Selasky wrote:
> The branch main has been updated by hselasky:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=7520b88860d7a79432e12ffcc47056844518bb62
>
> commit 7520b88860d7a79432e12ffcc47056844518bb62
> Author: Hans Petter Selasky <hselasky@FreeBSD.org>
> AuthorDate: 2022-02-21 08:24:28 +0000
> Commit: Hans Petter Selasky <hselasky@FreeBSD.org>
> CommitDate: 2022-02-24 09:28:55 +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
> MFC after: 1 week
> Sponsored by: NVIDIA Networking
> ---
> 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 65c72b06e244..674c12186f86 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 634507fc65ca..6564182a97b0 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);