From owner-p4-projects@FreeBSD.ORG Fri May 11 20:22:17 2007 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 9F22516A407; Fri, 11 May 2007 20:22:17 +0000 (UTC) X-Original-To: perforce@FreeBSD.org Delivered-To: perforce@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 7070216A405 for ; Fri, 11 May 2007 20:22:17 +0000 (UTC) (envelope-from hselasky@FreeBSD.org) Received: from repoman.freebsd.org (repoman.freebsd.org [69.147.83.41]) by mx1.freebsd.org (Postfix) with ESMTP id 5F82213C44C for ; Fri, 11 May 2007 20:22:17 +0000 (UTC) (envelope-from hselasky@FreeBSD.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.13.8/8.13.8) with ESMTP id l4BKMH1G091134 for ; Fri, 11 May 2007 20:22:17 GMT (envelope-from hselasky@FreeBSD.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.13.8/8.13.8/Submit) id l4BKMG3i091127 for perforce@freebsd.org; Fri, 11 May 2007 20:22:16 GMT (envelope-from hselasky@FreeBSD.org) Date: Fri, 11 May 2007 20:22:16 GMT Message-Id: <200705112022.l4BKMG3i091127@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 119690 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: Fri, 11 May 2007 20:22:18 -0000 http://perforce.freebsd.org/chv.cgi?CH=119690 Change 119690 by hselasky@hselasky_mini_itx on 2007/05/11 20:22:08 Resolve some locking issues in the USB network stack when passing packets up via "if_input". Affected files ... .. //depot/projects/usb/src/sys/dev/usb/if_aue.c#24 edit .. //depot/projects/usb/src/sys/dev/usb/if_axe.c#23 edit .. //depot/projects/usb/src/sys/dev/usb/if_cdce.c#16 edit .. //depot/projects/usb/src/sys/dev/usb/if_cue.c#19 edit .. //depot/projects/usb/src/sys/dev/usb/if_kue.c#21 edit .. //depot/projects/usb/src/sys/dev/usb/if_rue.c#20 edit .. //depot/projects/usb/src/sys/dev/usb/if_rum.c#3 edit .. //depot/projects/usb/src/sys/dev/usb/if_udav.c#20 edit .. //depot/projects/usb/src/sys/dev/usb/if_ural.c#26 edit .. //depot/projects/usb/src/sys/dev/usb/usb_port.h#14 edit Differences ... ==== //depot/projects/usb/src/sys/dev/usb/if_aue.c#24 (text+ko) ==== @@ -1079,7 +1079,7 @@ { struct aue_softc *sc = xfer->priv_sc; struct ifnet *ifp = sc->sc_ifp; - struct mbuf *m; + struct mbuf *m = NULL; USBD_CHECK_STATUS(xfer); @@ -1142,8 +1142,6 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len = xfer->actlen; - (ifp->if_input)(ifp, m); - tr_setup: if (sc->sc_flags & AUE_FLAG_READ_STALL) { @@ -1151,6 +1149,17 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); + (ifp->if_input)(ifp, m); + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_axe.c#23 (text+ko) ==== @@ -1059,6 +1059,11 @@ struct axe_sframe_hdr hdr; struct ifnet *ifp = sc->sc_ifp; struct mbuf *m; + struct { /* mini-queue */ + struct mbuf *ifq_head; + struct mbuf *ifq_tail; + uint16_t ifq_len; + } mq = { NULL, NULL, 0 }; uint16_t pos; uint16_t len; uint16_t adjust; @@ -1117,7 +1122,6 @@ } m = usbd_ether_get_mbuf(); - if (m == NULL) { /* we are out of memory */ break; @@ -1133,7 +1137,8 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len; - (ifp->if_input)(ifp, m); + /* enqueue */ + _IF_ENQUEUE(&(mq), m); skip: @@ -1159,6 +1164,27 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (mq.ifq_head) { + + mtx_unlock(&(sc->sc_mtx)); + + while (1) { + + _IF_DEQUEUE(&(mq), m); + + if (m == NULL) break; + + (ifp->if_input)(ifp, m); + } + + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_cdce.c#16 (text+ko) ==== @@ -717,7 +717,7 @@ { struct cdce_softc *sc = xfer->priv_sc; struct ifnet *ifp = sc->sc_ifp; - struct mbuf *m; + struct mbuf *m = NULL; USBD_CHECK_STATUS(xfer); @@ -761,8 +761,6 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len = xfer->actlen; - (ifp->if_input)(ifp, m); - tr_setup: if (sc->sc_flags & CDCE_FLAG_READ_STALL) { @@ -770,6 +768,17 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); + (ifp->if_input)(ifp, m); + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_cue.c#19 (text+ko) ==== @@ -650,7 +650,7 @@ { struct cue_softc *sc = xfer->priv_sc; struct ifnet *ifp = sc->sc_ifp; - struct mbuf *m; + struct mbuf *m = NULL; u_int8_t buf[2]; u_int16_t len; @@ -695,8 +695,6 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len = xfer->actlen; - (ifp->if_input)(ifp, m); - tr_setup: if (sc->sc_flags & CUE_FLAG_READ_STALL) { @@ -704,6 +702,17 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); + (ifp->if_input)(ifp, m); + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_kue.c#21 (text+ko) ==== @@ -690,7 +690,7 @@ { struct kue_softc *sc = xfer->priv_sc; struct ifnet *ifp = sc->sc_ifp; - struct mbuf *m; + struct mbuf *m = NULL; u_int8_t buf[2]; u_int16_t len; @@ -735,8 +735,6 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len = xfer->actlen; - (ifp->if_input)(ifp, m); - tr_setup: if (sc->sc_flags & KUE_FLAG_READ_STALL) { @@ -744,6 +742,17 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); + (ifp->if_input)(ifp, m); + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_rue.c#20 (text+ko) ==== @@ -968,7 +968,7 @@ struct rue_softc *sc = xfer->priv_sc; struct ifnet *ifp = sc->sc_ifp; u_int16_t status; - struct mbuf *m; + struct mbuf *m = NULL; USBD_CHECK_STATUS(xfer); @@ -1023,8 +1023,6 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len = xfer->actlen; - (ifp->if_input)(ifp, m); - tr_setup: if (sc->sc_flags & RUE_FLAG_READ_STALL) { @@ -1032,6 +1030,17 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); + (ifp->if_input)(ifp, m); + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_rum.c#3 (text+ko) ==== @@ -1009,11 +1009,11 @@ struct rum_softc *sc = xfer->priv_sc; struct ieee80211com *ic = &(sc->sc_ic); struct ifnet *ifp = ic->ic_ifp; - struct ieee80211_node *ni; + struct ieee80211_node *ni = NULL; struct mbuf *m = NULL; uint32_t flags; uint32_t max_len; - uint8_t rssi; + uint8_t rssi = 0; USBD_CHECK_STATUS(xfer); @@ -1074,6 +1074,8 @@ "descriptor, %u bytes, received %u bytes\n", m->m_len, max_len); ifp->if_ierrors++; + m_freem(m); + m = NULL; goto tr_setup; } @@ -1096,34 +1098,30 @@ ni = ieee80211_find_rxnode(ic, (void *)(m->m_data)); - mtx_unlock(&(sc->sc_mtx)); + tr_setup: + + if (sc->sc_flags & RUM_FLAG_READ_STALL) { + usbd_transfer_start(sc->sc_xfer[3]); + } else { + usbd_start_hardware(xfer); + } - /* XXX it is possibly not safe - * to do the following unlocked: - * --hps + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "ieee80211_input" here, and not + * some lines up! */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); - /* send the frame to the 802.11 layer */ - ieee80211_input(ic, m, ni, rssi, 0); + /* send the frame to the 802.11 layer */ + ieee80211_input(ic, m, ni, rssi, 0); - mtx_lock(&(sc->sc_mtx)); + mtx_lock(&(sc->sc_mtx)); - /* node is no longer needed */ - ieee80211_free_node(ni); - - m = NULL; - - tr_setup: - if (m) { - m_freem(m); + /* node is no longer needed */ + ieee80211_free_node(ni); } - - if (sc->sc_flags & RUM_FLAG_READ_STALL) { - usbd_transfer_start(sc->sc_xfer[3]); - return; - } - - usbd_start_hardware(xfer); return; } ==== //depot/projects/usb/src/sys/dev/usb/if_udav.c#20 (text+ko) ==== @@ -1009,7 +1009,7 @@ struct ifnet *ifp = sc->sc_ifp; u_int8_t status; u_int16_t total_len; - struct mbuf *m; + struct mbuf *m = NULL; USBD_CHECK_STATUS(xfer); @@ -1074,8 +1074,6 @@ m->m_pkthdr.rcvif = ifp; m->m_pkthdr.len = m->m_len = xfer->actlen; - (ifp->if_input)(ifp, m); - tr_setup: if (sc->sc_flags & UDAV_FLAG_READ_STALL) { @@ -1083,6 +1081,17 @@ } else { usbd_start_hardware(xfer); } + + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "if_input" here, and not + * some lines up! + */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); + (ifp->if_input)(ifp, m); + mtx_lock(&(sc->sc_mtx)); + } return; } ==== //depot/projects/usb/src/sys/dev/usb/if_ural.c#26 (text+ko) ==== @@ -1090,11 +1090,11 @@ struct ural_softc *sc = xfer->priv_sc; struct ieee80211com *ic = &(sc->sc_ic); struct ifnet *ifp = ic->ic_ifp; - struct ieee80211_node *ni; + struct ieee80211_node *ni = NULL; struct mbuf *m = NULL; u_int32_t flags; u_int32_t max_len; - uint8_t rssi; + uint8_t rssi = 0; USBD_CHECK_STATUS(xfer); @@ -1142,6 +1142,8 @@ */ DPRINTF(sc, 5, "PHY or CRC error\n"); ifp->if_ierrors++; + m_freem(m); + m = NULL; goto tr_setup; } @@ -1155,6 +1157,8 @@ "descriptor, %u bytes, received %u bytes\n", m->m_len, max_len); ifp->if_ierrors++; + m_freem(m); + m = NULL; goto tr_setup; } @@ -1177,35 +1181,30 @@ ni = ieee80211_find_rxnode(ic, (struct ieee80211_frame_min *) (m->m_data)); + tr_setup: - mtx_unlock(&(sc->sc_mtx)); + if (sc->sc_flags & URAL_FLAG_READ_STALL) { + usbd_transfer_start(sc->sc_xfer[3]); + } else { + usbd_start_hardware(xfer); + } - /* XXX it is possibly not safe - * to do the following unlocked: - * --hps + /* At the end of a USB callback it is always safe + * to unlock the private mutex of a device! That + * is why we do the "ieee80211_input" here, and not + * some lines up! */ + if (m) { + mtx_unlock(&(sc->sc_mtx)); - /* send the frame to the 802.11 layer */ - ieee80211_input(ic, m, ni, rssi, 0); + /* send the frame to the 802.11 layer */ + ieee80211_input(ic, m, ni, rssi, 0); - mtx_lock(&(sc->sc_mtx)); + mtx_lock(&(sc->sc_mtx)); - /* node is no longer needed */ - ieee80211_free_node(ni); - - m = NULL; - - tr_setup: - if (m) { - m_freem(m); + /* node is no longer needed */ + ieee80211_free_node(ni); } - - if (sc->sc_flags & URAL_FLAG_READ_STALL) { - usbd_transfer_start(sc->sc_xfer[3]); - return; - } - - usbd_start_hardware(xfer); return; } ==== //depot/projects/usb/src/sys/dev/usb/usb_port.h#14 (text+ko) ==== @@ -111,7 +111,6 @@ #endif #define Ether_ifattach ether_ifattach -#define IF_INPUT(ifp, m) (*(ifp)->if_input)((ifp), (m)) #elif defined(__OpenBSD__) /* @@ -167,13 +166,6 @@ #define Ether_ifattach(ifp, eaddr) ether_ifattach(ifp) #define if_deactivate(x) -#define IF_INPUT(ifp, m) do { \ - struct ether_header *eh; \ - \ - eh = mtod(m, struct ether_header *); \ - m_adj(m, sizeof(struct ether_header)); \ - ether_input((ifp), (eh), (m)); \ -} while (0) #define powerhook_establish(fn, sc) (fn) #define powerhook_disestablish(hdl)