PERFORCE change 119690 for review
Hans Petter Selasky
hselasky at FreeBSD.org
Fri May 11 20:22:18 UTC 2007
http://perforce.freebsd.org/chv.cgi?CH=119690
Change 119690 by hselasky at 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)
More information about the p4-projects
mailing list