Differential D30919 Diff 102587 sys/dev/usb/usb_msctest.c

Changeset View

Standalone View

sys/dev/usb/usb_msctest.c

/* $FreeBSD$ */		/* $FreeBSD$ */
/*-		/*-
* 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
* are met:		* are met:
* 1. Redistributions of source code must retain the above copyright		* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.		* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright		* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the		* notice, this list of conditions and the following disclaimer in the
Show All 10 Lines
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY		* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF		* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.		* SUCH DAMAGE.
*/		*/

/*		/*
* 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
#include USB_GLOBAL_INCLUDE_FILE		#include USB_GLOBAL_INCLUDE_FILE
#else		#else
#include <sys/stdint.h>		#include <sys/stdint.h>
#include <sys/stddef.h>		#include <sys/stddef.h>
#include <sys/param.h>		#include <sys/param.h>
Show All 21 Lines

#include <dev/usb/usb_busdma.h>		#include <dev/usb/usb_busdma.h>
#include <dev/usb/usb_process.h>		#include <dev/usb/usb_process.h>
#include <dev/usb/usb_transfer.h>		#include <dev/usb/usb_transfer.h>
#include <dev/usb/usb_msctest.h>		#include <dev/usb/usb_msctest.h>
#include <dev/usb/usb_debug.h>		#include <dev/usb/usb_debug.h>
#include <dev/usb/usb_device.h>		#include <dev/usb/usb_device.h>
#include <dev/usb/usb_request.h>		#include <dev/usb/usb_request.h>
#include <dev/usb/usb_util.h>		#include <dev/usb/usb_util.h>
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions This looks ugly, it's here to include usb_quirk_str[] vidwer_fbsdbugs_gmail.com: This looks ugly, it's here to include usb_quirk_str[]
		hselaskyAuthorUnsubmitted Done Inline Actions Please 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.
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Please export the structure via the usb_quirk.h, like an extern declaration. ..and drop the 'static' keyword? https://stackoverflow.com/questions/25288555/how-to-correctly-write-declarations-of-extern-arrays-in-cs-header-files vidwer_fbsdbugs_gmail.com: > Please export the structure via the usb_quirk.h, like an extern declaration. ..and drop the…
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Please export the structure via the usb_quirk.h, like an extern declaration. vidwer_fbsdbugs_gmail.com: > Please export the structure via the usb_quirk.h, like an extern declaration.
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Please export the structure via the usb_quirk.h, like an extern declaration. 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…
#include <dev/usb/quirk/usb_quirk.h>		#include <dev/usb/quirk/usb_quirk.h>
#endif /* USB_GLOBAL_INCLUDE_FILE */		#endif /* USB_GLOBAL_INCLUDE_FILE */

enum {		enum {
ST_COMMAND,		ST_COMMAND,
ST_DATA_RD,		ST_DATA_RD,
ST_DATA_RD_CS,		ST_DATA_RD_CS,
ST_DATA_WR,		ST_DATA_WR,
Show All 33 Lines
static uint8_t scsi_request_sense[] = { 0x03, 0x00, 0x00, 0x00, 0x12, 0x00,		static uint8_t scsi_request_sense[] = { 0x03, 0x00, 0x00, 0x00, 0x12, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };		0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
static uint8_t scsi_read_capacity[] = { 0x25, 0x00, 0x00, 0x00, 0x00, 0x00,		static uint8_t scsi_read_capacity[] = { 0x25, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00 };		0x00, 0x00, 0x00, 0x00 };
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 };

#ifndef USB_MSCTEST_BULK_SIZE		#ifndef USB_MSCTEST_BULK_SIZE
#define USB_MSCTEST_BULK_SIZE 64 /* dummy */		#define USB_MSCTEST_BULK_SIZE 64 /* dummy */
		hselaskyAuthorUnsubmitted Done Inline Actions Is 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?
#endif		#endif

		hselaskyAuthorUnsubmitted Done Inline Actions Does this code compile if you use "static const" here? hselasky: Does this code compile if you use "static const" here?
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Does this code compile if you use "static const" here? (..)/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…
		hselaskyAuthorUnsubmitted Done Inline Actions I see. hselasky: I see.
		hselaskyAuthorUnsubmitted Done Inline Actions Can 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!
#define ERR_CSW_FAILED -1		#define ERR_CSW_FAILED -1

