[PATCH] USB: Make removable-LUN support a non-test option in the g_file_storage driver

This patch follows the suggestions sent by Todd Fischer and Diego Dompe
for making removable-LUN support part of the normal non-testing version of
the g_file_storage driver.  It also moves LUN device registration to the
correct place and eliminates a code path that stalls the bulk-out pipe in
a racy way.

There are also some smaller changes: update some comments, add initial
debugging support for USB suspend/resume, and miscellaneous code cleanups.
Last but not least, the driver has been sufficiently stable for
sufficiently long that it's fair to remove the "(DEVELOPMENT)" warning in
Kconfig.
Sent-by: Todd Fischer <toddf@cadenux.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

drivers/usb/gadget/Kconfig

@@ -275,7 +275,7 @@ config USB_GADGETFS
 	  dynamically linked module called "gadgetfs".
 
 config USB_FILE_STORAGE
-	tristate "File-backed Storage Gadget (DEVELOPMENT)"
+	tristate "File-backed Storage Gadget"
 	# we don't support the SA1100 because of its limitations
 	depends on USB_GADGET_SA1100 = n
 	help
@@ -288,7 +288,7 @@ config USB_FILE_STORAGE
 	  dynamically linked module called "g_file_storage".
 
 config USB_FILE_STORAGE_TEST
-	bool "File-backed Storage Gadget test version"
+	bool "File-backed Storage Gadget testing version"
 	depends on USB_FILE_STORAGE
 	default n
 	help

drivers/usb/gadget/file_storage.c

@@ -46,17 +46,16 @@
  *
  * Backing storage is provided by a regular file or a block device, specified
  * by the "file" module parameter.  Access can be limited to read-only by
- * setting the optional "ro" module parameter.
+ * setting the optional "ro" module parameter.  The gadget will indicate that
+ * it has removable media if the optional "removable" module parameter is set.
  *
  * The gadget supports the Control-Bulk (CB), Control-Bulk-Interrupt (CBI),
  * and Bulk-Only (also known as Bulk-Bulk-Bulk or BBB) transports, selected
  * by the optional "transport" module parameter.  It also supports the
  * following protocols: RBC (0x01), ATAPI or SFF-8020i (0x02), QIC-157 (0c03),
  * UFI (0x04), SFF-8070i (0x05), and transparent SCSI (0x06), selected by
- * the optional "protocol" module parameter.  For testing purposes the
- * gadget will indicate that it has removable media if the optional
- * "removable" module parameter is set.  In addition, the default Vendor ID,
- * Product ID, and release number can be overridden.
+ * the optional "protocol" module parameter.  In addition, the default
+ * Vendor ID, Product ID, and release number can be overridden.
  *
  * There is support for multiple logical units (LUNs), each of which has
  * its own backing file.  The number of LUNs can be set using the optional
@@ -79,13 +78,13 @@
  *					the files or block devices used for
  *					backing storage
  *	ro=b[,b...]		Default false, booleans for read-only access
+ *	removable		Default false, boolean for removable media
  *	luns=N			Default N = number of filenames, number of
  *					LUNs to support
  *	transport=XXX		Default BBB, transport name (CB, CBI, or BBB)
  *	protocol=YYY		Default SCSI, protocol name (RBC, 8020 or
  *					ATAPI, QIC, UFI, 8070, or SCSI;
  *					also 1 - 6)
- *	removable		Default false, boolean for removable media
  *	vendor=0xVVVV		Default 0x0525 (NetChip), USB Vendor ID
  *	product=0xPPPP		Default 0xa4a5 (FSG), USB Product ID
  *	release=0xRRRR		Override the USB release number (bcdDevice)
@@ -97,16 +96,16 @@
  *					boolean to permit the driver to halt
  *					bulk endpoints
  *
- * If CONFIG_USB_FILE_STORAGE_TEST is not set, only the "file" and "ro"
- * options are available; default values are used for everything else.
+ * If CONFIG_USB_FILE_STORAGE_TEST is not set, only the "file", "ro",
+ * "removable", and "luns" options are available; default values are used
+ * for everything else.
  *
  * The pathnames of the backing files and the ro settings are available in
  * the attribute files "file" and "ro" in the lun<n> subdirectory of the
