[PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests

[PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests

[Roger Quadros]

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.

---
Roger Quadros (2):
  usb: gadget: composite: Allow function drivers to defer setup
    responses
  usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests

 drivers/usb/gadget/composite.c      |   61 ++++++++++++++++++++++++++++++++++-
 drivers/usb/gadget/f_mass_storage.c |    6 +++-
 include/linux/usb/composite.h       |   17 +++++++++-
 3 files changed, 81 insertions(+), 3 deletions(-)

[PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

[Roger Quadros]

Some USB function drivers (e.g. f_mass_storage.c) need to delay or defer the
status phase 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 setup transfer by not sending the Data/Status response.

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 phase of the setup transfer. It can do so by invoking
the new API usb_composite_setup_continue(). This is where the setup transfer's
data/status phases are completed and host can send 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 as such.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/composite.c |   61 +++++++++++++++++++++++++++++++++++++++-
 include/linux/usb/composite.h  |   17 ++++++++++-
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 1ba4bef..43be36e 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -461,6 +461,15 @@ 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 */
@@ -895,6 +904,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 +975,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 +984,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 +1310,41 @@ void usb_composite_unregister(struct usb_composite_driver *driver)
 		return;
 	usb_gadget_unregister_driver(&composite_driver);
 }
+
+/**
+ * usb_composite_setup_continue() - Continue the delayed setup transfer
+ * @cdev: the composite device who's setup transfer was delayed
+ *
+ * This function must be called by the USB function driver to continue
+ * with the setup transfer's data/status phase in case it had requested to
+ * delay the status phase. A USB function's setup handler (e.g. set_alt())
+ * can request the composite framework to delay the setup request's status phase
+ * 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;
+		req->zero = 1;
+		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 3d29a7d..930e860 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -37,6 +37,15 @@
 #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 status phase of the setup transfer till they are
+ * ready. The composite framework will then delay the data/status phase
+ * of the setup transfer 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 +294,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 +352,12 @@ struct usb_composite_dev {
 	 */
 	unsigned			deactivations;
 
-	/* protects at least deactivation count */
+	/* the composite driver won't complete the setup transfer's
+	 * data/status phase till delayed_status is zero.
+	 */
+	int				delayed_status;
+
+	/* protects deactivations and delayed_status counts*/
 	spinlock_t			lock;
 };
 
-- 
1.6.0.4

Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

[Alan Stern]

On Tue, 19 Apr 2011, Roger Quadros wrote:

> +/**
> + * usb_composite_setup_continue() - Continue the delayed setup transfer

You're should say "control transfer", not "setup transfer".  Control 
transfers consist of a setup stage, an optional data stage, and a 
status stage.  The setup stage has already ended by the time the 
function's setup handler is called.

> + * @cdev: the composite device who's setup transfer was delayed
> + *
> + * This function must be called by the USB function driver to continue
> + * with the setup transfer's data/status phase in case it had requested to

Data and Status are stages, not phases.  See section 8.5.3 of the
USB-2.0 spec.

You made these mistakes in a few different places throughout the patch.

> + * delay the status phase. A USB function's setup handler (e.g. set_alt())
> + * can request the composite framework to delay the setup request's status phase
> + * 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;
> +		req->zero = 1;

There's no reason to set this flag.  It has no effect when req->length 
is 0.

Alan Stern

Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

[Roger Quadros]

On 04/19/2011 05:14 PM, ext Alan Stern wrote:
> On Tue, 19 Apr 2011, Roger Quadros wrote:
>
>> +/**
>> + * usb_composite_setup_continue() - Continue the delayed setup transfer
>
> You're should say "control transfer", not "setup transfer".  Control
> transfers consist of a setup stage, an optional data stage, and a
> status stage.  The setup stage has already ended by the time the
> function's setup handler is called.

Yes I will re-word the things and re-send the patches.

>
>> + * @cdev: the composite device who's setup transfer was delayed
>> + *
>> + * This function must be called by the USB function driver to continue
>> + * with the setup transfer's data/status phase in case it had requested to
>
> Data and Status are stages, not phases.  See section 8.5.3 of the
> USB-2.0 spec.
>
> You made these mistakes in a few different places throughout the patch.
>
>> + * delay the status phase. A USB function's setup handler (e.g. set_alt())
>> + * can request the composite framework to delay the setup request's status phase
>> + * 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;
>> +		req->zero = 1;
>
> There's no reason to set this flag.  It has no effect when req->length
> is 0.
>
OK.

Thanks for review.

-- 
regards,
-roger

Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

[Michal Nazarewicz]

On Tue, 19 Apr 2011 15:33:52 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:
> @@ -895,6 +904,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++;

Do we need it to be a counter? Won't simple flag do?

> +			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))

> @@ -1289,3 +1310,41 @@ void usb_composite_unregister(struct  
> usb_composite_driver *driver)
>  		return;
>  	usb_gadget_unregister_driver(&composite_driver);
>  }
> +
> +/**
> + * usb_composite_setup_continue() - Continue the delayed setup transfer
> + * @cdev: the composite device who's setup transfer was delayed
> + *
> + * This function must be called by the USB function driver to continue
> + * with the setup transfer's data/status phase in case it had requested  
> to
> + * delay the status phase. A USB function's setup handler (e.g.  
> set_alt())
> + * can request the composite framework to delay the setup request's  
> status phase
> + * 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;
> +		req->zero = 1;
> +		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);
> +}

It's just my opinion but I dislike how it looks almost identical to the end
of composite_setup.  I would rather make it more general (take value as
argument for instance) and let composite_setup() call it.

> diff --git a/include/linux/usb/composite.h @@ -37,6 +37,15 @@
>  #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 status phase of the setup transfer till they are
> + * ready. The composite framework will then delay the data/status phase
> + * of the setup transfer 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 */

I was wondering if we could change it to something like EDELAYED and put  
it to
<include/linux/errno.h>.  Just thinking, it may be a bad idea.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

[Roger Quadros]

On 04/19/2011 05:23 PM, ext Michal Nazarewicz wrote:
> On Tue, 19 Apr 2011 15:33:52 +0200, Roger Quadros <roger.quadros@nokia.com> wrote:
>> @@ -895,6 +904,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++;
>
> Do we need it to be a counter? Won't simple flag do?

Counter because multiple interfaces could return USB_GADGET_DELAYED_STATUS
and we need to know how many of them call usb_composite_setup_continue() before 
we actually complete the control transfer.

>
>> + 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))
>
>> @@ -1289,3 +1310,41 @@ void usb_composite_unregister(struct
>> usb_composite_driver *driver)
>> return;
>> usb_gadget_unregister_driver(&composite_driver);
>> }
>> +
>> +/**
>> + * usb_composite_setup_continue() - Continue the delayed setup transfer
>> + * @cdev: the composite device who's setup transfer was delayed
>> + *
>> + * This function must be called by the USB function driver to continue
>> + * with the setup transfer's data/status phase in case it had requested to
>> + * delay the status phase. A USB function's setup handler (e.g. set_alt())
>> + * can request the composite framework to delay the setup request's status phase
>> + * 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;
>> + req->zero = 1;
>> + 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);
>> + }
>> + }Data/status stage.
>> +
>> + spin_unlock_irqrestore(&cdev->lock, flags);
>> +}
>
> It's just my opinion but I dislike how it looks almost identical to the end
> of composite_setup. I would rather make it more general (take value as
> argument for instance) and let composite_setup() call it.

Yes that could be done. I'll try to do so in the next iteration.

>
>> diff --git a/include/linux/usb/composite.h @@ -37,6 +37,15 @@
>> #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 status phase of the setup transfer till they are
>> + * ready. The composite framework will then delay the data/status phase
>> + * of the setup transfer 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 */
>
> I was wondering if we could change it to something like EDELAYED and put it to
> <include/linux/errno.h>. Just thinking, it may be a bad idea.
>

