Move the responsibility of syncing the data buffers from the USB

stack into the USB controller driver, since not all of the xfers
actually use DMA and some invalidations might even be harmful.

The important part is to sync before handing the buffer to the
controller, and to sync on a successful transfer before handing
it back to the USB stack.  For isoc transfers it's easier to sync
the complete length of the transfer, since the buffer to flush is
not filled in a contiguous fashion.

For dwc2 there's a common point which takes care of the start
of transfers from or to devices, where we can make sure that our
buffers are synced.  On completion, there's a common point before
handing it off to the USB stack.

For ehci there are three places which take care of the start
of transfers from or to devices, where one already does the sync,
while the two other places still need the sync.  There are two
completion handler (isoc and non-isoc), where one already has a
comment asking for the need of a sync.  The done handler for intr
xfers does a sync that is not needed anymore after adding the sync
in the completion handler.

For ohci there are three places which take care of the start
of transfers from or to devices, where all of them were still in
need of the sync.  For completion, there is one place for isoc
xfers and two places for handling successful generic xfers.

For uhci there are two places which take care of the start
of transfers from or to device, where all of them were still in
need of the sync.  For completion, there is one handler for both
isoc and non-isoc xfers where syncs need to occur.

For xhci there are three places which take care of the start
of transfers from or to device, where all of them were still in
need of the sync.  For completion, there is one handler for isoc
and one for non-isoc xfers where syncs need to occur.

With this we can revert the workaround that implicitly allocated
buffers are COHERENT, since now control transfers fulfilled by the
driver code (instead of the controller doing DMA) are not flushed
into oblivion anymore.

Tested by Janne Johansson with dwc2 (octeon)
Tested by kettenis@ with xhci(4) (octeon)
Tested by patrick@ with ehci(4) on a Cubox-i (armv7)
Tested by patrick@ with xhci(4) on an i.MX8MQ (arm64)
Tested by tobhe@ with dwc2 on a rPi 3b (arm64)
ok kettenis@

6 files changed, 114 insertions, 36 deletions

diff --git a/sys/dev/usb/dwc2/dwc2.c b/sys/dev/usb/dwc2/dwc2.c
index 6ca3cc658e5..3ad7dc479f4 100644
--- a/sys/dev/usb/dwc2/dwc2.c
+++ b/sys/dev/usb/dwc2/dwc2.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dwc2.c,v 1.51 2020/03/21 12:08:31 patrick Exp $	*/
+/*	$OpenBSD: dwc2.c,v 1.52 2020/04/03 20:11:47 patrick Exp $	*/
 /*	$NetBSD: dwc2.c,v 1.32 2014/09/02 23:26:20 macallan Exp $	*/
 
 /*-
@@ -1260,6 +1260,9 @@ dwc2_device_start(struct usbd_xfer *xfer)
 		dwc2_urb->usbdma = &xfer->dmabuf;
 		dwc2_urb->buf = KERNADDR(dwc2_urb->usbdma, 0);
 		dwc2_urb->dma = DMAADDR(dwc2_urb->usbdma, 0);
+		usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+		    usbd_xfer_isread(xfer) ?
+		    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
  	}
 	dwc2_urb->length = len;
  	dwc2_urb->flags = flags;
@@ -1649,6 +1652,17 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
 					xfer);
 	}
 
+	if (xfer->status == USBD_NORMAL_COMPLETION) {
+		if (xfertype == UE_ISOCHRONOUS)
+			usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+			    usbd_xfer_isread(xfer) ?
+			    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
+		else if (xfer->actlen)
+			usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
+			    usbd_xfer_isread(xfer) ?
+			    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
+	}
+
 	qtd->urb = NULL;
 	timeout_del(&xfer->timeout_handle);
 	usb_rem_task(xfer->device, &xfer->abort_task);
diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c
index a352d83eaf4..41851defe4f 100644
--- a/sys/dev/usb/ehci.c
+++ b/sys/dev/usb/ehci.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ehci.c,v 1.209 2020/03/30 22:29:04 krw Exp $ */
+/*	$OpenBSD: ehci.c,v 1.210 2020/04/03 20:11:47 patrick Exp $ */
 /*	$NetBSD: ehci.c,v 1.66 2004/06/30 03:11:56 mycroft Exp $	*/
 
 /*
@@ -856,6 +856,10 @@ ehci_isoc_idone(struct usbd_xfer *xfer)
 #endif
 	xfer->actlen = actlen;
 	xfer->status = USBD_NORMAL_COMPLETION;
+
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 	usb_transfer_complete(xfer);
 }
 
@@ -911,9 +915,10 @@ ehci_idone(struct usbd_xfer *xfer)
 	} else
 		xfer->status = USBD_NORMAL_COMPLETION;
 
-	/* XXX transfer_complete memcpys out transfer data (for in endpoints)
-	 * during this call, before methods->done is called: dma sync required
-	 * beforehand? */
+	if (xfer->actlen)
+		usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
+		    usbd_xfer_isread(xfer) ?
+		    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 	usb_transfer_complete(xfer);
 	DPRINTFN(/*12*/2, ("ehci_idone: ex=%p done\n", ex));
 }