		hselaskyAuthorUnsubmitted Done Inline Actions Ditto. hselasky: Ditto.
/* Command Block Wrapper */		/* Command Block Wrapper */
struct bbb_cbw {		struct bbb_cbw {
uDWord dCBWSignature;		uDWord dCBWSignature;
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions For 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…
#define CBWSIGNATURE 0x43425355		#define CBWSIGNATURE 0x43425355
uDWord dCBWTag;		uDWord dCBWTag;
uDWord dCBWDataTransferLength;		uDWord dCBWDataTransferLength;
uByte bCBWFlags;		uByte bCBWFlags;
#define CBWFLAGS_OUT 0x00		#define CBWFLAGS_OUT 0x00
#define CBWFLAGS_IN 0x80		#define CBWFLAGS_IN 0x80
uByte bCBWLUN;		uByte bCBWLUN;
uByte bCDBLength;		uByte bCDBLength;
▲ Show 20 Lines • Show All 617 Lines • ▼ Show 20 Lines	usb_msc_get_max_lun(struct usb_device *udev, uint8_t iface_index)
err = usbd_do_request(udev, NULL, &req, &buf);		err = usbd_do_request(udev, NULL, &req, &buf);
if (err)		if (err)
buf = 0;		buf = 0;

return (buf);		return (buf);
}		}

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,
		struct usb_attach_arg *uaa)
		hselaskyAuthorUnsubmitted Done Inline Actions You 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.
{		{
struct bbb_transfer *sc;		struct bbb_transfer *sc;
uint8_t timeout;		uint8_t timeout;
uint8_t is_no_direct;		uint8_t is_no_direct;
uint8_t sid_type;		uint8_t sid_type;
int err;		int err;
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions It should read DPRINTFN(), then it will compile. vidwer_fbsdbugs_gmail.com: It should read DPRINTFN(), then it will compile.
		vidwer_fbsdbugs_gmail.comUnsubmitted Not Done Inline Actions The 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…
		hselaskyAuthorUnsubmitted Done Inline Actions Did you add: options USB_DEBUG to your kernel configuration file? hselasky: Did you add: options USB_DEBUG to your kernel configuration file?
		hselaskyAuthorUnsubmitted Done Inline Actions And the code is compiled with: options USB_DEBUG ??? hselasky: And the code is compiled with: options USB_DEBUG ???
		vidwer_fbsdbugs_gmail.comUnsubmitted Not Done Inline Actions USB_DEBUG is set/present in sys/amd64/conf/GENERIC vidwer_fbsdbugs_gmail.com: USB_DEBUG is set/present in sys/amd64/conf/GENERIC

sc = bbb_attach(udev, iface_index, UICLASS_MASS);		sc = bbb_attach(udev, iface_index, UICLASS_MASS);
if (sc == NULL)		if (sc == NULL)
return (0);		return (0);

/*		/*
* Some devices need a delay after that the configuration		* Some devices need a delay after that the configuration
		vidwer_fbsdbugs_gmail.comUnsubmitted Not Done Inline Actions whitespace vidwer_fbsdbugs_gmail.com: whitespace
* value is set to function properly:		* value is set to function properly:
*/		*/
usb_pause_mtx(NULL, hz);		usb_pause_mtx(NULL, hz);

if (usb_msc_get_max_lun(udev, iface_index) == 0) {		if (usb_msc_get_max_lun(udev, iface_index) == 0) {
DPRINTF("Device has only got one LUN.\n");		DPRINTF("Device has only got one LUN.\n");
usbd_add_dynamic_quirk(udev, UQ_MSC_NO_GETMAXLUN);		usbd_add_dynamic_quirk(udev, UQ_MSC_NO_GETMAXLUN);
}		}
▲ Show 20 Lines • Show All 46 Lines • ▼ Show 20 Lines	if (err != 0) {
if (err != ERR_CSW_FAILED)		if (err != ERR_CSW_FAILED)
goto error;		goto error;
DPRINTF("Device doesn't handle prevent and allow removal\n");		DPRINTF("Device doesn't handle prevent and allow removal\n");
usbd_add_dynamic_quirk(udev, UQ_MSC_NO_PREVENT_ALLOW);		usbd_add_dynamic_quirk(udev, UQ_MSC_NO_PREVENT_ALLOW);
}		}

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,
		&scsi_test_unit_ready, sizeof(scsi_test_unit_ready),
		USB_MS_HZ);
		if (err == ERR_CSW_FAILED \|\| err == USB_ERR_TIMEOUT) {
		hselaskyAuthorUnsubmitted Done Inline Actions When err is zero, it means the command was successful, and then you apply the quirk? That will end up applying the TEST UNIT READY quirk for _all_ USB mass storage devices? 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…
		if (usb_get_manufacturer(udev) != NULL && usb_get_product(udev) != NULL) {
		DPRINTFN(0, "Applying dynamic quirk UQ_MSC_NO_TEST_UNIT_READY for USB mass storage device %s %s (VID/DID 0x%x:0x%x)\n",
		usb_get_manufacturer(udev),
		usb_get_product(udev),
		impUnsubmitted Done Inline Actions The 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…
		hselaskyAuthorUnsubmitted Done Inline Actions Maybe 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…
		impUnsubmitted Done Inline Actions Maybe if the quirks is already set, don't query for it? That would avoid hanging devices. 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.
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Maybe if the quirks is already set, don't query for it? That would avoid hanging devices. Right. It seems quirks are primarily set at sc_quirks in umass_softc. Also, a list of USB mass storage devices and detected quirks would be nice! 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.
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions I'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…
		hselaskyAuthorUnsubmitted Done Inline Actions Should be DIR_NONE when the datalength is zero. hselasky: Should be DIR_NONE when the datalength is zero.
		UGETW(udev->ddesc.idVendor),
		UGETW(udev->ddesc.idProduct)
		);
		hselaskyAuthorUnsubmitted Done Inline Actions For 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…
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions The 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.
		}
		usbd_add_dynamic_quirk(udev, UQ_MSC_NO_TEST_UNIT_READY);
		/* fall through */
		hselaskyAuthorUnsubmitted Done Inline Actions Please 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…
		}
		}
		impUnsubmitted Not Done Inline Actions Same comment as above imp: Same comment as above

		if (usb_test_quirk(uaa, UQ_MSC_NO_SYNC_CACHE) == 0) {
		err = bbb_command_start(sc, DIR_NONE, 0, NULL, 0,
		&scsi_sync_cache, sizeof(scsi_sync_cache),
		USB_MS_HZ);
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions The 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…
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions The 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. "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…
		if (err == ERR_CSW_FAILED \|\| err == USB_ERR_TIMEOUT) {
		hselaskyAuthorUnsubmitted Done Inline Actions You should also handle timeouts here, not only CSW_FAILED. hselasky: You should also handle timeouts here, not only CSW_FAILED.
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Handle 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_get_manufacturer(udev) != NULL && usb_get_product(udev) != NULL) {
		DPRINTFN(0, "Applying dynamic quirk UQ_MSC_NO_SYNC_CACHE for USB mass storage device %s %s (VID/DID 0x%x:0x%x)\n",
		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;
		hselaskyAuthorUnsubmitted Done Inline Actions Ditto. hselasky: Ditto.
		}
		}

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),
USB_MS_HZ);		USB_MS_HZ);

if (err != 0) {		if (err != 0) {
if (err != ERR_CSW_FAILED)		if (err != ERR_CSW_FAILED)
goto error;		goto error;
▲ Show 20 Lines • Show All 62 Lines • ▼ Show 20 Lines	retry_sync_cache:
}		}

done:		done:
bbb_detach(sc);		bbb_detach(sc);
return (0);		return (0);

error:		error:
bbb_detach(sc);		bbb_detach(sc);

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);
		hselaskyAuthorUnsubmitted Done Inline Actions Error 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.
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);
		vidwer_fbsdbugs_gmail.comUnsubmitted Done Inline Actions Is this line now obsolete? vidwer_fbsdbugs_gmail.com: Is this line now obsolete?
		hselaskyAuthorUnsubmitted Done Inline Actions No we still need this. hselasky: No we still need this.

return (USB_ERR_STALLED);		return (USB_ERR_STALLED);
}		}

usb_error_t		usb_error_t
usb_msc_eject(struct usb_device *udev, uint8_t iface_index, int method)		usb_msc_eject(struct usb_device *udev, uint8_t iface_index, int method)
{		{
struct bbb_transfer *sc;		struct bbb_transfer *sc;
▲ Show 20 Lines • Show All 174 Lines • Show Last 20 Lines