Date: Thu, 13 Jul 2006 12:47:01 -0400 From: John Baldwin <jhb@freebsd.org> To: Paolo Pisati <piso@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org> Subject: Re: PERFORCE change 101480 for review Message-ID: <200607131247.01523.jhb@freebsd.org> In-Reply-To: <200607131616.k6DGGVeg043616@repoman.freebsd.org> References: <200607131616.k6DGGVeg043616@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 13 July 2006 12:16, Paolo Pisati wrote: > http://perforce.freebsd.org/chv.cgi?CH=101480 > > Change 101480 by piso@piso_newluxor on 2006/07/13 16:16:08 > > Please welcome the first driver converted to use a filter+ithread > handler. Hmm, I would have thought that you would have converted em(4) first. In this case you have races with access to sc->bfe_istat, and it looks like you should be returning FILTER_HANDLED | FILTER_SCHEDULE_THREAD. > Affected files ... > > .. //depot/projects/soc2006/intr_filter/dev/bfe/if_bfe.c#3 edit > .. //depot/projects/soc2006/intr_filter/dev/bfe/if_bfereg.h#2 edit > > Differences ... > > ==== //depot/projects/soc2006/intr_filter/dev/bfe/if_bfe.c#3 (text+ko) ==== > > @@ -88,7 +88,11 @@ > static int bfe_attach (device_t); > static int bfe_detach (device_t); > static void bfe_release_resources (struct bfe_softc *); > +#if 0 > static void bfe_intr (void *); > +#endif > +static int bfe_filter (void *); > +static void bfe_handler (void *); > static void bfe_start (struct ifnet *); > static void bfe_start_locked (struct ifnet *); > static int bfe_ioctl (struct ifnet *, u_long, caddr_t); > @@ -418,8 +422,12 @@ > /* > * Hook interrupt last to avoid having to lock softc > */ > +#if 0 > error = bus_setup_intr(dev, sc->bfe_irq, INTR_TYPE_NET | INTR_MPSAFE, > NULL, bfe_intr, sc, &sc->bfe_intrhand); > +#endif > + error = bus_setup_intr(dev, sc->bfe_irq, INTR_TYPE_NET | INTR_MPSAFE, > + bfe_filter, bfe_handler, sc, &sc->bfe_intrhand); > > if (error) { > printf("bfe%d: couldn't set up irq\n", unit); > @@ -1182,6 +1190,95 @@ > sc->bfe_rx_cons = cons; > } > > +static int > +bfe_filter(void *xsc) > +{ > + struct bfe_softc *sc = xsc; > + u_int32_t imask; > + > + sc->bfe_istat = CSR_READ_4(sc, BFE_ISTAT); > + imask = CSR_READ_4(sc, BFE_IMASK); > + > + /* > + * Defer unsolicited interrupts - This is necessary because setting the > + * chips interrupt mask register to 0 doesn't actually stop the > + * interrupts > + */ > + sc->bfe_istat &= imask; > + CSR_WRITE_4(sc, BFE_ISTAT, sc->bfe_istat); > + CSR_READ_4(sc, BFE_ISTAT); > + > + /* not expecting this interrupt, disregard it */ > + if(sc->bfe_istat == 0) > + return (FILTER_STRAY); > + > + /* disable interrupts - not that it actually does..*/ > + CSR_WRITE_4(sc, BFE_IMASK, 0); > + CSR_READ_4(sc, BFE_IMASK); > + > + return (FILTER_SCHEDULE_THREAD); > +} > + > +static void > +bfe_handler(void *xsc) > +{ > + struct bfe_softc *sc = xsc; > + struct ifnet *ifp; > + u_int32_t istat, flag; > + > + ifp = sc->bfe_ifp; > + > + BFE_LOCK(sc); > + > + istat = sc->bfe_istat; > + if(istat & BFE_ISTAT_ERRORS) { > + > + if (istat & BFE_ISTAT_DSCE) { > + printf("if_bfe Descriptor Error\n"); > + bfe_stop(sc); > + BFE_UNLOCK(sc); > + return; > + } > + > + if (istat & BFE_ISTAT_DPE) { > + printf("if_bfe Descriptor Protocol Error\n"); > + bfe_stop(sc); > + BFE_UNLOCK(sc); > + return; > + } > + > + flag = CSR_READ_4(sc, BFE_DMATX_STAT); > + if(flag & BFE_STAT_EMASK) > + ifp->if_oerrors++; > + > + flag = CSR_READ_4(sc, BFE_DMARX_STAT); > + if(flag & BFE_RX_FLAG_ERRORS) > + ifp->if_ierrors++; > + > + ifp->if_drv_flags &= ~IFF_DRV_RUNNING; > + bfe_init_locked(sc); > + } > + > + /* A packet was received */ > + if(istat & BFE_ISTAT_RX) > + bfe_rxeof(sc); > + > + /* A packet was sent */ > + if(istat & BFE_ISTAT_TX) > + bfe_txeof(sc); > + > + /* We have packets pending, fire them out */ > + if (ifp->if_drv_flags & IFF_DRV_RUNNING && > + !IFQ_DRV_IS_EMPTY(&ifp->if_snd)) > + bfe_start_locked(ifp); > + > + BFE_UNLOCK(sc); > + > + /* Enable interrupts */ > + CSR_WRITE_4(sc, BFE_IMASK, BFE_IMASK_DEF); > +} > + > +#if 0 > static void > bfe_intr(void *xsc) > { > @@ -1254,6 +1351,7 @@ > > BFE_UNLOCK(sc); > } > +#endif > > static int > bfe_encap(struct bfe_softc *sc, struct mbuf **m_head, u_int32_t *txidx) > > ==== //depot/projects/soc2006/intr_filter/dev/bfe/if_bfereg.h#2 (text+ko) ==== > > @@ -510,6 +510,7 @@ > struct bfe_data bfe_tx_ring[BFE_TX_LIST_CNT]; /* XXX */ > struct bfe_data bfe_rx_ring[BFE_RX_LIST_CNT]; /* XXX */ > struct mtx bfe_mtx; > + u_int32_t bfe_istat; > u_int32_t bfe_flags; > u_int32_t bfe_imask; > u_int32_t bfe_dma_offset; > -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200607131247.01523.jhb>