- * gadget's sysfs directory.  If CONFIG_USB_FILE_STORAGE_TEST and the
- * "removable" option are both set, writing to these files will simulate
- * ejecting/loading the medium (writing an empty line means eject) and
- * adjusting a write-enable tab.  Changes to the ro setting are not allowed
- * when the medium is loaded.
+ * gadget's sysfs directory.  If the "removable" option is set, writing to
+ * these files will simulate ejecting/loading the medium (writing an empty
+ * line means eject) and adjusting a write-enable tab.  Changes to the ro
+ * setting are not allowed when the medium is loaded.
  *
  * This gadget driver is heavily based on "Gadget Zero" by David Brownell.
  */
@@ -178,7 +177,10 @@
  * Bulk-only specification requires a stall.  In such cases the driver
  * will halt the endpoint and set a flag indicating that it should clear
  * the halt in software during the next device reset.  Hopefully this
- * will permit everything to work correctly.
+ * will permit everything to work correctly.  Furthermore, although the
+ * specification allows the bulk-out endpoint to halt when the host sends
+ * too much data, implementing this would cause an unavoidable race.
+ * The driver will always use the "no-stall" approach for OUT transfers.
  *
  * One subtle point concerns sending status-stage responses for ep0
  * requests.  Some of these requests, such as device reset, can involve
@@ -246,7 +248,7 @@
 
 #define DRIVER_DESC		"File-backed Storage Gadget"
 #define DRIVER_NAME		"g_file_storage"
-#define DRIVER_VERSION		"21 March 2004"
+#define DRIVER_VERSION		"28 July 2004"
 
 static const char longname[] = DRIVER_DESC;
 static const char shortname[] = DRIVER_NAME;
@@ -371,14 +373,17 @@ MODULE_PARM_DESC(file, "names of backing files or devices");
 module_param_array(ro, bool, mod_data.num_ros, S_IRUGO);
 MODULE_PARM_DESC(ro, "true to force read-only");
 
+module_param_named(luns, mod_data.nluns, uint, S_IRUGO);
+MODULE_PARM_DESC(luns, "number of LUNs");
 
-/* In the non-TEST version, only the file and ro module parameters
+module_param_named(removable, mod_data.removable, bool, S_IRUGO);
+MODULE_PARM_DESC(removable, "true to simulate removable media");
+
+
+/* In the non-TEST version, only the module parameters listed above
  * are available. */
 #ifdef CONFIG_USB_FILE_STORAGE_TEST
 
-module_param_named(luns, mod_data.nluns, uint, S_IRUGO);
-MODULE_PARM_DESC(luns, "number of LUNs");
-
 module_param_named(transport, mod_data.transport_parm, charp, S_IRUGO);
 MODULE_PARM_DESC(transport, "type of transport (BBB, CBI, or CB)");
 
@@ -386,9 +391,6 @@ module_param_named(protocol, mod_data.protocol_parm, charp, S_IRUGO);
 MODULE_PARM_DESC(protocol, "type of protocol (RBC, 8020, QIC, UFI, "
 		"8070, or SCSI)");
 
-module_param_named(removable, mod_data.removable, bool, S_IRUGO);
-MODULE_PARM_DESC(removable, "true to simulate removable media");
-
 module_param_named(vendor, mod_data.vendor, ushort, S_IRUGO);
 MODULE_PARM_DESC(vendor, "USB Vendor ID");
 
