Changeset View
Standalone View
sys/dev/usb/usb_msctest.c
| Context not available. | |||||
| * SPDX-License-Identifier: BSD-2-Clause-FreeBSD | * SPDX-License-Identifier: BSD-2-Clause-FreeBSD | ||||
| * | * | ||||
| * Copyright (c) 2008,2011 Hans Petter Selasky. All rights reserved. | * Copyright (c) 2008,2011 Hans Petter Selasky. All rights reserved. | ||||
| * Copyright (c) 2021,2022 Idwer Vollering. All rights reserved. | |||||
| * | * | ||||
| * Redistribution and use in source and binary forms, with or without | * Redistribution and use in source and binary forms, with or without | ||||
| * modification, are permitted provided that the following conditions | * modification, are permitted provided that the following conditions | ||||
| Context not available. | |||||
| /* | /* | ||||
| * The following file contains code that will detect USB autoinstall | * The following file contains code that will detect USB autoinstall | ||||
| * disks. | * disks. | ||||
| * | |||||
| * TODO: Potentially we could add code to automatically detect USB | |||||
| * mass storage quirks for not supported SCSI commands! | |||||
| */ | */ | ||||
| #ifdef USB_GLOBAL_INCLUDE_FILE | #ifdef USB_GLOBAL_INCLUDE_FILE | ||||
vidwer_fbsdbugs_gmail.com: This looks ugly, it's here to include usb_quirk_str[] | |||||
Done Inline ActionsPlease export the structure via the usb_quirk.h, like an extern declaration. hselasky: Please export the structure via the usb_quirk.h, like an extern declaration. | |||||
Done Inline Actions
..and drop the 'static' keyword? vidwer_fbsdbugs_gmail.com: > Please export the structure via the usb_quirk.h, like an extern declaration.
..and drop the… | |||||
Done Inline Actions
vidwer_fbsdbugs_gmail.com: > Please export the structure via the usb_quirk.h, like an extern declaration.
| |||||
Done Inline Actions
Yes, this needs to be marked as 'external', otherwise the printed quirk (e.g usb_quirk_str[UQ_MSC_NO_TEST_UNIT_READY] in the printf() below) will be (null). vidwer_fbsdbugs_gmail.com: > Please export the structure via the usb_quirk.h, like an extern declaration.
Yes, this needs… | |||||
| Context not available. | |||||
| static uint8_t scsi_prevent_removal[] = { 0x1e, 0, 0, 0, 1, 0 }; | static uint8_t scsi_prevent_removal[] = { 0x1e, 0, 0, 0, 1, 0 }; | ||||
| static uint8_t scsi_allow_removal[] = { 0x1e, 0, 0, 0, 0, 0 }; | static uint8_t scsi_allow_removal[] = { 0x1e, 0, 0, 0, 0, 0 }; | ||||
| /** | |||||
| * The first byte refers to the position in usb_quirk_str[], | |||||
Done Inline ActionsIs this comment correct? Isn't the first byte part of a SCSI command? hselasky: Is this comment correct? Isn't the first byte part of a SCSI command? | |||||
| * found in usb_quirk.c | |||||
| */ | |||||
Done Inline ActionsDoes this code compile if you use "static const" here? hselasky: Does this code compile if you use "static const" here? | |||||
Done Inline Actions
(..)/sys/dev/usb/usb_msctest.c:849:7: error: passing 'const uint8_t (*)[6]' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] &scsi_quirk_no_test_unit_ready, sizeof(scsi_quirk_no_test_unit_ready), vidwer_fbsdbugs_gmail.com: > Does this code compile if you use "static const" here?
(..)/sys/dev/usb/usb_msctest.c:849:7… | |||||
Done Inline ActionsI see. hselasky: I see. | |||||
Done Inline ActionsCan you explain what SCSI commands the entries above represent. Looks invalid to me! hselasky: Can you explain what SCSI commands the entries above represent. Looks invalid to me! | |||||
| static uint8_t scsi_quirk_no_test_unit_ready[] = { 0x1b, 0, 0, 0, 0, 0 }; | |||||
| static uint8_t scsi_quirk_no_sync_cache[] = { 0x22, 0, 0, 0, 0, 0 }; | |||||
Done Inline ActionsDitto. hselasky: Ditto. | |||||
| #ifndef USB_MSCTEST_BULK_SIZE | #ifndef USB_MSCTEST_BULK_SIZE | ||||
| #define USB_MSCTEST_BULK_SIZE 64 /* dummy */ | #define USB_MSCTEST_BULK_SIZE 64 /* dummy */ | ||||
Done Inline ActionsFor UQ_MSC_FORCE_PROTO_SCSI, the byte sequence probably will look like this: static uint8_t scsi_quirk_force_proto_scsi[] = { 0x2e, 0, 0, 0, 0, 0 }; vidwer_fbsdbugs_gmail.com: For UQ_MSC_FORCE_PROTO_SCSI, the byte sequence probably will look like this:
static uint8_t… | |||||
| #endif | #endif | ||||
Done Inline ActionsYou should use a pointer to uaa, and not copy the structure itself. hselasky: You should use a pointer to uaa, and not copy the structure itself. | |||||
| Context not available. | |||||
| } | } | ||||
| usb_error_t | 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, | ||||
Done Inline ActionsIt should read DPRINTFN(), then it will compile. vidwer_fbsdbugs_gmail.com: It should read DPRINTFN(), then it will compile. | |||||
Not Done Inline ActionsThe parameter text 'Applying ...' seems to get lost in both the if and else case. In other words: I don't see per-device quirk details in dmesg. vidwer_fbsdbugs_gmail.com: The parameter text 'Applying ...' seems to get lost in both the if and else case. In other… | |||||
Done Inline ActionsDid you add: options USB_DEBUG to your kernel configuration file? hselasky: Did you add:
options USB_DEBUG
to your kernel configuration file? | |||||
Done Inline ActionsAnd the code is compiled with: options USB_DEBUG ??? hselasky: And the code is compiled with:
options USB_DEBUG
??? | |||||
Not Done Inline ActionsUSB_DEBUG is set/present in sys/amd64/conf/GENERIC vidwer_fbsdbugs_gmail.com: USB_DEBUG is set/present in sys/amd64/conf/GENERIC | |||||
| struct usb_attach_arg uaa) | |||||
| { | { | ||||
| struct bbb_transfer *sc; | struct bbb_transfer *sc; | ||||
| uint8_t timeout; | uint8_t timeout; | ||||
Not Done Inline Actionswhitespace vidwer_fbsdbugs_gmail.com: whitespace | |||||
| Context not available. | |||||
Done Inline ActionsWhen err is zero, it means the command was successful, and then you apply the quirk? Please explain the logic here. I don't get it! --HPS hselasky: When err is zero, it means the command was successful, and then you apply the quirk?
That will… | |||||
| timeout = 1; | timeout = 1; | ||||
| if (usb_test_quirk(&uaa, UQ_MSC_NO_TEST_UNIT_READY) == 0) { | |||||
| err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0, | |||||
Done Inline ActionsThe question here is whether or not sending this command, at all, hangs the drive and if so, do we have a quirk that prevents that. imp: The question here is whether or not sending this command, at all, hangs the drive and if so, do… | |||||
Done Inline ActionsMaybe if the quirks is already set, don't query for it? That would avoid hanging devices. Also, a list of USB mass storage devices and detected quirks would be nice! hselasky: Maybe if the quirks is already set, don't query for it? That would avoid hanging devices.
Also… | |||||
Done Inline Actions
That's what I'm thinking. There's no need to probe fore something that's already quirked out. And that would be the best way to 'fail safe' with this bug. It's no worse than today, but future entries can be avoided... imp: > Maybe if the quirks is already set, don't query for it? That would avoid hanging devices. | |||||
Done Inline Actions
Right. It seems quirks are primarily set at sc_quirks in umass_softc.
So print vendor and product from scsi_inquiry_data. vidwer_fbsdbugs_gmail.com: > Maybe if the quirks is already set, don't query for it? That would avoid hanging devices. | |||||
Done Inline ActionsI'm not sure how to add and pass umass_probe_proto or umass_softc (for quirks or sc_quirks) in usb_msctest.c vidwer_fbsdbugs_gmail.com: I'm not sure how to add and pass umass_probe_proto or umass_softc (for quirks or sc_quirks) in… | |||||
Done Inline ActionsShould be DIR_NONE when the datalength is zero. hselasky: Should be DIR_NONE when the datalength is zero. | |||||
| &scsi_quirk_no_test_unit_ready, sizeof(scsi_quirk_no_test_unit_ready), | |||||
| USB_MS_HZ); | |||||
| if (err == 0) { | |||||
Done Inline ActionsFor the final version the printf() should be replaced with debug prints: DPRINTF()'s Controlled by hw.usb.debug hselasky: For the final version the printf() should be replaced with debug prints:
DPRINTF()'s… | |||||
Done Inline ActionsThe additional text is now falling off, even with hw.usb.debug=0. vidwer_fbsdbugs_gmail.com: The additional text is now falling off, even with hw.usb.debug=0. | |||||
| if (usb_get_manufacturer(udev) != NULL && usb_get_product(udev) != NULL) { | |||||
| DPRINTF("Applying dynamic quirk %s for USB mass storage device %s %s (VID/DID 0x%x:0x%x)\n", | |||||
| usb_quirk_str[UQ_MSC_NO_TEST_UNIT_READY], | |||||
Done Inline ActionsPlease use usb_get_manufacturer() and usb_get_product(). udev->manufacturer and udev->product may be NULL. hselasky: Please use usb_get_manufacturer() and usb_get_product().
udev->manufacturer and udev->product… | |||||
| usb_get_manufacturer(udev), | |||||
| usb_get_product(udev), | |||||
Not Done Inline ActionsSame comment as above imp: Same comment as above | |||||
| UGETW(udev->ddesc.idVendor), | |||||
| UGETW(udev->ddesc.idProduct) | |||||
| ); | |||||
| } | |||||
| usbd_add_dynamic_quirk(udev, UQ_MSC_NO_TEST_UNIT_READY); | |||||
Done Inline ActionsThe amount of broken hardware I have or can borrow is finite. Future contributions would the use of the 'error' label to be present in the last check before the 'retry_sync_cache' label. vidwer_fbsdbugs_gmail.com: The amount of broken hardware I have or can borrow is finite. Future contributions would the… | |||||
Done Inline Actions
"Future contributions would require the use of the 'error' label to be present in the last check before the 'retry_sync_cache' label." vidwer_fbsdbugs_gmail.com: > The amount of broken hardware I have or can borrow is finite. Future contributions would the… | |||||
| /* fall through */ | |||||
Done Inline ActionsYou should also handle timeouts here, not only CSW_FAILED. hselasky: You should also handle timeouts here, not only CSW_FAILED. | |||||
Done Inline ActionsHandle timeouts (with USB_ERR_TIMEOUT) just in this place, or in both places? vidwer_fbsdbugs_gmail.com: Handle timeouts (with USB_ERR_TIMEOUT) just in this place, or in both places? | |||||
| } | |||||
| } | |||||
| if (usb_test_quirk(&uaa, UQ_MSC_NO_SYNC_CACHE) == 0) { | |||||
| err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0, | |||||
| &scsi_quirk_no_sync_cache, sizeof(scsi_quirk_no_sync_cache), | |||||
| USB_MS_HZ); | |||||
| if (err == ERR_CSW_FAILED) { | |||||
| if (usb_get_manufacturer(udev) != NULL && usb_get_product(udev) != NULL) { | |||||
| DPRINTF("Applying dynamic quirk %s for USB mass storage device %s %s (VID/DID 0x%x:0x%x)\n", | |||||
Done Inline ActionsDitto. hselasky: Ditto. | |||||
| usb_quirk_str[UQ_MSC_NO_SYNC_CACHE], | |||||
| usb_get_manufacturer(udev), | |||||
| usb_get_product(udev), | |||||
| UGETW(udev->ddesc.idVendor), | |||||
| UGETW(udev->ddesc.idProduct) | |||||
| ); | |||||
| } | |||||
| usbd_add_dynamic_quirk(udev, UQ_MSC_NO_SYNC_CACHE); | |||||
| goto error; | |||||
| } | |||||
| } | |||||
| retry_sync_cache: | retry_sync_cache: | ||||
| err = bbb_command_start(sc, DIR_IN, 0, NULL, 0, | err = bbb_command_start(sc, DIR_IN, 0, NULL, 0, | ||||
| &scsi_sync_cache, sizeof(scsi_sync_cache), | &scsi_sync_cache, sizeof(scsi_sync_cache), | ||||
Done Inline ActionsError means we are no longer able to communicate and need to reset the device. hselasky: Error means we are no longer able to communicate and need to reset the device. | |||||
Done Inline ActionsIs this line now obsolete? vidwer_fbsdbugs_gmail.com: Is this line now obsolete? | |||||
Done Inline ActionsNo we still need this. hselasky: No we still need this. | |||||
| Context not available. | |||||
| DPRINTF("Device did not respond, enabling all quirks\n"); | 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_PREVENT_ALLOW); | ||||
| usbd_add_dynamic_quirk(udev, UQ_MSC_NO_TEST_UNIT_READY); | |||||
| /* Need to re-enumerate the device */ | /* Need to re-enumerate the device */ | ||||
| usbd_req_re_enumerate(udev, NULL); | usbd_req_re_enumerate(udev, NULL); | ||||
| Context not available. | |||||
This looks ugly, it's here to include usb_quirk_str[]