diff mbox series

[1/1] usb: gadget: musb: fix short isoc packets with inventra dma

Message ID 20190128200151.15319-2-b-liu@ti.com (mailing list archive)
State Mainlined
Commit c418fd6c01fbc5516a2cd1eaf1df1ec86869028a
Headers show
Series musb fixes for v5.0-rc5 | expand

Commit Message

Bin Liu Jan. 28, 2019, 8:01 p.m. UTC 
From: Paul Elder <paul.elder@ideasonboard.com>

Handling short packets (length < max packet size) in the Inventra DMA
engine in the MUSB driver causes the MUSB DMA controller to hang. An
example of a problem that is caused by this problem is when streaming
video out of a UVC gadget, only the first video frame is transferred.

For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
set manually by the driver. This was previously done in musb_g_tx
(musb_gadget.c), but incorrectly (all csr flags were cleared, and only
MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
allows some requests to be transferred correctly, but multiple requests
were often put together in one USB packet, and caused problems if the
packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
in dma_controller_irq (musbhsdma.c), just like host mode transfers.

This topic was originally tackled by Nicolas Boichat [0] [1] and is
discussed further at [2] as part of his GSoC project [3].

[0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
[1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
[2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
[3] http://elinux.org/BeagleBoard/GSoC/USBSniffer

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Bin Liu <b-liu@ti.com>
---
 drivers/usb/musb/musb_gadget.c | 13 +------------
 drivers/usb/musb/musbhsdma.c   | 21 +++++++++++----------
 2 files changed, 12 insertions(+), 22 deletions(-)

Comments

Greg Kroah-Hartman Jan. 30, 2019, 8:24 a.m. UTC | #1 
On Mon, Jan 28, 2019 at 02:01:51PM -0600, Bin Liu wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> Handling short packets (length < max packet size) in the Inventra DMA
> engine in the MUSB driver causes the MUSB DMA controller to hang. An
> example of a problem that is caused by this problem is when streaming
> video out of a UVC gadget, only the first video frame is transferred.
> 
> For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> set manually by the driver. This was previously done in musb_g_tx
> (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
> allows some requests to be transferred correctly, but multiple requests
> were often put together in one USB packet, and caused problems if the
> packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
> in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> 
> This topic was originally tackled by Nicolas Boichat [0] [1] and is
> discussed further at [2] as part of his GSoC project [3].
> 
> [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Bin Liu <b-liu@ti.com>

No "Fixes:" tag?

No need for stable?

That seems odd...
Bin Liu Jan. 30, 2019, 1:33 p.m. UTC | #2 
Greg,

On Wed, Jan 30, 2019 at 09:24:04AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Jan 28, 2019 at 02:01:51PM -0600, Bin Liu wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > Handling short packets (length < max packet size) in the Inventra DMA
> > engine in the MUSB driver causes the MUSB DMA controller to hang. An
> > example of a problem that is caused by this problem is when streaming
> > video out of a UVC gadget, only the first video frame is transferred.
> > 
> > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> > set manually by the driver. This was previously done in musb_g_tx
> > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
> > allows some requests to be transferred correctly, but multiple requests
> > were often put together in one USB packet, and caused problems if the
> > packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
> > in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> > 
> > This topic was originally tackled by Nicolas Boichat [0] [1] and is
> > discussed further at [2] as part of his GSoC project [3].
> > 
> > [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> > [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> > [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Bin Liu <b-liu@ti.com>
> 
> No "Fixes:" tag?

This patch changes the logic which was there in the very first commit of
the musb drivers more than a decade ago, so it seems the issue was there
from day one. I didn't add the 'Fixes:' tag, was thinking it doesn't add
any value.

Do you want me to add it?
	Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
> 
> No need for stable?

The patch might not be applied on stable cleanly. I was thinking to send
out backport patches for applicable stable trees once this patch got in
v5.0 -rc.

> 
> That seems odd...

Indeed, this is a forward-port from 2.6.34.

Regards,
-Bin.
Greg Kroah-Hartman Jan. 30, 2019, 1:59 p.m. UTC | #3 
On Wed, Jan 30, 2019 at 07:33:20AM -0600, Bin Liu wrote:
> Greg,
> 
> On Wed, Jan 30, 2019 at 09:24:04AM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Jan 28, 2019 at 02:01:51PM -0600, Bin Liu wrote:
> > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > Handling short packets (length < max packet size) in the Inventra DMA
> > > engine in the MUSB driver causes the MUSB DMA controller to hang. An
> > > example of a problem that is caused by this problem is when streaming
> > > video out of a UVC gadget, only the first video frame is transferred.
> > > 
> > > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> > > set manually by the driver. This was previously done in musb_g_tx
> > > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> > > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY were set). Fixing that problem
> > > allows some requests to be transferred correctly, but multiple requests
> > > were often put together in one USB packet, and caused problems if the
> > > packet size was not a multiple of 4. Instead, set MUSB_TXCSR_TXPKTRDY
> > > in dma_controller_irq (musbhsdma.c), just like host mode transfers.
> > > 
> > > This topic was originally tackled by Nicolas Boichat [0] [1] and is
> > > discussed further at [2] as part of his GSoC project [3].
> > > 
> > > [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> > > [1] https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> > > [2] http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> > > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Signed-off-by: Bin Liu <b-liu@ti.com>
> > 
> > No "Fixes:" tag?
> 
> This patch changes the logic which was there in the very first commit of
> the musb drivers more than a decade ago, so it seems the issue was there
> from day one. I didn't add the 'Fixes:' tag, was thinking it doesn't add
> any value.
> 
> Do you want me to add it?
> 	Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")

Yes please.

> > No need for stable?
> 
> The patch might not be applied on stable cleanly. I was thinking to send
> out backport patches for applicable stable trees once this patch got in
> v5.0 -rc.

Ok, that's fine.

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..ffe462a657b1 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -452,13 +452,10 @@  void musb_g_tx(struct musb *musb, u8 epnum)
 	}
 
 	if (request) {
-		u8	is_dma = 0;
-		bool	short_packet = false;
 
 		trace_musb_req_tx(req);
 
 		if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
-			is_dma = 1;
 			csr |= MUSB_TXCSR_P_WZC_BITS;
 			csr &= ~(MUSB_TXCSR_DMAENAB | MUSB_TXCSR_P_UNDERRUN |
 				 MUSB_TXCSR_TXPKTRDY | MUSB_TXCSR_AUTOSET);
@@ -476,16 +473,8 @@  void musb_g_tx(struct musb *musb, u8 epnum)
 		 */
 		if ((request->zero && request->length)
 			&& (request->length % musb_ep->packet_sz == 0)
-			&& (request->actual == request->length))
-				short_packet = true;
+			&& (request->actual == request->length)) {
 
-		if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
-			(is_dma && (!dma->desired_mode ||
-				(request->actual &
-					(musb_ep->packet_sz - 1)))))
-				short_packet = true;
-
-		if (short_packet) {
 			/*
 			 * On DMA completion, FIFO may not be
 			 * available yet...
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f87829..5fc6825745f2 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -346,12 +346,10 @@  static irqreturn_t dma_controller_irq(int irq, void *private_data)
 				channel->status = MUSB_DMA_STATUS_FREE;
 
 				/* completed */
-				if ((devctl & MUSB_DEVCTL_HM)
-					&& (musb_channel->transmit)
-					&& ((channel->desired_mode == 0)
-					    || (channel->actual_len &
-					    (musb_channel->max_packet_sz - 1)))
-				    ) {
+				if (musb_channel->transmit &&
+					(!channel->desired_mode ||
+					(channel->actual_len %
+					    musb_channel->max_packet_sz))) {
 					u8  epnum  = musb_channel->epnum;
 					int offset = musb->io.ep_offset(epnum,
 								    MUSB_TXCSR);
@@ -363,11 +361,14 @@  static irqreturn_t dma_controller_irq(int irq, void *private_data)
 					 */
 					musb_ep_select(mbase, epnum);
 					txcsr = musb_readw(mbase, offset);
-					txcsr &= ~(MUSB_TXCSR_DMAENAB
+					if (channel->desired_mode == 1) {
+						txcsr &= ~(MUSB_TXCSR_DMAENAB
 							| MUSB_TXCSR_AUTOSET);
-					musb_writew(mbase, offset, txcsr);
-					/* Send out the packet */
-					txcsr &= ~MUSB_TXCSR_DMAMODE;
+						musb_writew(mbase, offset, txcsr);
+						/* Send out the packet */
+						txcsr &= ~MUSB_TXCSR_DMAMODE;
+						txcsr |= MUSB_TXCSR_DMAENAB;
+					}
 					txcsr |=  MUSB_TXCSR_TXPKTRDY;
 					musb_writew(mbase, offset, txcsr);
 				}