RE: [PATCH] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Wed, Aug 20, 2014 at 03:18:47PM -0400, Alan Stern wrote:
On Wed, 20 Aug 2014, Felipe Balbi wrote:
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1956,6 +1956,7 @@ void composite_dev_cleanup(struct
usb_composite_dev *cdev)
}
if (cdev-req) {
kfree(cdev-req-buf);
+ usb_ep_dequeue(cdev-gadget-ep0, cdev-req);
it's best to dequeue the request before freeing its buffer.
In fact, it's best to wait for the request's completion routine
to be called before freeing the buffer. The hardware can access
the buffer's memory at any time before the completion routine
runs.
dequeue should cause the transfer to be cancelled and given back.
There's a slight possible race window because we release the
controller lock in order to call gadget driver's -complete().
The dp has already been pulled down and the flush pending FIFO buffer
has finished, so the controller should not try to access memory buffer
after that.
In that case, why does the patch add a call to usb_ep_dequeue()?
Shouldn't the request be unlinked already?
I mean the controller should not try to access memory buffer after flush
pending buffer which should be done at usb_ep_dequeue, so it is safe we free
the request buffer after usb_ep_dequeue
Other than that, it should be fine. No ?
Dequeue causes the transfer to be cancelled and given back, yes.
But
usb_ep_dequeue() is allowed to return before those things happen.
If usb_ep_dequeue is returned before doing any flush and cancel
transfer, it means there is no request on the queue, we don't need to
cancel any requests.
It can be different for different UDCs. The documentation doesn't say
that usb_ep_dequeue has to wait until the transfer has been cancelled and
flushed.
I think it should add this content, it is strange that we still need to
Wait the transfer has finished/cancelled after calling dequeue, we de-queue
the request, of cos we hope the request has finished, and its resource can be
re-used again.
Peter
--
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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Alan Stern Sent: Thursday, August 21, 2014 8:38 AM On Thu, 21 Aug 2014, Peter Chen wrote: dequeue should cause the transfer to be cancelled and given back. There's a slight possible race window because we release the controller lock in order to call gadget driver's -complete(). The dp has already been pulled down and the flush pending FIFO buffer has finished, so the controller should not try to access memory buffer after that. In that case, why does the patch add a call to usb_ep_dequeue()? Shouldn't the request be unlinked already? I mean the controller should not try to access memory buffer after flush pending buffer which should be done at usb_ep_dequeue, so it is safe we free the request buffer after usb_ep_dequeue Host controller hardware most definitely _does_ continue to access buffer memory after a dequeue, because the hardware caches the transfer descriptors. I don't know how similar device controller hardware is to host controller hardware, but it certainly seems possible that there could be similar caching going on. It can be different for different UDCs. The documentation doesn't say that usb_ep_dequeue has to wait until the transfer has been cancelled and flushed. I think it should add this content, it is strange that we still need to Wait the transfer has finished/cancelled after calling dequeue, we de-queue the request, of cos we hope the request has finished, and its resource can be re-used again. That is not true for usb_unlink_urb(). So why should it be true for usb_ep_dequeue()? Maybe what you want is something like usb_kill_urb(), for the gadget stack. AFAIK, it has always been a requirement that usb_ep_dequeue() release the data buffer and call the urb's completion routine before it returns. Quite a few of the function drivers depend on that, after the call to usb_ep_dequeue() they immediately free the data buffer. So if it's not explicitly documented like that, it should be. -- Paul -- 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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Thu, 21 Aug 2014, Paul Zimmerman wrote: AFAIK, it has always been a requirement that usb_ep_dequeue() release the data buffer and call the urb's completion routine before it returns. Hmmm. I don't know if that really has been a requirement, but it does seem to be true in the UDC drivers I'm familiar with. Quite a few of the function drivers depend on that, after the call to usb_ep_dequeue() they immediately free the data buffer. So if it's not explicitly documented like that, it should be. Okay, would you like to write a documentation patch? 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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
From: Alan Stern [mailto:st...@rowland.harvard.edu] Sent: Thursday, August 21, 2014 12:16 PM On Thu, 21 Aug 2014, Paul Zimmerman wrote: AFAIK, it has always been a requirement that usb_ep_dequeue() release the data buffer and call the urb's completion routine before it returns. Hmmm. I don't know if that really has been a requirement, but it does seem to be true in the UDC drivers I'm familiar with. Quite a few of the function drivers depend on that, after the call to usb_ep_dequeue() they immediately free the data buffer. So if it's not explicitly documented like that, it should be. Okay, would you like to write a documentation patch? Something like this? I didn't find any references to usp_ep_dequeue() under Documentation/, so I guess the only place is the kerneldoc in the header. I also fixed up some strange capitalization in the existing text while I was there. If this looks OK to you I will send a proper patch submission to Felipe. [PATCH] usb: gadget: document a usb_ep_dequeue() requirement Document the requirement that the request be dequeued and its completion routine called before usb_ep_dequeue() returns. diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index c3a6185..c540557 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -345,12 +345,13 @@ static inline int usb_ep_queue(struct usb_ep *ep, * @ep:the endpoint associated with the request * @req:the request being canceled * - * if the request is still active on the endpoint, it is dequeued and its + * If the request is still active on the endpoint, it is dequeued and its * completion routine is called (with status -ECONNRESET); else a negative - * error code is returned. + * error code is returned. This is guaranteed to happen before the call to + * usb_ep_dequeue() returns. * - * note that some hardware can't clear out write fifos (to unlink the request - * at the head of the queue) except as part of disconnecting from usb. such + * Note that some hardware can't clear out write fifos (to unlink the request + * at the head of the queue) except as part of disconnecting from usb. Such * restrictions prevent drivers from supporting configuration changes, * even to configuration zero (a chapter 9 requirement). */ -- Paul -- 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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Thu, 21 Aug 2014, Paul Zimmerman wrote: From: Alan Stern [mailto:st...@rowland.harvard.edu] Sent: Thursday, August 21, 2014 12:16 PM On Thu, 21 Aug 2014, Paul Zimmerman wrote: AFAIK, it has always been a requirement that usb_ep_dequeue() release the data buffer and call the urb's completion routine before it returns. Hmmm. I don't know if that really has been a requirement, but it does seem to be true in the UDC drivers I'm familiar with. Quite a few of the function drivers depend on that, after the call to usb_ep_dequeue() they immediately free the data buffer. So if it's not explicitly documented like that, it should be. Okay, would you like to write a documentation patch? Something like this? I didn't find any references to usp_ep_dequeue() under Documentation/, so I guess the only place is the kerneldoc in the header. Yes, that's where it is. I also fixed up some strange capitalization in the existing text while I was there. If this looks OK to you I will send a proper patch submission to Felipe. [PATCH] usb: gadget: document a usb_ep_dequeue() requirement Document the requirement that the request be dequeued and its completion routine called before usb_ep_dequeue() returns. diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index c3a6185..c540557 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -345,12 +345,13 @@ static inline int usb_ep_queue(struct usb_ep *ep, * @ep:the endpoint associated with the request * @req:the request being canceled * - * if the request is still active on the endpoint, it is dequeued and its + * If the request is still active on the endpoint, it is dequeued and its * completion routine is called (with status -ECONNRESET); else a negative - * error code is returned. + * error code is returned. This is guaranteed to happen before the call to + * usb_ep_dequeue() returns. * - * note that some hardware can't clear out write fifos (to unlink the request - * at the head of the queue) except as part of disconnecting from usb. such + * Note that some hardware can't clear out write fifos (to unlink the request + * at the head of the queue) except as part of disconnecting from usb. Such * restrictions prevent drivers from supporting configuration changes, * even to configuration zero (a chapter 9 requirement). */ Looks good to me. Acked-by: Alan Stern st...@rowland.harvard.edu 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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Thu, Aug 21, 2014 at 03:55:13PM -0400, Alan Stern wrote: On Thu, 21 Aug 2014, Paul Zimmerman wrote: From: Alan Stern [mailto:st...@rowland.harvard.edu] Sent: Thursday, August 21, 2014 12:16 PM On Thu, 21 Aug 2014, Paul Zimmerman wrote: AFAIK, it has always been a requirement that usb_ep_dequeue() release the data buffer and call the urb's completion routine before it returns. Hmmm. I don't know if that really has been a requirement, but it does seem to be true in the UDC drivers I'm familiar with. Quite a few of the function drivers depend on that, after the call to usb_ep_dequeue() they immediately free the data buffer. So if it's not explicitly documented like that, it should be. Okay, would you like to write a documentation patch? Something like this? I didn't find any references to usp_ep_dequeue() under Documentation/, so I guess the only place is the kerneldoc in the header. Yes, that's where it is. I also fixed up some strange capitalization in the existing text while I was there. If this looks OK to you I will send a proper patch submission to Felipe. [PATCH] usb: gadget: document a usb_ep_dequeue() requirement Document the requirement that the request be dequeued and its completion routine called before usb_ep_dequeue() returns. diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index c3a6185..c540557 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -345,12 +345,13 @@ static inline int usb_ep_queue(struct usb_ep *ep, * @ep:the endpoint associated with the request * @req:the request being canceled * - * if the request is still active on the endpoint, it is dequeued and its + * If the request is still active on the endpoint, it is dequeued and its * completion routine is called (with status -ECONNRESET); else a negative - * error code is returned. + * error code is returned. This is guaranteed to happen before the call to + * usb_ep_dequeue() returns. * - * note that some hardware can't clear out write fifos (to unlink the request - * at the head of the queue) except as part of disconnecting from usb. such + * Note that some hardware can't clear out write fifos (to unlink the request + * at the head of the queue) except as part of disconnecting from usb. Such * restrictions prevent drivers from supporting configuration changes, * even to configuration zero (a chapter 9 requirement). */ Looks good to me. Acked-by: Alan Stern st...@rowland.harvard.edu Thanks Alan and Paul :-) Paul, can you just send this as a formal patch with Alan's Acked-by ? Thank you -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Tue, Jul 15, 2014 at 10:07:40PM +0800, Li Jun wrote:
This patch try to dequeue the cdev-req to guarantee the request is not queued
before free it.
Signed-off-by: Li Jun b47...@freescale.com
---
drivers/usb/gadget/composite.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index f801519..6935a82 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1956,6 +1956,7 @@ void composite_dev_cleanup(struct usb_composite_dev
*cdev)
}
if (cdev-req) {
kfree(cdev-req-buf);
+ usb_ep_dequeue(cdev-gadget-ep0, cdev-req);
it's best to dequeue the request before freeing its buffer.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Wed, 20 Aug 2014, Felipe Balbi wrote:
On Tue, Jul 15, 2014 at 10:07:40PM +0800, Li Jun wrote:
This patch try to dequeue the cdev-req to guarantee the request is not
queued
before free it.
Signed-off-by: Li Jun b47...@freescale.com
---
drivers/usb/gadget/composite.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index f801519..6935a82 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1956,6 +1956,7 @@ void composite_dev_cleanup(struct usb_composite_dev
*cdev)
}
if (cdev-req) {
kfree(cdev-req-buf);
+ usb_ep_dequeue(cdev-gadget-ep0, cdev-req);
it's best to dequeue the request before freeing its buffer.
In fact, it's best to wait for the request's completion routine to be
called before freeing the buffer. The hardware can access the buffer's
memory at any time before the completion routine runs.
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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Wed, Aug 20, 2014 at 03:06:28PM -0400, Alan Stern wrote:
On Wed, 20 Aug 2014, Felipe Balbi wrote:
On Tue, Jul 15, 2014 at 10:07:40PM +0800, Li Jun wrote:
This patch try to dequeue the cdev-req to guarantee the request is not
queued
before free it.
Signed-off-by: Li Jun b47...@freescale.com
---
drivers/usb/gadget/composite.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c
b/drivers/usb/gadget/composite.c
index f801519..6935a82 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1956,6 +1956,7 @@ void composite_dev_cleanup(struct usb_composite_dev
*cdev)
}
if (cdev-req) {
kfree(cdev-req-buf);
+ usb_ep_dequeue(cdev-gadget-ep0, cdev-req);
it's best to dequeue the request before freeing its buffer.
In fact, it's best to wait for the request's completion routine to be
called before freeing the buffer. The hardware can access the buffer's
memory at any time before the completion routine runs.
dequeue should cause the transfer to be cancelled and given back.
There's a slight possible race window because we release the controller
lock in order to call gadget driver's -complete().
Other than that, it should be fine. No ?
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
On Wed, Aug 20, 2014 at 03:18:47PM -0400, Alan Stern wrote:
On Wed, 20 Aug 2014, Felipe Balbi wrote:
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1956,6 +1956,7 @@ void composite_dev_cleanup(struct
usb_composite_dev *cdev)
}
if (cdev-req) {
kfree(cdev-req-buf);
+ usb_ep_dequeue(cdev-gadget-ep0, cdev-req);
it's best to dequeue the request before freeing its buffer.
In fact, it's best to wait for the request's completion routine to be
called before freeing the buffer. The hardware can access the buffer's
memory at any time before the completion routine runs.
dequeue should cause the transfer to be cancelled and given back.
There's a slight possible race window because we release the controller
lock in order to call gadget driver's -complete().
The dp has already been pulled down and the flush pending FIFO buffer
has finished, so the controller should not try to access memory buffer
after that.
Other than that, it should be fine. No ?
Dequeue causes the transfer to be cancelled and given back, yes. But
usb_ep_dequeue() is allowed to return before those things happen.
If usb_ep_dequeue is returned before doing any flush and cancel
transfer, it means there is no request on the queue, we don't need
to cancel any requests.
Felipe's suggestion is safe way, we can have a patch for it.
It depends on the implementation; in general the only way to know that
the hardware has finished aborting or completing the request is to wait
until the completion routine is called.
Alan Stern
--
Best Regards,
Peter Chen
--
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] usb: gadget: composite: dequeue cdev-req before free it in composite_dev_cleanup
This patch try to dequeue the cdev-req to guarantee the request is not queued
before free it.
Signed-off-by: Li Jun b47...@freescale.com
---
drivers/usb/gadget/composite.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index f801519..6935a82 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1956,6 +1956,7 @@ void composite_dev_cleanup(struct usb_composite_dev *cdev)
}
if (cdev-req) {
kfree(cdev-req-buf);
+ usb_ep_dequeue(cdev-gadget-ep0, cdev-req);
usb_ep_free_request(cdev-gadget-ep0, cdev-req);
}
cdev-next_string_id = 0;
--
1.7.9.5
--
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
