[RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[tom.leiming]

This patch set fixes two kinds of musb-gadget bug:

	-fix kernel panic if using out ep with FIFO_TXRX style

	-make double buffer mode usable, fix infinate hangs and buffer
	corrupt bug when using double buffer mode to do data transfer;
	now we can use musb double buffer reliably, so bootst performance
	about 30% compared with single buffer mode.
	
		[RESEND/PATCH 1/6] USB: musb-gadget: fix kernel panic if using out ep with FIFO_TXRX style(v1)
		[RESEND/PATCH 2/6] USB: musb-gadget: fix bulk IN infinite hangs in double buffer case
		[RESEND/PATCH 3/6] USB: musb-gadget: enable autoclear for OUT transfer in both DMA 0 and DMA 1
		[RESEND/PATCH 4/6] USB: musb-gadget: fix DMA length for OUT transfer
		[RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over
		[RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

thanks,
Lei Ming

[RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[tom.leiming-Re5JQEeQqe8AvxtiuMwx3w]

This patch set fixes two kinds of musb-gadget bug:

	-fix kernel panic if using out ep with FIFO_TXRX style

	-make double buffer mode usable, fix infinate hangs and buffer
	corrupt bug when using double buffer mode to do data transfer;
	now we can use musb double buffer reliably, so bootst performance
	about 30% compared with single buffer mode.
	
		[RESEND/PATCH 1/6] USB: musb-gadget: fix kernel panic if using out ep with FIFO_TXRX style(v1)
		[RESEND/PATCH 2/6] USB: musb-gadget: fix bulk IN infinite hangs in double buffer case
		[RESEND/PATCH 3/6] USB: musb-gadget: enable autoclear for OUT transfer in both DMA 0 and DMA 1
		[RESEND/PATCH 4/6] USB: musb-gadget: fix DMA length for OUT transfer
		[RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over
		[RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

thanks,
Lei Ming

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESEND/PATCH 1/6] USB: musb-gadget: fix kernel panic if using out ep with FIFO_TXRX style(v1)

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

For shared fifo hw endpoint(with FIFO_TXRX style), only ep_in
field of musb_hw_ep is intialized in musb_g_init_endpoints, and
ep_out is not initialized, but musb_g_rx and rxstate may access
ep_out field of musb_hw_ep by the method below:

	musb_ep = &musb->endpoints[epnum].ep_out

which can cause the kernel panic[1] below, this patch fixes the issue
by getting 'musb_ep' from '&musb->endpoints[epnum].ep_in' for shared fifo
endpoint.

[1], kernel panic
[root@OMAP3EVM /]# musb_interrupt 1583: ** IRQ peripheral usb0008 tx0000 rx4000
musb_stage0_irq 460: <== Power=f0, DevCtl=99, int_usb=0x8
musb_g_rx 772: <== (null), rxcsr 4007 ffffffe8
musb_g_rx 786:  iso overrun on ffffffe8
Unable to handle kernel NULL pointer dereference at virtual address 00000008
pgd = c0004000
[00000008] *pgd=00000000
Internal error: Oops: 17 [#1] PREEMPT
last sysfs file: /sys/devices/platform/musb_hdrc/usb1/usb_device/usbdev1.1/dev
Modules linked in: g_zero
CPU: 0    Tainted: G        W    (2.6.35-rc6-gkh-wl+ #92)
PC is at musb_g_rx+0xfc/0x2ec
LR is at vprintk+0x3f4/0x458
pc : [<c02c07a4>]    lr : [<c006ccb0>]    psr: 20000193
sp : c760bd78  ip : c03c9d70  fp : c760bdbc
r10: 00000000  r9 : fa0ab1e0  r8 : 0000000e
r7 : c7e80158  r6 : ffffffe8  r5 : 00000001  r4 : 00004003
r3 : 00010003  r2 : c760bcd8  r1 : c03cd030  r0 : 0000002e
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 8778c019  DAC: 00000017
Process kmemleak (pid: 421, stack limit = 0xc760a2e8)
Stack: (0xc760bd78 to 0xc760c000)
bd60:                                                       ffffffe8 c04b1b58
bd80: ffffffe8 c7c01ac0 00000000 c7e80d24 c0084238 00000001 00000001 c7e80158
bda0: 0000000e 00000008 00000099 000000f0 c760be04 c760bdc0 c02bcd68 c02c06b4
bdc0: 00000099 00000008 00004000 c760bdd8 c03cc4f8 00000000 00000002 c7e80158
bde0: c7d2e300 60000193 c760a000 0000005c 00000000 00000000 c760be24 c760be08
be00: c02bcecc c02bc1ac c7d2e300 c7d2e300 0000005c c760a000 c760be54 c760be28
be20: c00ad698 c02bce6c 00000000 c7d2e300 c067c258 0000005c c067c294 00000001
be40: c760a000 00000000 c760be74 c760be58 c00af984 c00ad5fc 0000005c 00000000
be60: 00000000 00000002 c760be8c c760be78 c0039080 c00af8d0 ffffffff fa200000
be80: c760beec c760be90 c0039b6c c003900c 00000001 00000000 c7d1e240 00000000
bea0: 00000000 c068bae8 00000000 60000013 00000001 00000000 00000000 c760beec
bec0: c0064ecc c760bed8 c00ff7d0 c003a0a8 60000013 ffffffff 00000000 c068bae8
bee0: c760bf24 c760bef0 c00ff7d0 c0064ec4 00000001 00000000 c00ff700 00000000
bf00: c0087f00 00000000 60000013 c0d76a70 c0e23795 00000001 c760bf4c c760bf28
bf20: c00ffdd8 c00ff70c c068bb08 c068bae8 60000013 c0100938 c068bb30 00000000
bf40: c760bf84 c760bf50 c010014c c00ffd84 00000001 00000000 c010000c 00012c00
bf60: c7c33f04 00012c00 c7c33f04 00000000 c0100938 00000000 c760bf9c c760bf88
bf80: c01009a8 c0100018 c760bfa8 c7c33f04 c760bff4 c760bfa0 c0088000 c0100944
bfa0: c760bf98 00000000 00000000 00000001 dead4ead ffffffff ffffffff c08ba2bc
bfc0: 00000000 c049e7fa 00000000 c0087f70 c760bfd0 c760bfd0 c7c33f04 c0087f70
bfe0: c006f5e8 00000013 00000000 c760bff8 c006f5e8 c0087f7c 7f0004ff df2000ff
Backtrace:
[<c02c06a8>] (musb_g_rx+0x0/0x2ec) from [<c02bcd68>] (musb_interrupt+0xbc8/0xcc0)
[<c02bc1a0>] (musb_interrupt+0x0/0xcc0) from [<c02bcecc>] (generic_interrupt+0x6c/0x84)
[<c02bce60>] (generic_interrupt+0x0/0x84) from [<c00ad698>] (handle_IRQ_event+0xa8/0x1ec)
 r7:c760a000 r6:0000005c r5:c7d2e300 r4:c7d2e300
[<c00ad5f0>] (handle_IRQ_event+0x0/0x1ec) from [<c00af984>] (handle_level_irq+0xc0/0x13c)
[<c00af8c4>] (handle_level_irq+0x0/0x13c) from [<c0039080>] (asm_do_IRQ+0x80/0xa0)
 r7:00000002 r6:00000000 r5:00000000 r4:0000005c
[<c0039000>] (asm_do_IRQ+0x0/0xa0) from [<c0039b6c>] (__irq_svc+0x4c/0xb4)
Exception stack(0xc760be90 to 0xc760bed8)
be80:                                     00000001 00000000 c7d1e240 00000000
bea0: 00000000 c068bae8 00000000 60000013 00000001 00000000 00000000 c760beec
bec0: c0064ecc c760bed8 c00ff7d0 c003a0a8 60000013 ffffffff
 r5:fa200000 r4:ffffffff
[<c0064eb8>] (sub_preempt_count+0x0/0x100) from [<c00ff7d0>] (find_and_get_object+0xd0/0x110)
 r5:c068bae8 r4:00000000
[<c00ff700>] (find_and_get_object+0x0/0x110) from [<c00ffdd8>] (scan_block+0x60/0x104)
 r8:00000001 r7:c0e23795 r6:c0d76a70 r5:60000013 r4:00000000
[<c00ffd78>] (scan_block+0x0/0x104) from [<c010014c>] (kmemleak_scan+0x140/0x484)
[<c010000c>] (kmemleak_scan+0x0/0x484) from [<c01009a8>] (kmemleak_scan_thread+0x70/0xcc)
 r8:00000000 r7:c0100938 r6:00000000 r5:c7c33f04 r4:00012c00
[<c0100938>] (kmemleak_scan_thread+0x0/0xcc) from [<c0088000>] (kthread+0x90/0x98)
 r5:c7c33f04 r4:c760bfa8
[<c0087f70>] (kthread+0x0/0x98) from [<c006f5e8>] (do_exit+0x0/0x684)
 r7:00000013 r6:c006f5e8 r5:c0087f70 r4:c7c33f04
Code: e3002312 e58d6000 e2833e16 eb0422d5 (e5963020)
---[ end trace f3d5e96f75c297b7 ]---

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Reviewed-by:   Sergei Shtylyov <sshtylyov@mvista.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <me@felipebalbi.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: stable <stable@kernel.org>
---
 drivers/usb/musb/musb_gadget.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 6fca870..de0ca90 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -568,11 +568,19 @@ static void rxstate(struct musb *musb, struct musb_request *req)
 {
 	const u8		epnum = req->epnum;
 	struct usb_request	*request = &req->request;
-	struct musb_ep		*musb_ep = &musb->endpoints[epnum].ep_out;
+	struct musb_ep		*musb_ep;
 	void __iomem		*epio = musb->endpoints[epnum].regs;
 	unsigned		fifo_count = 0;
-	u16			len = musb_ep->packet_sz;
+	u16			len;
 	u16			csr = musb_readw(epio, MUSB_RXCSR);
+	struct musb_hw_ep	*hw_ep = &musb->endpoints[epnum];
+
+	if (hw_ep->is_shared_fifo)
+		musb_ep = &hw_ep->ep_in;
+	else
+		musb_ep = &hw_ep->ep_out;
+
+	len = musb_ep->packet_sz;
 
 	/* We shouldn't get here while DMA is active, but we do... */
 	if (dma_channel_status(musb_ep->dma) == MUSB_DMA_STATUS_BUSY) {
@@ -740,9 +748,15 @@ void musb_g_rx(struct musb *musb, u8 epnum)
 	u16			csr;
 	struct usb_request	*request;
 	void __iomem		*mbase = musb->mregs;
-	struct musb_ep		*musb_ep = &musb->endpoints[epnum].ep_out;
+	struct musb_ep		*musb_ep;
 	void __iomem		*epio = musb->endpoints[epnum].regs;
 	struct dma_channel	*dma;
+	struct musb_hw_ep	*hw_ep = &musb->endpoints[epnum];
+
+	if (hw_ep->is_shared_fifo)
+		musb_ep = &hw_ep->ep_in;
+	else
+		musb_ep = &hw_ep->ep_out;
 
 	musb_ep_select(mbase, epnum);
 
-- 
1.6.2.5

[RESEND/PATCH 2/6] USB: musb-gadget: fix bulk IN infinite hangs in double buffer case

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

This patch fixes one infinite hang of bulk IN transfer in double buffer
case, the hang can be observed easily by test #6 of usbtest if musb is
configured as g_zero and fifo mode 3 is taken to enable double fifo.

In fact, the patch only removes the check for non-empty fifo before
loading data from new request into fifo since the check is not correct:

	-in double buffer case, fifo may accommodate more than one packet,
	even though it has contained one packet already and is non-empty

	-since last DMA is completed before calling musb_g_tx, it is sure
	that fifo may accommodate at least one packet

Without applying the patch, new requst enqueued from .complte may not
have a chance to be loaded into fifo, then will never be completed and
cause infinite hangs.

With the patch, on my beagle B5, test#6(queued bulk in) can be passed and
test result may go beyond 33Mbyte/s if musb is configured as g_zero and
fifo mode 3 is taken, follows the test command:

	#testusb -D DEV_NAME -c 1024 -t 6 -s 32768 -g 8   [1]

[1],
    -source of testusb : tools/usb/testusb.c under linux kernel;

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Acked-by:   Anand Gadiyar <gadiyar@ti.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <me@felipebalbi.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/usb/musb/musb_gadget.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index de0ca90..f206c94 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -504,18 +504,6 @@ void musb_g_tx(struct musb *musb, u8 epnum)
 			/* ... or if not, then complete it. */
 			musb_g_giveback(musb_ep, request, 0);
 
-			/*
-			 * Kickstart next transfer if appropriate;
-			 * the packet that just completed might not
-			 * be transmitted for hours or days.
-			 * REVISIT for double buffering...
-			 * FIXME revisit for stalls too...
-			 */
-			musb_ep_select(mbase, epnum);
-			csr = musb_readw(epio, MUSB_TXCSR);
-			if (csr & MUSB_TXCSR_FIFONOTEMPTY)
-				return;
-
 			request = musb_ep->desc ? next_request(musb_ep) : NULL;
 			if (!request) {
 				DBG(4, "%s idle now\n",
-- 
1.6.2.5

[RESEND/PATCH 3/6] USB: musb-gadget: enable autoclear for OUT transfer in both DMA 0 and DMA 1

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

This patch fixes one bugs of OUT transfer in double buffer case:

	-the current code only enable autoclear for dma mode 1, and not
	for dma mode 0

Without this patch, test #5 of usbtest can't be passed if we
configure musb as g_zero and use fifo mode 3 to enable double
buffer mode.

With this patch and the following patch(fix dma length),
on my beagle B5, test#5(queued bulk out) may go beyond
18Mbyte/s(seems dma mode 0 is quicker in double buffer case)
if musb is configured as g_zero and fifo mode 3 is taken, follows
the test command:

    #./testusb -D DEV_NAME -c 1024 -t 5 -s 32768 -g 8   [1]

Also I have tested this patch can't make g_ether broken.

[1],source of testusb : tools/usb/testusb.c under linux kernel;

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <me@felipebalbi.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/usb/musb/musb_gadget.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index f206c94..176e127 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -643,8 +643,8 @@ static void rxstate(struct musb *musb, struct musb_request *req)
 	 */
 
 				csr |= MUSB_RXCSR_DMAENAB;
-#ifdef USE_MODE1
 				csr |= MUSB_RXCSR_AUTOCLEAR;
+#ifdef USE_MODE1
 				/* csr |= MUSB_RXCSR_DMAMODE; */
 
 				/* this special sequence (enabling and then
-- 
1.6.2.5

[RESEND/PATCH 4/6] USB: musb-gadget: fix DMA length for OUT transfer

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

DMA length should not go beyond the availabe space of request buffer,
so fix it.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Acked-by:   Anand Gadiyar <gadiyar@ti.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <me@felipebalbi.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/usb/musb/musb_gadget.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 176e127..f3ee04f 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -659,10 +659,11 @@ static void rxstate(struct musb *musb, struct musb_request *req)
 				if (request->actual < request->length) {
 					int transfer_size = 0;
 #ifdef USE_MODE1
-					transfer_size = min(request->length,
+					transfer_size = min(request->length - request->actual,
 							channel->max_len);
 #else
-					transfer_size = len;
+					transfer_size = min(request->length - request->actual,
+							(unsigned)len);
 #endif
 					if (transfer_size <= musb_ep->packet_sz)
 						musb_ep->dma->desired_mode = 0;
-- 
1.6.2.5

[RESEND/PATCH 4/6] USB: musb-gadget: fix DMA length for OUT transfer

[tom.leiming-Re5JQEeQqe8AvxtiuMwx3w]

From: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

DMA length should not go beyond the availabe space of request buffer,
so fix it.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by:   Anand Gadiyar <gadiyar-l0cyMroinI0@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org>
Cc: Anand Gadiyar <gadiyar-l0cyMroinI0@public.gmane.org>
Cc: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Cc: Sergei Shtylyov <sshtylyov-hkdhdckH98+B+jHODAdFcQ@public.gmane.org>
---
 drivers/usb/musb/musb_gadget.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 176e127..f3ee04f 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -659,10 +659,11 @@ static void rxstate(struct musb *musb, struct musb_request *req)
 				if (request->actual < request->length) {
 					int transfer_size = 0;
 #ifdef USE_MODE1
-					transfer_size = min(request->length,
+					transfer_size = min(request->length - request->actual,
 							channel->max_len);
 #else
-					transfer_size = len;
+					transfer_size = min(request->length - request->actual,
+							(unsigned)len);
 #endif
 					if (transfer_size <= musb_ep->packet_sz)
 						musb_ep->dma->desired_mode = 0;
-- 
1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

Complete the current request only if the data transfer is over.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <me@felipebalbi.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/usb/musb/musb_gadget.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index f3ee04f..fa826f9 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -501,14 +501,14 @@ void musb_g_tx(struct musb *musb, u8 epnum)
 				request->zero = 0;
 			}
 
-			/* ... or if not, then complete it. */
-			musb_g_giveback(musb_ep, request, 0);
-
-			request = musb_ep->desc ? next_request(musb_ep) : NULL;
-			if (!request) {
-				DBG(4, "%s idle now\n",
-					musb_ep->end_point.name);
-				return;
+			if (request->actual == request->length) {
+				musb_g_giveback(musb_ep, request, 0);
+				request = musb_ep->desc ? next_request(musb_ep) : NULL;
+				if (!request) {
+					DBG(4, "%s idle now\n",
+						musb_ep->end_point.name);
+					return;
+				}
 			}
 		}
 
-- 
1.6.2.5

[RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[tom.leiming-Re5JQEeQqe8AvxtiuMwx3w]

From: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Complete the current request only if the data transfer is over.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org>
Cc: Anand Gadiyar <gadiyar-l0cyMroinI0@public.gmane.org>
Cc: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Cc: Sergei Shtylyov <sshtylyov-hkdhdckH98+B+jHODAdFcQ@public.gmane.org>
---
 drivers/usb/musb/musb_gadget.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index f3ee04f..fa826f9 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -501,14 +501,14 @@ void musb_g_tx(struct musb *musb, u8 epnum)
 				request->zero = 0;
 			}
 
-			/* ... or if not, then complete it. */
-			musb_g_giveback(musb_ep, request, 0);

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>

> Complete the current request only if the data transfer is over.

> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Cc: Felipe Balbi <me@felipebalbi.com>
> Cc: Anand Gadiyar <gadiyar@ti.com>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> ---
>  drivers/usb/musb/musb_gadget.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)

> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index f3ee04f..fa826f9 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -501,14 +501,14 @@ void musb_g_tx(struct musb *musb, u8 epnum)
>  				request->zero = 0;
>  			}
>  
> -			/* ... or if not, then complete it. */
> -			musb_g_giveback(musb_ep, request, 0);
> -
> -			request = musb_ep->desc ? next_request(musb_ep) : NULL;
> -			if (!request) {
> -				DBG(4, "%s idle now\n",
> -					musb_ep->end_point.name);
> -				return;
> +			if (request->actual == request->length) {

    But why not modify the conditional above all that code, just excluding 
'is_dma' from it. This conditional already includes (request->actual == 
request->length) check. Please recast this patch.

> +				musb_g_giveback(musb_ep, request, 0);
> +				request = musb_ep->desc ? next_request(musb_ep) : NULL;
> +				if (!request) {
> +					DBG(4, "%s idle now\n",
> +						musb_ep->end_point.name);
> +					return;
> +				}
>  			}
>  		}
>  

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Ming Lei]

2010/9/13 Sergei Shtylyov <sshtylyov@mvista.com>:
>   But why not modify the conditional above all that code, just excluding
> 'is_dma' from it. This conditional already includes (request->actual ==
> request->length) check. Please recast this patch.

The two condition is OR relation, not and, so we can't exclude 'is_dma' simply.
Anyway, the change is not wrong, right?


thanks,
-- 
Lei Ming

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Ming Lei]

2010/9/13 Sergei Shtylyov <sshtylyov@mvista.com>:
>   But why not modify the conditional above all that code, just excluding
> 'is_dma' from it. This conditional already includes (request->actual ==
> request->length) check. Please recast this patch.

The two condition is OR relation, not and, so we can't exclude 'is_dma' simply.
Anyway, the change is not wrong, right?


thanks,
-- 
Lei Ming
--
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

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

Ming Lei wrote:

>>   But why not modify the conditional above all that code, just excluding
>> 'is_dma' from it. This conditional already includes (request->actual ==
>> request->length) check. Please recast this patch.

> The two condition is OR relation, not and, so we can't exclude 'is_dma' simply.

    Yes, we can. You're clearly handling only the DMA case with your added 
check, the PIO case was already handled.

> Anyway, the change is not wrong, right?

    Not wrong, but the check is duplicate.

> thanks,

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

I wrote:

>>>   But why not modify the conditional above all that code, just excluding
>>> 'is_dma' from it. This conditional already includes (request->actual ==
>>> request->length) check. Please recast this patch.

>> The two condition is OR relation, not and, so we can't exclude 
>> 'is_dma' simply.

>    Yes, we can. You're clearly handling only the DMA case with your 
> added check, the PIO case was already handled.

>> Anyway, the change is not wrong, right?

>    Not wrong, but the check is duplicate.

    Oops, I've been too fast and haven't realized that the check done here _is_ 
actually wrong. It causes ZLP send to trigger too fast in the DMA case. So 
please fix this patch. Felipe, please drop it for now.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Mon, Sep 13, 2010 at 11:26:52AM -0500, Sergei Shtylyov wrote:
> Oops, I've been too fast and haven't realized that the check done here
> _is_ actually wrong. It causes ZLP send to trigger too fast in the DMA
> case. So please fix this patch. Felipe, please drop it for now.

patch is not even touching that part of the code, not even
reading/writing TXCSR_TXPKTRDY bit care to explain please.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

On 14-09-2010 10:56, Felipe Balbi wrote:

>> Oops, I've been too fast and haven't realized that the check done here
>> _is_ actually wrong. It causes ZLP send to trigger too fast in the DMA
>> case. So please fix this patch. Felipe, please drop it for now.

> patch is not even touching that part of the code,

    Yeah, and that's the problem.

> not even
> reading/writing TXCSR_TXPKTRDY bit care to explain please.

    If a DMA interrupt comes when the whole transfer is not yet complete (and 
other Ming Lei's patches are making this possible), it will pass due to the 
'ís_dma' condition above the patched code:

                 if (is_dma || request->actual == request->length) {

and then it will hit the code sending the final ZLP (above this patched code too):

                         /*
                          * First, maybe a terminating short packet. Some DMA
                          * engines might handle this by themselves.
                          */
                         if ((request->zero && request->length
                                 && request->length % musb_ep->packet_sz == 0)
#ifdef CONFIG_USB_INVENTRA_DMA
                                 || (is_dma && (!dma->desired_mode ||
                                         (request->actual &
                                                 (musb_ep->packet_sz - 1))))
#endif
                         ) {

before the transfer is complete while it should only be hit when and only when 
the whole transfer is complete. The current code doesn't look correct as well 
though, all due to this 'ís_dma' condition. Surely this needs fixing.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

On 14-09-2010 10:56, Felipe Balbi wrote:

>> Oops, I've been too fast and haven't realized that the check done here
>> _is_ actually wrong. It causes ZLP send to trigger too fast in the DMA
>> case. So please fix this patch. Felipe, please drop it for now.

> patch is not even touching that part of the code,

    Yeah, and that's the problem.

> not even
> reading/writing TXCSR_TXPKTRDY bit care to explain please.

    If a DMA interrupt comes when the whole transfer is not yet complete (and 
other Ming Lei's patches are making this possible), it will pass due to the 
'ís_dma' condition above the patched code:

                 if (is_dma || request->actual == request->length) {

and then it will hit the code sending the final ZLP (above this patched code too):

                         /*
                          * First, maybe a terminating short packet. Some DMA
                          * engines might handle this by themselves.
                          */
                         if ((request->zero && request->length
                                 && request->length % musb_ep->packet_sz == 0)
#ifdef CONFIG_USB_INVENTRA_DMA
                                 || (is_dma && (!dma->desired_mode ||
                                         (request->actual &
                                                 (musb_ep->packet_sz - 1))))
#endif
                         ) {

before the transfer is complete while it should only be hit when and only when 
the whole transfer is complete. The current code doesn't look correct as well 
though, all due to this 'ís_dma' condition. Surely this needs fixing.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Tue, Sep 14, 2010 at 05:46:22AM -0500, Sergei Shtylyov wrote:
>    If a DMA interrupt comes when the whole transfer is not yet complete (and
>other Ming Lei's patches are making this possible), it will pass due to the

than this is the actual problem, no ? If we're using mode1 dma (as we
are on tx path), we should only get dma interrupt when the whole
transfer has been completed.

>'ís_dma' condition above the patched code:
>
>                 if (is_dma || request->actual == request->length) {
>
>and then it will hit the code sending the final ZLP (above this patched code too):

but this was already there before the patch.

>                         /*
>                          * First, maybe a terminating short packet. Some DMA
>                          * engines might handle this by themselves.
>                          */
>                         if ((request->zero && request->length
>                                 && request->length % musb_ep->packet_sz == 0)
>#ifdef CONFIG_USB_INVENTRA_DMA
>                                 || (is_dma && (!dma->desired_mode ||
>                                         (request->actual &
>                                                 (musb_ep->packet_sz - 1))))
>#endif
>                         ) {
>
>before the transfer is complete while it should only be hit when and only when
>the whole transfer is complete. The current code doesn't look correct as well
>though, all due to this 'ís_dma' condition. Surely this needs fixing.

likewise, this was there before the patch. I don't think the real
problem lies with this patch, it's been there for a while, don't you
agree ?

the problem is not on the extra if () added below the quoted code,
it's on the quoted code itself, which wasn't changed in any way.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Tue, Sep 14, 2010 at 05:46:22AM -0500, Sergei Shtylyov wrote:
>    If a DMA interrupt comes when the whole transfer is not yet complete (and
>other Ming Lei's patches are making this possible), it will pass due to the

than this is the actual problem, no ? If we're using mode1 dma (as we
are on tx path), we should only get dma interrupt when the whole
transfer has been completed.

>'ís_dma' condition above the patched code:
>
>                 if (is_dma || request->actual == request->length) {
>
>and then it will hit the code sending the final ZLP (above this patched code too):

but this was already there before the patch.

>                         /*
>                          * First, maybe a terminating short packet. Some DMA
>                          * engines might handle this by themselves.
>                          */
>                         if ((request->zero && request->length
>                                 && request->length % musb_ep->packet_sz == 0)
>#ifdef CONFIG_USB_INVENTRA_DMA
>                                 || (is_dma && (!dma->desired_mode ||
>                                         (request->actual &
>                                                 (musb_ep->packet_sz - 1))))
>#endif
>                         ) {
>
>before the transfer is complete while it should only be hit when and only when
>the whole transfer is complete. The current code doesn't look correct as well
>though, all due to this 'ís_dma' condition. Surely this needs fixing.

likewise, this was there before the patch. I don't think the real
problem lies with this patch, it's been there for a while, don't you
agree ?

the problem is not on the extra if () added below the quoted code,
it's on the quoted code itself, which wasn't changed in any way.

-- 
balbi
--
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

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

Felipe Balbi wrote:

>>    If a DMA interrupt comes when the whole transfer is not yet 
>> complete (and
>> other Ming Lei's patches are making this possible),

    Oh, here I mixed some other patch with Ming Lei's ones...

>> it will pass due to the

> than this is the actual problem, no ? If we're using mode1 dma (as we
> are on tx path), we should only get dma interrupt when the whole
> transfer has been completed.

    The Inventra DMA controller has serious DMA length limitation, so the whole 
transfer may take more than one DMA.

>> 'ís_dma' condition above the patched code:

>>                 if (is_dma || request->actual == request->length) {

>> and then it will hit the code sending the final ZLP (above this 
>> patched code too):

> but this was already there before the patch.

    Yes, and here lies the problem.

>>                         /*
>>                          * First, maybe a terminating short packet. 
>> Some DMA
>>                          * engines might handle this by themselves.
>>                          */
>>                         if ((request->zero && request->length
>>                                 && request->length % 
>> musb_ep->packet_sz == 0)
>> #ifdef CONFIG_USB_INVENTRA_DMA
>>                                 || (is_dma && (!dma->desired_mode ||
>>                                         (request->actual &
>>                                                 (musb_ep->packet_sz - 
>> 1))))
>> #endif
>>                         ) {

>> before the transfer is complete while it should only be hit when and 
>> only when
>> the whole transfer is complete. The current code doesn't look correct 
>> as well
>> though, all due to this 'ís_dma' condition. Surely this needs fixing.

> likewise, this was there before the patch. I don't think the real
> problem lies with this patch, it's been there for a while, don't you
> agree ?

    Then what problem this patch fixes, if not this one?

> the problem is not on the extra if () added below the quoted code,
> it's on the quoted code itself, which wasn't changed in any way.

    Let me repeat: in the PIO mode the added check is just duplicate, in the DMA 
mode it's added too late in the code, after ZLP/short packet send is triggered.
We should modify the check at the top of that code instead.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

Felipe Balbi wrote:

>>    If a DMA interrupt comes when the whole transfer is not yet 
>> complete (and
>> other Ming Lei's patches are making this possible),

    Oh, here I mixed some other patch with Ming Lei's ones...

>> it will pass due to the

> than this is the actual problem, no ? If we're using mode1 dma (as we
> are on tx path), we should only get dma interrupt when the whole
> transfer has been completed.

    The Inventra DMA controller has serious DMA length limitation, so the whole 
transfer may take more than one DMA.

>> 'ís_dma' condition above the patched code:

>>                 if (is_dma || request->actual == request->length) {

>> and then it will hit the code sending the final ZLP (above this 
>> patched code too):

> but this was already there before the patch.

    Yes, and here lies the problem.

>>                         /*
>>                          * First, maybe a terminating short packet. 
>> Some DMA
>>                          * engines might handle this by themselves.
>>                          */
>>                         if ((request->zero && request->length
>>                                 && request->length % 
>> musb_ep->packet_sz == 0)
>> #ifdef CONFIG_USB_INVENTRA_DMA
>>                                 || (is_dma && (!dma->desired_mode ||
>>                                         (request->actual &
>>                                                 (musb_ep->packet_sz - 
>> 1))))
>> #endif
>>                         ) {

>> before the transfer is complete while it should only be hit when and 
>> only when
>> the whole transfer is complete. The current code doesn't look correct 
>> as well
>> though, all due to this 'ís_dma' condition. Surely this needs fixing.

> likewise, this was there before the patch. I don't think the real
> problem lies with this patch, it's been there for a while, don't you
> agree ?

    Then what problem this patch fixes, if not this one?

> the problem is not on the extra if () added below the quoted code,
> it's on the quoted code itself, which wasn't changed in any way.

    Let me repeat: in the PIO mode the added check is just duplicate, in the DMA 
mode it's added too late in the code, after ZLP/short packet send is triggered.
We should modify the check at the top of that code instead.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

On Tue, Sep 14, 2010 at 12:51:09PM -0500, Sergei Shtylyov wrote:
>Hello.
>
>Felipe Balbi wrote:
>
>>>    If a DMA interrupt comes when the whole transfer is not yet
>>> complete (and
>>> other Ming Lei's patches are making this possible),
>
>    Oh, here I mixed some other patch with Ming Lei's ones...
>
>>> it will pass due to the
>
>> than this is the actual problem, no ? If we're using mode1 dma (as we
>> are on tx path), we should only get dma interrupt when the whole
>> transfer has been completed.
>
>    The Inventra DMA controller has serious DMA length limitation, so the whole
>transfer may take more than one DMA.

When I said 'whole transfer' I meant the transfer size you programmed
dma to transfer, see that we have (not actual code):

if (transfer_size > dma->max_len)
	transfer_size = dma->max_len;

dma->channel_program(...,..., transfer_size,...);

with mode1, we will only get irq when dma has transferred transfer_size
bytes.

>> likewise, this was there before the patch. I don't think the real
>> problem lies with this patch, it's been there for a while, don't you
>> agree ?
>
>    Then what problem this patch fixes, if not this one?

if request->length == 1MB and dma->max_len = 128KB, when is_dma is true,
request->actual will be different from request->length for 7
'iterations', then only on the 8th it will be the same. I believe that's
what Ming is trying to fix.

Ming, am I correct ?

>    Let me repeat: in the PIO mode the added check is just duplicate, in the DMA

it is duplicate for PIO, but not for DMA.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Ming Lei]

2010/9/15 Felipe Balbi <balbi@ti.com>:
> if request->length == 1MB and dma->max_len = 128KB, when is_dma is true,
> request->actual will be different from request->length for 7
> 'iterations', then only on the 8th it will be the same. I believe that's
> what Ming is trying to fix.
>
> Ming, am I correct ?

Yes, it is the initial motivation of the patch.

-- 
Lei Ming

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

On 15-09-2010 10:53, Felipe Balbi wrote:

>>>> If a DMA interrupt comes when the whole transfer is not yet
>>>> complete (and
>>>> other Ming Lei's patches are making this possible),

>> Oh, here I mixed some other patch with Ming Lei's ones...

>>>> it will pass due to the

>>> than this is the actual problem, no ? If we're using mode1 dma (as we
>>> are on tx path), we should only get dma interrupt when the whole
>>> transfer has been completed.

>> The Inventra DMA controller has serious DMA length limitation, so the whole
>> transfer may take more than one DMA.

> When I said 'whole transfer' I meant the transfer size you programmed
> dma to transfer, see that we have (not actual code):

> if (transfer_size > dma->max_len)
> transfer_size = dma->max_len;

> dma->channel_program(...,..., transfer_size,...);

    Ah, I didn't mean the same thing -- I meant the transfer size as specified 
by 'struct usb_request'.

> with mode1, we will only get irq when dma has transferred transfer_size
> bytes.

    Sure.

>>> likewise, this was there before the patch. I don't think the real
>>> problem lies with this patch, it's been there for a while, don't you
>>> agree ?

>> Then what problem this patch fixes, if not this one?

> if request->length == 1MB and dma->max_len = 128KB, when is_dma is true,
> request->actual will be different from request->length for 7
> 'iterations', then only on the 8th it will be the same. I believe that's
> what Ming is trying to fix.

    I repeat once again: it's too late to check this where Ming is inserting 
the check.

> Ming, am I correct ?
>
>> Let me repeat: in the PIO mode the added check is just duplicate, in the DMA

> it is duplicate for PIO, but not for DMA.

    I didn't say it was duplicate for DMA, just too late.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 05:01:05AM -0500, Sergei Shtylyov wrote:
>    I didn't say it was duplicate for DMA, just too late.

how come ? we need to send ZLP before giving back the request.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

On 15-09-2010 14:05, Felipe Balbi wrote:

>> I didn't say it was duplicate for DMA, just too late.

> how come ? we need to send ZLP before giving back the request.

    Well, look at the code ionce again. We need to send ZLP *after* 
request->actual == request->length, but as the check is inserted after the ZLP 
send, ZLP *may* be sent once the first DMA completes, not the last.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Ming Lei]

2010/9/15 Sergei Shtylyov <sshtylyov@mvista.com>:
> Hello.
>
> On 15-09-2010 14:05, Felipe Balbi wrote:
>
>>> I didn't say it was duplicate for DMA, just too late.
>
>> how come ? we need to send ZLP before giving back the request.
>
>   Well, look at the code ionce again. We need to send ZLP *after*
> request->actual == request->length, but as the check is inserted after the
> ZLP send, ZLP *may* be sent once the first DMA completes, not the last.

Yes, it is really a problem, as said by balbi.  And the problem should be
in the check for zlp or the 'is_dma' condition.

But this patch is not addressed for the zlp problem, and is is only for
completing the request only if the data transfer in usb_request
is over, as explained before, right?

-- 
Lei Ming

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

On 15-09-2010 14:14, Ming Lei wrote:

>>>> I didn't say it was duplicate for DMA, just too late.

>>> how come ? we need to send ZLP before giving back the request.

>>    Well, look at the code ionce again. We need to send ZLP *after*
>> request->actual == request->length, but as the check is inserted after the
>> ZLP send, ZLP *may* be sent once the first DMA completes, not the last.

> Yes, it is really a problem, as said by balbi.  And the problem should be
> in the check for zlp or the 'is_dma' condition.

> But this patch is not addressed for the zlp problem, and is is only for
> completing the request only if the data transfer in usb_request
> is over, as explained before, right?

    I don't see why we should fix only this problem, while it's obvious tha 
the fix is incomplete and leaves the other problem exposed. Please recast the 
patch.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 05:18:47AM -0500, Sergei Shtylyov wrote:
>    I don't see why we should fix only this problem, while it's obvious tha
>the fix is incomplete and leaves the other problem exposed. Please recast the
>patch.

IMO, the ZLP fix is *another* fix and as such subject to a different
patch.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 05:18:47AM -0500, Sergei Shtylyov wrote:
>    I don't see why we should fix only this problem, while it's obvious tha
>the fix is incomplete and leaves the other problem exposed. Please recast the
>patch.

IMO, the ZLP fix is *another* fix and as such subject to a different
patch.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

On 15.09.2010 14:22, Felipe Balbi wrote:

>> I don't see why we should fix only this problem, while it's obvious tha
>> the fix is incomplete and leaves the other problem exposed. Please recast the
>> patch.

> IMO, the ZLP fix is *another* fix and as such subject to a different
> patch.

    IMHO, this fix as it is now is quite stupid. It's clear that the check is 
misplaced and will be removed once the ZLP fix is done. So why not do it once 
and for all? Is it so hard to do? FWIW, I NAK this patch as it is now.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

On 15.09.2010 14:22, Felipe Balbi wrote:

>> I don't see why we should fix only this problem, while it's obvious tha
>> the fix is incomplete and leaves the other problem exposed. Please recast the
>> patch.

> IMO, the ZLP fix is *another* fix and as such subject to a different
> patch.

    IMHO, this fix as it is now is quite stupid. It's clear that the check is 
misplaced and will be removed once the ZLP fix is done. So why not do it once 
and for all? Is it so hard to do? FWIW, I NAK this patch as it is now.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 05:27:37AM -0500, Sergei Shtylyov wrote:
>    IMHO, this fix as it is now is quite stupid. It's clear that the check is
>misplaced and will be removed once the ZLP fix is done. So why not do it once

your forgetting the fact that not always you need to send ZLP after
completing a packet_size-aligned transfer, so that check will have to
stay where it is.

>and for all? Is it so hard to do? FWIW, I NAK this patch as it is now.

ok, but I don't.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Hello.

On 15-09-2010 14:31, Felipe Balbi wrote:

>> IMHO, this fix as it is now is quite stupid. It's clear that the check is
>> misplaced and will be removed once the ZLP fix is done. So why not do it once

> your forgetting the fact that not always you need to send ZLP after
> completing a packet_size-aligned transfer,

    So what?

> so that check will have to stay where it is.

    I don't see how these two are connected at all.

 >> and for all? Is it so hard to do? FWIW, I NAK this patch as it is now.

 > ok, but I don't.

    Well, you're the maintainer.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 05:41:39AM -0500, Sergei Shtylyov wrote:
>> your forgetting the fact that not always you need to send ZLP after
>> completing a packet_size-aligned transfer,
>
>    So what?

what you're saying is that when we change the ZLP handling, the
request->actual == request->lenght check will have to be removed, but
that's not true because ZLP is only needed if request->zero is set.

So the final code would be something like (pseudo-code):

if (request->lenght % musb_ep->packet_sz)
	set_last_packet_is_short_flag(musb_request);

if (request->zero || last_packet_is_short(request)) {
	set_txpktrdy(musb);
	set_musb_request_completed_flag(musb_request);
	return;
}

if (request->acual == request->length)
	musb_g_giveback(musb, request);

restart_ep_if_more_requests(musb_ep);


> > ok, but I don't.
>
>    Well, you're the maintainer.

the thing is that this discussion is preventing me from sending me the
other 10 patches to Greg. So either we drop it now and send the exact
same patch (or a similar version) for next -rc or I send them all now.
Since I'm not seeing the problem you're describing I'm more inclined to
send them all.

-- 
balbi

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Sergei Shtylyov]

Felipe Balbi wrote:

>>> your forgetting the fact that not always you need to send ZLP after
>>> completing a packet_size-aligned transfer,

>>    So what?

> what you're saying is that when we change the ZLP handling, the
> request->actual == request->lenght check will have to be removed, but
> that's not true because ZLP is only needed if request->zero is set.

> So the final code would be something like (pseudo-code):

> if (request->lenght % musb_ep->packet_sz)
>     set_last_packet_is_short_flag(musb_request);

> if (request->zero || last_packet_is_short(request)) {
>     set_txpktrdy(musb);
>     set_musb_request_completed_flag(musb_request);
>     return;
> }

> if (request->acual == request->length)
>     musb_g_giveback(musb, request);

> restart_ep_if_more_requests(musb_ep);

    No, this code will still send ZLP before the whole requested transfer is 
done. The (request->actual == request->length) check is needed before we even 
check that request->zero is set (and it is there but not for the DMA case). I 
thought that this was quite obvious, and I was surprised that this caused such a 
lengthy discussion.

WBR, Sergei

Re: [RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over

[Ming Lei]

2010/9/15 Sergei Shtylyov <sshtylyov@mvista.com>:
> On 15.09.2010 14:22, Felipe Balbi wrote:
>
>>> I don't see why we should fix only this problem, while it's obvious tha
>>> the fix is incomplete and leaves the other problem exposed. Please recast
>>> the
>>> patch.
>
>> IMO, the ZLP fix is *another* fix and as such subject to a different
>> patch.
>
>   IMHO, this fix as it is now is quite stupid. It's clear that the check is
> misplaced and will be removed once the ZLP fix is done. So why not do it
> once and for all? Is it so hard to do? FWIW, I NAK this patch as it is now.

Maybe we should open a new thread to discuss the ZLP fix, I'll do it later.

-- 
Lei Ming

[RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

DMA length should not go beyond the availabe space of request buffer,
so fix it.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Felipe Balbi <me@felipebalbi.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/usb/musb/musb_gadget.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index fa826f9..cacae96 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -300,6 +300,11 @@ static void txstate(struct musb *musb, struct musb_request *req)
 #ifndef	CONFIG_MUSB_PIO_ONLY
 	if (is_dma_capable() && musb_ep->dma) {
 		struct dma_controller	*c = musb->dma_controller;
+		size_t request_size;
+
+		/* setup DMA, then program endpoint CSR */
+		request_size = min_t(size_t, request->length - request->actual,
+					musb_ep->dma->max_len);
 
 		use_dma = (request->dma != DMA_ADDR_INVALID);
 
@@ -307,11 +312,6 @@ static void txstate(struct musb *musb, struct musb_request *req)
 
 #ifdef CONFIG_USB_INVENTRA_DMA
 		{
-			size_t request_size;
-
-			/* setup DMA, then program endpoint CSR */
-			request_size = min_t(size_t, request->length,
-						musb_ep->dma->max_len);
 			if (request_size < musb_ep->packet_sz)
 				musb_ep->dma->desired_mode = 0;
 			else
@@ -373,8 +373,8 @@ static void txstate(struct musb *musb, struct musb_request *req)
 		use_dma = use_dma && c->channel_program(
 				musb_ep->dma, musb_ep->packet_sz,
 				0,
-				request->dma,
-				request->length);
+				request->dma + request->actual,
+				request_size);
 		if (!use_dma) {
 			c->channel_release(musb_ep->dma);
 			musb_ep->dma = NULL;
@@ -386,8 +386,8 @@ static void txstate(struct musb *musb, struct musb_request *req)
 		use_dma = use_dma && c->channel_program(
 				musb_ep->dma, musb_ep->packet_sz,
 				request->zero,
-				request->dma,
-				request->length);
+				request->dma + request->actual,
+				request_size);
 #endif
 	}
 #endif
-- 
1.6.2.5

[RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

[tom.leiming-Re5JQEeQqe8AvxtiuMwx3w]

From: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

DMA length should not go beyond the availabe space of request buffer,
so fix it.

Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org>
Cc: Anand Gadiyar <gadiyar-l0cyMroinI0@public.gmane.org>
Cc: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Cc: Sergei Shtylyov <sshtylyov-hkdhdckH98+B+jHODAdFcQ@public.gmane.org>
---
 drivers/usb/musb/musb_gadget.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index fa826f9..cacae96 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -300,6 +300,11 @@ static void txstate(struct musb *musb, struct musb_request *req)
 #ifndef	CONFIG_MUSB_PIO_ONLY
 	if (is_dma_capable() && musb_ep->dma) {
 		struct dma_controller	*c = musb->dma_controller;
+		size_t request_size;
+
+		/* setup DMA, then program endpoint CSR */
+		request_size = min_t(size_t, request->length - request->actual,
+					musb_ep->dma->max_len);
 
 		use_dma = (request->dma != DMA_ADDR_INVALID);
 
@@ -307,11 +312,6 @@ static void txstate(struct musb *musb, struct musb_request *req)
 
 #ifdef CONFIG_USB_INVENTRA_DMA
 		{
-			size_t request_size;
-
-			/* setup DMA, then program endpoint CSR */
-			request_size = min_t(size_t, request->length,
-						musb_ep->dma->max_len);
 			if (request_size < musb_ep->packet_sz)
 				musb_ep->dma->desired_mode = 0;
 			else
@@ -373,8 +373,8 @@ static void txstate(struct musb *musb, struct musb_request *req)
 		use_dma = use_dma && c->channel_program(
 				musb_ep->dma, musb_ep->packet_sz,
 				0,
-				request->dma,
-				request->length);
+				request->dma + request->actual,
+				request_size);
 		if (!use_dma) {
 			c->channel_release(musb_ep->dma);
 			musb_ep->dma = NULL;
@@ -386,8 +386,8 @@ static void txstate(struct musb *musb, struct musb_request *req)
 		use_dma = use_dma && c->channel_program(
 				musb_ep->dma, musb_ep->packet_sz,
 				request->zero,
-				request->dma,
-				request->length);
+				request->dma + request->actual,
+				request_size);
 #endif
 	}
 #endif
-- 
1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

[Sergei Shtylyov]

Hello.

tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>

> DMA length should not go beyond the availabe space of request buffer,
> so fix it.

> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Cc: Felipe Balbi <me@felipebalbi.com>
> Cc: Anand Gadiyar <gadiyar@ti.com>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>

[...]

> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index fa826f9..cacae96 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -300,6 +300,11 @@ static void txstate(struct musb *musb, struct musb_request *req)
>  #ifndef	CONFIG_MUSB_PIO_ONLY
>  	if (is_dma_capable() && musb_ep->dma) {
>  		struct dma_controller	*c = musb->dma_controller;
> +		size_t request_size;
> +
> +		/* setup DMA, then program endpoint CSR */
> +		request_size = min_t(size_t, request->length - request->actual,
> +					musb_ep->dma->max_len);

    Er, you're moving this from under #ifdef CONFIG_USB_INVENTRA_DMA to the 
common code, right? Do you know that not all DMA drivers initialize max_len? For 
example CPPI driver doesn't, so it's left at zero. You're going to break DMA for 
CPPI. Please extend your patch, adding cppi_dma.c to it.

> @@ -307,11 +312,6 @@ static void txstate(struct musb *musb, struct musb_request *req)
>  
>  #ifdef CONFIG_USB_INVENTRA_DMA
>  		{
> -			size_t request_size;
> -
> -			/* setup DMA, then program endpoint CSR */
> -			request_size = min_t(size_t, request->length,
> -						musb_ep->dma->max_len);
>  			if (request_size < musb_ep->packet_sz)
>  				musb_ep->dma->desired_mode = 0;
>  			else

WBR, Sergei

Re: [RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

[Felipe Balbi]

On Tue, Sep 14, 2010 at 12:43:25PM -0500, Sergei Shtylyov wrote:
>Hello.
>
>tom.leiming@gmail.com wrote:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>
>> DMA length should not go beyond the availabe space of request buffer,
>> so fix it.
>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> Cc: David Brownell <dbrownell@users.sourceforge.net>
>> Cc: Felipe Balbi <me@felipebalbi.com>
>> Cc: Anand Gadiyar <gadiyar@ti.com>
>> Cc: Mike Frysinger <vapier@gentoo.org>
>> Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>
>[...]
>
>> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
>> index fa826f9..cacae96 100644
>> --- a/drivers/usb/musb/musb_gadget.c
>> +++ b/drivers/usb/musb/musb_gadget.c
>> @@ -300,6 +300,11 @@ static void txstate(struct musb *musb, struct musb_request *req)
>>  #ifndef	CONFIG_MUSB_PIO_ONLY
>>  	if (is_dma_capable() && musb_ep->dma) {
>>  		struct dma_controller	*c = musb->dma_controller;
>> +		size_t request_size;
>> +
>> +		/* setup DMA, then program endpoint CSR */
>> +		request_size = min_t(size_t, request->length - request->actual,
>> +					musb_ep->dma->max_len);
>
>    Er, you're moving this from under #ifdef CONFIG_USB_INVENTRA_DMA to the
>common code, right? Do you know that not all DMA drivers initialize max_len? For
>example CPPI driver doesn't, so it's left at zero. You're going to break DMA for
>CPPI. Please extend your patch, adding cppi_dma.c to it.

yes, please set some value on cppi_dma.c max_len field.

-- 
balbi

Re: [RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[Greg KH]

On Tue, Sep 07, 2010 at 11:23:28PM +0800, tom.leiming@gmail.com wrote:
> This patch set fixes two kinds of musb-gadget bug:
> 
> 	-fix kernel panic if using out ep with FIFO_TXRX style
> 
> 	-make double buffer mode usable, fix infinate hangs and buffer
> 	corrupt bug when using double buffer mode to do data transfer;
> 	now we can use musb double buffer reliably, so bootst performance
> 	about 30% compared with single buffer mode.
> 	
> 		[RESEND/PATCH 1/6] USB: musb-gadget: fix kernel panic if using out ep with FIFO_TXRX style(v1)
> 		[RESEND/PATCH 2/6] USB: musb-gadget: fix bulk IN infinite hangs in double buffer case
> 		[RESEND/PATCH 3/6] USB: musb-gadget: enable autoclear for OUT transfer in both DMA 0 and DMA 1
> 		[RESEND/PATCH 4/6] USB: musb-gadget: fix DMA length for OUT transfer
> 		[RESEND/PATCH 5/6] USB: musb-gadget: complete request only if data is transfered over
> 		[RESEND/PATCH 6/6] USB: musb-gadget: fix dma length in txstate

I seem to not have 1/6 here, did it get stuck somewhere?

Anyway, I need Felipe's ack before I can accept these.

thanks,

greg k-h

Re: [RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[Ming Lei]

CC Felipe / David / Gadiyar / Mike /  Sergei

2010/9/8 Greg KH <greg@kroah.com>:
> I seem to not have 1/6 here, did it get stuck somewhere?

Here is 1/6:

         http://marc.info/?l=linux-usb&m=128387307419605&w=2

If you need, I can send the 1/6 to you directly.

>
> Anyway, I need Felipe's ack before I can accept these.

Felipe, expect your comments on the patch set...

BTW: The mailbox of felipe.balbi@nokia.com in MAINTAINERS has been
obsolete now, and his current box is me@felipebalbi.com, seems MAINTAINERS
should be updated for it.

-- 
Lei Ming

Re: [RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[Ming Lei]

CC Felipe / David / Gadiyar / Mike /  Sergei

2010/9/8 Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>:
> I seem to not have 1/6 here, did it get stuck somewhere?

Here is 1/6:

         http://marc.info/?l=linux-usb&m=128387307419605&w=2

If you need, I can send the 1/6 to you directly.

>
> Anyway, I need Felipe's ack before I can accept these.

Felipe, expect your comments on the patch set...

BTW: The mailbox of felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org in MAINTAINERS has been
obsolete now, and his current box is me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org, seems MAINTAINERS
should be updated for it.

-- 
Lei Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[Greg KH]

On Wed, Sep 08, 2010 at 12:32:19PM +0800, Ming Lei wrote:
> CC Felipe / David / Gadiyar / Mike /  Sergei
> 
> 2010/9/8 Greg KH <greg@kroah.com>:
> > I seem to not have 1/6 here, did it get stuck somewhere?
> 
> Here is 1/6:
> 
>          http://marc.info/?l=linux-usb&m=128387307419605&w=2
> 
> If you need, I can send the 1/6 to you directly.

No need, I'll wait for Felipe to send them to me :)

> >
> > Anyway, I need Felipe's ack before I can accept these.
> 
> Felipe, expect your comments on the patch set...
> 
> BTW: The mailbox of felipe.balbi@nokia.com in MAINTAINERS has been
> obsolete now, and his current box is me@felipebalbi.com, seems MAINTAINERS
> should be updated for it.

Someone should send me a patch...

thanks,

greg k-h

Re: [RESEND/PATCH 0/6] USB: musb-gadget: bug fix

[Greg KH]

On Wed, Sep 08, 2010 at 12:32:19PM +0800, Ming Lei wrote:
> CC Felipe / David / Gadiyar / Mike /  Sergei
> 
> 2010/9/8 Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>:
> > I seem to not have 1/6 here, did it get stuck somewhere?
> 
> Here is 1/6:
> 
>          http://marc.info/?l=linux-usb&m=128387307419605&w=2
> 
> If you need, I can send the 1/6 to you directly.

No need, I'll wait for Felipe to send them to me :)

> >
> > Anyway, I need Felipe's ack before I can accept these.
> 
> Felipe, expect your comments on the patch set...
> 
> BTW: The mailbox of felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org in MAINTAINERS has been
> obsolete now, and his current box is me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org, seems MAINTAINERS
> should be updated for it.

Someone should send me a patch...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html