Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
On Wed, Jul 24, 2013 at 02:01:27PM -0400, Alan Stern wrote: On Wed, 24 Jul 2013, Felipe Balbi wrote: Hi, On Tue, Jul 23, 2013 at 12:53:53PM -0400, Alan Stern wrote: I see. Doesn't the mass-storage gadget also use delayed status when going into the _un_configured state? no it doesn't, we bail out early if config number is zero, look at composite.c and you'll see in case of configuration zero, we just make sure to disable all endpoints and don't call -set_alt(): This is my fault, for not being more familiar with the composite framework, and no doubt I will regret asking this, but... Doesn't that approach leave the function drivers sitting around with all their resources still allocated? When do those resources get deallocated? When the gadget disconnects from the host? which resources are you talking about ? Endpoints ? -disable() should make sure to disable the endpoints, but it will only free the endpoints when the gadget driver is unbound. struct usb_requests will also be freed at -disable() time. Does this answer your question ? Yeah, I was mainly wondering about the usb_requests. is this a signal for you're good to go with your fix ? :-) -- balbi signature.asc Description: Digital signature
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
On Thu, 25 Jul 2013, Felipe Balbi wrote: On Wed, Jul 24, 2013 at 02:01:27PM -0400, Alan Stern wrote: On Wed, 24 Jul 2013, Felipe Balbi wrote: Hi, On Tue, Jul 23, 2013 at 12:53:53PM -0400, Alan Stern wrote: I see. Doesn't the mass-storage gadget also use delayed status when going into the _un_configured state? no it doesn't, we bail out early if config number is zero, look at composite.c and you'll see in case of configuration zero, we just make sure to disable all endpoints and don't call -set_alt(): This is my fault, for not being more familiar with the composite framework, and no doubt I will regret asking this, but... Doesn't that approach leave the function drivers sitting around with all their resources still allocated? When do those resources get deallocated? When the gadget disconnects from the host? which resources are you talking about ? Endpoints ? -disable() should make sure to disable the endpoints, but it will only free the endpoints when the gadget driver is unbound. struct usb_requests will also be freed at -disable() time. Does this answer your question ? Yeah, I was mainly wondering about the usb_requests. is this a signal for you're good to go with your fix ? :-) Sure, go ahead. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
Hi, On Tue, Jul 23, 2013 at 12:53:53PM -0400, Alan Stern wrote: I see. Doesn't the mass-storage gadget also use delayed status when going into the _un_configured state? no it doesn't, we bail out early if config number is zero, look at composite.c and you'll see in case of configuration zero, we just make sure to disable all endpoints and don't call -set_alt(): This is my fault, for not being more familiar with the composite framework, and no doubt I will regret asking this, but... Doesn't that approach leave the function drivers sitting around with all their resources still allocated? When do those resources get deallocated? When the gadget disconnects from the host? which resources are you talking about ? Endpoints ? -disable() should make sure to disable the endpoints, but it will only free the endpoints when the gadget driver is unbound. struct usb_requests will also be freed at -disable() time. Does this answer your question ? cheers -- balbi signature.asc Description: Digital signature
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
On Wed, 24 Jul 2013, Felipe Balbi wrote: Hi, On Tue, Jul 23, 2013 at 12:53:53PM -0400, Alan Stern wrote: I see. Doesn't the mass-storage gadget also use delayed status when going into the _un_configured state? no it doesn't, we bail out early if config number is zero, look at composite.c and you'll see in case of configuration zero, we just make sure to disable all endpoints and don't call -set_alt(): This is my fault, for not being more familiar with the composite framework, and no doubt I will regret asking this, but... Doesn't that approach leave the function drivers sitting around with all their resources still allocated? When do those resources get deallocated? When the gadget disconnects from the host? which resources are you talking about ? Endpoints ? -disable() should make sure to disable the endpoints, but it will only free the endpoints when the gadget driver is unbound. struct usb_requests will also be freed at -disable() time. Does this answer your question ? Yeah, I was mainly wondering about the usb_requests. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
Hi,
On Mon, Jul 22, 2013 at 10:55:28AM -0400, Alan Stern wrote:
On Mon, 22 Jul 2013, Felipe Balbi wrote:
before changing to configured state, we need
to wait until gadget driver has had a chance
to process the request.
In case of USB_GADGET_DELAYED_STATUS, that means
we need to defer usb_gadget_set_state() until
the upcoming usb_ep_queue().
Reported-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/usb/dwc3/ep0.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 007651c..7fa93f4 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -148,6 +148,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
direction = !dwc-ep0_expect_in;
dwc-delayed_status = false;
+ usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED);
Isn't this overkill? Do you really want to call usb_gadget_set_state()
every time the gadget driver queues a transfer on ep0?
Or am I missing an important part of the context?
heh, you're missing context, that will only be called when we had
delayed status flag set:
| static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
| struct dwc3_request *req)
| {
[ ... ]
| /*
|* In case gadget driver asked us to delay the STATUS phase,
|* handle it here.
|*/
| if (dwc-delayed_status) {
| unsigneddirection;
|
| direction = !dwc-ep0_expect_in;
| dwc-delayed_status = false;
| usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED);
|
| if (dwc-ep0state == EP0_STATUS_PHASE)
| __dwc3_ep0_do_control_status(dwc, dwc-eps[direction]);
| else
| dev_dbg(dwc-dev, too early for delayed status\n);
|
| return 0;
| }
[ ... ]
| return 0;
| }
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
On Tue, 23 Jul 2013, Felipe Balbi wrote:
@@ -148,6 +148,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep
*dep,
direction = !dwc-ep0_expect_in;
dwc-delayed_status = false;
+ usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED);
Isn't this overkill? Do you really want to call usb_gadget_set_state()
every time the gadget driver queues a transfer on ep0?
Or am I missing an important part of the context?
heh, you're missing context, that will only be called when we had
delayed status flag set:
| static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
| struct dwc3_request *req)
| {
[ ... ]
| /*
| * In case gadget driver asked us to delay the STATUS phase,
| * handle it here.
| */
| if (dwc-delayed_status) {
| unsigneddirection;
|
| direction = !dwc-ep0_expect_in;
| dwc-delayed_status = false;
| usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED);
I see. Doesn't the mass-storage gadget also use delayed status when
going into the _un_configured state?
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
Hi,
On Tue, Jul 23, 2013 at 10:06:15AM -0400, Alan Stern wrote:
@@ -148,6 +148,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep
*dep,
direction = !dwc-ep0_expect_in;
dwc-delayed_status = false;
+ usb_gadget_set_state(dwc-gadget,
USB_STATE_CONFIGURED);
Isn't this overkill? Do you really want to call usb_gadget_set_state()
every time the gadget driver queues a transfer on ep0?
Or am I missing an important part of the context?
heh, you're missing context, that will only be called when we had
delayed status flag set:
| static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
| struct dwc3_request *req)
| {
[ ... ]
| /*
|* In case gadget driver asked us to delay the STATUS phase,
|* handle it here.
|*/
| if (dwc-delayed_status) {
| unsigneddirection;
|
| direction = !dwc-ep0_expect_in;
| dwc-delayed_status = false;
| usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED);
I see. Doesn't the mass-storage gadget also use delayed status when
going into the _un_configured state?
no it doesn't, we bail out early if config number is zero, look at
composite.c and you'll see in case of configuration zero, we just make
sure to disable all endpoints and don't call -set_alt():
| static void reset_config(struct usb_composite_dev *cdev)
| {
| struct usb_function *f;
|
| DBG(cdev, reset config\n);
|
| list_for_each_entry(f, cdev-config-functions, list) {
| if (f-disable)
| f-disable(f);
|
| bitmap_zero(f-endpoints, 32);
| }
| cdev-config = NULL;
| }
|
| static int set_config(struct usb_composite_dev *cdev,
| const struct usb_ctrlrequest *ctrl, unsigned number)
| {
| struct usb_gadget *gadget = cdev-gadget;
| struct usb_configuration *c = NULL;
| int result = -EINVAL;
| unsignedpower = gadget_is_otg(gadget) ? 8 : 100;
| int tmp;
|
| if (number) {
| list_for_each_entry(c, cdev-configs, list) {
| if (c-bConfigurationValue == number) {
| /*
|* We disable the FDs of the previous
|* configuration only if the new configuration
|* is a valid one
|*/
| if (cdev-config)
| reset_config(cdev);
| result = 0;
| break;
| }
| }
| if (result 0)
| goto done;
| } else { /* Zero configuration value - need to reset the config */
| if (cdev-config)
| reset_config(cdev);
| result = 0;
| }
|
| INFO(cdev, %s config #%d: %s\n,
|usb_speed_string(gadget-speed),
|number, c ? c-label : unconfigured);
|
| if (!c)
| goto done;
[ ... ]
| done:
| usb_gadget_vbus_draw(gadget, power);
| if (result = 0 cdev-delayed_status)
| result = USB_GADGET_DELAYED_STATUS;
| return result;
| }
|
| int
| composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
| {
| struct usb_composite_dev*cdev = get_gadget_data(gadget);
| struct usb_request *req = cdev-req;
| int value = -EOPNOTSUPP;
| int status = 0;
| u16 w_index = le16_to_cpu(ctrl-wIndex);
| u8 intf = w_index 0xFF;
| u16 w_value = le16_to_cpu(ctrl-wValue);
| u16 w_length = le16_to_cpu(ctrl-wLength);
| struct usb_function *f = NULL;
| u8 endp;
[ ... ]
| switch (ctrl-bRequest) {
[ ... ]
| /* any number of configs can work */
| case USB_REQ_SET_CONFIGURATION:
| if (ctrl-bRequestType != 0)
| goto unknown;
| if (gadget_is_otg(gadget)) {
| if (gadget-a_hnp_support)
| DBG(cdev, HNP available\n);
| else if (gadget-a_alt_hnp_support)
| DBG(cdev, HNP on another port\n);
| else
| VDBG(cdev, HNP inactive\n);
| }
| spin_lock(cdev-lock);
| value = set_config(cdev, ctrl, w_value);
| spin_unlock(cdev-lock);
| break;
[ ...]
| }
|
| /* respond with data
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
On Tue, 23 Jul 2013, Felipe Balbi wrote: I see. Doesn't the mass-storage gadget also use delayed status when going into the _un_configured state? no it doesn't, we bail out early if config number is zero, look at composite.c and you'll see in case of configuration zero, we just make sure to disable all endpoints and don't call -set_alt(): This is my fault, for not being more familiar with the composite framework, and no doubt I will regret asking this, but... Doesn't that approach leave the function drivers sitting around with all their resources still allocated? When do those resources get deallocated? When the gadget disconnects from the host? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
before changing to configured state, we need
to wait until gadget driver has had a chance
to process the request.
In case of USB_GADGET_DELAYED_STATUS, that means
we need to defer usb_gadget_set_state() until
the upcoming usb_ep_queue().
Reported-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/usb/dwc3/ep0.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 007651c..7fa93f4 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -148,6 +148,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
direction = !dwc-ep0_expect_in;
dwc-delayed_status = false;
+ usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED);
if (dwc-ep0state == EP0_STATUS_PHASE)
__dwc3_ep0_do_control_status(dwc, dwc-eps[direction]);
@@ -533,8 +534,16 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
usb_ctrlrequest *ctrl)
ret = dwc3_ep0_delegate_req(dwc, ctrl);
/* if the cfg matches and the cfg is non zero */
if (cfg (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
- usb_gadget_set_state(dwc-gadget,
- USB_STATE_CONFIGURED);
+
+ /*
+* only change state if set_config has already
+* been processed. If gadget driver returns
+* USB_GADGET_DELAYED_STATUS, we will wait
+* to change the state on the next usb_ep_queue()
+*/
+ if (ret == 0)
+ usb_gadget_set_state(dwc-gadget,
+ USB_STATE_CONFIGURED);
/*
* Enable transition to U1/U2 state when
--
1.8.3.3.754.g9c3c367
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: ep0: don't change to configured state too early
On Mon, 22 Jul 2013, Felipe Balbi wrote: before changing to configured state, we need to wait until gadget driver has had a chance to process the request. In case of USB_GADGET_DELAYED_STATUS, that means we need to defer usb_gadget_set_state() until the upcoming usb_ep_queue(). Reported-by: Alan Stern st...@rowland.harvard.edu Signed-off-by: Felipe Balbi ba...@ti.com --- drivers/usb/dwc3/ep0.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 007651c..7fa93f4 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -148,6 +148,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep, direction = !dwc-ep0_expect_in; dwc-delayed_status = false; + usb_gadget_set_state(dwc-gadget, USB_STATE_CONFIGURED); Isn't this overkill? Do you really want to call usb_gadget_set_state() every time the gadget driver queues a transfer on ep0? Or am I missing an important part of the context? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
