Date: Tue, 17 Feb 2009 16:19:02 GMT From: Hans Petter Selasky <hselasky@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 157847 for review Message-ID: <200902171619.n1HGJ2NG070060@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=157847 Change 157847 by hselasky@hselasky_laptop001 on 2009/02/17 16:18:09 USB CORE: Improvements to "usb2_transfer_setup()" and "usb2_transfer_unsetup()". Set "ppxfer[n]" when the transfer setup is complete to prevent races. Remove redundant NULL-checks from "usb2_transfer_unsetup()". Affected files ... .. //depot/projects/usb/src/sys/dev/usb2/core/usb2_transfer.c#44 edit Differences ... ==== //depot/projects/usb/src/sys/dev/usb2/core/usb2_transfer.c#44 (text+ko) ==== @@ -887,18 +887,14 @@ parm.size[0] += ((-parm.size[0]) & (USB_HOST_ALIGN - 1)); if (buf) { - /* * Common initialization of the * "usb2_xfer" structure. */ xfer = USB_ADD_BYTES(buf, parm.size[0]); - - ppxfer[n] = xfer; xfer->address = udev->address; xfer->priv_sc = priv_sc; xfer->xroot = info; - info->setup_refcount++; usb2_callout_init_mtx(&xfer->timeout_handle, &udev->bus->bus_mtx, 0); @@ -915,9 +911,22 @@ refcount++; } + /* set transfer pipe pointer */ + xfer->pipe = pipe; + parm.size[0] += sizeof(xfer[0]); + parm.methods = xfer->pipe->methods; + parm.curr_xfer = xfer; - xfer->pipe = pipe; + /* + * Call the Host or Device controller transfer + * setup routine: + */ + (udev->bus->methods->xfer_setup) (&parm); + + /* check for error */ + if (parm.err) + goto done; if (buf) { /* @@ -930,18 +939,19 @@ * want more information. */ xfer->pipe->refcount++; - } - parm.methods = xfer->pipe->methods; - parm.curr_xfer = xfer; - /* - * Call the Host or Device controller transfer setup - * routine: - */ - (udev->bus->methods->xfer_setup) (&parm); + /* + * Whenever we set ppxfer[] then we + * also need to increment the + * "setup_refcount": + */ + info->setup_refcount++; - if (parm.err) { - goto done; + /* + * Transfer is successfully setup and + * can be used: + */ + ppxfer[n] = xfer; } } @@ -1121,72 +1131,60 @@ while (n_setup--) { xfer = pxfer[n_setup]; - if (xfer) { - if (xfer->pipe) { - USB_XFER_LOCK(xfer); - USB_BUS_LOCK(xfer->xroot->bus); + if (xfer == NULL) + continue; + + info = xfer->xroot; - /* - * HINT: when you start/stop a transfer, it - * might be a good idea to directly use the - * "pxfer[]" structure: - * - * usb2_transfer_start(sc->pxfer[0]); - * usb2_transfer_stop(sc->pxfer[0]); - * - * That way, if your code has many parts that - * will not stop running under the same - * lock, in other words "xfer_mtx", the - * usb2_transfer_start and - * usb2_transfer_stop functions will simply - * return when they detect a NULL pointer - * argument. - * - * To avoid any races we clear the "pxfer[]" - * pointer while holding the private mutex - * of the driver: - */ - pxfer[n_setup] = NULL; + USB_XFER_LOCK(xfer); + USB_BUS_LOCK(info->bus); - USB_BUS_UNLOCK(xfer->xroot->bus); - USB_XFER_UNLOCK(xfer); + /* + * HINT: when you start/stop a transfer, it might be a + * good idea to directly use the "pxfer[]" structure: + * + * usb2_transfer_start(sc->pxfer[0]); + * usb2_transfer_stop(sc->pxfer[0]); + * + * That way, if your code has many parts that will not + * stop running under the same lock, in other words + * "xfer_mtx", the usb2_transfer_start and + * usb2_transfer_stop functions will simply return + * when they detect a NULL pointer argument. + * + * To avoid any races we clear the "pxfer[]" pointer + * while holding the private mutex of the driver: + */ + pxfer[n_setup] = NULL; - usb2_transfer_drain(xfer); + USB_BUS_UNLOCK(info->bus); + USB_XFER_UNLOCK(xfer); - if (xfer->flags_int.bdma_enable) { - needs_delay = 1; - } - /* - * NOTE: default pipe does not have an - * interface, even if pipe->iface_index == 0 - */ - xfer->pipe->refcount--; + usb2_transfer_drain(xfer); - } else { - /* clear the transfer pointer */ - pxfer[n_setup] = NULL; - } + if (xfer->flags_int.bdma_enable) + needs_delay = 1; - usb2_callout_drain(&xfer->timeout_handle); + /* + * NOTE: default pipe does not have an + * interface, even if pipe->iface_index == 0 + */ + xfer->pipe->refcount--; - if (xfer->xroot) { - info = xfer->xroot; + usb2_callout_drain(&xfer->timeout_handle); - USB_BUS_LOCK(info->bus); + USB_BUS_LOCK(info->bus); - USB_ASSERT(info->setup_refcount != 0, - ("Invalid setup " - "reference count!\n")); + USB_ASSERT(info->setup_refcount != 0, ("Invalid setup " + "reference count!\n")); - info->setup_refcount--; + info->setup_refcount--; - if (info->setup_refcount == 0) { - usb2_transfer_unsetup_sub(info, - needs_delay); - } else { - USB_BUS_UNLOCK(info->bus); - } - } + if (info->setup_refcount == 0) { + usb2_transfer_unsetup_sub(info, + needs_delay); + } else { + USB_BUS_UNLOCK(info->bus); } } }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200902171619.n1HGJ2NG070060>