@@ -525,11 +527,6 @@ struct interrupt_data {
  * parts of the driver that aren't used in the non-TEST version.  Even gcc
  * can recognize when a test of a constant expression yields a dead code
  * path.
- *
- * Also, in the non-TEST version, open_backing_file() is only used during
- * initialization and the sysfs attribute store_xxx routines aren't used
- * at all.  We will define NORMALLY_INIT to mark them as __init so they
- * don't occupy kernel code space unnecessarily.
  */
 
 #ifdef CONFIG_USB_FILE_STORAGE_TEST
@@ -537,16 +534,12 @@ struct interrupt_data {
 #define transport_is_bbb()	(mod_data.transport_type == USB_PR_BULK)
 #define transport_is_cbi()	(mod_data.transport_type == USB_PR_CBI)
 #define protocol_is_scsi()	(mod_data.protocol_type == USB_SC_SCSI)
-#define backing_file_is_open(curlun)	((curlun)->filp != NULL)
-#define NORMALLY_INIT
 
 #else
 
 #define transport_is_bbb()	1
 #define transport_is_cbi()	0
 #define protocol_is_scsi()	1
-#define backing_file_is_open(curlun)	1
-#define NORMALLY_INIT		__init
 
 #endif /* CONFIG_USB_FILE_STORAGE_TEST */
 
@@ -567,6 +560,8 @@ struct lun {
 	struct device	dev;
 };
 
+#define backing_file_is_open(curlun)	((curlun)->filp != NULL)
+
 static inline struct lun *dev_to_lun(struct device *dev)
 {
 	return container_of(dev, struct lun, dev);
@@ -659,6 +654,7 @@ struct fsg_dev {
 	unsigned long		atomic_bitflags;
 #define REGISTERED		0
 #define CLEAR_BULK_HALTS	1
+#define SUSPENDED		2
 
 	struct usb_ep		*bulk_in;
 	struct usb_ep		*bulk_out;
@@ -1041,8 +1037,6 @@ static int populate_config_buf(enum usb_device_speed speed,
 		function = fs_function;
 
 	len = usb_gadget_config_buf(&config_desc, buf, EP0_BUFSIZE, function);
-	if (len < 0)
-		return len;
 	((struct usb_config_descriptor *) buf)->bDescriptorType = type;
 	return len;
 }
@@ -1172,9 +1166,10 @@ static void bulk_out_complete(struct usb_ep *ep, struct usb_request *req)
 	wakeup_thread(fsg);
 }
 
+
+#ifdef CONFIG_USB_FILE_STORAGE_TEST
 static void intr_in_complete(struct usb_ep *ep, struct usb_request *req)
 {
-#ifdef CONFIG_USB_FILE_STORAGE_TEST
 	struct fsg_dev		*fsg = (struct fsg_dev *) ep->driver_data;
 	struct fsg_buffhd	*bh = (struct fsg_buffhd *) req->context;
 
@@ -1190,17 +1185,21 @@ static void intr_in_complete(struct usb_ep *ep, struct usb_request *req)
 	bh->state = BUF_STATE_EMPTY;
 	spin_unlock(&fsg->lock);
 	wakeup_thread(fsg);
-#endif /* CONFIG_USB_FILE_STORAGE_TEST */
 }
 
+#else
+static void intr_in_complete(struct usb_ep *ep, struct usb_request *req)
+{}
+#endif /* CONFIG_USB_FILE_STORAGE_TEST */
+
 
 /*-------------------------------------------------------------------------*/
 
 /* Ep0 class-specific handlers.  These always run in_irq. */
 
+#ifdef CONFIG_USB_FILE_STORAGE_TEST
 static void received_cbi_adsc(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 {
-#ifdef CONFIG_USB_FILE_STORAGE_TEST
 	struct usb_request	*req = fsg->ep0req;
 	static u8		cbi_reset_cmnd[6] = {
 			SC_SEND_DIAGNOSTIC, 4, 0xff, 0xff, 0xff, 0xff};
@@ -1238,9 +1237,13 @@ static void received_cbi_adsc(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 
 	spin_unlock(&fsg->lock);
 	wakeup_thread(fsg);
-#endif /* CONFIG_USB_FILE_STORAGE_TEST */
 }
 
+#else
+static void received_cbi_adsc(struct fsg_dev *fsg, struct fsg_buffhd *bh)
+{}
+#endif /* CONFIG_USB_FILE_STORAGE_TEST */
+
 
 static int class_setup_req(struct fsg_dev *fsg,
 		const struct usb_ctrlrequest *ctrl)
@@ -1465,8 +1468,8 @@ static int fsg_setup(struct usb_gadget *gadget,
 	/* Respond with data/status or defer until later? */
 	if (rc >= 0 && rc != DELAYED_STATUS) {
 		fsg->ep0req->length = rc;
-		fsg->ep0req->zero = rc < ctrl->wLength
-				&& (rc % gadget->ep0->maxpacket) == 0;
+		fsg->ep0req->zero = (rc < ctrl->wLength &&
+				(rc % gadget->ep0->maxpacket) == 0);
 		fsg->ep0req_name = (ctrl->bRequestType & USB_DIR_IN ?
 				"ep0-in" : "ep0-out");
 		rc = ep0_queue(fsg);
@@ -2443,14 +2446,19 @@ static int finish_reply(struct fsg_dev *fsg)
 			rc = -EINTR;
 		}
 
-		/* We haven't processed all the incoming data.  If we are
-		 * allowed to stall, halt the bulk-out endpoint and cancel
-		 * any outstanding requests. */
+		/* We haven't processed all the incoming data.  Even though
+		 * we may be allowed to stall, doing so would cause a race.
+		 * The controller may already have ACK'ed all the remaining
+		 * bulk-out packets, in which case the host wouldn't see a
+		 * STALL.  Not realizing the endpoint was halted, it wouldn't
+		 * clear the halt -- leading to problems later on. */
+#if 0
 		else if (mod_data.can_stall) {
 			fsg_set_halt(fsg, fsg->bulk_out);
 			raise_exception(fsg, FSG_STATE_ABORT_BULK_OUT);
 			rc = -EINTR;
 		}
+#endif
 
 		/* We can't stall.  Read in the excess data and throw it
 		 * all away. */
@@ -2513,7 +2521,7 @@ static int send_status(struct fsg_dev *fsg)
 
 	} else if (mod_data.transport_type == USB_PR_CB) {
 
-		/* Control-Bulk transport has no status stage! */
+		/* Control-Bulk transport has no status phase! */
 		return 0;
 
 	} else {			// USB_PR_CBI
@@ -2603,8 +2611,10 @@ static int check_command(struct fsg_dev *fsg, int cmnd_size,
 	fsg->residue = fsg->usb_amount_left = fsg->data_size;
 
 	/* Conflicting data directions is a phase error */
-	if (fsg->data_dir != data_dir && fsg->data_size_from_cmnd > 0)
-		goto phase_error;
+	if (fsg->data_dir != data_dir && fsg->data_size_from_cmnd > 0) {
+		fsg->phase_error = 1;
+		return -EINVAL;
+	}
 
 	/* Verify the length of the command itself */
 	if (cmnd_size != fsg->cmnd_size) {
@@ -2613,8 +2623,10 @@ static int check_command(struct fsg_dev *fsg, int cmnd_size,
 		 * with cbw->Length == 12 (it should be 6). */
 		if (fsg->cmnd[0] == SC_REQUEST_SENSE && fsg->cmnd_size == 12)
 			cmnd_size = fsg->cmnd_size;
-		else
-			goto phase_error;
+		else {
+			fsg->phase_error = 1;
+			return -EINVAL;
+		}
 	}
 
 	/* Check that the LUN values are oonsistent */
@@ -2674,10 +2686,6 @@ static int check_command(struct fsg_dev *fsg, int cmnd_size,
 	}
 
 	return 0;
-
-phase_error:
-	fsg->phase_error = 1;
-	return -EINVAL;
 }
 
 
@@ -3424,8 +3432,7 @@ static int fsg_main_thread(void *fsg_)
 /* If the next two routines are called while the gadget is registered,
  * the caller must own fsg->filesem for writing. */
 
-static int NORMALLY_INIT open_backing_file(struct lun *curlun,
-		const char *filename)
+static int open_backing_file(struct lun *curlun, const char *filename)
 {
 	int				ro;
 	struct file			*filp = NULL;
@@ -3550,8 +3557,7 @@ static ssize_t show_file(struct device *dev, char *buf)
 }
 
 
-ssize_t NORMALLY_INIT store_ro(struct device *dev, const char *buf,
-		size_t count)
+ssize_t store_ro(struct device *dev, const char *buf, size_t count)
 {
 	ssize_t		rc = count;
 	struct lun	*curlun = dev_to_lun(dev);
@@ -3575,8 +3581,7 @@ ssize_t NORMALLY_INIT store_ro(struct device *dev, const char *buf,
 	return rc;
 }
 
-ssize_t NORMALLY_INIT store_file(struct device *dev, const char *buf,
-		size_t count)
+ssize_t store_file(struct device *dev, const char *buf, size_t count)
 {
 	struct lun	*curlun = dev_to_lun(dev);
 	struct fsg_dev	*fsg = (struct fsg_dev *) dev_get_drvdata(dev);
@@ -3805,9 +3810,8 @@ static int __init fsg_bind(struct usb_gadget *gadget)
 		goto out;
 	}
 
-	/* Create the LUNs and open their backing files.  We can't register
-	 * the LUN devices until the gadget itself is registered, which
-	 * doesn't happen until after fsg_bind() returns. */
+	/* Create the LUNs, open their backing files, and register the
+	 * LUN devices in sysfs. */
 	fsg->luns = kmalloc(i * sizeof(struct lun), GFP_KERNEL);
 	if (!fsg->luns) {
 		rc = -ENOMEM;
@@ -3825,6 +3829,15 @@ static int __init fsg_bind(struct usb_gadget *gadget)
 		snprintf(curlun->dev.bus_id, BUS_ID_SIZE,
 				"%s-lun%d", gadget->dev.bus_id, i);
 
+		if ((rc = device_register(&curlun->dev)) != 0)
+			INFO(fsg, "failed to register LUN%d: %d\n", i, rc);
+		else {
+			curlun->registered = 1;
+			curlun->dev.release = lun_release;
+			device_create_file(&curlun->dev, &dev_attr_ro);
+			device_create_file(&curlun->dev, &dev_attr_file);
+		}
+
 		if (file[i] && *file[i]) {
 			if ((rc = open_backing_file(curlun, file[i])) != 0)
 				goto out;
@@ -3972,6 +3985,25 @@ static int __init fsg_bind(struct usb_gadget *gadget)
 }
 
 
+/*-------------------------------------------------------------------------*/
+
+static void fsg_suspend(struct usb_gadget *gadget)
+{
+	struct fsg_dev		*fsg = get_gadget_data(gadget);
+
+	DBG(fsg, "suspend\n");
+	set_bit(SUSPENDED, &fsg->atomic_bitflags);
+}
+
+static void fsg_resume(struct usb_gadget *gadget)
+{
+	struct fsg_dev		*fsg = get_gadget_data(gadget);
+
+	DBG(fsg, "resume\n");
+	clear_bit(SUSPENDED, &fsg->atomic_bitflags);
+}
+
+
 /*-------------------------------------------------------------------------*/
 
 static struct usb_gadget_driver		fsg_driver = {
@@ -3985,6 +4017,8 @@ static struct usb_gadget_driver		fsg_driver = {
 	.unbind		= fsg_unbind,
 	.disconnect	= fsg_disconnect,
 	.setup		= fsg_setup,
+	.suspend	= fsg_suspend,
+	.resume		= fsg_resume,
 
 	.driver		= {
 		.name		= (char *) shortname,
@@ -4024,8 +4058,6 @@ static int __init fsg_init(void)
 {
 	int		rc;
 	struct fsg_dev	*fsg;
-	int		i;
-	struct lun	*curlun;
 
 	if ((rc = fsg_alloc()) != 0)
 		return rc;
@@ -4036,19 +4068,6 @@ static int __init fsg_init(void)
 	}
 	set_bit(REGISTERED, &fsg->atomic_bitflags);
 
-	/* Register the LUN devices and their attribute files */
-	for (i = 0; i < fsg->nluns; ++i) {
-		curlun = &fsg->luns[i];
-		if ((rc = device_register(&curlun->dev)) != 0)
-			INFO(fsg, "failed to register LUN%d: %d\n", i, rc);
-		else {
-			curlun->registered = 1;
-			curlun->dev.release = lun_release;
-			device_create_file(&curlun->dev, &dev_attr_ro);
-			device_create_file(&curlun->dev, &dev_attr_file);
-		}
-	}
-
 	/* Tell the thread to start working */
 	complete(&fsg->thread_notifier);
 	return 0;