diff mbox

[2/3] usb: musb: fix bug in musb_start_urb

Message ID 1233921146-4046-1-git-send-email-ajay.gupta@ti.com
State Rejected, archived
Delegated to: Felipe Balbi
Headers show

Commit Message

Ajay Kumar Gupta Feb. 6, 2009, 11:52 a.m. UTC
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(-)

Comments

Sergei Shtylyov Feb. 6, 2009, 3:03 p.m. UTC | #1
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
David Brownell Feb. 23, 2009, 10:05 p.m. UTC | #2
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
Sergei Shtylyov Feb. 24, 2009, 3:50 p.m. UTC | #3
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 mbox

Patch

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",