From owner-p4-projects@FreeBSD.ORG Thu Jul 13 17:13:06 2006 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 1BF9C16A4E2; Thu, 13 Jul 2006 17:13:06 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id D2E8C16A4DD; Thu, 13 Jul 2006 17:13:05 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id 4F53B43D45; Thu, 13 Jul 2006 17:12:58 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.4/8.13.4) with ESMTP id k6DHCvjI098599; Thu, 13 Jul 2006 13:12:57 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Paolo Pisati Date: Thu, 13 Jul 2006 12:47:01 -0400 User-Agent: KMail/1.9.1 References: <200607131616.k6DGGVeg043616@repoman.freebsd.org> In-Reply-To: <200607131616.k6DGGVeg043616@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200607131247.01523.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Thu, 13 Jul 2006 13:12:57 -0400 (EDT) X-Virus-Scanned: ClamAV 0.87.1/1598/Thu Jul 13 07:38:16 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.1.0 X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on server.baldwin.cx Cc: Perforce Change Reviews Subject: Re: PERFORCE change 101480 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: Thu, 13 Jul 2006 17:13:06 -0000 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