| Message ID | 1233921146-4046-1-git-send-email-ajay.gupta@ti.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Felipe Balbi |
| Headers | show |
Ajay Kumar Gupta wrote: > urb->transfer_buffer_length and urb->transfer_buffer should be > updated based on urb->actual_length.For a fresh and first time urb, > actual_length will be zero but for urbs which has been stopped and > restarted (as bulk nak scheme does) actual_length may not be zero. > Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com> NAK, this is not a problem for the current driver since URBs do not ever get restarted. Also, musb_host_tx() doesn't update urb->actual_length -- please fix it too. Also, you must not clear qh->iso_idx when restarting -- you must not start ISO transfer all over again too. Also, you should not set musb->ep0_state to MUSB_EP0_START again in this case (I agree that control transfers will remain not restartable from an arbitatry place even then). If you're trying to make musb_start_urb() able to re-start, please be consistent. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 06 February 2009, Sergei Shtylyov wrote: > Ajay Kumar Gupta wrote: > > > urb->transfer_buffer_length and urb->transfer_buffer should be > > updated based on urb->actual_length.For a fresh and first time urb, > > actual_length will be zero but for urbs which has been stopped and > > restarted (as bulk nak scheme does) actual_length may not be zero. > > > Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com> > > NAK, this is not a problem for the current driver since URBs do not ever > get restarted. Resolvable by changing the patch description to not say this is a "fix". However, since this is a two line change, I think I'll just merge this with the patch adding the bulk RX retry logic. > Also, musb_host_tx() doesn't update urb->actual_length -- > please fix it too. That would be a good point if the retry patch touched any TX paths. But it doesn't. > Also, you must not clear qh->iso_idx when restarting -- you > must not start ISO transfer all over again too. Also, you should not set > musb->ep0_state to MUSB_EP0_START again in this case (I agree that control > transfers will remain not restartable from an arbitatry place even then). But the [3/3] patch only adds NAK timeout support for bulk RX. And ISO transfers can't NAK in the first place. Plus, as noted in a comment you could see in this patch, this only touches (re)start for bulk/interrupt transfers. Not ISO; not control. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. David Brownell wrote: >>Ajay Kumar Gupta wrote: >>>urb->transfer_buffer_length and urb->transfer_buffer should be >>>updated based on urb->actual_length.For a fresh and first time urb, >>>actual_length will be zero but for urbs which has been stopped and >>>restarted (as bulk nak scheme does) actual_length may not be zero. >>>Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com> >> NAK, this is not a problem for the current driver since URBs do not ever >>get restarted. > Resolvable by changing the patch description to not say > this is a "fix". > However, since this is a two line change, I think I'll > just merge this with the patch adding the bulk RX retry > logic. Yes, that's what should've been done from the start. >>Also, musb_host_tx() doesn't update urb->actual_length -- >>please fix it too. > That would be a good point if the retry patch touched > any TX paths. But it doesn't. If one teaches musb_start_urb() to restart, that one should at least be consistent I think. >>Also, you must not clear qh->iso_idx when restarting -- you >>must not start ISO transfer all over again too. Also, you should not set >>musb->ep0_state to MUSB_EP0_START again in this case (I agree that control >>transfers will remain not restartable from an arbitatry place even then). > But the [3/3] patch only adds NAK timeout support for > bulk RX. And ISO transfers can't NAK in the first place. > Plus, as noted in a comment you could see in this patch, > this only touches (re)start for bulk/interrupt transfers. > Not ISO; not control. All I was asking for was a bit of consistency. Note that I have already done all the changes that I requested for and can post them. URB restart is also going to be used for the interrupt transfers BTW. > - Dave WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/musb/musb_host.c b/drivers/usb/musb/musb_host.c index a776951..1a78f00 100644 --- a/drivers/usb/musb/musb_host.c +++ b/drivers/usb/musb/musb_host.c @@ -201,8 +201,8 @@ musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh) len = urb->iso_frame_desc[0].length; break; default: /* bulk, interrupt */ - buf = urb->transfer_buffer; - len = urb->transfer_buffer_length; + buf = urb->transfer_buffer + urb->actual_length; + len = urb->transfer_buffer_length - urb->actual_length; } DBG(4, "qh %p urb %p dev%d ep%d%s%s, hw_ep %d, %p/%d\n",
urb->transfer_buffer_length and urb->transfer_buffer should be updated based on urb->actual_length.For a fresh and first time urb, actual_length will be zero but for urbs which has been stopped and restarted (as bulk nak scheme does) actual_length may not be zero. Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com> --- drivers/usb/musb/musb_host.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)