From owner-svn-src-head@FreeBSD.ORG Thu Nov 19 19:25:48 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 608BB106566B; Thu, 19 Nov 2009 19:25:48 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id 4E7D78FC13; Thu, 19 Nov 2009 19:25:48 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id nAJJPmAO016379; Thu, 19 Nov 2009 19:25:48 GMT (envelope-from jhb@svn.freebsd.org) Received: (from jhb@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id nAJJPmIZ016374; Thu, 19 Nov 2009 19:25:48 GMT (envelope-from jhb@svn.freebsd.org) Message-Id: <200911191925.nAJJPmIZ016374@svn.freebsd.org> From: John Baldwin Date: Thu, 19 Nov 2009 19:25:48 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r199542 - head/sys/dev/pdq X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Nov 2009 19:25:48 -0000 Author: jhb Date: Thu Nov 19 19:25:47 2009 New Revision: 199542 URL: http://svn.freebsd.org/changeset/base/199542 Log: Several fixes to these drivers. Note that these two drivers are actually just two different attachments (EISA and PCI) to a single driver. - Add real locking. Previously these drivers only acquired their lock in their interrupt handler or in the ioctl routine (but too broadly in the latter). No locking was used for the stack calling down into the driver via if_init() or if_start(), for device shutdown or detach. Also, the interrupt handler held the driver lock while calling if_input(). All this stuff should be fixed in the locking changes. - Really fix these drivers to handle if_alloc(). The front-end attachments were using if_initname() before the ifnet was allocated. Fix this by moving some of the duplicated logic from each driver into pdq_ifattach(). While here, make pdq_ifattach() return an error so that the driver just fails to attach if if_alloc() fails rather than panic'ing. Also, defer freeing the ifnet until the driver has stopped using it during detach. - Add a new private timer to drive the watchdog timer. - Pass the softc pointer to the interrupt handlers instead of the device_t so we can avoid the use of device_get_softc() and to better match what other drivers do. Modified: head/sys/dev/pdq/if_fea.c head/sys/dev/pdq/if_fpa.c head/sys/dev/pdq/pdq_freebsd.h head/sys/dev/pdq/pdq_ifsubr.c Modified: head/sys/dev/pdq/if_fea.c ============================================================================== --- head/sys/dev/pdq/if_fea.c Thu Nov 19 18:56:06 2009 (r199541) +++ head/sys/dev/pdq/if_fea.c Thu Nov 19 19:25:47 2009 (r199542) @@ -163,11 +163,9 @@ static void pdq_eisa_ifintr(arg) void * arg; { - device_t dev; pdq_softc_t * sc; - dev = (device_t)arg; - sc = device_get_softc(dev); + sc = arg; PDQ_LOCK(sc); (void) pdq_interrupt(sc->sc_pdq); @@ -181,11 +179,9 @@ pdq_eisa_attach (dev) device_t dev; { pdq_softc_t * sc; - struct ifnet * ifp; int error; sc = device_get_softc(dev); - ifp = sc->ifp; sc->dev = dev; @@ -222,28 +218,20 @@ pdq_eisa_attach (dev) goto bad; } - if_initname(ifp, device_get_name(dev), device_get_unit(dev)); - pdq_eisa_devinit(sc); - sc->sc_pdq = pdq_initialize(sc->mem_bst, sc->mem_bsh, - ifp->if_xname, -1, - (void *)sc, PDQ_DEFEA); - if (sc->sc_pdq == NULL) { - device_printf(dev, "Initialization failed.\n"); - error = ENXIO; + error = pdq_ifattach(sc, sc->sc_pdq->pdq_hwaddr.lanaddr_bytes, + PDQ_DEFEA); + if (error) goto bad; - } - error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET, - NULL, pdq_eisa_ifintr, dev, &sc->irq_ih); + error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET | INTR_MPSAFE, + NULL, pdq_eisa_ifintr, sc, &sc->irq_ih); if (error) { device_printf(dev, "Failed to setup interrupt handler.\n"); - error = ENXIO; - goto bad; + pdq_ifdetach(sc); + return (error); } - pdq_ifattach(sc, sc->sc_pdq->pdq_hwaddr.lanaddr_bytes); - return (0); bad: pdq_free(dev); @@ -269,7 +257,9 @@ pdq_eisa_shutdown(dev) pdq_softc_t * sc; sc = device_get_softc(dev); + PDQ_LOCK(sc); pdq_hwreset(sc->sc_pdq); + PDQ_UNLOCK(sc); return (0); } Modified: head/sys/dev/pdq/if_fpa.c ============================================================================== --- head/sys/dev/pdq/if_fpa.c Thu Nov 19 18:56:06 2009 (r199541) +++ head/sys/dev/pdq/if_fpa.c Thu Nov 19 19:25:47 2009 (r199542) @@ -73,11 +73,9 @@ static void pdq_pci_ifintr (void *); static void pdq_pci_ifintr(void *arg) { - device_t dev; pdq_softc_t *sc; - dev = (device_t)arg; - sc = device_get_softc(dev); + sc = arg; PDQ_LOCK(sc); (void) pdq_interrupt(sc->sc_pdq); @@ -105,12 +103,10 @@ static int pdq_pci_attach(device_t dev) { pdq_softc_t *sc; - struct ifnet *ifp; u_int32_t command; int error; sc = device_get_softc(dev); - ifp = sc->ifp; sc->dev = dev; @@ -146,26 +142,18 @@ pdq_pci_attach(device_t dev) goto bad; } - if_initname(ifp, device_get_name(dev), device_get_unit(dev)); - - sc->sc_pdq = pdq_initialize(sc->mem_bst, sc->mem_bsh, - ifp->if_xname, -1, - (void *)sc, PDQ_DEFPA); - if (sc->sc_pdq == NULL) { - device_printf(dev, "Initialization failed.\n"); - error = ENXIO; + error = pdq_ifattach(sc, sc->sc_pdq->pdq_hwaddr.lanaddr_bytes, PDQ_DEFPA); + if (error) goto bad; - } - - error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET, NULL, - pdq_pci_ifintr, dev, &sc->irq_ih); + + error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET | INTR_MPSAFE, NULL, + pdq_pci_ifintr, sc, &sc->irq_ih); if (error) { device_printf(dev, "Failed to setup interrupt handler.\n"); - error = ENXIO; - goto bad; + pdq_ifdetach(sc); + return (error); } - pdq_ifattach(sc, sc->sc_pdq->pdq_hwaddr.lanaddr_bytes); return (0); bad: @@ -191,7 +179,9 @@ pdq_pci_shutdown(device_t dev) pdq_softc_t *sc; sc = device_get_softc(dev); + PDQ_LOCK(sc); pdq_hwreset(sc->sc_pdq); + PDQ_UNLOCK(sc); return (0); } Modified: head/sys/dev/pdq/pdq_freebsd.h ============================================================================== --- head/sys/dev/pdq/pdq_freebsd.h Thu Nov 19 18:56:06 2009 (r199541) +++ head/sys/dev/pdq/pdq_freebsd.h Thu Nov 19 19:25:47 2009 (r199542) @@ -124,10 +124,13 @@ typedef struct _pdq_os_ctx_t { void * irq_ih; struct mtx mtx; + struct callout watchdog; + int timer; } pdq_softc_t; #define PDQ_LOCK(_sc) mtx_lock(&(_sc)->mtx) #define PDQ_UNLOCK(_sc) mtx_unlock(&(_sc)->mtx) +#define PDQ_LOCK_ASSERT(_sc) mtx_assert(&(_sc)->mtx, MA_OWNED) #define PDQ_OS_HDR_OFFSET PDQ_RX_FC_OFFSET @@ -255,7 +258,8 @@ pdq_state_t pdq_stop (pdq_t *pdq); * OS dependent functions provided by * pdq_ifsubr.c or pdq.c to the bus front ends */ -void pdq_ifattach (pdq_softc_t *, const pdq_uint8_t *); +int pdq_ifattach (pdq_softc_t *, const pdq_uint8_t *, + pdq_type_t type); void pdq_ifdetach (pdq_softc_t *); void pdq_free (device_t); int pdq_interrupt (pdq_t *pdq); Modified: head/sys/dev/pdq/pdq_ifsubr.c ============================================================================== --- head/sys/dev/pdq/pdq_ifsubr.c Thu Nov 19 18:56:06 2009 (r199541) +++ head/sys/dev/pdq/pdq_ifsubr.c Thu Nov 19 19:25:47 2009 (r199542) @@ -69,10 +69,23 @@ __FBSDID("$FreeBSD$"); devclass_t pdq_devclass; +static void pdq_watchdog(void *); + +static void +pdq_ifstop(pdq_softc_t *sc) +{ + + PDQ_IFNET(sc)->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); + sc->sc_pdq->pdq_flags &= ~PDQ_RUNNING; + pdq_stop(sc->sc_pdq); + callout_stop(&sc->watchdog); +} + static void -pdq_ifinit( - pdq_softc_t *sc) +pdq_ifinit_locked(pdq_softc_t *sc) { + + PDQ_LOCK_ASSERT(sc); if (PDQ_IFNET(sc)->if_flags & IFF_UP) { PDQ_IFNET(sc)->if_drv_flags |= IFF_DRV_RUNNING; if (PDQ_IFNET(sc)->if_flags & IFF_PROMISC) { @@ -87,24 +100,40 @@ pdq_ifinit( } sc->sc_pdq->pdq_flags |= PDQ_RUNNING; pdq_run(sc->sc_pdq); - } else { - PDQ_IFNET(sc)->if_drv_flags &= ~IFF_DRV_RUNNING; - sc->sc_pdq->pdq_flags &= ~PDQ_RUNNING; - pdq_stop(sc->sc_pdq); - } + callout_reset(&sc->watchdog, hz, pdq_watchdog, sc); + } else + pdq_ifstop(sc); +} + +static void +pdq_ifinit(void *arg) +{ + pdq_softc_t *sc; + + sc = arg; + PDQ_LOCK(sc); + pdq_ifinit_locked(sc); + PDQ_UNLOCK(sc); } static void -pdq_ifwatchdog( - struct ifnet *ifp) +pdq_watchdog(void *arg) { + pdq_softc_t *sc; + struct ifnet *ifp; + + sc = arg; + PDQ_LOCK_ASSERT(sc); + callout_reset(&sc->watchdog, hz, pdq_watchdog, sc); + if (sc->timer == 0 || --sc->timer > 0) + return; + /* * No progress was made on the transmit queue for PDQ_OS_TX_TRANSMIT * seconds. Remove all queued packets. */ - + ifp = PDQ_IFNET(sc); ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; - ifp->if_timer = 0; for (;;) { struct mbuf *m; IFQ_DEQUEUE(&ifp->if_snd, m); @@ -115,18 +144,18 @@ pdq_ifwatchdog( } static void -pdq_ifstart( - struct ifnet *ifp) +pdq_ifstart_locked(struct ifnet *ifp) { pdq_softc_t * const sc = PDQ_OS_IFP_TO_SOFTC(ifp); struct mbuf *m; int tx = 0; + PDQ_LOCK_ASSERT(sc); if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) return; - if (PDQ_IFNET(sc)->if_timer == 0) - PDQ_IFNET(sc)->if_timer = PDQ_OS_TX_TIMEOUT; + if (sc->timer == 0) + sc->timer = PDQ_OS_TX_TIMEOUT; if ((sc->sc_pdq->pdq_flags & PDQ_TXOK) == 0) { PDQ_IFNET(sc)->if_drv_flags |= IFF_DRV_OACTIVE; @@ -177,6 +206,16 @@ pdq_ifstart( PDQ_DO_TYPE2_PRODUCER(sc->sc_pdq); sc->sc_flags &= ~PDQIF_DOWNCALL; } + +static void +pdq_ifstart(struct ifnet *ifp) +{ + pdq_softc_t * const sc = PDQ_OS_IFP_TO_SOFTC(ifp); + + PDQ_LOCK(sc); + pdq_ifstart_locked(ifp); + PDQ_UNLOCK(sc); +} void pdq_os_receive_pdu( @@ -218,7 +257,9 @@ pdq_os_receive_pdu( } m->m_pkthdr.rcvif = ifp; + PDQ_UNLOCK(sc); (*ifp->if_input)(ifp, m); + PDQ_LOCK(sc); } void @@ -228,11 +269,11 @@ pdq_os_restart_transmitter( pdq_softc_t *sc = pdq->pdq_os_ctx; PDQ_IFNET(sc)->if_drv_flags &= ~IFF_DRV_OACTIVE; if (IFQ_IS_EMPTY(&PDQ_IFNET(sc)->if_snd) == 0) { - PDQ_IFNET(sc)->if_timer = PDQ_OS_TX_TIMEOUT; + sc->timer = PDQ_OS_TX_TIMEOUT; if ((sc->sc_flags & PDQIF_DOWNCALL) == 0) - pdq_ifstart(PDQ_IFNET(sc)); + pdq_ifstart_locked(PDQ_IFNET(sc)); } else { - PDQ_IFNET(sc)->if_timer = 0; + sc->timer = 0; } } @@ -305,6 +346,7 @@ pdq_ifmedia_change( { pdq_softc_t * const sc = PDQ_OS_IFP_TO_SOFTC(ifp); + PDQ_LOCK(sc); if (sc->sc_ifmedia.ifm_media & IFM_FDX) { if ((sc->sc_pdq->pdq_flags & PDQ_WANT_FDX) == 0) { sc->sc_pdq->pdq_flags |= PDQ_WANT_FDX; @@ -316,6 +358,7 @@ pdq_ifmedia_change( if (sc->sc_pdq->pdq_flags & PDQ_RUNNING) pdq_run(sc->sc_pdq); } + PDQ_UNLOCK(sc); return 0; } @@ -327,6 +370,7 @@ pdq_ifmedia_status( { pdq_softc_t * const sc = PDQ_OS_IFP_TO_SOFTC(ifp); + PDQ_LOCK(sc); ifmr->ifm_status = IFM_AVALID; if (sc->sc_pdq->pdq_flags & PDQ_IS_ONRING) ifmr->ifm_status |= IFM_ACTIVE; @@ -334,6 +378,7 @@ pdq_ifmedia_status( ifmr->ifm_active = (ifmr->ifm_current & ~IFM_FDX); if (sc->sc_pdq->pdq_flags & PDQ_IS_FDX) ifmr->ifm_active |= IFM_FDX; + PDQ_UNLOCK(sc); } void @@ -369,8 +414,6 @@ pdq_ifioctl( pdq_softc_t *sc = PDQ_OS_IFP_TO_SOFTC(ifp); int error = 0; - PDQ_LOCK(sc); - switch (cmd) { case SIOCSIFFLAGS: { pdq_ifinit(sc); @@ -379,10 +422,12 @@ pdq_ifioctl( case SIOCADDMULTI: case SIOCDELMULTI: { + PDQ_LOCK(sc); if (PDQ_IFNET(sc)->if_drv_flags & IFF_DRV_RUNNING) { pdq_run(sc->sc_pdq); error = 0; } + PDQ_UNLOCK(sc); break; } @@ -401,7 +446,6 @@ pdq_ifioctl( } } - PDQ_UNLOCK(sc); return error; } @@ -409,25 +453,27 @@ pdq_ifioctl( #define IFF_NOTRAILERS 0 #endif -void -pdq_ifattach(pdq_softc_t *sc, const pdq_uint8_t *llc) +int +pdq_ifattach(pdq_softc_t *sc, const pdq_uint8_t *llc, pdq_type_t type) { struct ifnet *ifp; ifp = PDQ_IFNET(sc) = if_alloc(IFT_FDDI); - if (ifp == NULL) - panic("%s: can not if_alloc()", device_get_nameunit(sc->dev)); + if (ifp == NULL) { + device_printf(sc->dev, "can not if_alloc()\n"); + return (ENOSPC); + } mtx_init(&sc->mtx, device_get_nameunit(sc->dev), MTX_NETWORK_LOCK, - MTX_DEF | MTX_RECURSE); + MTX_DEF); + callout_init_mtx(&sc->watchdog, &sc->mtx, 0); + if_initname(ifp, device_get_name(sc->dev), device_get_unit(sc->dev)); ifp->if_softc = sc; - ifp->if_init = (if_init_f_t *)pdq_ifinit; + ifp->if_init = pdq_ifinit; ifp->if_snd.ifq_maxlen = IFQ_MAXLEN; ifp->if_flags = IFF_BROADCAST|IFF_SIMPLEX|IFF_NOTRAILERS|IFF_MULTICAST; - ifp->if_watchdog = pdq_ifwatchdog; - ifp->if_ioctl = pdq_ifioctl; ifp->if_start = pdq_ifstart; @@ -441,7 +487,15 @@ pdq_ifattach(pdq_softc_t *sc, const pdq_ } #endif + sc->sc_pdq = pdq_initialize(sc->mem_bst, sc->mem_bsh, ifp->if_xname, -1, + sc, type); + if (sc->sc_pdq == NULL) { + device_printf(sc->dev, "Initialization failed.\n"); + return (ENXIO); + } + fddi_ifattach(ifp, llc, FDDI_BPF_SUPPORTED); + return (0); } void @@ -452,8 +506,10 @@ pdq_ifdetach (pdq_softc_t *sc) ifp = sc->ifp; fddi_ifdetach(ifp, FDDI_BPF_SUPPORTED); - if_free(ifp); - pdq_stop(sc->sc_pdq); + PDQ_LOCK(sc); + pdq_ifstop(sc); + PDQ_UNLOCK(sc); + callout_drain(&sc->watchdog); pdq_free(sc->dev); return; @@ -474,6 +530,8 @@ pdq_free (device_t dev) bus_teardown_intr(dev, sc->irq, sc->irq_ih); if (sc->irq) bus_release_resource(dev, SYS_RES_IRQ, sc->irq_rid, sc->irq); + if (sc->ifp) + if_free(sc->ifp); /* * Destroy the mutex.