On Sun, Apr 14, 2013 at 12:48:28PM +0200, Martin Pieuchot wrote:
> So I'd like to remove the following hack from uhci(4) which is the
> only piece of code that justifies another layer of abstraction around
> the memory allocation logic for the various USB controllers.
>
> I couldn't find any historical explanation for this code, not even from
> the NetBSD source repository. But what this hack does is to allocate
> then free some TDs, if the requested size is bigger than some magic
> value, to put them in the free list to avoid allocating them in
> interrupt context.
>
> But as the comment says there's no guarantee that these TDs will still
> be on the free list when you'll need them. There's also no guarantee
> that the number of TDs that you're pre-allocating will be big enough for
> your needs. Anyway, after some time, the free list should already have
> enough TDs so that you won't need to allocate new ones.
>
> I'm running this without problem on amd64 with an "Intel 82801GB USB"
> and I'm quite confident this hack can goes away as neither ohci(4) nor
> ehci(4) use this kind of TDs pre-allocation when allocating memory for
> a transfer.
Tested on
uhci0 at pci0 dev 7 function 2 "Intel 82371AB USB" rev 0x01: apic 2 int 19
pushing almost(!) 4Mbps over one of these usb network thingies.
Seems OK to me.
>
>
> While here, put some splusb() around the code that modify the pointer to
> the first element of the list of free TDs, because it can also be
> modified from interrupt context.
I could not spot where it's modified, but then I didn't look very hard.
>
>
> ok?
>
> Index: uhci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhci.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 uhci.c
> --- uhci.c 28 Mar 2013 03:58:03 -0000 1.93
> +++ uhci.c 12 Apr 2013 15:11:55 -0000
> @@ -628,31 +628,6 @@ usbd_status
> uhci_allocm(struct usbd_bus *bus, usb_dma_t *dma, u_int32_t size)
> {
> struct uhci_softc *sc = (struct uhci_softc *)bus;
> - u_int32_t n;
> -
> - /*
> - * XXX
> - * Since we are allocating a buffer we can assume that we will
> - * need TDs for it. Since we don't want to allocate those from
> - * an interrupt context, we allocate them here and free them again.
> - * This is no guarantee that we'll get the TDs next time...
> - */
> - n = size / 8;
> - if (n > 16) {
> - u_int32_t i;
> - uhci_soft_td_t **stds;
> - DPRINTF(("uhci_allocm: get %d TDs\n", n));
> - stds = malloc(sizeof(uhci_soft_td_t *) * n, M_TEMP,
> - M_NOWAIT | M_ZERO);
> - if (stds == NULL)
> - panic("uhci_allocm");
> - for(i=0; i < n; i++)
> - stds[i] = uhci_alloc_std(sc);
> - for(i=0; i < n; i++)
> - if (stds[i] != NULL)
> - uhci_free_std(sc, stds[i]);
> - free(stds, M_TEMP);
> - }
>
> return (usb_allocmem(&sc->sc_bus, size, 0, dma));
> }
> @@ -1584,6 +1559,7 @@ uhci_alloc_std(uhci_softc_t *sc)
> usbd_status err;
> int i, offs;
> usb_dma_t dma;
> + int s;
>
> if (sc->sc_freetds == NULL) {
> DPRINTFN(2,("uhci_alloc_std: allocating chunk\n"));
> @@ -1591,6 +1567,7 @@ uhci_alloc_std(uhci_softc_t *sc)
> UHCI_TD_ALIGN, &dma);
> if (err)
> return (0);
> + s = splusb();
> for(i = 0; i < UHCI_STD_CHUNK; i++) {
> offs = i * UHCI_STD_SIZE;
> std = KERNADDR(&dma, offs);
> @@ -1598,16 +1575,23 @@ uhci_alloc_std(uhci_softc_t *sc)
> std->link.std = sc->sc_freetds;
> sc->sc_freetds = std;
> }
> + splx(s);
> }
> +
> + s = splusb();
> std = sc->sc_freetds;
> sc->sc_freetds = std->link.std;
> memset(&std->td, 0, sizeof(uhci_td_t));
> - return std;
> + splx(s);
> +
> + return (std);
> }
>
> void
> uhci_free_std(uhci_softc_t *sc, uhci_soft_td_t *std)
> {
> + int s;
> +
> #ifdef DIAGNOSTIC
> #define TD_IS_FREE 0x12345678
> if (letoh32(std->td.td_token) == TD_IS_FREE) {
> @@ -1616,8 +1600,11 @@ uhci_free_std(uhci_softc_t *sc, uhci_sof
> }
> std->td.td_token = htole32(TD_IS_FREE);
> #endif
> +
> + s = splusb();
> std->link.std = sc->sc_freetds;
> sc->sc_freetds = std;
> + splx(s);
> }
>
> uhci_soft_qh_t *
>
