* [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests
@ 2011-04-19 13:33 Roger Quadros
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Roger Quadros @ 2011-04-19 13:33 UTC (permalink / raw)
To: gregkh; +Cc: stern, mina86, m-sonasath, linux-usb, linux-kernel
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(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses
2011-04-19 13:33 [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests Roger Quadros
@ 2011-04-19 13:33 ` Roger Quadros
2011-04-19 14:14 ` Alan Stern
` (2 more replies)
2011-04-19 13:33 ` [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests Roger Quadros
2011-04-19 20:44 ` [PATCH 0/2] usb: gadget: Make mass_storage " Sonasath, Moiz
2 siblings, 3 replies; 10+ messages in thread
From: Roger Quadros @ 2011-04-19 13:33 UTC (permalink / raw)
To: gregkh; +Cc: stern, mina86, m-sonasath, linux-usb, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests
2011-04-19 13:33 [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests Roger Quadros
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
@ 2011-04-19 13:33 ` Roger Quadros
2011-04-19 14:33 ` Michal Nazarewicz
2011-04-19 20:44 ` [PATCH 0/2] usb: gadget: Make mass_storage " Sonasath, Moiz
2 siblings, 1 reply; 10+ messages in thread
From: Roger Quadros @ 2011-04-19 13:33 UTC (permalink / raw)
To: gregkh; +Cc: stern, mina86, m-sonasath, linux-usb, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
@ 2011-04-19 14:14 ` Alan Stern
2011-04-21 13:27 ` Roger Quadros
2011-04-19 14:23 ` Michal Nazarewicz
2011-05-09 9:42 ` Roger Quadros
2 siblings, 1 reply; 10+ messages in thread
From: Alan Stern @ 2011-04-19 14:14 UTC (permalink / raw)
To: Roger Quadros
Cc: gregkh, mina86, m-sonasath, USB list, Kernel development list
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
2011-04-19 14:14 ` Alan Stern
@ 2011-04-19 14:23 ` Michal Nazarewicz
2011-04-21 13:30 ` Roger Quadros
2011-05-09 9:42 ` Roger Quadros
2 siblings, 1 reply; 10+ messages in thread
From: Michal Nazarewicz @ 2011-04-19 14:23 UTC (permalink / raw)
To: gregkh, Roger Quadros; +Cc: stern, m-sonasath, linux-usb, linux-kernel
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--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests
2011-04-19 13:33 ` [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests Roger Quadros
@ 2011-04-19 14:33 ` Michal Nazarewicz
0 siblings, 0 replies; 10+ messages in thread
From: Michal Nazarewicz @ 2011-04-19 14:33 UTC (permalink / raw)
To: gregkh, Roger Quadros; +Cc: stern, m-sonasath, linux-usb, linux-kernel
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--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests
2011-04-19 13:33 [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests Roger Quadros
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
2011-04-19 13:33 ` [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests Roger Quadros
@ 2011-04-19 20:44 ` Sonasath, Moiz
2 siblings, 0 replies; 10+ messages in thread
From: Sonasath, Moiz @ 2011-04-19 20:44 UTC (permalink / raw)
To: Roger Quadros; +Cc: gregkh, stern, mina86, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses
2011-04-19 14:14 ` Alan Stern
@ 2011-04-21 13:27 ` Roger Quadros
0 siblings, 0 replies; 10+ messages in thread
From: Roger Quadros @ 2011-04-21 13:27 UTC (permalink / raw)
To: ext Alan Stern
Cc: gregkh, mina86, m-sonasath, USB list, Kernel development list
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses
2011-04-19 14:23 ` Michal Nazarewicz
@ 2011-04-21 13:30 ` Roger Quadros
0 siblings, 0 replies; 10+ messages in thread
From: Roger Quadros @ 2011-04-21 13:30 UTC (permalink / raw)
To: ext Michal Nazarewicz; +Cc: gregkh, stern, m-sonasath, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
2011-04-19 14:14 ` Alan Stern
2011-04-19 14:23 ` Michal Nazarewicz
@ 2011-05-09 9:42 ` Roger Quadros
2 siblings, 0 replies; 10+ messages in thread
From: Roger Quadros @ 2011-05-09 9:42 UTC (permalink / raw)
To: ext Roger Quadros
Cc: gregkh, stern, mina86, m-sonasath, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-05-09 9:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-19 13:33 [PATCH 0/2] usb: gadget: Make mass_storage pass USBCV MSC Compliance tests Roger Quadros
2011-04-19 13:33 ` [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses Roger Quadros
2011-04-19 14:14 ` Alan Stern
2011-04-21 13:27 ` Roger Quadros
2011-04-19 14:23 ` Michal Nazarewicz
2011-04-21 13:30 ` Roger Quadros
2011-05-09 9:42 ` Roger Quadros
2011-04-19 13:33 ` [PATCH 2/2] usb: gadget: f_mass_storage: Make us pass USBCV MSC Compliance tests Roger Quadros
2011-04-19 14:33 ` Michal Nazarewicz
2011-04-19 20:44 ` [PATCH 0/2] usb: gadget: Make mass_storage " Sonasath, Moiz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.