Hi,
The first patch makes composite framework support a mechanism of defering
setup transfer's data/status phase.
Second patch makes f_mass_storage use that mechanism.
Together they help the composite devices using f_mass_storage interface to
pass USBCV Mass Storage Class compliance tests.
Changes in v2:
- Review comments addressed
- Regression fix - USBCV CH-9 fails on 2nd run on composite gadget with storage
---
Roger Quadros (2):
usb: gadget: composite: Allow function drivers to pause control
transfers
usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests
drivers/usb/gadget/composite.c | 62 ++++++++++++++++++++++++++++++++++-
drivers/usb/gadget/f_mass_storage.c | 6 +++-
include/linux/usb/composite.h | 16 ++++++++-
3 files changed, 81 insertions(+), 3 deletions(-)
Some USB function drivers (e.g. f_mass_storage.c) need to delay or defer the
data/status stages of standard control requests like SET_CONFIGURATION or
SET_INTERFACE till they are done with their bookkeeping and are actually ready
for accepting new commands to their interface.
They can now achieve this functionality by returning USB_GADGET_DELAYED_STATUS
in their setup handlers (e.g. set_alt()). The composite framework will then
defer completion of the control transfer by not completing the data/status stages.
This ensures that the host does not send new packets to the interface till the
function driver is ready to take them.
When the function driver that requested for USB_GADGET_DELAYED_STATUS is done
with its bookkeeping, it should signal the composite framework to continue with
the data/status stages of the control transfer. It can do so by invoking
the new API usb_composite_setup_continue(). This is where the control transfer's
data/status stages are completed and host can initiate new transfers.
The DELAYED_STATUS mechanism is currently only supported if the expected data phase
is 0 bytes (i.e. w_length == 0). Since SET_CONFIGURATION and SET_INTERFACE are the
only cases that will use this mechanism, this is not a limitation.
Signed-off-by: Roger Quadros <[email protected]>
---
drivers/usb/gadget/composite.c | 62 +++++++++++++++++++++++++++++++++++++++-
include/linux/usb/composite.h | 16 +++++++++-
2 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 82314ed..5cbb1a4 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -461,12 +461,23 @@ static int set_config(struct usb_composite_dev *cdev,
reset_config(cdev);
goto done;
}
+
+ if (result == USB_GADGET_DELAYED_STATUS) {
+ DBG(cdev,
+ "%s: interface %d (%s) requested delayed status\n",
+ __func__, tmp, f->name);
+ cdev->delayed_status++;
+ DBG(cdev, "delayed_status count %d\n",
+ cdev->delayed_status);
+ }
}
/* when we return, be sure our power usage is valid */
power = c->bMaxPower ? (2 * c->bMaxPower) : CONFIG_USB_GADGET_VBUS_DRAW;
done:
usb_gadget_vbus_draw(gadget, power);
+ if (result >= 0 && cdev->delayed_status)
+ result = USB_GADGET_DELAYED_STATUS;
return result;
}
@@ -895,6 +906,14 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
if (w_value && !f->set_alt)
break;
value = f->set_alt(f, w_index, w_value);
+ if (value == USB_GADGET_DELAYED_STATUS) {
+ DBG(cdev,
+ "%s: interface %d (%s) requested delayed status\n",
+ __func__, intf, f->name);
+ cdev->delayed_status++;
+ DBG(cdev, "delayed_status count %d\n",
+ cdev->delayed_status);
+ }
break;
case USB_REQ_GET_INTERFACE:
if (ctrl->bRequestType != (USB_DIR_IN|USB_RECIP_INTERFACE))
@@ -958,7 +977,7 @@ unknown:
}
/* respond with data transfer before status phase? */
- if (value >= 0) {
+ if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
req->length = value;
req->zero = value < w_length;
value = usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
@@ -967,6 +986,10 @@ unknown:
req->status = 0;
composite_setup_complete(gadget->ep0, req);
}
+ } else if (value == USB_GADGET_DELAYED_STATUS && w_length != 0) {
+ WARN(cdev,
+ "%s: Delayed status not supported for w_length != 0",
+ __func__);
}
done:
@@ -1289,3 +1312,40 @@ void usb_composite_unregister(struct usb_composite_driver *driver)
return;
usb_gadget_unregister_driver(&composite_driver);
}
+
+/**
+ * usb_composite_setup_continue() - Continue with the control transfer
+ * @cdev: the composite device who's control transfer was kept waiting
+ *
+ * This function must be called by the USB function driver to continue
+ * with the control transfer's data/status stage in case it had requested to
+ * delay the data/status stages. A USB function's setup handler (e.g. set_alt())
+ * can request the composite framework to delay the setup request's data/status
+ * stages by returning USB_GADGET_DELAYED_STATUS.
+ */
+void usb_composite_setup_continue(struct usb_composite_dev *cdev)
+{
+ int value;
+ struct usb_request *req = cdev->req;
+ unsigned long flags;
+
+ DBG(cdev, "%s\n", __func__);
+ spin_lock_irqsave(&cdev->lock, flags);
+
+ if (cdev->delayed_status == 0) {
+ WARN(cdev, "%s: Unexpected call\n", __func__);
+
+ } else if (--cdev->delayed_status == 0) {
+ DBG(cdev, "%s: Completing delayed status\n", __func__);
+ req->length = 0;
+ value = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
+ if (value < 0) {
+ DBG(cdev, "ep_queue --> %d\n", value);
+ req->status = 0;
+ composite_setup_complete(cdev->gadget->ep0, req);
+ }
+ }
+
+ spin_unlock_irqrestore(&cdev->lock, flags);
+}
+
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 882a084..b78cba4 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -37,6 +37,14 @@
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
+/*
+ * USB function drivers should return USB_GADGET_DELAYED_STATUS if they
+ * wish to delay the data/status stages of the control transfer till they
+ * are ready. The control transfer will then be kept from completing till
+ * all the function drivers that requested for USB_GADGET_DELAYED_STAUS
+ * invoke usb_composite_setup_continue().
+ */
+#define USB_GADGET_DELAYED_STATUS 0x7fff /* Impossibly large value */
struct usb_configuration;
@@ -285,6 +293,7 @@ struct usb_composite_driver {
extern int usb_composite_probe(struct usb_composite_driver *driver,
int (*bind)(struct usb_composite_dev *cdev));
extern void usb_composite_unregister(struct usb_composite_driver *driver);
+extern void usb_composite_setup_continue(struct usb_composite_dev *cdev);
/**
@@ -342,7 +351,12 @@ struct usb_composite_dev {
*/
unsigned deactivations;
- /* protects at least deactivation count */
+ /* the composite driver won't complete the control transfer's
+ * data/status stages till delayed_status is zero.
+ */
+ int delayed_status;
+
+ /* protects deactivations and delayed_status counts*/
spinlock_t lock;
};
--
1.6.0.4
Defer the SET_CONFIG and SET_INTERFACE control transfer's data/status
stages till we are ready to process new CBW from the host. This way we
ensure that we don't loose any CBW during MSC compliance tests and cause
lock up.
Signed-off-by: Roger Quadros <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 6d8e533..0d1deee 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -347,6 +347,7 @@ struct fsg_operations {
/* Data shared by all the FSG instances. */
struct fsg_common {
struct usb_gadget *gadget;
+ struct usb_composite_dev *cdev;
struct fsg_dev *fsg, *new_fsg;
wait_queue_head_t fsg_wait;
@@ -2468,7 +2469,7 @@ static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
struct fsg_dev *fsg = fsg_from_func(f);
fsg->common->new_fsg = fsg;
raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
- return 0;
+ return USB_GADGET_DELAYED_STATUS;
}
static void fsg_disable(struct usb_function *f)
@@ -2604,6 +2605,8 @@ static void handle_exception(struct fsg_common *common)
case FSG_STATE_CONFIG_CHANGE:
do_set_interface(common, common->new_fsg);
+ if (common->new_fsg)
+ usb_composite_setup_continue(common->cdev);
break;
case FSG_STATE_EXIT:
@@ -2774,6 +2777,7 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
common->gadget = gadget;
common->ep0 = gadget->ep0;
common->ep0req = cdev->req;
+ common->cdev = cdev;
/* Maybe allocate device-global string IDs, and patch descriptors */
if (fsg_strings[FSG_STRING_INTERFACE].id == 0) {
--
1.6.0.4
On Mon, 09 May 2011 12:08:07 +0200, Roger Quadros wrote:
> Defer the SET_CONFIG and SET_INTERFACE control transfer's data/status
> stages till we are ready to process new CBW from the host. This way we
> ensure that we don't loose any CBW during MSC compliance tests and cause
> lock up.
>
> Signed-off-by: Roger Quadros <[email protected]>
Acked-by: Michal Nazarewicz <[email protected]>
> @@ -2468,7 +2469,7 @@ static int fsg_set_alt(struct usb_function *f,
> unsigned intf, unsigned alt)
> struct fsg_dev *fsg = fsg_from_func(f);
> fsg->common->new_fsg = fsg;
> raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
> - return 0;
> + return USB_GADGET_DELAYED_STATUS;
> }
> static void fsg_disable(struct usb_function *f)
Just a side note, an idea that came to my mind: How about instead
of creating meaning of an arbitrary return value, creating a
usb_composite_setup_pause() or usb_composite_setup_delay()
function? It would be more symmetrical that way. Just an idea,
feel free to ignore.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--