I would not do it.

Thanks for review.

-- 
regards,
-roger

Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

[Roger Quadros]

Hi,

On 04/19/2011 04:33 PM, ext Roger Quadros wrote:
> Some USB function drivers (e.g. f_mass_storage.c) need to delay or defer the
> status phase 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 setup transfer by not sending the Data/Status response.
>
> 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 phase of the setup transfer. It can do so by invoking
> the new API usb_composite_setup_continue(). This is where the setup transfer's
> data/status phases are completed and host can send 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 as such.
>
> Signed-off-by: Roger Quadros<roger.quadros@nokia.com>
> ---
>   drivers/usb/gadget/composite.c |   61 +++++++++++++++++++++++++++++++++++++++-
>   include/linux/usb/composite.h  |   17 ++++++++++-
>   2 files changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 1ba4bef..43be36e 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -461,6 +461,15 @@ 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);
> +		}
>   	}
>

The below snippet needs to be added to the patch, else it causes a regression.

         /* 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;
  }

I will fix this in the next version of the patch-set which I'll send soon.

-- 
regards,
-roger

[PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests

[Roger Quadros]

We delay the SET_CONFIG and SET_INTERFACE setup transfers status phase
till we are ready to process new CBW from the host. This way we ensure
that we don't loose any CBW and cause lock up.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 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..a70468e 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:
@@ -3083,6 +3086,7 @@ static int fsg_bind_config(struct usb_composite_dev *cdev,
 	fsg->function.disable     = fsg_disable;
 
 	fsg->common               = common;
+	fsg->common->cdev	  = cdev;
 	/*
 	 * Our caller holds a reference to common structure so we
 	 * don't have to be worry about it being freed until we return
-- 
1.6.0.4

Re: [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests

[Michal Nazarewicz]

On Tue, 19 Apr 2011 15:33:53 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:
> @@ -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:

At first I thought that it's a mismatch since USB_GADGET_DELAYED_STATUS
is returned all the time but usb_composite_setup_continue() is called
only if common->new_fsg is set but after more careful investigation
this seems to be correct.

> @@ -3083,6 +3086,7 @@ static int fsg_bind_config(struct  
> usb_composite_dev *cdev,
>  	fsg->function.disable     = fsg_disable;
> 	fsg->common               = common;
> +	fsg->common->cdev	  = cdev;
>  	/*
>  	 * Our caller holds a reference to common structure so we
>  	 * don't have to be worry about it being freed until we return

common->cdev initialisation should be moved to fsg_common_init().

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests

[Sonasath, Moiz]

Roger,

On Tue, Apr 19, 2011 at 8:33 AM, Roger Quadros <roger.quadros@nokia.com> wrote:
>
> 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.
>
> ---
> Roger Quadros (2):
>  usb: gadget: composite: Allow function drivers to defer setup
>    responses
>  usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests

Apart from the comments going on, With these patches I see USBCV MSC
tests passing. thanks for the effort Roger :)

--Acked

>
>  drivers/usb/gadget/composite.c      |   61 ++++++++++++++++++++++++++++++++++-
>  drivers/usb/gadget/f_mass_storage.c |    6 +++-
>  include/linux/usb/composite.h       |   17 +++++++++-
>  3 files changed, 81 insertions(+), 3 deletions(-)
>




--
Regards
Moiz Sonasath
Android Kernel Team