From owner-p4-projects@FreeBSD.ORG Tue Feb 17 16:19:03 2009 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 225931065674; Tue, 17 Feb 2009 16:19:03 +0000 (UTC) Delivered-To: perforce@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D54A21065670 for ; Tue, 17 Feb 2009 16:19:02 +0000 (UTC) (envelope-from hselasky@FreeBSD.org) Received: from repoman.freebsd.org (repoman.freebsd.org [IPv6:2001:4f8:fff6::29]) by mx1.freebsd.org (Postfix) with ESMTP id C30E78FC22 for ; Tue, 17 Feb 2009 16:19:02 +0000 (UTC) (envelope-from hselasky@FreeBSD.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.14.3/8.14.3) with ESMTP id n1HGJ2J3070062 for ; Tue, 17 Feb 2009 16:19:02 GMT (envelope-from hselasky@FreeBSD.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.14.3/8.14.3/Submit) id n1HGJ2NG070060 for perforce@freebsd.org; Tue, 17 Feb 2009 16:19:02 GMT (envelope-from hselasky@FreeBSD.org) Date: Tue, 17 Feb 2009 16:19:02 GMT Message-Id: <200902171619.n1HGJ2NG070060@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to hselasky@FreeBSD.org using -f From: Hans Petter Selasky To: Perforce Change Reviews Cc: Subject: PERFORCE change 157847 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Feb 2009 16:19:04 -0000 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); } } }