From 225f9723dcff1436770fff4b8955fb7252f2964c Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Mon, 4 Aug 2008 21:14:24 +0000 Subject: [PATCH] Lock the consumers of the iicbus(4) infrastructure: - ad7418(4) uses an sx lock instead of a mtx since the iicbus(4) stuff it calls can sleep (request_bus()). Also, I expanded the locking slightly to serialize writes to data stored in the softc. - Similarly, the icee(4) driver now uses an sx lock instead of a mutex. I also removed the pointless OPENED flag and flags field from the softc. - The locking for the ic(4) driver was a bit trickier: - Add a mutex to the softc to protect softc data. - The driver uses malloc'd buffers that are the size of the interface MTU to send and receive packets. Previously, these were allocated every time the interface was brought up and anytime the MTU was changed, with various races that could result in memory leaks. I changed this to be a bit simpler and more like other NIC drivers in that we allocate buffers during attach for the default MTU size and only reallocate them on MTU changes. The reallocation procedure goes to some lengths with various flags to not replace either the the receive or transmit buffers while the driver is busy receiving or transmitting a packet. - Store the device_t of the driver in the softc instead of detours into new-bus using if_dunit from the ifnet and an even more bizarre detour to get the softc instead of using if_softc. - Drop the driver mutex when invoking netisr_dispatch() to pass the packet up to IP. - Use if_printf(). --- sys/dev/iicbus/ad7418.c | 20 +++--- sys/dev/iicbus/icee.c | 32 +++------ sys/dev/iicbus/if_ic.c | 156 ++++++++++++++++++++++------------------ 3 files changed, 105 insertions(+), 103 deletions(-) diff --git a/sys/dev/iicbus/ad7418.c b/sys/dev/iicbus/ad7418.c index ec68fc19743..b7e99d77c89 100644 --- a/sys/dev/iicbus/ad7418.c +++ b/sys/dev/iicbus/ad7418.c @@ -32,11 +32,11 @@ __FBSDID("$FreeBSD$"); #include #include #include -#include #include #include #include #include +#include #include #include @@ -66,7 +66,7 @@ __FBSDID("$FreeBSD$"); struct ad7418_softc { device_t sc_dev; - struct mtx sc_mtx; + struct sx sc_lock; int sc_curchan; /* current channel */ int sc_curtemp; int sc_curvolt; @@ -91,8 +91,10 @@ ad7418_sysctl_temp(SYSCTL_HANDLER_ARGS) struct ad7418_softc *sc = arg1; int temp; + sx_xlock(&sc->sc_lock); ad7418_update(sc); temp = (sc->sc_curtemp / 64) * 25; + sx_xunlock(&sc->sc_lock); return sysctl_handle_int(oidp, &temp, 0, req); } @@ -102,8 +104,10 @@ ad7418_sysctl_voltage(SYSCTL_HANDLER_ARGS) struct ad7418_softc *sc = arg1; int volt; + sx_xlock(&sc->sc_lock); ad7418_update(sc); volt = (sc->sc_curvolt >> 6) * 564 / 10; + sx_xunlock(&sc->sc_lock); return sysctl_handle_int(oidp, &volt, 0, req); } @@ -116,7 +120,7 @@ ad7418_attach(device_t dev) int conf; sc->sc_dev = dev; - mtx_init(&sc->sc_mtx, "ad7418", "ad7418", MTX_DEF); + sx_init(&sc->sc_mtx, "ad7418"); SYSCTL_ADD_PROC(ctx, SYSCTL_CHILDREN(tree), OID_AUTO, "temp", CTLTYPE_INT | CTLFLAG_RD, sc, 0, @@ -170,13 +174,10 @@ ad7418_set_channel(struct ad7418_softc *sc, int chan) /* * NB: Linux driver delays here but chip data sheet * says nothing and things appear to work fine w/o - * a delay on channel change. If this is enabled - * be sure to account for losing the mutex below - * in ad7418_update. + * a delay on channel change. */ - mtx_assert(&sc->sc_mtx, MA_OWNED); /* let channel change settle, 1 tick should be 'nuf (need ~1ms) */ - msleep(sc, &sc->sc_mtx, 0, "ad7418", 1); + tsleep(sc, 0, "ad7418", hz/1000); #endif } @@ -199,7 +200,7 @@ ad7418_update(struct ad7418_softc *sc) { int v; - mtx_lock(&sc->sc_mtx); + sx_assert(&sc->sc_lock, SA_XLOCKED); /* NB: no point in updating any faster than the chip */ if (ticks - sc->sc_lastupdate > hz) { ad7418_set_channel(sc, AD7418_CHAN_TEMP); @@ -212,7 +213,6 @@ ad7418_update(struct ad7418_softc *sc) sc->sc_curvolt = v; sc->sc_lastupdate = ticks; } - mtx_unlock(&sc->sc_mtx); } static device_method_t ad7418_methods[] = { diff --git a/sys/dev/iicbus/icee.c b/sys/dev/iicbus/icee.c index 03de150702b..8e408d86daa 100644 --- a/sys/dev/iicbus/icee.c +++ b/sys/dev/iicbus/icee.c @@ -33,8 +33,8 @@ __FBSDID("$FreeBSD$"); #include #include #include -#include #include +#include #include #include #include @@ -48,24 +48,21 @@ __FBSDID("$FreeBSD$"); struct icee_softc { device_t sc_dev; /* Myself */ - struct mtx sc_mtx; /* basically a perimeter lock */ + struct sx sc_lock; /* basically a perimeter lock */ struct cdev *cdev; /* user interface */ int addr; - int flags; -#define OPENED 1 int size; /* How big am I? */ int type; /* What type 8 or 16 bit? */ int rd_sz; /* What's the read page size */ int wr_sz; /* What's the write page size */ }; -#define ICEE_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx) -#define ICEE_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx) -#define ICEE_LOCK_INIT(_sc) \ - mtx_init(&_sc->sc_mtx, device_get_nameunit(_sc->sc_dev), "icee", MTX_DEF) -#define ICEE_LOCK_DESTROY(_sc) mtx_destroy(&_sc->sc_mtx); -#define ICEE_ASSERT_LOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_OWNED); -#define ICEE_ASSERT_UNLOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_NOTOWNED); +#define ICEE_LOCK(_sc) sx_xlock(&(_sc)->sc_lock) +#define ICEE_UNLOCK(_sc) sx_xunlock(&(_sc)->sc_lock) +#define ICEE_LOCK_INIT(_sc) sx_init(&_sc->sc_lock, "icee") +#define ICEE_LOCK_DESTROY(_sc) sx_destroy(&_sc->sc_lock); +#define ICEE_ASSERT_LOCKED(_sc) sx_assert(&_sc->sc_lock, SA_XLOCKED); +#define ICEE_ASSERT_UNLOCKED(_sc) sx_assert(&_sc->sc_lock, SA_UNLOCKED); #define CDEV2SOFTC(dev) ((dev)->si_drv1) /* cdev routines */ @@ -77,6 +74,7 @@ static d_write_t icee_write; static struct cdevsw icee_cdevsw = { .d_version = D_VERSION, + .d_flags = D_TRACKCLOSE, .d_open = icee_open, .d_close = icee_close, .d_read = icee_read, @@ -127,26 +125,14 @@ out:; static int icee_open(struct cdev *dev, int oflags, int devtype, struct thread *td) { - struct icee_softc *sc; - sc = CDEV2SOFTC(dev); - ICEE_LOCK(sc); - if (!(sc->flags & OPENED)) { - sc->flags |= OPENED; - } - ICEE_UNLOCK(sc); return (0); } static int icee_close(struct cdev *dev, int fflag, int devtype, struct thread *td) { - struct icee_softc *sc; - sc = CDEV2SOFTC(dev); - ICEE_LOCK(sc); - sc->flags &= ~OPENED; - ICEE_UNLOCK(sc); return (0); } diff --git a/sys/dev/iicbus/if_ic.c b/sys/dev/iicbus/if_ic.c index beca6019f0a..1c48f711452 100644 --- a/sys/dev/iicbus/if_ic.c +++ b/sys/dev/iicbus/if_ic.c @@ -38,7 +38,9 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include #include +#include #include #include #include @@ -71,10 +73,11 @@ __FBSDID("$FreeBSD$"); struct ic_softc { struct ifnet *ic_ifp; + device_t ic_dev; u_char ic_addr; /* peer I2C address */ - int ic_sending; + int ic_flags; char *ic_obuf; char *ic_ifbuf; @@ -83,8 +86,16 @@ struct ic_softc { int ic_xfercnt; int ic_iferrs; + + struct mtx ic_lock; }; +#define IC_SENDING 0x0001 +#define IC_OBUF_BUSY 0x0002 +#define IC_IFBUF_BUSY 0x0004 +#define IC_BUFFERS_BUSY (IC_OBUF_BUSY | IC_IFBUF_BUSY) +#define IC_BUFFER_WAITER 0x0004 + static devclass_t ic_devclass; static int icprobe(device_t); @@ -113,6 +124,29 @@ static driver_t ic_driver = { sizeof(struct ic_softc), }; +static void +ic_alloc_buffers(struct ic_softc *sc, int mtu) +{ + char *obuf, *ifbuf; + + obuf = malloc(mtu + ICHDRLEN, M_DEVBUF, M_WAITOK); + ifbuf = malloc(mtu + ICHDRLEN, M_DEVBUF, M_WAITOK); + + mtx_lock(&sc->ic_lock); + while (sc->ic_flags & IC_BUFFERS_BUSY) { + sc->ic_flags |= IC_BUFFER_WAITER; + mtx_sleep(sc, &sc->ic_lock, 0, "icalloc", 0); + sc->ic_flags &= ~IC_BUFFER_WAITER; + } + + free(sc->ic_obuf, M_DEVBUF); + free(sc->ic_ifbuf, M_DEVBUF); + sc->ic_obuf = obuf; + sc->ic_ifbuf = ifbuf; + sc->ic_ifp->if_mtu = mtu; + mtx_unlock(&sc->ic_lock); +} + /* * icprobe() */ @@ -121,7 +155,7 @@ icprobe(device_t dev) { return (0); } - + /* * icattach() */ @@ -135,19 +169,22 @@ icattach(device_t dev) if (ifp == NULL) return (ENOSPC); + mtx_init(&sc->ic_lock, device_get_nameunit(dev), MTX_NETWORK_LOCK, + MTX_DEF); sc->ic_addr = PCF_MASTER_ADDRESS; /* XXX only PCF masters */ + sc->ic_dev = dev; ifp->if_softc = sc; if_initname(ifp, device_get_name(dev), device_get_unit(dev)); - ifp->if_mtu = ICMTU; - ifp->if_flags = IFF_SIMPLEX | IFF_POINTOPOINT | IFF_MULTICAST | - IFF_NEEDSGIANT; + ifp->if_flags = IFF_SIMPLEX | IFF_POINTOPOINT | IFF_MULTICAST; ifp->if_ioctl = icioctl; ifp->if_output = icoutput; ifp->if_hdrlen = 0; ifp->if_addrlen = 0; ifp->if_snd.ifq_maxlen = IFQ_MAXLEN; + ic_alloc_buffers(sc, ICMTU); + if_attach(ifp); bpfattach(ifp, DLT_NULL, ICHDRLEN); @@ -161,14 +198,11 @@ icattach(device_t dev) static int icioctl(struct ifnet *ifp, u_long cmd, caddr_t data) { - device_t icdev = devclass_get_device(ic_devclass, ifp->if_dunit); + struct ic_softc *sc = ifp->if_softc; + device_t icdev = sc->ic_dev; device_t parent = device_get_parent(icdev); - struct ic_softc *sc = (struct ic_softc *)device_get_softc(icdev); - struct ifaddr *ifa = (struct ifaddr *)data; struct ifreq *ifr = (struct ifreq *)data; - - u_char *iptr, *optr; int error; switch (cmd) { @@ -178,14 +212,18 @@ icioctl(struct ifnet *ifp, u_long cmd, caddr_t data) case SIOCSIFADDR: if (ifa->ifa_addr->sa_family != AF_INET) return (EAFNOSUPPORT); + mtx_lock(&sc->ic_lock); ifp->if_flags |= IFF_UP; - /* FALLTHROUGH */ + goto locked; case SIOCSIFFLAGS: + mtx_lock(&sc->ic_lock); + locked: if ((!(ifp->if_flags & IFF_UP)) && (ifp->if_drv_flags & IFF_DRV_RUNNING)) { /* XXX disable PCF */ ifp->if_drv_flags &= ~IFF_DRV_RUNNING; + mtx_unlock(&sc->ic_lock); /* IFF_UP is not set, try to release the bus anyway */ iicbus_release_bus(parent, icdev); @@ -193,57 +231,25 @@ icioctl(struct ifnet *ifp, u_long cmd, caddr_t data) } if (((ifp->if_flags & IFF_UP)) && (!(ifp->if_drv_flags & IFF_DRV_RUNNING))) { + mtx_unlock(&sc->ic_lock); if ((error = iicbus_request_bus(parent, icdev, IIC_WAIT | IIC_INTR))) return (error); - sc->ic_obuf = malloc(sc->ic_ifp->if_mtu + ICHDRLEN, - M_DEVBUF, M_WAITOK); - if (!sc->ic_obuf) { - iicbus_release_bus(parent, icdev); - return (ENOBUFS); - } - sc->ic_ifbuf = malloc(sc->ic_ifp->if_mtu + ICHDRLEN, - M_DEVBUF, M_WAITOK); - if (!sc->ic_ifbuf) { - iicbus_release_bus(parent, icdev); - return (ENOBUFS); - } + mtx_lock(&sc->ic_lock); iicbus_reset(parent, IIC_FASTEST, 0, NULL); ifp->if_drv_flags |= IFF_DRV_RUNNING; } + mtx_unlock(&sc->ic_lock); break; case SIOCSIFMTU: - /* save previous buffers */ - iptr = sc->ic_ifbuf; - optr = sc->ic_obuf; - - /* allocate input buffer */ - sc->ic_ifbuf = malloc(ifr->ifr_mtu+ICHDRLEN, M_DEVBUF, M_NOWAIT); - if (!sc->ic_ifbuf) { - sc->ic_ifbuf = iptr; - sc->ic_obuf = optr; - return (ENOBUFS); - } - - /* allocate output buffer */ - sc->ic_ifbuf = malloc(ifr->ifr_mtu+ICHDRLEN, M_DEVBUF, M_NOWAIT); - if (!sc->ic_obuf) { - free(sc->ic_ifbuf,M_DEVBUF); - sc->ic_ifbuf = iptr; - sc->ic_obuf = optr; - return (ENOBUFS); - } - - if (iptr) - free(iptr,M_DEVBUF); - if (optr) - free(optr,M_DEVBUF); - sc->ic_ifp->if_mtu = ifr->ifr_mtu; + ic_alloc_buffers(sc, ifr->ifr_mtu); break; case SIOCGIFMTU: + mtx_lock(&sc->ic_lock); ifr->ifr_mtu = sc->ic_ifp->if_mtu; + mtx_unlock(&sc->ic_lock); break; case SIOCADDMULTI: @@ -270,11 +276,10 @@ static void icintr(device_t dev, int event, char *ptr) { struct ic_softc *sc = (struct ic_softc *)device_get_softc(dev); - int unit = device_get_unit(dev); - int s, len; struct mbuf *top; - - s = splhigh(); + int len; + + mtx_lock(&sc->ic_lock); switch (event) { @@ -282,12 +287,17 @@ icintr(device_t dev, int event, char *ptr) case INTR_START: sc->ic_cp = sc->ic_ifbuf; sc->ic_xfercnt = 0; + sc->ic_flags |= IC_IFBUF_BUSY; break; case INTR_STOP: /* if any error occured during transfert, * drop the packet */ + sc->ic_flags &= ~IC_IFBUF_BUSY; + if ((sc->ic_flags & (IC_BUFFERS_BUSY | IC_BUFFER_WAITER)) == + IC_BUFFER_WAITER) + wakeup(&sc); if (sc->ic_iferrs) goto err; if ((len = sc->ic_xfercnt) == 0) @@ -299,17 +309,20 @@ icintr(device_t dev, int event, char *ptr) sc->ic_ifp->if_ibytes += len; BPF_TAP(sc->ic_ifp, sc->ic_ifbuf, len + ICHDRLEN); top = m_devget(sc->ic_ifbuf + ICHDRLEN, len, 0, sc->ic_ifp, 0); - if (top) + if (top) { + mtx_unlock(&sc->ic_lock); netisr_dispatch(NETISR_IP, top); + mtx_lock(&sc->ic_lock); + } break; err: - printf("ic%d: errors (%d)!\n", unit, sc->ic_iferrs); + if_printf(sc->ic_ifp, "errors (%d)!\n", sc->ic_iferrs); sc->ic_iferrs = 0; /* reset error count */ sc->ic_ifp->if_ierrors++; break; case INTR_RECEIVE: - if (sc->ic_xfercnt >= sc->ic_ifp->if_mtu+ICHDRLEN) { + if (sc->ic_xfercnt >= sc->ic_ifp->if_mtu + ICHDRLEN) { sc->ic_iferrs++; } else { *sc->ic_cp++ = *ptr; @@ -332,7 +345,7 @@ icintr(device_t dev, int event, char *ptr) panic("%s: unknown event (%d)!", __func__, event); } - splx(s); + mtx_unlock(&sc->ic_lock); return; } @@ -343,11 +356,10 @@ static int icoutput(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst, struct rtentry *rt) { - device_t icdev = devclass_get_device(ic_devclass, ifp->if_dunit); + struct ic_softc *sc = ifp->if_softc; + device_t icdev = sc->ic_dev; device_t parent = device_get_parent(icdev); - struct ic_softc *sc = (struct ic_softc *)device_get_softc(icdev); - - int s, len, sent; + int len, sent; struct mbuf *mm; u_char *cp; u_int32_t hdr; @@ -358,12 +370,11 @@ icoutput(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst, else hdr = dst->sa_family; + mtx_lock(&sc->ic_lock); ifp->if_drv_flags |= IFF_DRV_RUNNING; - s = splhigh(); - /* already sending? */ - if (sc->ic_sending) { + if (sc->ic_flags & IC_SENDING) { ifp->if_oerrors++; goto error; } @@ -376,7 +387,7 @@ icoutput(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst, mm = m; do { if (len + mm->m_len > sc->ic_ifp->if_mtu) { - /* packet to large */ + /* packet too large */ ifp->if_oerrors++; goto error; } @@ -389,10 +400,10 @@ icoutput(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst, BPF_MTAP2(ifp, &hdr, sizeof(hdr), m); - sc->ic_sending = 1; + sc->ic_flags |= (IC_SENDING | IC_OBUF_BUSY); m_freem(m); - splx(s); + mtx_unlock(&sc->ic_lock); /* send the packet */ if (iicbus_block_write(parent, sc->ic_addr, sc->ic_obuf, @@ -402,15 +413,20 @@ icoutput(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst, else { ifp->if_opackets++; ifp->if_obytes += len; - } + } - sc->ic_sending = 0; + mtx_lock(&sc->ic_lock); + sc->ic_flags &= ~(IC_SENDING | IC_OBUF_BUSY); + if ((sc->ic_flags & (IC_BUFFERS_BUSY | IC_BUFFER_WAITER)) == + IC_BUFFER_WAITER) + wakeup(&sc); + mtx_unlock(&sc->ic_lock); return (0); error: m_freem(m); - splx(s); + mtx_unlock(&sc->ic_lock); return(0); }