@@ -3208,9 +3213,6 @@ ehci_device_intr_done(struct usbd_xfer *xfer)
 	if (xfer->pipe->repeat) {
 		ehci_free_sqtd_chain(sc, ex);
 
-		usb_syncmem(&xfer->dmabuf, 0, xfer->length,
-		    usbd_xfer_isread(xfer) ?
-		    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 		sqh = epipe->sqh;
 
 		err = ehci_alloc_sqtd_chain(sc, xfer->length, xfer, &data, &dataend);
@@ -3408,6 +3410,9 @@ ehci_alloc_itd_chain(struct ehci_softc *sc, struct usbd_xfer *xfer)
 	if (nframes == 0)
 		return (1);
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
 	for (i = 0; i < nframes; i++) {
 		uint32_t froffs = offs;
 
@@ -3522,6 +3527,9 @@ ehci_alloc_sitd_chain(struct ehci_softc *sc, struct usbd_xfer *xfer)
 	if (usbd_xfer_isread(xfer))
 		endp |= EHCI_SITD_SET_DIR(1);
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
 	for (i = 0; i < nframes; i++) {
 		uint32_t addr = DMAADDR(&xfer->dmabuf, offs);
 		uint32_t page = EHCI_PAGE(addr + xfer->frlengths[i] - 1);
diff --git a/sys/dev/usb/ohci.c b/sys/dev/usb/ohci.c
index 7aa2303eabd..3155068633e 100644
--- a/sys/dev/usb/ohci.c
+++ b/sys/dev/usb/ohci.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ohci.c,v 1.160 2020/03/21 12:08:31 patrick Exp $ */
+/*	$OpenBSD: ohci.c,v 1.161 2020/04/03 20:11:47 patrick Exp $ */
 /*	$NetBSD: ohci.c,v 1.139 2003/02/22 05:24:16 tsutsui Exp $	*/
 /*	$FreeBSD: src/sys/dev/usb/ohci.c,v 1.22 1999/11/17 22:33:40 n_hibma Exp $	*/
 
@@ -491,6 +491,10 @@ ohci_alloc_std_chain(struct ohci_softc *sc, u_int alen, struct usbd_xfer *xfer,
 
 	DPRINTFN(alen < 4096,("ohci_alloc_std_chain: start len=%u\n", alen));
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	len = alen;
 	cur = sp;
 	end = NULL;
@@ -1279,6 +1283,12 @@ ohci_softintr(void *v)
 
 			ohci_free_std(sc, std);
 			if (done) {
+				if (xfer->actlen)
+					usb_syncmem(&xfer->dmabuf, 0,
+					    xfer->actlen,
+					    usbd_xfer_isread(xfer) ?
+					    BUS_DMASYNC_POSTREAD :
+					    BUS_DMASYNC_POSTWRITE);
 				xfer->status = USBD_NORMAL_COMPLETION;
 				s = splusb();
 				usb_transfer_complete(xfer);
@@ -1309,9 +1319,15 @@ ohci_softintr(void *v)
 
 			if (cc == OHCI_CC_STALL)
 				xfer->status = USBD_STALLED;
-			else if (cc == OHCI_CC_DATA_UNDERRUN)
+			else if (cc == OHCI_CC_DATA_UNDERRUN) {
+				if (xfer->actlen)
+					usb_syncmem(&xfer->dmabuf, 0,
+					    xfer->actlen,
+					    usbd_xfer_isread(xfer) ?
+					    BUS_DMASYNC_POSTREAD :
+					    BUS_DMASYNC_POSTWRITE);
 				xfer->status = USBD_NORMAL_COMPLETION;
-			else
+			} else
 				xfer->status = USBD_IOERROR;
 			s = splusb();
 			usb_transfer_complete(xfer);
@@ -1389,6 +1405,13 @@ ohci_softintr(void *v)
 				xfer->actlen = actlen;
 			xfer->hcpriv = NULL;
 
+			if (xfer->status == USBD_NORMAL_COMPLETION) {
+				usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+				    usbd_xfer_isread(xfer) ?
+				    BUS_DMASYNC_POSTREAD :
+				    BUS_DMASYNC_POSTWRITE);
+			}
+
 			s = splusb();
 			usb_transfer_complete(xfer);
 			splx(s);
@@ -2846,6 +2869,10 @@ ohci_device_intr_start(struct usbd_xfer *xfer)
 		panic("ohci_device_intr_transfer: a request");
 #endif
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	len = xfer->length;
 	endpt = xfer->pipe->endpoint->edesc->bEndpointAddress;
 
@@ -3057,6 +3084,10 @@ ohci_device_isoc_enter(struct usbd_xfer *xfer)
 	if (sc->sc_bus.dying)
 		return;
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	if (iso->next == -1) {
 		/* Not in use yet, schedule it a few frames ahead. */
 		iso->next = letoh32(sc->sc_hcca->hcca_frame_number) + 5;
diff --git a/sys/dev/usb/uhci.c b/sys/dev/usb/uhci.c
index bae6c1b81a9..ff4eb530fcb 100644
--- a/sys/dev/usb/uhci.c
+++ b/sys/dev/usb/uhci.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uhci.c,v 1.151 2020/03/21 12:08:31 patrick Exp $	*/
+/*	$OpenBSD: uhci.c,v 1.152 2020/04/03 20:11:47 patrick Exp $	*/
 /*	$NetBSD: uhci.c,v 1.172 2003/02/23 04:19:26 simonb Exp $	*/
 /*	$FreeBSD: src/sys/dev/usb/uhci.c,v 1.33 1999/11/17 22:33:41 n_hibma Exp $	*/
 
@@ -1236,6 +1236,9 @@ uhci_idone(struct usbd_xfer *xfer)
 			actlen += len;
 		}
 		upipe->u.iso.inuse -= nframes;
+		usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+		    usbd_xfer_isread(xfer) ?
+		    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 		xfer->actlen = actlen;
 		xfer->status = USBD_NORMAL_COMPLETION;
 		goto end;
@@ -1299,6 +1302,10 @@ uhci_idone(struct usbd_xfer *xfer)
 		else
 			xfer->status = USBD_IOERROR; /* more info XXX */
 	} else {
+		if (xfer->actlen)
+			usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
+			    usbd_xfer_isread(xfer) ?
+			    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 		xfer->status = USBD_NORMAL_COMPLETION;
 	}
 
@@ -1527,6 +1534,10 @@ uhci_alloc_std_chain(struct uhci_softc *sc, u_int len, struct usbd_xfer *xfer,
 	    __func__, addr, UE_GET_ADDR(endpt), len, xfer->device->speed,
 	    flags));
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	mps = UGETW(xfer->pipe->endpoint->edesc->wMaxPacketSize);
 	if (mps == 0) {
 		printf("uhci_alloc_std_chain: mps=0\n");
@@ -2127,6 +2138,10 @@ uhci_device_isoc_enter(struct usbd_xfer *xfer)
 		printf("uhci_device_isoc_enter: overflow!\n");
 #endif
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	next = iso->next;
 	if (next == -1) {
 		/* Not in use yet, schedule it a few frames ahead. */
diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
index f6707432e41..ecb17c3bf4f 100644
--- a/sys/dev/usb/usbdi.c
+++ b/sys/dev/usb/usbdi.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: usbdi.c,v 1.105 2020/04/01 08:43:33 patrick Exp $ */
+/*	$OpenBSD: usbdi.c,v 1.106 2020/04/03 20:11:47 patrick Exp $ */
 /*	$NetBSD: usbdi.c,v 1.103 2002/09/27 15:37:38 provos Exp $	*/
 /*	$FreeBSD: src/sys/dev/usb/usbdi.c,v 1.28 1999/11/17 22:33:49 n_hibma Exp $	*/
 
@@ -305,22 +305,15 @@ usbd_transfer(struct usbd_xfer *xfer)
 		if (xfer->rqflags & URQ_AUTO_DMABUF)
 			printf("usbd_transfer: has old buffer!\n");
 #endif
-		err = usb_allocmem(bus, xfer->length, 0, USB_DMA_COHERENT,
-		    &xfer->dmabuf);
+		err = usb_allocmem(bus, xfer->length, 0, 0, &xfer->dmabuf);
 		if (err)
 			return (err);
 		xfer->rqflags |= URQ_AUTO_DMABUF;
 	}
 
-	if (!usbd_xfer_isread(xfer)) {
-		if ((xfer->flags & USBD_NO_COPY) == 0)
-			memcpy(KERNADDR(&xfer->dmabuf, 0), xfer->buffer,
-			    xfer->length);
-		usb_syncmem(&xfer->dmabuf, 0, xfer->length,
-		    BUS_DMASYNC_PREWRITE);
-	} else
-		usb_syncmem(&xfer->dmabuf, 0, xfer->length,
-		    BUS_DMASYNC_PREREAD);
+	if (!usbd_xfer_isread(xfer) && (xfer->flags & USBD_NO_COPY) == 0)
+		memcpy(KERNADDR(&xfer->dmabuf, 0), xfer->buffer,
+		    xfer->length);
 
 	usb_tap(bus, xfer, USBTAP_DIR_OUT);
 
@@ -750,17 +743,10 @@ usb_transfer_complete(struct usbd_xfer *xfer)
 	}
 #endif
 
-	if (xfer->actlen != 0) {
-		if (usbd_xfer_isread(xfer)) {
-			usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
-			    BUS_DMASYNC_POSTREAD);
-			if (!(xfer->flags & USBD_NO_COPY))
-				memcpy(xfer->buffer, KERNADDR(&xfer->dmabuf, 0),
-				    xfer->actlen);
-		} else
-			usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
-			    BUS_DMASYNC_POSTWRITE);
-	}
+	if (usbd_xfer_isread(xfer) && xfer->actlen != 0 &&
+	    (xfer->flags & USBD_NO_COPY) == 0)
+		memcpy(xfer->buffer, KERNADDR(&xfer->dmabuf, 0),
+		    xfer->actlen);
 
 	/* if we allocated the buffer in usbd_transfer() we free it here. */
 	if (xfer->rqflags & URQ_AUTO_DMABUF) {
diff --git a/sys/dev/usb/xhci.c b/sys/dev/usb/xhci.c
index f98f9a0b8fe..2d65208f3db 100644
--- a/sys/dev/usb/xhci.c
+++ b/sys/dev/usb/xhci.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: xhci.c,v 1.113 2020/03/02 16:30:39 visa Exp $ */
+/* $OpenBSD: xhci.c,v 1.114 2020/04/03 20:11:47 patrick Exp $ */
 
 /*
  * Copyright (c) 2014-2015 Martin Pieuchot
@@ -851,6 +851,10 @@ xhci_event_xfer_generic(struct xhci_softc *sc, struct usbd_xfer *xfer,
 			else
 				xfer->actlen = xfer->length;
 		}
+		if (xfer->actlen)
+			usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
+			    usbd_xfer_isread(xfer) ?
+			    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 		xfer->status = USBD_NORMAL_COMPLETION;
 		break;
 	case XHCI_CODE_SHORT_XFER:
@@ -871,6 +875,10 @@ xhci_event_xfer_generic(struct xhci_softc *sc, struct usbd_xfer *xfer,
 			    DEVNAME(sc), xfer, xx->index));
 			return (1);
 		}
+		if (xfer->actlen)
+			usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
+			    usbd_xfer_isread(xfer) ?
+			    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 		xfer->status = USBD_NORMAL_COMPLETION;
 		break;
 	case XHCI_CODE_TXERR:
@@ -975,6 +983,9 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xhci_pipe *xp,
 		xp->skip = 0;
 	}
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 	xfer->status = USBD_NORMAL_COMPLETION;
 
 	return (0);
@@ -2809,6 +2820,11 @@ xhci_device_ctrl_start(struct usbd_xfer *xfer)
 	if (xp->free_trbs < 3)
 		return (USBD_NOMEM);
 
+	if (len != 0)
+		usb_syncmem(&xfer->dmabuf, 0, len,
+		    usbd_xfer_isread(xfer) ?
+		    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	/* We'll toggle the setup TRB once we're finished with the stages. */
 	trb0 = xhci_xfer_get_trb(sc, xfer, &toggle, 0);
 
@@ -2935,6 +2951,10 @@ xhci_device_generic_start(struct usbd_xfer *xfer)
 	if (xp->free_trbs < (ntrb + zerotd))
 		return (USBD_NOMEM);
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	/* We'll toggle the first TRB once we're finished with the chain. */
 	trb0 = xhci_xfer_get_trb(sc, xfer, &toggle, (ntrb == 1));
 	flags = XHCI_TRB_TYPE_NORMAL | (toggle ^ 1);
@@ -3091,6 +3111,10 @@ xhci_device_isoc_start(struct usbd_xfer *xfer)
 	if (xp->free_trbs < ntrb)
 		return (USBD_NOMEM);
 
+	usb_syncmem(&xfer->dmabuf, 0, xfer->length,
+	    usbd_xfer_isread(xfer) ?
+	    BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE);
+
 	paddr = DMAADDR(&xfer->dmabuf, 0);
 
 	for (i = 0, trb0 = NULL; i < xfer->nframes; i++) {