CPU Cache and busdma usage in USB

[M. Warner Losh]

In message: <200906231912.20741.hselasky at c2i.net>
            Hans Petter Selasky <hselasky at c2i.net> writes:
: On Tuesday 23 June 2009 11:11:29 Alexandr Rybalko wrote:
: > On Tue, 23 Jun 2009 10:35:42 +0200
: >
: > Piotr Zięcik <kosmo at semihalf.com> wrote:
: > >> While bringing up EHCI  (8-CURRENT, new USB stack) on ARM machine we
: > >> have found cache-related problem in the USB stack.
: > >>
: > >> The usb_pc_cpu_flush() and usb_pc_cpu_invalidate() functions are used to
: > >> flush/invalidate CPU caches in various places in USB code. Internally,
: > >> the functions are implemented using bus_dmamap_sync(). In our case, on
: > >> ARM machine, flags passed to the bus_dmamap_sync() function did not
: > >> correspond with requested operation. We have fixed the problem by
: > >> changing flags passed to the bus_dmamap_sync() function (see attached
: > >> patch).
: > >>
: > >> My question is about general idea of bus_dma usage for cache operations.
: > >> In my opinion we should not rely on bus_dmamap_sync() behaviour as this
: > >> function may do different things on different architectures.  This not
: > >> always works as expected, which is clearly visible in our case. Better
: > >> solution is to use cpu-specific functions implementing cache operations.
: > >> Please comment on why CPU-specific functions are not used...

I think because busdma is supposed to abstract this out.  The problem
is that the usb code chose different terms to represent these
operations than is typically used.

: > >> Patch fixing our problem:
: > >> diff --git a/sys/dev/usb/usb_busdma.c b/sys/dev/usb/usb_busdma.c
: > >> index 3d6a5be..69a6fff 100644
: > >> --- a/sys/dev/usb/usb_busdma.c
: > >> +++ b/sys/dev/usb/usb_busdma.c
: > >> @@ -658,8 +658,7 @@ usb_pc_cpu_invalidate(struct usb_page_cache *pc)
: > >>                 /* nothing has been loaded into this page cache! */
: > >>                 return;
: > >>         }
: > >> -       bus_dmamap_sync(pc->tag, pc->map,
: > >> -           BUS_DMASYNC_POSTWRITE | BUS_DMASYNC_POSTREAD);
: > >> +       bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREREAD);
: > >>  }

I think this patch is currect.  Invalidate should be done to a region
before you read into it.

: > >> /*----------------------------------------------------------------------
: > >>--* @@ -672,8 +671,7 @@ usb_pc_cpu_flush(struct usb_page_cache *pc) /*
: > >> nothing has been loaded into this page cache! */ return;
: > >>         }
: > >> -       bus_dmamap_sync(pc->tag, pc->map,
: > >> -           BUS_DMASYNC_PREWRITE | BUS_DMASYNC_PREREAD);
: > >> +       bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
: > >>  }

This makes sense as well.  Flushing the cache to memory is the right
logical operation before writing to the device with a DMA transfer.

: > >> /*----------------------------------------------------------------------
: > >>--*
: > >>
: 
: >
: > Great thanks Piotr!
: > I work on MIPS BCM5354 and BCM5836 and after apply your patch USB work
: > correct.
: 
: We are currently investigating if your patch is correct. Thanks for your patch 
: suggestion!

From the comments in the code, they look correct.  I don't know if all
the usages of these functions is reflected in their comments.  I've
not had a chance to audit them all...

Warner