USB: musb-gadget: fix bulk IN infinite hangs in double buffer case
Subsystems (2)
PATCH 1/2 USB: musb-gadget: fix bulk IN infinite hangs in double buffer case
From: Ming Lei <[email protected]>
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;
--- drivers/usb/musb/musb_gadget.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-)
drivers/usb/musb/musb_gadget.c +0 -12
@@ -506,18 +506,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",
Signature · 1 line
1.6.2.5
Quoted 26 lines · [email protected] wrote:
From: Ming Lei <[email protected]>
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_zeroand 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 packetWithout 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_zeroand fifo mode 3 is taken, follows the test command:#testusb -D DEV_NAME -c 1024 -t 6 -s 32768 -g 8 [1]
Quoted 8 lines
[1],
-source of testusb : tools/usb/testusb.c under linux kernel;Signed-off-by Ming Lei <[email protected]>Cc Anand Gadiyar <[email protected]>Cc Mike Frysinger <[email protected]>Cc Sergei Shtylyov <[email protected]>
For OMAPs,
I sent out this patch a while ago [1]. I've tested it on OMAP3 and OMAP4 silicon.
It does look like a logic bug to me, but I'd like the Blackfin and Davinci guys to take a look at the code as well.
- Anand
[1] <http://marc.info/?l=linux-usb&m=127055442703499&w=2>
Quoted 31 lines
--- drivers/usb/musb/musb_gadget.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-)drivers/usb/musb/musb_gadget.c +0
-12@@ -506,18 +506,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",Signature · 1 line
1.6.2.5
A message that is not in the archive (sent off-list or not yet received): 5A47E75E594F054BAF48C5E4FC4B92AB0328C1CB42-/tLxBxkBPtCIQmiDNMet8wC/[email protected]
2010/8/27 Gadiyar, Anand <[email protected]>:
Quoted 36 lines
Quoted 26 lines · [email protected] wrote:
From: Ming Lei <[email protected]>
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_zeroand 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 packetWithout 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_zeroand fifo mode 3 is taken, follows the test command:#testusb -D
DEV_NAME-c 1024 -t 6 -s 32768 -g 8 [1]Quoted 8 lines
[1],
-source of testusb :tools/usb/testusb.cunder linux kernel;Signed-off-by Ming Lei <[email protected]>Cc Anand Gadiyar <[email protected]>Cc Mike Frysinger <[email protected]>Cc Sergei Shtylyov <[email protected]>
Quoted 5 lines
For OMAPs,
Acked-by Anand Gadiyar <[email protected]>I sent out this patch a while ago [1]. I've tested it on OMAP3 and OMAP4 silicon.
Oh, I did not see your patch before, you have seen the issue for long time, why not merge into mainline?
Quoted 39 lines
It does look like a logic bug to me, but I'd like the Blackfin and Davinci guys to take a look at the code as well.
- Anand
[1] <http://marc.info/?l=linux-usb&m=127055442703499&w=2>
Quoted 31 lines
---
drivers/usb/musb/musb_gadget.c | 12 ------------1 files changed, 0 insertions(+), 12 deletions(-)drivers/usb/musb/musb_gadget.c +0
-13@@ -506,18 +506,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
Signature · 5 lines
Lei Ming -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · Ming Lei
2010/8/27 Gadiyar, Anand <[email protected]>:
Quoted 3 lines
Quoted 2 lines · [email protected] wrote:
From: Ming Lei <[email protected]>
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
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_zeroand fifo mode 3 is taken to enable double fifo.I sent out this patch a while ago [1]. I've tested it on OMAP3 and OMAP4 silicon.
Oh, I did not see your patch before, you have seen the issue for long time, why not merge into mainline?
I discovered this a week or so before sending it out doing some weekend spare-time debugging of the double-buffering problem. I posted it out to get some feedback, and then forgot about it. :(
MUSB in general has been hammered by many lost patches in the recent past - I know folks have been busy getting products out, and haven't followed up as well as they could have. Hopefully this will change in future.
- Anand
Quoted 50 lines · On Friday, August 27, 2010 07:37:20 [email protected] wrote:
From: Ming Lei <[email protected]>
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_zeroand 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 packetWithout 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_zeroand 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 <[email protected]>Cc Anand Gadiyar <[email protected]>Cc Mike Frysinger <[email protected]>Cc Sergei Shtylyov <[email protected]>--- drivers/usb/musb/musb_gadget.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-)(quoted diff) +0
-0
b/drivers/usb/musb/musb_gadget.cindex 550b397..e0bd1c1 100644drivers/usb/musb/musb_gadget.c +0
-5@@ -506,18 +506,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...
drivers/usb/musb/musb_gadget.c void musb_g_tx(struct musb *musb, u8 epnum) · line 511
Quoted 10 lines
drivers/usb/musb/musb_gadget.c +0
-7-* 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",
maybe this change will make a difference ?
From 13fa6c9cec387893e2b22db6c055028f96fea5b8 Mon Sep 17 00:00:00 2001
From: Cliff Cai <[email protected]>
Date: Fri, 3 Jul 2009 06:32:02 +0000
Subject: [PATCH] USB: musb: only complete the request when all data has been transfered [local/log]
bug[#5269],bug[#5268],bug[#5264]only complete the request when all data has been transfered
git-svn-id: svn://localhost/svn/linux-kernel/trunk@6915 526b6c2d-f592-4532- a319-5dd88ccb003d
--- drivers/usb/musb/musb_gadget.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/usb/musb/musb_gadget.c +2 -0
@@ -500,6 +500,8 @@ void musb_g_tx(struct musb *musb, u8 epnum) | MUSB_TXCSR_TXPKTRDY); request->zero =0 ; } +if (request->actual < request->length) +return ;/* ... or if not, then complete it. */ musb_g_giveback (musb_ep, request,0 );
Signature · 1 line
1.7.2.2
A message that is not in the archive (sent off-list or not yet received): 201008271627.08137.vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org
2010/8/28 Mike Frysinger <[email protected]>:
Quoted 55 lines · On Friday, August 27, 2010 07:37:20 [email protected]…
Quoted 54 lines
From: Ming Lei <[email protected]>
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_zeroand 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 packetWithout 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_zeroand 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.cunder linux kernel;Signed-off-by Ming Lei <[email protected]>Cc Anand Gadiyar <[email protected]>Cc Mike Frysinger <[email protected]>Cc Sergei Shtylyov <[email protected]>---
drivers/usb/musb/musb_gadget.c | 12 ------------1 files changed, 0 insertions(+), 12 deletions(-)(quoted diff) +0
-0
b/drivers/usb/musb/musb_gadget.cindex 550b397..e0bd1c1 100644drivers/usb/musb/musb_gadget.c +0
-9@@ -506,18 +506,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);
Quoted 8 lines
drivers/usb/musb/musb_gadget.c void musb_g_tx(struct musb *musb, u8 epnum) · line 511
Quoted 6 lines
drivers/usb/musb/musb_gadget.c +0
-3- if (csr & MUSB_TXCSR_FIFONOTEMPTY) - return; - request = musb_ep->desc ? next_request(musb_ep) : NULL; if (!request) { DBG(4, "%s idle now\n",maybe this change will make a difference ?
No, only your change can not prevent the infinite hangs for OUT transfer, see the explanation in my commit log. Even the change may cause infinite hangs again, once return if request->actual is less than request->length, musb will stop loading data from requst->buf into fifo, so hangs...
Quoted 8 lines
From
13fa6c9cec387893e2b22db6c055028f96fea5b8Mon Sep 17 00:00:00 2001
From: Cliff Cai <[email protected]>
Date: Fri, 3 Jul 2009 06:32:02 +0000
Subject: [PATCH] USB: musb: only complete the request when all data has been transfered [local/log]bug[#5269],bug[#5268],bug[#5264]only complete the request when all data has been transfered
Oh, another local change...
Quoted 12 lines
git-svn-id: svn://localhost/svn/linux-kernel/trunk@6915 526b6c2d-f592-4532- a319-5dd88ccb003d
---
drivers/usb/musb/musb_gadget.c | 2 ++1 files changed, 2 insertions(+), 0 deletions(-)drivers/usb/musb/musb_gadget.c +0
-0
drivers/usb/musb/musb_gadget.c void musb_g_tx(struct musb *musb, u8 epnum) · line 504
Quoted 5 lines
drivers/usb/musb/musb_gadget.c +2
-0@@ -500,6 +500,8 @@ void musb_g_tx(struct musb *musb, u8 epnum) | MUSB_TXCSR_TXPKTRDY); request->zero = 0; } + if (request->actual < request->length) + return;
As said above, this change above should be wrong, follows the fix which may complete request only if data is transfered over for the request.
drivers/usb/musb/musb_gadget.c +0 -2
@@ -503,14 +503,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 );