| Message ID | 1436282660-10905-1-git-send-email-rui.silva@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Alan, On Tue, Jul 07, 2015 at 01:53:14PM -0400, Alan Stern wrote: >On Tue, 7 Jul 2015, Rui Miguel Silva wrote: > >> When the request actual state is equal to lenght, besides setting the > >"state"? Do you mean "status"? > >"lenght" is a typo. > >> request status to done, the status variable also need to be updated. >> If not, the status will be EINPROGRESS and the transfer will never be >> set as completed. > >The status variable points to the URB, not to the request. > >> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> >> --- >> drivers/usb/gadget/udc/dummy_hcd.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c >> index 181112c..f5400b8 100644 >> --- a/drivers/usb/gadget/udc/dummy_hcd.c >> +++ b/drivers/usb/gadget/udc/dummy_hcd.c >> @@ -1367,8 +1367,10 @@ top: >> /* many requests terminate without a short packet */ >> } else { >> if (req->req.length == req->req.actual >> - && !req->req.zero) >> + && !req->req.zero) { >> req->req.status = 0; >> + *status = 0; >> + } >> if (urb->transfer_buffer_length == urb->actual_length >> && !(urb->transfer_flags >> & URB_ZERO_PACKET)) > >This patch is wrong. *status should indeed remain -EINPROGRESS and the >transfer should remain incomplete. > >Here's an example to illustrate what I mean. Let's say the endpoint's >maxpacket value is 512, and suppose the host queues a 1024-byte IN URB. >In response, suppose the gadget queues two 512-byte requests, each with >req->req.zero clear. Consider what happens when the first request is >processed. > >len, req->req.length and req->req.actual are all equal to 512, and >is_short is false. req therefore completes with req->req.status set to >0. But the URB is still in progress! It still has 512 bytes left to >transfer, and it won't complete until the second request finishes. >Therefore *status (which is the URB's status) should remain set to >-EINPROGRESS. Yes, in this case the patch breaks the flow and remove the URB status of -EINPROGRESS to soon. But what about, and taken your example above, the gadget queues only one 512-byte request? is_short is false, and urb->transfer_buffer_length != urb->actual_length and so the URB status is never changed. Cheers, Rui > >Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, On Wed, Jul 08, 2015 at 10:43:48AM -0400, Alan Stern wrote: >On Wed, 8 Jul 2015, Rui Miguel Silva wrote: > >> >> --- a/drivers/usb/gadget/udc/dummy_hcd.c >> >> +++ b/drivers/usb/gadget/udc/dummy_hcd.c >> >> @@ -1367,8 +1367,10 @@ top: >> >> /* many requests terminate without a short packet */ >> >> } else { >> >> if (req->req.length == req->req.actual >> >> - && !req->req.zero) >> >> + && !req->req.zero) { >> >> req->req.status = 0; >> >> + *status = 0; >> >> + } >> >> if (urb->transfer_buffer_length == urb->actual_length >> >> && !(urb->transfer_flags >> >> & URB_ZERO_PACKET)) >> > >> >This patch is wrong. *status should indeed remain -EINPROGRESS and the >> >transfer should remain incomplete. >> > >> >Here's an example to illustrate what I mean. Let's say the endpoint's >> >maxpacket value is 512, and suppose the host queues a 1024-byte IN URB. >> >In response, suppose the gadget queues two 512-byte requests, each with >> >req->req.zero clear. Consider what happens when the first request is >> >processed. >> > >> >len, req->req.length and req->req.actual are all equal to 512, and >> >is_short is false. req therefore completes with req->req.status set to >> >0. But the URB is still in progress! It still has 512 bytes left to >> >transfer, and it won't complete until the second request finishes. >> >Therefore *status (which is the URB's status) should remain set to >> >-EINPROGRESS. >> >> Yes, in this case the patch breaks the flow and remove the URB status >> of -EINPROGRESS to soon. But what about, and taken your example above, >> the gadget queues only one 512-byte request? is_short is false, and >> urb->transfer_buffer_length != urb->actual_length and so the URB >> status is never changed. > >Yes, the status never changes. And in fact, that is exactly what would >happen if you used a real UDC and a separate host computer instead of >dummy-hcd -- the URB would not complete. (In practice the URB would >eventually time out or be unlinked for some other reason.) Understood. > >Since we want dummy-hcd to behave as much as possible like a real UDC, >we should keep the current code. > Make sense. Thank for the info. Cheers, Rui >Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index 181112c..f5400b8 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -1367,8 +1367,10 @@ top: /* many requests terminate without a short packet */ } else { if (req->req.length == req->req.actual - && !req->req.zero) + && !req->req.zero) { req->req.status = 0; + *status = 0; + } if (urb->transfer_buffer_length == urb->actual_length && !(urb->transfer_flags & URB_ZERO_PACKET))
When the request actual state is equal to lenght, besides setting the request status to done, the status variable also need to be updated. If not, the status will be EINPROGRESS and the transfer will never be set as completed. Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> --- drivers/usb/gadget/udc/dummy_hcd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)